-
Notifications
You must be signed in to change notification settings - Fork 4.3k
[Gradle] Refactor Java toolchain selection using Gradle toolchain management #34915
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
base: master
Are you sure you want to change the base?
Changes from all commits
96f6c82
357cadd
2ed0a97
49740ac
185a651
5772557
f849bd6
fb5d0fc
71dcc2d
c447529
50733cc
97b012b
4fbb93b
918c068
1579cc1
1ac8f88
99975ce
4aae884
4cbeb65
a98a16f
0a8fffc
e8ffce4
1f0d535
caf3f63
28ad2f6
c86952a
af56d3c
1724dc9
4cd0819
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -95,8 +95,7 @@ jobs: | |||
with: | ||||
gradle-command: :sdks:java:testing:load-tests:run | ||||
arguments: | | ||||
-PtestJavaVersion=${{ matrix.java_version }} \ | ||||
-Pjava${{ matrix.java_version }}Home=$JAVA_HOME_${{ matrix.java_version }}_X64 \ | ||||
-PjavaVersion=${{ matrix.java_version }} \ | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does -PjavaVersion affects the JDK used to compile main sources? Tests running on Java8/17/21 JVM should still have main sources compiled on JDK11, as we only ship jars compiled on java other than JDK11. Relevant test case is running JDK11 compiled classes on different JVM There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, if we keep "testJavaVersion" property these tests are going to pass on current PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The way I wired it up makes it affect the JDK selection for all BeamModulePlugin.groovy L1023:
Much like Maven's toolchain plugin this results in the task requesting the Gradle process to select or discover a JDK fitting the toolchain spec, which could also include requests about the vendor (e.g. Adoptium, IBM, Oracle) or implementation (e.g. J9). runners/google-cloud-dataflow-java/examples/build.gradle L150:
These version-specific tasks aren't necessary, I've only found it necessary to override this property at the tesk level to deal with the HCatalog IO project due to incompatibilities with Java 11. sdks/java/io/hcatalog/build.gradle L109:
Given that the distributed JARs target Java 8 as the minimum supported language version the compilation output for any variation of beam/sdks/java/core/src/test/java/org/apache/beam/sdk/util/common/ReflectHelpersTest.java Line 141 in 5f9cd73
test test suite target and configure each Test task to override javaLauncher , producing :path:to:project:test8 and :path:to:project:test11 for example.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On the other point regarding the current binary distribution process and hence only targeting the compiler that ships with JDK 11, that seems like a sizable blind spot in the testing process to me. While the It also makes it slightly harder for the few (none?) users who may opt to build Beam from source using their own toolchains and container images in organizations with restricted or centralized build infrastructure when they trip on these issues because these workflows don't cover Beam's stated version support range. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Said differently, after this change it adds requirements for Beam to build on all supported Java versions (for components with versioning tests. Previously, it only requires tests build and run on supported Java versions, that is aligned with user adoption scenarios). This significantly increases scope of maintainence. Most importantly it would make new JDK support harder and delayed - when I did JDK21 support, at first Gradle 8.3- didn't support build on JDK21, but the work wasn't blocked by Gradle 8.4 release, and can proceed because we only need to make sure Beam runs on JDK21 fine. It won't be possible after this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, this sounds a good idea replacing |
||||
-PloadTest.mainClass=org.apache.beam.sdk.loadtests.CoGroupByKeyLoadTest \ | ||||
-Prunner=:runners:google-cloud-dataflow-java \ | ||||
'-PloadTest.args=${{ env.beam_LoadTests_Java_CoGBK_Dataflow_V2_Batch_JavaVersions_test_arguments_1 }} --appName=load_tests_Java${{ matrix.java_version }}_Dataflow_V2_batch_CoGBK_1' \ | ||||
|
@@ -105,8 +104,7 @@ jobs: | |||
with: | ||||
gradle-command: :sdks:java:testing:load-tests:run | ||||
arguments: | | ||||
-PtestJavaVersion=${{ matrix.java_version }} \ | ||||
-Pjava${{ matrix.java_version }}Home=$JAVA_HOME_${{ matrix.java_version }}_X64 \ | ||||
-PjavaVersion=${{ matrix.java_version }} \ | ||||
-PloadTest.mainClass=org.apache.beam.sdk.loadtests.CoGroupByKeyLoadTest \ | ||||
-Prunner=:runners:google-cloud-dataflow-java \ | ||||
'-PloadTest.args=${{ env.beam_LoadTests_Java_CoGBK_Dataflow_V2_Batch_JavaVersions_test_arguments_2 }} --appName=load_tests_Java${{ matrix.java_version }}_Dataflow_V2_batch_CoGBK_2' \ | ||||
|
@@ -115,8 +113,7 @@ jobs: | |||
with: | ||||
gradle-command: :sdks:java:testing:load-tests:run | ||||
arguments: | | ||||
-PtestJavaVersion=${{ matrix.java_version }} \ | ||||
-Pjava${{ matrix.java_version }}Home=$JAVA_HOME_${{ matrix.java_version }}_X64 \ | ||||
-PjavaVersion=${{ matrix.java_version }} \ | ||||
-PloadTest.mainClass=org.apache.beam.sdk.loadtests.CoGroupByKeyLoadTest \ | ||||
-Prunner=:runners:google-cloud-dataflow-java \ | ||||
'-PloadTest.args=${{ env.beam_LoadTests_Java_CoGBK_Dataflow_V2_Batch_JavaVersions_test_arguments_3 }} --appName=load_tests_Java${{ matrix.java_version }}_Dataflow_V2_batch_CoGBK_3' \ | ||||
|
@@ -125,8 +122,7 @@ jobs: | |||
with: | ||||
gradle-command: :sdks:java:testing:load-tests:run | ||||
arguments: | | ||||
-PtestJavaVersion=${{ matrix.java_version }} \ | ||||
-Pjava${{ matrix.java_version }}Home=$JAVA_HOME_${{ matrix.java_version }}_X64 \ | ||||
-PjavaVersion=${{ matrix.java_version }} \ | ||||
-PloadTest.mainClass=org.apache.beam.sdk.loadtests.CoGroupByKeyLoadTest \ | ||||
-Prunner=:runners:google-cloud-dataflow-java \ | ||||
'-PloadTest.args=${{ env.beam_LoadTests_Java_CoGBK_Dataflow_V2_Batch_JavaVersions_test_arguments_4 }} --appName=load_tests_Java${{ matrix.java_version }}_Dataflow_V2_batch_CoGBK_4' |
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.
self-hosted runner comes with a java in PATH. We don't install java for most workflow runs, if such workflow not explicitly asked to do
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'm aware and while it is just another build of OpenJDK it's also subtly different from the Temurin release which is packaged in the default container images. On top of that, the default JDK version on Ubuntu 24.04 is their build of OpenJDK 21 and decoupling this build configuration's toolchains from the system toolchains makes it easier to upgrade machine images if needed.
Ubuntu 20.04's OpenJDK 11 build is configured with (build logs):
Adoptium's Temurin OpenJDK 11 build is configured with (build logs):
Most of this shouldn't matter, but it's hard to tell whether Ubuntu's choice to mostly use system libraries, compile with GCC 10 and dynamically linking to stdc++ compared to Temurin's choice to bundle zlib, compile with GCC 7.5 and statically link stdc++ would ever matter in Beam's test suites. If the default JDK for Beam's provided container images is Temurin, then the test suites should at least target that same JDK distribution.
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.
It's using zulu jdk 11 actually -
beam/.github/gh-actions-self-hosted-runners/arc/images/Dockerfile
Line 43 in cbe100a
Yeah this is a good point. Strictly speaking we should use tumerin there
But Beam repo has many other tests doesn't require JDK. It justs need a java runtime to run gradle. I don't think we want to install JDK on every single workflow run (e.g. those python/go/website tests). Arguably Java package is much heavier than Python/Go