Skip to content
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

Revert "Use target_compatible_with for Java runtimes" #17659

Closed

Conversation

jlaxson
Copy link
Contributor

@jlaxson jlaxson commented Mar 3, 2023

See #17085

d5559c1 changed the JDK definitions used when you specify --java_runtime_version=remotejdk_11 (or other version) so that the toolchains were resolved using target_compatible_with instead of exec_compatible_with. I think that this was not correct because:

  1. Semantically, most JDKs compile .class files that can run on any platform. The current form requires you be targeting a platform that has a known-to-bazel JDK.
  2. This breaks RBE and cross compilation. Because the toolchains specify only target_compatible_with, bazel will completely disregard the exec platform's OS and other constraints when choosing the appropriate JDK to use.
  3. This breaks builds entirely in polyglot repos that have java_library targets. If you accidentally hit that target with a glob when targeting a platform not available in the JDK list, the build fails with a toolchain resolution error. Can't even fix it with target_compatible_with.

Closes #17085

@sgowroji sgowroji added type: documentation (cleanup) team-Rules-Java Issues for Java rules team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Mar 3, 2023
@gregestren
Copy link
Contributor

@cushon can you advise on this?

@cushon
Copy link
Contributor

cushon commented Mar 14, 2023

This change is incorrect.

The target platform is the platform "on which a final output resides and executes".

When building a java_binary, the included JDK needs to be compatible with the target platform, because that's where it will be executed.

Part of the issue here is that the JDK is both a runtime, and a set of tools that can be used during the build. When using the tools from the JDK during the build (e.g. javac), they need to be compatible with the exec platform. The way Bazel approaches this currently is to use a different toolchain to resolve the java_toolchain rule, which provides its own runtime, and uses exec_compatible_with.

@jlaxson
Copy link
Contributor Author

jlaxson commented Mar 15, 2023

Thanks @cushon @gregestren

I definitely see how that is how supposed to work, but the commit to be reverted causes all sorts of breakage in RBE and cross-platform builds, I enumerated a few of them in #17085

When building a java_binary, the included JDK needs to be compatible with the target platform, because that's where it will be executed.

Is/was this true for java_test? My experience is that *_test rules exec on the same platform their build toolchain execs on, having nothing to do with the target platform. Then bazel brings the wrong runtime architecture when they get executed. I just added this example to my test repo in #17085.

What sort of changes would address this collection of issues?

@hvadehra
Copy link
Member

The files changed here have been moved out of bazel and now reside in @rules_java. If this is still an issue with Bazel 7, please consider opening a PR in that repo.

@hvadehra hvadehra closed this Oct 23, 2023
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Documentation Documentation improvements that cannot be directly linked to other team labels team-Rules-Java Issues for Java rules type: documentation (cleanup)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel 6 Remote JDK Toolchains Incorrect Configuration
5 participants