-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Verify bootclasspath version is at most compilation runtime version #18343
Conversation
@cushon Could you review this PR? It implements one more version check that wasn't mentioned in #17281, but discovered in #18309. The tests fail in CI as for some reason I don't understand - and only in CI - Bazel picks up remote JDKs from rules_java, which doesn't include 7556e11. As a side effect, this means that Bzlmod also won't be picking up CC @comius |
This is due to the CI test setup to reusing remote JDKs fetched by the outter Bazel. Can be reproduced by:
The root cause is the Line 313 in 03ca286
Instead, the template at bazel/tools/jdk/jdk_build_file.bzl Line 17 in 03ca286
|
A even better solution is to switch to use rules_java as the only source of truth as discussed in #18373 |
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.
Thanks, this generally LGTM
I wondered if there mgiht be any merit to doing this as runtime check. One benefit of that is that it avoids needing to set the version in BootClassPathInfo
.
One disadvantage is that we'd have to spend a little time inspecting the bootclasspath before passing it to javac. Another disadvantage is that the error isn't reported to analysis time, but if we can provide a better error than error: cannot access module-info
maybe that would be good enough?
@@ -160,6 +161,17 @@ public ConfiguredTarget create(RuleContext ruleContext) | |||
|
|||
JavaRuntimeInfo javaRuntime = JavaRuntimeInfo.from(ruleContext, "java_runtime"); | |||
|
|||
if (javaRuntime.version() != 0 && javaRuntime.version() < bootclasspath.version()) { |
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 wondered about using OptionalInt
in the Java code here, but I don't have a strong opinion. If starlark is using 0
, it might be clearer to be consistent
Address #18343 (comment) RELNOTES: None PiperOrigin-RevId: 532041694 Change-Id: I46fa41073470d042c9955c66b513b899424c77b1
The bootclasspath is loaded by the Java toolchain's runtime used for compilation and thus needs to be compatible with it, otherwise this triggers errors such as: ``` error: cannot access module-info bad class file: /modules/jdk.net/module-info.class class file has wrong version 64.0, should be 61.0 Please remove or make sure it appears in the correct subdirectory of the classpath. error: cannot access module-info bad class file: /modules/jdk.accessibility/module-info.class class file has wrong version 64.0, should be 61.0 Please remove or make sure it appears in the correct subdirectory of the classpath. error: cannot access module-info bad class file: /modules/jdk.internal.vm.compiler.management/module-info.class class file has wrong version 64.0, should be 61.0 Please remove or make sure it appears in the correct subdirectory of the classpath. ```
20fe048
to
d3ee688
Compare
Address bazelbuild#18343 (comment) RELNOTES: None PiperOrigin-RevId: 532041694 Change-Id: I46fa41073470d042c9955c66b513b899424c77b1
Oops, this dropped off my radar somehow. @fmeum could you please rebase and resolve conflicts? |
I can no longer reproduce the original failure. I will have to dig into this again. |
While investigating this, I ran into an actual bug: #19537 |
At least partially superseded by #19547. The missing case is |
The bootclasspath is loaded by the Java toolchain's runtime used for compilation and thus needs to be compatible with it, otherwise this triggers errors such as:
Work towards #17281