-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: main
Are you sure you want to change the base?
Conversation
# 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ad11d1e
to
1a987f3
Compare
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 <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
2fc91c1
to
0d24285
Compare
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure 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 <sam@elide.ventures>
64eda9d
to
687ff90
Compare
- create base attributes, split into main/shared target attrs - create new macro and rules Signed-off-by: Sam Gammon <sam@elide.ventures>
- support `main_module` attribute - format with `main_module` attr if present Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
- add transitive inputs for compile-time jars - mark applicable dependencies with `neverlink` Relates-To: #243 Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
687ff90
to
85cfed7
Compare
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
- 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 <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Relates-To: #220 Relates-To: bazelbuild/bazel#12714 Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
- 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 <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
name = "mylib", | ||
deps = [":java"], | ||
strict = True, | ||
lib_name = "libmylib", # becomes `libmylib.so`, `libmylib.dylib`, etc. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this nesting intentional?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Signed-off-by: Sam Gammon <sam@elide.ventures>
|
Summary
This changeset adds
out_headers
andadditional_outputs
, which, together, allow a developer to add additional outputs to a regularnative_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 thenative_image
target to facilitate downsream support forrules_cc
targets.Tasklist
jvm_flags
#250javac
options #246neverlink
#243Related PRs and issues:
Other changes enclosed:
rules_cc
use downstreamstrict
attribute applies latest strictness flagsChangelog
additional_outputs
attributedefault_outputs
attribute to opt-out of new behaviorCcInfo
to shared library targetsnative_image_shared_library
ruleneverlink
libs$(location ...)
expansiondata
attribute in direct inputsGRAALVM_HOME
tonative-image
build