Propagate .jdeps for java_import#363
Conversation
fmeum
force-pushed
the
java-import-reduced-classpath
branch
from
1c5b6ea to
bbf650b
Compare
fmeum
changed the title
java_import participate in Java classpath reduction.jdeps for java_import
fmeum
force-pushed
the
java-import-reduced-classpath
branch
2 times, most recently
from
bd8f649 to
1de17e5
Compare
|
|
||
| # Default java_toolchain parameters | ||
| _BASE_TOOLCHAIN_CONFIGURATION = dict( | ||
| deps_checker = Label("@remote_java_tools//:ImportDepsChecker"), |
There was a problem hiding this comment.
This is a breaking change for Bazel as the hardcoded mode is error. Can I add a new attribute that makes it configurable on the toolchain level? I can set the default to silence to match the current behavior or to error as a breaking change with a simple migration.
Are you precompiling the tool to a native image at Google?
There was a problem hiding this comment.
Are you precompiling the tool to a native image at Google?
It's a prebuilt jar that runs on the JVM, but it's also a validation action that is not on the critical path (#362 (comment)).
Using a native image seems like a good idea if it's going to run as a non-validation action so the .jdeps can be used for reduced classpaths.
There was a problem hiding this comment.
Can I add a new attribute that makes it configurable on the toolchain level?
Bazel supports tags = ["incomplete-deps"] on java_import. Having a real attribute (on the toolchain and/or individual java_imports) is probably better than leaning on tags for this.
cc @hvadehra
There was a problem hiding this comment.
My main concern is that external repos using java_import would be very difficult to opt out unless there is a global (toolchain) switch.
There was a problem hiding this comment.
This should only be a breaking change for Bazel 7, which is okay I guess. I also don't see why we need a separate switch - one can just unset deps_checker on the toolchain.
fmeum
force-pushed
the
java-import-reduced-classpath
branch
4 times, most recently
from
1206db6 to
14e031c
Compare
With `--experimental_java_classpath=bazel_no_fallback` (and as a double-compilation pessimization under the default `bazel` mode), a `java_import` defeated the reduced-classpath optimization for every target depending on it. Unlike a `java_library` header jar, which is built by turbine (emitting a jdeps and repackaging the supertype closure), a `java_import` interface jar is built by ijar and contributed no compile-time dependency artifact, so downstream reduced classpaths pruned the import's transitive jars. `java_import` now runs `ImportDepsChecker` (newly wired into the default toolchain's `deps_checker`) over its jars and propagates the resulting jdeps proto as `compile_jdeps`. The checker records the full transitive supertype closure, so this is correct for supertype chains of any depth. It runs in `silence` mode (purely additive, never a new build failure) and only when the import has deps/exports; the jdeps is consumed lazily. Fixes bazelbuild#362
fmeum
force-pushed
the
java-import-reduced-classpath
branch
from
14e031c to
08211fd
Compare
|
|
||
| # Default java_toolchain parameters | ||
| _BASE_TOOLCHAIN_CONFIGURATION = dict( | ||
| deps_checker = Label("@remote_java_tools//:ImportDepsChecker"), |
There was a problem hiding this comment.
This should only be a breaking change for Bazel 7, which is okay I guess. I also don't see why we need a separate switch - one can just unset deps_checker on the toolchain.
| _test_simple, | ||
| _test_with_java_library, | ||
| _test_deps, | ||
| _test_compile_jdeps_propagated_for_deps, |
There was a problem hiding this comment.
I think tests for the ImportDepsChecker action would be useful as well - particularly around the value of --checking_mode
There was a problem hiding this comment.
I added a test and realized that Bazel always forces the silence mode. Which is good for backwards compatibility, I guess, but probably worth making configurable at some point.
hvadehra
added
P3
- Actually test the previously unused incomplete-deps-tagged target and fix the assertions that all checked the same target. - Use java_info_subject.from_target() for readable failure messages. - Tag the test with min_bazel_8 since the rules_java Starlark implementation is used from Bazel 8 on. - Add a test for the ImportDepsChecker action's --checking_mode (and --rule_label) flag, covering both the default java_import behavior (always silence) and a rule that enforces the check via skip_incomplete_deps_check = False (error, or silence with the incomplete-deps tag).
fmeum
force-pushed
the
java-import-reduced-classpath
branch
from
7a08799 to
463ff11
Compare
3 participants
Downstream Java compilation actions need accurate
.jdepsfiles for their reduced classpath optimization. With this change,java_importnow always runsImportDepsCheckerto generate this file, but possibly suppresses its outputs. Note that Bazel (but not Blaze) always silences the output.Fixes #362