-
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
Java 16 toolchain #13274
Java 16 toolchain #13274
Conversation
I released diff --git a/distdir_deps.bzl b/distdir_deps.bzl
index e2ec4469a3..3ab07efa5c 100644
--- a/distdir_deps.bzl
+++ b/distdir_deps.bzl
@@ -267,11 +267,10 @@ DIST_DEPS = {
"remote_java_tools_test",
"remote_java_tools_for_testing",
],
- "archive": "java_tools-v11.2.zip",
- "sha256": "b6c468410a371728fe703ef7a6e386bb7f69bb46b900fed0460eb68c54f99895",
+ "archive": "java_tools-v16.0.zip",
+ "sha256": "6f036ca629f3e30f3c32527e447647aff00b4c2cf1e3ff57635ee49b6e17c8d5",
"urls": [
- "https://mirror.bazel.build/bazel_java_tools/releases/java/v11.2/java_tools-v11.2.zip",
- "https://github.com/bazelbuild/java_tools/releases/download/java_v11.2/java_tools-v11.2.zip",
+ "https://github.com/davido/java_tools/releases/download/v16.0/java_tools_linux-v16.0.zip",
],
"used_in": [
"additional_distfiles",
I can build gerrit with JDK 16 from this change: [1]:
And can confirm, that major byte code version is 60, as expected:
[1] https://gerrit-review.googlesource.com/c/gerrit/+/301362 |
Could you please upload JDK 16 archives to Bazel mirror? |
d064ee3
to
6706428
Compare
e66b2fc
to
b85dcc9
Compare
b85dcc9
to
3211800
Compare
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.
Would you mind also adding "16" to the JAVA_VERSIONS in src/test/shell/BUILD? Those tests have much better coverage and will make sure JDK16 works.
nit: there are some "14" left in the tests and those test fail because JDK14 was removed. If you also care replacing them with "15" or removing them.
Thanks for the review.
After adding the "16" to the
And this diff fixed it: diff --git a/tools/jdk/default_java_toolchain.bzl b/tools/jdk/default_java_toolchain.bzl
index fee344a45f..ebd2b16f0d 100644
--- a/tools/jdk/default_java_toolchain.bzl
+++ b/tools/jdk/default_java_toolchain.bzl
@@ -26,6 +26,7 @@ BASE_JDK9_JVM_OPTS = [
"--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED",
+ "--add-exports=jdk.compiler/com.sun.tools.javac.resources=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
Done. |
3211800
to
13430e5
Compare
This test is failing: //src/test/shell/bazel:bazel_coverage_java_jdk16_toolchain_released_test FAILED in 104.9s I can reproduce it locally but it's hard to see what is the root cause?! |
I think I see what's going on. I extracted a reproducer repository for coverage based on the test: https://github.com/davido/bazel_coverage_src_and_test_same_package And all I changed is added these lines to cat .bazelrc
Now, running coverage with new
IOW, currently used jacoco distribution on Bazel@HEAD doesn't support Java 16 yet. Bazel is still using JaCoCo 0.8.3: cat third_party/java/jacoco/BUILD
While the experimental support for Java 16 was added in release 0.8.6: [1] in this PR: [2]. [1] https://github.com/jacoco/jacoco/releases/tag/v0.8.6 |
Thanks for the pointer, I see, that
should just pass?! |
Closes bazelbuild/java_tools#49 Closes #11674 Unblocks #13270, #13274. Closes #13343. PiperOrigin-RevId: 368428107
13430e5
to
8d6288f
Compare
Thanks for bumping If it works, could you merge it and release yet another |
No need. None of the files in this PR are a part of java_tools. I reorganised this a while ago to make things easier. We merge this PR and you're done. |
Great news. How can I activate the new toolchain built from this PR in bazel? I'm asking because coverage Java 16 tests are still failing. Even though, I've rebased this PR on Bazel@HEAD. |
I can reproduce the failure locally, and see this if I try to run coverage with Java 16 toochain:
|
If I disable the sandbox, it doesn't find Jacoco either:
Looking into the directory structure, it's here:
|
Just using --{,tool_}java_language_version=16, --{,tool_}java_runtime_version=16 flags. I can reproduce the same problems locally. They are weird though:
Possibly connected to some new visibility/exports requirement in JDK16? Would you mind disabling coverage test on JDK16, opening a separate issue for it and pushing this PR forward? |
Sure, will update PR in a moment and open coverage issue for JDK 16. Thanks! |
Closes bazelbuild#13270. Change-Id: I215c2f86a35f87bea0102cf6da5248b19a319d3e
8d6288f
to
a724f85
Compare
Done. PTAL. |
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.
Perfect, thanks!
Closes #13270.