Propagate CcInfo for native dependencies
#257
Conversation
Yannic
force-pushed
the
java_library_cc_info
branch
4 times, most recently
from
4901d3f to
a535387
Compare
| args.add("--output", native_header) | ||
|
|
||
| ctx.actions.run( | ||
| executable = java_toolchain.native_header_generator, |
There was a problem hiding this comment.
An alternative could be to generate a tree artifact of headers as in https://github.com/fmeum/rules_jni/blob/e93011edbf0dcd48a00bd3147081b9c435ceda9b/jni/private/jni_headers.bzl#L20, which avoids a custom tool and may work better with .d pruning or include scanning.
There was a problem hiding this comment.
I'm not sure yet what's the best approach here TBH.
- having a single header file per
java_binaryseems nice, but requires tooling to merge the headers from the jar file - having one header per class is also not too bad, but also requires a dedicated tool (e.g.,
@bazel_tools//tools/zip:zipperlike you used), could cause issues with duplicate headers if builds are not one-version clean (or because ofMETA-INFfrom the-native.jar), and may add one-Iper Java target for the tree artifact, which makes header lookup much more expensive - not doing anything with the header jar at all and keep support for native dependencies minimal in
rules_javaand rely on something likerules_jnito providejni_{cc,rust,...}_library
My main interest here is to get launchers working in Bazel, so I'll probably go with the third option and remove the generated native header.
There was a problem hiding this comment.
Makes sense. I'm not sure myself and definitely didn't want to suggest the approach of rules_jni is better overall. Keeping this out of rules_java until we find a great solution is probably the easiest way to achieve your goal.
Sidenote: In rules_jni, I also faced the problem that JNI headers and their native implementation tend to create cyclic dependencies: the native implementation needs to depend on the Java target for the headers, but the Java target needs to depend on the implementation to have it linked in. I "solved" this by adding implicit targets via a macro. What's your plan for this?
There was a problem hiding this comment.
Sidenote: In rules_jni, I also faced the problem that JNI headers and their native implementation tend to create cyclic dependencies: the native implementation needs to depend on the Java target for the headers, but the Java target needs to depend on the implementation to have it linked in. I "solved" this by adding implicit targets via a macro. What's your plan for this?
I don't think there's a cyclic dependency at build time. The Java part will compile just fine, and so does the native code (% possible the generated native header, but we can get that from JavaInfo). There is, however, a runtime dependency cycle of course.
I think we need 2 modes here:
- "shell launcher" mode, where we need to link a
.sowith all native deps andSystem.loadLibrary()it somehow, and - "custom native launcher" mode, where the user sets
--java_launcher(orjava_{binary,test}#launcher) tocc_library(or something equivalent that exportsCcInfo) andjava_{binary,test}links the launcher and all native dependencies into a single executable and concatenates the_deploy.jarto the binary (i.e., what java_binary#launcher documents, % acceptingcc_libraryinstead of relying onCcLauncherInfo).
For the former, I think we can generate a wrapper main class that calls System.loadLibrary() before calling the "real" main function. I think that's somehow easier in rules_java as we control java_{binary,test} and can do whatever is needed. For the latter, it's up to the user to provide a native main() that starts the JVM (which then automatically has all native methods registered IIUC). We should provide a library to make that easier for users here, but it's not strictly required as one could roll their own.
There was a problem hiding this comment.
I have a working prototype locally, but it unfortunately modifies JavaInfo and breaks without some further patches because rules_java wraps the Starlark provider with the native provider in some places and the field with CcLinkingContext is lost.
Yannic
force-pushed
the
java_library_cc_info
branch
2 times, most recently
from
c0a0a56 to
cc14213
Compare
This will, in a follow-up, allow us to create and link `java_{binary,test}#laucher`
as the docs suggest.
Unfortunately, `JavaInfo#transitive_native_libraries` fails to propagate
sufficient information about transitive dependencies of the native
libraries, so linking fails because of missing symbols from transitive
libraries that should be there.
Yannic
force-pushed
the
java_library_cc_info
branch
from
cc14213 to
79bf77c
Compare
| debug_context = dependencies_cc_info.debug_context(), | ||
| ) | ||
| else: | ||
| target["CcInfo"] = CcInfo() |
There was a problem hiding this comment.
As this is a common case, you can reduce retained heap by sharing a single empty instance.
There was a problem hiding this comment.
I'm not sure how much of a difference this makes in practice: java_library will output CcInfo, so every target with deps will take the if case and produce a different, non-equal CcInfo with probably no native deps.
There was a problem hiding this comment.
Hmm, I see. I don't know whether CcInfo merging attempts to preserve empty instances, which would help here.
That said, I now realize that I don't quite understand why each java_library now needs to advertise CcInfo. What information is missing from transitive_native_libraries? Why can't this be a private provider distinct from CcInfo up to the java_binary level?
|
FYI, the missing info was deliberately removed. See https://docs.google.com/document/d/10isTEK5W9iCPp4BIyGBrLY5iti3Waaam6EeGVSjq3r8/edit?tab=t.0#heading=h.fs8cvfw0s0um @comius can hopefully provide more on the rationale. |
This will, in a follow-up, allow us to create and link
java_{binary,test}#laucheras the docs suggest.Unfortunately,
JavaInfo#transitive_native_librariesfails to propagate sufficient information about transitive dependencies of the native libraries, so linking fails because of missing symbols from transitive libraries that should be there.