-
Notifications
You must be signed in to change notification settings - Fork 251
Provide associate jars when compile java sources in _run_kt_java_builder_actions #1468
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
Closed
fredm-spotify
wants to merge
1
commit into
bazel-contrib:master
from
fredm-spotify:fredm/fredm/add-associated-jars-to-kt-java-builder-actions-latest
+225
−1
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,32 @@ | ||
| load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") | ||
| load("//kotlin:core.bzl", "define_kt_toolchain") | ||
| load(":associate_java_compile_tests.bzl", associate_test_suite = "test_suite") | ||
| load(":rule_tests.bzl", "test_suite") | ||
|
|
||
| test_suite( | ||
| name = "rules_tests", | ||
| ) | ||
|
|
||
| associate_test_suite( | ||
| name = "associate_java_compile_tests", | ||
| ) | ||
|
|
||
| # Test-only toolchain with experimental_remove_private_classes_in_abi_jars enabled. | ||
| # Activated via config_settings in analysis tests to verify that associate class jars | ||
| # (not just ABI jars) are passed to java_common.compile(). | ||
|
|
||
| bool_flag( | ||
| name = "use_abi_stripping_toolchain", | ||
| build_setting_default = False, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "abi_stripping_enabled", | ||
| flag_values = {":use_abi_stripping_toolchain": "True"}, | ||
| ) | ||
|
|
||
| define_kt_toolchain( | ||
| name = "abi_stripping_test_toolchain", | ||
| experimental_remove_private_classes_in_abi_jars = True, | ||
| target_settings = [":abi_stripping_enabled"], | ||
| ) |
79 changes: 79 additions & 0 deletions
79
src/test/starlark/compile/associate_java_compile_tests.bzl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| load("@rules_java//java/common:java_info.bzl", "JavaInfo") | ||
| load("@rules_testing//lib:analysis_test.bzl", "analysis_test") | ||
| load("@rules_testing//lib:util.bzl", "util") | ||
| load("//kotlin:jvm.bzl", "kt_jvm_library") | ||
|
|
||
| _ABI_STRIPPING_FLAG = str(Label("//src/test/starlark/compile:use_abi_stripping_toolchain")) | ||
|
|
||
| def _java_compile_has_associate_class_jars_impl(env, target): | ||
| """Verify the Javac action receives associate CLASS jars (not just ABI jars). | ||
|
|
||
| When experimental_remove_private_classes_in_abi_jars is enabled: | ||
| - The associate's JavaInfo.compile_jars contains ABI jars with internal classes stripped. | ||
| - compile_deps.associate_jars contains the full class jars. | ||
| - compile.bzl wraps associate_jars in synthetic JavaInfos and adds them to | ||
| java_common.compile() deps so javac can resolve internal symbols (e.g. Dagger). | ||
|
|
||
| Without the fix, only the stripped ABI jars would reach javac (via compile_deps.deps). | ||
| With the fix, the full class jars are also present. | ||
| """ | ||
| got_target = env.expect.that_target(target) | ||
|
|
||
| # java_common.compile() registers an action with mnemonic "Javac" | ||
| javac_action = got_target.action_named("Javac") | ||
|
|
||
| # With experimental_remove_private_classes_in_abi_jars enabled, associate_jars | ||
| # contains the class jars (java_outputs[].class_jar), which are distinct from | ||
| # the ABI jars (compile_jars). The fix adds these class jars to java_common.compile() | ||
| # deps via synthetic JavaInfos. | ||
| associate_target = env.ctx.attr.associate_target | ||
| associate_class_jar_paths = [ | ||
| output.class_jar.short_path | ||
| for output in associate_target[JavaInfo].java_outputs | ||
| ] | ||
| javac_action.inputs().contains_at_least(associate_class_jar_paths) | ||
|
|
||
| def _test_java_compile_has_associate_class_jars(name): | ||
| """Mixed Kotlin/Java target with associates passes associate class jars to javac.""" | ||
| util.helper_target( | ||
| kt_jvm_library, | ||
| name = name + "_associate_lib", | ||
| srcs = [util.empty_file(name + "_Internal.kt")], | ||
| ) | ||
|
|
||
| util.helper_target( | ||
| kt_jvm_library, | ||
| name = name + "_main_lib", | ||
| srcs = [ | ||
| util.empty_file(name + "_Main.kt"), | ||
| util.empty_file(name + "_Generated.java"), | ||
| ], | ||
| associates = [name + "_associate_lib"], | ||
| ) | ||
|
|
||
| analysis_test( | ||
| name = name, | ||
| impl = _java_compile_has_associate_class_jars_impl, | ||
| target = name + "_main_lib", | ||
| config_settings = { | ||
| _ABI_STRIPPING_FLAG: True, | ||
| }, | ||
| attrs = { | ||
| "associate_target": attr.label(providers = [JavaInfo]), | ||
| }, | ||
| attr_values = { | ||
| "associate_target": ":" + name + "_associate_lib", | ||
| }, | ||
| ) | ||
|
|
||
| def test_suite(name): | ||
| _test_java_compile_has_associate_class_jars( | ||
| name = "test_java_compile_has_associate_class_jars", | ||
| ) | ||
|
|
||
| native.test_suite( | ||
| name = name, | ||
| tests = [ | ||
| "test_java_compile_has_associate_class_jars", | ||
| ], | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hm. If I'm not wrong, this undoes the advantages of the ABI jars for associates.
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.
oh no! My thinking is that since it's associated I should be able to access the internal symbols as a dependency to the compile task.
And I thought that whatever goes in
depsdoesn't end up in the final jar which would keep the internal symbols not-exposed.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 we could flag this change behind the pair of
experimental_treat_internal_as_private_in_abi_jars, experimental_remove_private_classes_in_abi_jarsflags, but this concept (a modules java code should have access to its associates internals) is whats for discussion in #1467There 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.
Inputs are part of the cache key, so changes would invalidate on any associate change. It's not a whole tree invalidation (just immediate downstream actions), but it will have an impact.
I suspect we'd really want to extend abi here -- a change to a private field shouldn't invalidate. Hm.
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.
Ok, one thing I'm not sure I follow. Let me come up with a specific example. I've hit this issue when I was compiling dagger in a bazel target called
wiringwhich was associated with the target that had the implementation -impl.The way I understand this, is that the dagger generated code placed in
wiringmight reference internal symbols fromimpl. shouldn't, in theory, a change to the internal code fromimplactually invalidate the build fromwiring?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.
sounds good! thank you
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.
hmmm, so i have tried this out... assuming i could use the skip code gen plugin to avoid a full recompilation for the associates-abi.jar. Looks like that plugin is only good for k1 and i couldnt see any low risk options for having something similar for k2. So I could either do a full compilation and discard the output class jar in order to get an associates-abi.jar or maybe there is some jar processing that could be done to get something that would work, but that dosnt sound too great either
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.
the other thing im trying is to actually use the abigen plugin twice, one with the flags to produce a public only abi jar and the other to make the "associates abi jar" (containing the internal symbols). Given you cant register the plugin twice im trying a thin wrapper that registers it under a different ID in order to configure it differently
Uh oh!
There was an error while loading. Please reload this page.
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.
WDYT of #1479 as a starter for ten @restingbull
you might want a cup of tea and a biscuit to take you through 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.
@restingbull I'm closing this PR. I understand now that the jar I'm providing also include private symbols that once changed shouldn't invalidate the cache.
I've also gave a go at @rbeazleyspot's solution and it works within the Spotify project. I think we would prefer to go with his approach. Thanks for all the help!