Fix Java runtime toolchain resolution in cross-compilation scenarios by fmeum · Pull Request #18262 · bazelbuild/bazel

fmeum · 2023-04-28T15:00:25Z

The Java rules used toolchains of type @bazel_tools//tools/jdk:runtime_toolchain_type for two different purposes requiring different exec/target platform constraints, which led to issues when cross-compiling: The runtime added to the runfiles of an executable Java target has to have constraints on the target platform, whereas the runtime used to extract the bootclasspath should not have constraints on the target platform. As a result:

@local_jdk did not define any target constraints for its runtime, which allowed the runtime to provide the bootclasspath required by Android rules, but could also end up building java_binary targets that wouldn't run on the target (see Bazel treats local_jdk as a "can run anywhere" toolchain #18265).
Remote JDKs defined a target constraint for the their runtimes, which prevented them from being added to the runfiles of targets they couldn't run on, but broke Android compilation due to the failure to resolve a runtime for bootclasspath extraction (see Bazel 6 Remote JDK Toolchains Incorrect Configuration #17085).

This is resolved by adding a third toolchain type, bootstrap_runtime_toolchain_type, that is only used by the bootclasspath rule (realizes #17085 (comment)). Detailed explanations of the different types and their intended constraints have been added to @bazel_tools//tools/jdk:BUILD.

Addressing the cross-compilation errors then required the following changes:

Direct dependencies on the Java runtime have been removed from rules that do not need them (e.g. android_library and java_binary with create_executable = False).
Implicit dependencies on the Java runtime through the bootclasspath rule have been replaced with dependencies on the bootstrap runtime.
{local,remote}_java_repository now use the user-provided exec constraints of the Java toolchain as the target constraints of the associated runtime.
@local_jdk uses the auto-detected host constraints as exec constraints for the local Java toolchain.

Fixes #17085
Fixes #18265
Fixes bazelbuild/rules_java#64

RELNOTES[INC]: Java runtime toolchains created via local_java_repository from @bazel_tools//tools/jdk:local_java_repository.bzl, which includes local_jdk, now have target_compatible_with set to the auto-detected host constraints. This can result in errors about toolchain resolution failures for @bazel_tools//tools/jdk:runtime_toolchain_type, especially when cross-compiling. These failures can be fixed in the following ways (listed in decreasing order of preference):

Replace java_binary targets that aren't meant to be run with bazel run or as tools during the build with java_single_jar (available in @rules_java//java:java_single_jar.bzl). Such targets do not require a Java runtime for the target configuration.
Set --java_runtime_version=remotejdk_N for some Java version N to let Bazel choose and download an appropriate remote JDK for the current target platform. This setting defaults to local_jdk, which means that Bazel can only use the local JDK, which isn't compatible with any other platform.
Manually define and register a local_java_runtime with no value set for exec_compatible_with (defaults to []) and select it by setting --java_runtime_version to its name. This fully restores the previous behavior, but can result in incorrect results when cross-compiling (see Bazel treats local_jdk as a "can run anywhere" toolchain #18265).

fmeum · 2023-05-02T11:36:38Z

@cushon Could you review the first three commits of this PR? They implement what you described in #17085 (comment).

@hvadehra Could you review the last commit as it touches the Starlark version of java_binary? Since I can't make changes to Google's java_semantics.bzl, I added a placeholder file with the changes required to make that commit a noop for Google.

Please let me know if you prefer having this PR split into multiple ones. I tried to group independently reviewable changes into separate commits, but only the combination of these commits truly resolves the cross-compilation issues.

FYI @illicitonion @jlaxson Please test with your setups if possible.

illicitonion · 2023-05-02T16:33:32Z

I tested this out and it worked for me - I got a useful "no toolchain found" error when I didn't have one registered, and when I registered one, everything worked as I expected. Thanks so much!

cushon

I'm on board with the overall approach here, thanks for tackling this.

I would like @hvadehra's input as well.

I think it probably makes sense to split some of the commits into separate PRs, maybe after everyone's taken a first pass.

tools/jdk/BUILD.tools

hvadehra · 2023-05-15T10:23:15Z

LGTM in general.

+1 to splitting this into separate commits. In particular, I think I'd like to do the changes in semantics.bzl slightly differently. But it's okay to leave as is on github, I'll handle that while importing.

fmeum · 2023-05-19T13:22:22Z

I will wait for bazelbuild/rules_java#110 to land before I continue working on this.

Work towards bazelbuild#17085 Work towards bazelbuild/rules_java#64 Split off from bazelbuild#18262

The `java_binary` Starlark implementation now only directly depends on a Java runtime toolchain when it is used as an executable rule. This work will eventually allow `java_binary` with `create_executable = False` to be compiled for platforms that do not provide a regular JDK, such as Android. Work towards bazelbuild#17085 Work towards bazelbuild/rules_java#64 Split off from bazelbuild#18262

hvadehra · 2023-10-19T08:56:50Z

@fmeum you might need some of the changes from #19864 (especially 74f8caa)

(i rebased that on top of the earlier version of this, and things look good, so if it's easier, we could go with the alternative and merge the update to 6.5.1, and then proceed with #19864 after)

fmeum · 2023-10-19T09:53:39Z

@fmeum you might need some of the changes from #19864 (especially 74f8caa)

I added the changes here, but I think that the failure is legitimate: According to https://github.com/bazelbuild/rules_java/blob/0c538b8720a8b8df5f50ce92debffc8b27b84af4/toolchains/BUILD#L260-L315, rules_java will use the following JDKs to compile for the different target Java versions:

8, 9, 10, 11, 21: JDK 21
14: JDK 14
15: JDK 15
16: JDK 16
17: JDK 17

This is also what we are seeing in the test: --java_language_version=17 results in compilation with JDK 17, not JDK 21.

@hvadehra Should we change the linked part of the BUILD file to something like this?

RELEASES = (8, 9, 10, 11, 17, 21)

[
    default_java_toolchain(
        name = ("toolchain_java%d" if release <= 11 else "toolchain_jdk_%d") % release,
        configuration = DEFAULT_TOOLCHAIN_CONFIGURATION,
        source_version = "%s" % release,
        target_version = "%s" % release,
    )
    for release in RELEASES
]

I think that we can drop JDK 14, 15, and 16. We could also decide to consolidate the name to toolchain_jdk_%d.

hvadehra · 2023-10-19T10:25:48Z

@hvadehra Should we change the linked part of the BUILD file to something like this?

Yeah, that makes sense, I'll make the change.

Is the RBE failure due to the attempted workaround for #19837 ?

fmeum · 2023-10-19T10:32:36Z

Is the RBE failure due to the attempted workaround for #19837 ?

Good question, no idea yet, but luckily I can debug this locally :-)

Edit: It is, I just don't know why yet.

fmeum · 2023-10-19T11:00:34Z

This is caused by the register_toolchains("//:bazel_java_toolchain_definition") line in MODULE.bazel, but I don't understand why.

Without this line:

bazel build --nobuild //:bazel_java_toolchain_definition --config=remote # passes
bazel build --nobuild //:bazel_java_toolchain --config=remote # fails with the expected missing toolchain error since this toolchain requires a bootstrap runtime, but none is registered with the rbe_jdk prefix

This behavior is expected. But why does register_toolchains("//:bazel_java_toolchain_definition") result in the analysis of //:bazel_java_toolchain?

fmeum · 2023-10-19T11:01:54Z

If we can't figure this out and can accept the warnings, I can revert the workaround. Unfortunately there doesn't seem to be a way to register a certain toolchain by default, but not with a particular --config.

I will look into why this doesn't behave as expected.

fmeum · 2023-10-19T11:07:41Z

Nvm, the RBE toolchains only include Java runtime toolchains, not Java compilation toolchains, resulting in the selection of the //:bazel_java_toolchain. I will add another workaround.

fmeum · 2023-10-19T13:45:13Z

@hvadehra Updated

fmeum · 2023-10-19T13:56:36Z

I forgot to update MODULE.bazel.lock after running bazel run //src/test/tools/bzlmod:update_default_lock_file. I also added a comment about that.

fmeum · 2023-10-19T14:02:55Z

cc_integration_test keeps failing, presumably because I haven't updated rules_java in all the right places. I forgot distdir_deps.bzl, not sure whether was the last one.

hvadehra · 2023-10-19T14:04:54Z

not sure whether was the last one.

🤞

fmeum · 2023-10-19T14:55:17Z

Great, I won't touch this anymore.

hvadehra · 2023-10-19T15:13:58Z

@ahumesky Do you have any concerns about the change to src/test/java/com/google/devtools/build/android/dexer/BUILD? (context is that we're now using jdk21, which does not support java 7)

ahumesky · 2023-10-19T16:06:16Z

@ahumesky Do you have any concerns about the change to src/test/java/com/google/devtools/build/android/dexer/BUILD? (context is that we're now using jdk21, which does not support java 7)

thanks -- the dx parts are not used anymore and will be removed soon so this looks fine

keertk · 2023-10-19T16:09:45Z

@bazel-io fork 7.0.0

illicitonion mentioned this pull request Apr 28, 2023

Bazel treats local_jdk as a "can run anywhere" toolchain #18265

Closed

fmeum changed the title ~~Mark local Java runtimes as target_compatible_with the toolchain's e~~ Mark local Java runtimes as target_compatible_with the toolchain's exec constraints Apr 28, 2023

This comment was marked as outdated.

Sign in to view

fmeum force-pushed the missing-local-jdk-constraint branch 7 times, most recently from fa43c17 to bfb7536 Compare May 2, 2023 09:16

fmeum changed the title ~~Mark local Java runtimes as target_compatible_with the toolchain's exec constraints~~ Fix Java runtime toolchain resolution in cross-compilation scenarios May 2, 2023

fmeum force-pushed the missing-local-jdk-constraint branch 4 times, most recently from 2c119b0 to c8aa335 Compare May 2, 2023 11:08

fmeum marked this pull request as ready for review May 2, 2023 11:36

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Rules-Java Issues for Java rules labels May 2, 2023

fmeum requested review from cushon and hvadehra May 2, 2023 11:36

fmeum force-pushed the missing-local-jdk-constraint branch from c8aa335 to ee1b095 Compare May 7, 2023 08:51

cushon reviewed May 13, 2023

View reviewed changes

tools/jdk/BUILD.tools Show resolved Hide resolved

fmeum added a commit to fmeum/bazel that referenced this pull request Jun 8, 2023

Remove unnecessary Java runtime dependency of non-binary rules

4a02190

Work towards bazelbuild#17085 Work towards bazelbuild/rules_java#64 Split off from bazelbuild#18262

fmeum mentioned this pull request Jun 8, 2023

Remove unnecessary Java runtime dependency of non-binary rules #18618

Closed

fmeum mentioned this pull request Jun 9, 2023

Remove runtime dependency of non-executable java_binary #18620

Closed

fmeum force-pushed the missing-local-jdk-constraint branch from 9ac40fd to 5c47637 Compare October 19, 2023 08:47

fmeum requested a review from kevin1e100 as a code owner October 19, 2023 08:47

fmeum force-pushed the missing-local-jdk-constraint branch from 5c47637 to b0c94a1 Compare October 19, 2023 08:48

Update to rules_java 7.0.0

828e0cd

fmeum force-pushed the missing-local-jdk-constraint branch from b0c94a1 to e73a7cf Compare October 19, 2023 10:19

Temporarily work around bazelbuild#19837

6daa568

fmeum force-pushed the missing-local-jdk-constraint branch from e73a7cf to 6daa568 Compare October 19, 2023 11:10

fmeum force-pushed the missing-local-jdk-constraint branch from b0bc42b to 933da39 Compare October 19, 2023 13:53

Update to rules_java 7.0.6

081a254

hvadehra approved these changes Oct 19, 2023

View reviewed changes

ahumesky approved these changes Oct 19, 2023

View reviewed changes

bazel-io mentioned this pull request Oct 19, 2023

[7.0.0] Fix Java runtime toolchain resolution in cross-compilation scenarios #19895

Closed

iancha1992 mentioned this pull request Oct 19, 2023

[7.0.0] Fix Java runtime toolchain resolution in cross-compilation scenarios #19900

Merged

hvadehra mentioned this pull request Nov 16, 2023

Update handling of JavaInfo.compilation_info.javac_opts kythe/kythe#5934

Merged

plaird mentioned this pull request Dec 22, 2024

failed attempt at using java_runtime_version for bazel run [do not merge] salesforce/rules_spring#244

Closed

Conversation

fmeum commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

fmeum commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illicitonion commented May 2, 2023

Uh oh!

cushon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvadehra commented May 15, 2023

Uh oh!

fmeum commented May 19, 2023

Uh oh!

hvadehra commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Oct 19, 2023

Uh oh!

hvadehra commented Oct 19, 2023

Uh oh!

fmeum commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Oct 19, 2023

Uh oh!

fmeum commented Oct 19, 2023

Uh oh!

fmeum commented Oct 19, 2023

Uh oh!

fmeum commented Oct 19, 2023

Uh oh!

fmeum commented Oct 19, 2023

Uh oh!

hvadehra commented Oct 19, 2023

Uh oh!

fmeum commented Oct 19, 2023

Uh oh!

hvadehra commented Oct 19, 2023

Uh oh!

ahumesky commented Oct 19, 2023

Uh oh!

keertk commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fmeum commented Apr 28, 2023 •

edited

Loading

fmeum commented May 2, 2023 •

edited

Loading

hvadehra commented Oct 19, 2023 •

edited

Loading

fmeum commented Oct 19, 2023 •

edited

Loading

fmeum commented Oct 19, 2023 •

edited

Loading