Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: shared headers, rules_cc support #221

Closed
wants to merge 41 commits into from
Closed

Conversation

sgammon
Copy link
Owner

@sgammon sgammon commented Jan 2, 2024

Summary

This changeset adds out_headers and additional_outputs, which, together, allow a developer to add additional outputs to a regular native_image build.

Because a header is generated per C entrypoint class, the developer needs to be able to declare as many as they want.

Additionally, a CcInfo provider needs to be returned from the native_image target to facilitate downsream support for rules_cc targets.

Tasklist

Related PRs and issues:

Other changes enclosed:

  • Initial support for rules_cc use downstream
  • File-based attributes now have multi-label alternatives
  • Debug builds now keep symbols and activate source-level debugging by default
  • New strict attribute applies latest strictness flags

Changelog

  • feat: add additional_outputs attribute
  • feat: add default_outputs attribute to opt-out of new behavior
  • feat: resolve and add appropriate CcInfo to shared library targets
  • feat: implement new native_image_shared_library rule
  • feat: implement output groups
  • feat: implement support for neverlink libs
  • feat: implement multi-label config attributes
  • feat: integrate graalvm build sandbox with bazel
  • feat: support for $(location ...) expansion
  • feat: support for make var expansion
  • fix: include data attribute in direct inputs
  • fix: provide GRAALVM_HOME to native-image build

@sgammon sgammon added feature Mainline feature work native-image Features and issues relating to the Native Image tool 🚧 WIP Work-in-progress, do not merge labels Jan 2, 2024
@sgammon sgammon added this to the 1.0.0 milestone Jan 2, 2024
@sgammon sgammon requested a review from fmeum January 2, 2024 05:46
@sgammon sgammon self-assigned this Jan 2, 2024
graalvm/nativeimage/rules.bzl Outdated Show resolved Hide resolved
# and `graal_isolate_dynamic.h` files are included. additional headers can be added by the `out_headers`
# attribute.
if ctx.attr.shared_library and ctx.attr.default_outputs:
additional_outputs.append(_GRAALVM_ISOLATE_HEADER)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any case in which adding these headers would be problematic for a shared library build? If not, I think we should not add default_outputs for now. I am a bit worried of native_image gaining too many knobs that depend on each other.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these headers are produced automatically by the native-image build -- so if they aren't added here, they end up as undeclared outputs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm in favor of adding them here. But maybe we could just get rid of default_outputs and have this behavior in place unconditionally?

Copy link
Owner Author

@sgammon sgammon Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm concerned about multiple shared library targets within the same package, but that would be something the user has to deal with anyway?

In my downstream test codebase, I ended up needing to manage graal_*.h headers manually, because that codebase expected angle syntax for importing headers. So I needed to pull them into their own cc_library target with an include = path set. In that case, I flip default_outputs to False.

If the headers are considered outputs unconditionally by the rule, I worry there would be no easy way to prevent a file collision under this use case. Maybe I'm missing something.

The default functionality for the rules seems to align with including them as defaults, otherwise a user would have to declare them unconditionally, so I agree this isn't ideal and would love a better way.

FWIW I think I may merge additional_outputs and out_headers, but those may need to remain separate so that we can attach headers to providers. We could also sniff the filename, if that's preferable let me know. Open to your advice on all aspects above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we separate the headers by having them emitted into a subdirectory? E.g., if we declare the headers for target foo with package relative path foo/graal_something.h, we can have multiple native images coexist in a package and also provide a way to disambiguate headers via the prefix.

Copy link
Owner Author

@sgammon sgammon Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can check for hidden/expert native-image flags, but AFAIK those are calculated from the binary name, so they are already under the user's control via executable_name. I'm not sure about a subfolder with executable_name, I think we'd have to slice that and append it to -H:Path, because the binary name is passed separately from the output prefix (to native-image).

In any case, I agree with you, I'd like to find a cleaner way to do it.

Copy link
Owner Author

@sgammon sgammon Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Lots of this is telling me that a dedicated shared_native_image rule might be a good idea, by the way, because the shared library function of native-image demands radically different Bazel behavior than an executable, as evidenced by executable_name being a little wonky in this case. Just food for later thought, your ideas are always welcome of course.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also like a dedicated rule better. For example, such a rule could be marked as non-executable and could also statically declare that it emits CcSharedLibraryInfo.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, to clarify, the regular header names are under the user's control, and follow the pattern of the executable_name (an executable name of libsample gets libsample.so and libsample.h, etc).

The standard GraalVM headers, i.e. graal_isolate.h and family, are generated unconditionally by native-image at those names. I don't see a way to override those.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we prepend foo/ to shared_library_name and then have a separate action symlink it to the location without the prefix? That would get us prefixed headers without the user having to refer to the shared library with the prefix.

internal/native_image/rules.bzl Outdated Show resolved Hide resolved
internal/native_image/rules.bzl Outdated Show resolved Hide resolved
@sgammon sgammon force-pushed the feat/shared-headers branch 2 times, most recently from ad11d1e to 1a987f3 Compare January 2, 2024 23:16
@sgammon sgammon linked an issue Jan 2, 2024 that may be closed by this pull request
This changeset adds `out_headers` and `additional_outputs`, which,
together, allow a developer to add additional outputs to a regular
`native_image` build.

Because a header is generated per C entrypoint class, the developer
needs to be able to declare as many as they want.

Additionally, a `CcInfo` provider needs to be returned from the
`native_image` target to facilitate downsream support for `rules_cc`
targets.

- feat: add `out_headers` and `additional_outputs` attributes
- feat: add `default_outputs` attribute to opt-out of new behavior
- feat: resolve and add `CcInfo` to shared library targets

Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Copy link

github-actions bot commented Jan 13, 2024

Dependency Review

✅ No vulnerabilities or license issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 454dc5c.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Manifest Files

Adds Makefile variable expansion support to the `extra_args` flags
passed to `native-image`.

Signed-off-by: Sam Gammon <[email protected]>
@sgammon sgammon force-pushed the feat/shared-headers branch 3 times, most recently from 64eda9d to 687ff90 Compare January 13, 2024 23:58
- feat: full implementation this time of make variable and location
  expansion

- feat: managed build temp directory for `native-image`

Relates-To: #244
Relates-To: #245
Signed-off-by: Sam Gammon <[email protected]>
- create base attributes, split into main/shared target attrs
- create new macro and rules

Signed-off-by: Sam Gammon <[email protected]>
- support `main_module` attribute
- format with `main_module` attr if present

Signed-off-by: Sam Gammon <[email protected]>
@sgammon sgammon linked an issue Jan 14, 2024 that may be closed by this pull request
Signed-off-by: Sam Gammon <[email protected]>
- add new `module_deps` attribute
- add example of modular java use
- wire in `main_module` attribute
- process into separate module path (direct only)
- append to `native-image` invocation as `--module-path`

Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
- add rule and macro definition/entrypoint for test targets
- rollup test targets dependencies in native image build
- inject native test feature, runner as entrypoint
- provide dependencies for junit5

Signed-off-by: Sam Gammon <[email protected]>
name = "mylib",
deps = [":java"],
strict = True,
lib_name = "libmylib", # becomes `libmylib.so`, `libmylib.dylib`, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would customarily be called mylib.dll on Windows though. How about letting the user specify just mylib? This matches the behavior of cc_* rules.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libmylib is literal, so users can specify mylib if they want to. Are you saying I should auto-add/manage the prefix? I'm happy to do so but I figured users would want it under their control.

SDK = struct(
artifact = "graal-sdk",
group = "org.graalvm.sdk",
options = {"neverlink": True},
NATIVEIMAGE = struct(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this nesting intentional?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because it allows references like graalvm.catalog.SDK.NATIVEIMAGE in the artifact system

executable = True,
fragments = [
_TEST_NAME_CONDITION = select({
"@bazel_tools//src/conditions:windows": "%target%-test.exe",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These targets may go away soon, we should define our own ones.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I pull from @platforms instead? They ship default constraints, right?

fragments = [
_TEST_NAME_CONDITION = select({
"@bazel_tools//src/conditions:windows": "%target%-test.exe",
"//conditions:default": "%target%-test",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most test rule names will end with test anyway. Do we need the suffix?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think we need it. Will drop

Copy link

sonarcloud bot commented Jan 25, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sgammon
Copy link
Owner Author

sgammon commented Sep 29, 2024

closing for now until there is more energy behind this feature (it needs a rebase anyway)

@sgammon sgammon closed this Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Mainline feature work native-image Features and issues relating to the Native Image tool 🚧 WIP Work-in-progress, do not merge
Projects
2 participants