[Gradle] Refactor Java toolchain selection using Gradle toolchain management #34915
Conversation
sjvanrossum
force-pushed
the
gradle-toolchain-management
branch
from
421dd8d to
96f6c82
Compare
sjvanrossum
changed the title
|
Assigning reviewers: R: @tvalentyn for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34915 +/- ##
============================================
+ Coverage 54.54% 56.48% +1.94%
- Complexity 1479 3301 +1822
============================================
Files 1011 1183 +172
Lines 160536 181700 +21164
Branches 1079 3409 +2330
============================================
+ Hits 87561 102637 +15076
- Misses 70874 75796 +4922
- Partials 2101 3267 +1166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
R: @Abacn who probably has most context on Java tooling. |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
changes in Plan to push this PR to a separate branch upstream then manually trigger some tests to confirm it works |
|
@Abacn thanks! There are a few more places where Gradle is not called through
The playground, code completion and tour of Beam flows don't seem to be affected by this PR, but the remaining flows certainly are. In a few cases the job steps write to ~/.m2/settings.xml which would subsequently be deleted by the gradle action and I wasn't sure if those jobs were purposely running custom steps or if they simply hadn't been updated yet. I'm taking another look at tasks that run sdks/go/test/run_validatesrunner_tests.sh since those tasks are not passing on the toolchain configuration to the script's invocations of Gradle and will likely break when |
| @@ -70,11 +70,16 @@ runs: | |||
| tox-${{ runner.os }}-py${{ inputs.python-version == 'default' && '39' || inputs.python-version }}- | |||
|
|
|||
| - name: Install Java | |||
| if: ${{ inputs.java-version != '' }} | |||
| if: ${{ inputs.java-version != 'default' && '11' }} | |||
There was a problem hiding this comment.
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.
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):
using configure arguments '--with-jvm-variants=server --with-boot-jdk=/usr/lib/jvm/java-11-openjdk-amd64 --disable-precompiled-headers --with-jvm-features=zgc,shenandoahgc --with-extra-cflags='-Wdate-time -D_FORTIFY_SOURCE=2 -g -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -fno-stack-protector -Wno-deprecated-declarations -Wdate-time -D_FORTIFY_SOURCE=2' --with-extra-cxxflags='-Wdate-time -D_FORTIFY_SOURCE=2 -g -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -fno-stack-protector -Wno-deprecated-declarations -Wno-deprecated-declarations' --with-extra-ldflags='-Xlinker -z -Xlinker relro -Xlinker -Bsymbolic-functions -Xlinker --no-as-needed' --enable-dtrace --disable-ccache --with-jtreg=/usr/share/jtreg --with-vendor-name=Ubuntu --with-vendor-url=https://ubuntu.com/ --with-vendor-bug-url=https://bugs.launchpad.net/ubuntu/+source/openjdk-lts --with-vendor-vm-bug-url=https://bugs.launchpad.net/ubuntu/+source/openjdk-lts --with-version-pre= --with-version-build=6 --with-version-opt='post-Ubuntu-0ubuntu1~20.04' --with-copyright-year=2025 --with-debug-level=release --with-native-debug-symbols=external --enable-unlimited-crypto --with-zlib=system --with-giflib=system --with-libpng=system --with-libjpeg=system --with-lcms=system --with-pcsclite=system --disable-warnings-as-errors --disable-javac-server --with-harfbuzz=system --with-stdc++lib=dynamic --with-num-cores=4'.
Adoptium's Temurin OpenJDK 11 build is configured with (build logs):
using configure arguments '--verbose --with-vendor-name='Eclipse Adoptium' --with-vendor-url=https://adoptium.net/ --with-vendor-bug-url=https://github.com/adoptium/adoptium-support/issues --with-vendor-vm-bug-url=https://github.com/adoptium/adoptium-support/issues --with-version-opt=202504181134 --with-version-pre=beta --with-version-build=6 --with-vendor-version-string=Temurin-11.0.27+6-202504181134 --with-boot-jdk=/usr/lib/jvm/jdk10 --with-jvm-features=shenandoahgc --with-debug-level=release --enable-ccache --with-alsa=/home/jenkins/workspace/./build//installedalsa --with-jvm-variants=server --with-cacerts-file=/home/jenkins/sbin/../security/cacerts --disable-warnings-as-errors --with-native-debug-symbols=none --with-zlib=bundled'.
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.
It's using zulu jdk 11 actually -
OpenJDK it's also subtly different from the Temurin release
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
| @@ -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.
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.
also, if we keep "testJavaVersion" property these tests are going to pass on current PR
There was a problem hiding this comment.
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
The way I wired it up makes it affect the JDK selection for all JavaCompile, JavaExec and Test tasks, but not the JDK selection used for buildSrc since that uses the same JDK as the running Gradle process.
BeamModulePlugin.groovy L1023:
project.java {
toolchain {
languageVersion = JavaLanguageVersion.of(project.javaVersion)
}
}
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).
The javaVersion property sets the default for all compilations and executions, but individual tasks can modify the toolchain spec as needed.
runners/google-cloud-dataflow-java/examples/build.gradle L150:
task postCommitLegacyWorkerJava8(type: Test) {
javaLauncher = javaToolchains.launcherFor {
languageVersion = JavaLanguageVersion.of(8)
}
dependsOn ":runners:google-cloud-dataflow-java:worker:shadowJar"
def dataflowWorkerJar = project.findProperty('dataflowWorkerJar') ?: project(":runners:google-cloud-dataflow-java:worker").shadowJar.archivePath
with commonConfig(dataflowWorkerJar: dataflowWorkerJar, runWordCount: 'only')
}
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:
tasks.withType(Test).configureEach {
// TODO(https://github.com/apache/beam/issues/32189) remove when embedded hive supports Java11
// Also change .github/workflows/beam_PostCommit_Java_Hadoop_Versions.yml
javaLauncher = javaToolchains.launcherFor {
languageVersion = JavaLanguageVersion.of(8)
}
}
Given that the distributed JARs target Java 8 as the minimum supported language version the compilation output for any variation of javaVersion should be close to the output generated by the compiler that shipped in JDK 8, aside from compiler bugs like JDK-8204322. Executing those JARs might behave differently for variations of javaVersion (e.g.
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.
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
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 --release flag should make the current compiler behave much like the targeted release compiler, it may still generate additional warnings (which are treated as errors by this build configuration) or trip on a compiler bug. Using a single compiler version would hide these issues until the next time this build configuration changes the minimum supported JDK version.
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.
it affect the JDK selection for all JavaCompile, JavaExec and Test tasks
Using a single compiler version would hide these issues until the next time this build configuration changes the minimum supported JDK version.
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.
I think the next step would be to use the test suite plugin to copy the default test test suite target and configure each Test task to override javaLauncher, producing :path:to:project:test8 and :path:to:project:test11 for example.
Yeah, this sounds a good idea replacing -PtestJavaVersion=xx -PjavaxxHome that is used currently in java github action workflows involving testing java version
| @@ -83,6 +83,8 @@ jobs: | |||
| run: | | |||
| gcloud auth configure-docker us.gcr.io | |||
| - name: run goPostCommitDataflowARM script | |||
| run: ./gradlew :goPostCommitDataflowARM | |||
| uses: ./.github/actions/gradle-command-self-hosted-action | |||
There was a problem hiding this comment.
The pure ./gradlew .... command should still work? What would happen if -Porg.gradle.java.installations.auto-detect=false and -Porg.gradle.java.installations.auto-download=false aren't present? If these options are needed in all projects. We can just set it in gradle.properties
There was a problem hiding this comment.
I've set org.gradle.java.installations.auto-download=false and org.gradle.java.installations.auto-detect=true in gradle.properties, but I recall that Gradle was unable to find any toolchains (regardless of org.gradle.java.installations.auto-detect) on the GitHub Actions runner so manually setting one of org.gradle.java.installations.fromEnv, org.gradle.java.installations.idea-jdks-directory, org.gradle.java.installations.maven-toolchains-file or org.gradle.java.installations.paths still seems to be necessary.
| if (project.hasProperty("testJavaVersion")) { | ||
| var testVer = project.property("testJavaVersion") | ||
|
|
||
| tasks.getByName("javaPreCommitPortabilityApi").dependsOn(":sdks:java:testing:test-utils:verifyJavaVersion$testVer") |
There was a problem hiding this comment.
I think we want to preserve JVM verification tests exercised when testJavaVersion is specified. Admittedly this place is not a good place to config. We can keep it for now or move to a more relevant place
There was a problem hiding this comment.
I guess we could add this task dependency unconditionally then, would you agree?
| // If not publishing ,disable the project. Otherwise, fail the build | ||
| def msg = "project ${project.name} needs newer Java version to compile. Consider set -Pjava${project.javaVersion}Home" | ||
| if (publishEnabledByCommand) { | ||
| throw new GradleException("Publish enabled but " + msg + ".") |
There was a problem hiding this comment.
I think we want to preserve the logic that
-
when not publishing, unsupported projects (requires higher java version than currently have) are skipped
-
when publishing enabled, unsupported projects (requires higher java version than currently have) should fail
Currently tested that ./gradlew :sdks:java:container:agent:build fails on Java8. It should skip.
There was a problem hiding this comment.
I added a commit to select the minimum language version when the requested language version is incompatible. When a suitable toolchain can't be discovered this will still result in a build failure.
> Configure project :sdks:java:container:agent
Requested a toolchain for language version 11 because a toolchain for language version 8 is incompatible with the minimum supported language version.
FAILURE: Build failed with an exception.
* What went wrong:
Could not determine the dependencies of task ':sdks:java:container:agent:compileJava'.
> Failed to query the value of extension 'errorprone' property 'enabled'.
> Failed to calculate the value of task ':sdks:java:container:agent:compileJava' property 'javaCompiler'.
> No matching toolchains found for requested specification: {languageVersion=11, vendor=any, implementation=vendor-specific} for MAC_OS on aarch64.
> No locally installed toolchains match and toolchain auto-provisioning is not enabled.
* Try:
> Learn more about toolchain auto-detection at https://docs.gradle.org/8.4/userguide/toolchains.html#sec:auto_detection.
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Get more help at https://help.gradle.org.
BUILD FAILED in 7s
If a compatible toolchain can be discovered (e.g. requested Java 8, but provided both Java 8 and Java 11 toolchain locations) then it still prints a warning, but the build will succeed with a successful build of the affected project instead of skipping it. The requested build target depends on that incompatible target so we should ensure that it builds.
> Configure project :sdks:java:container:agent
Requested a toolchain for language version 11 because a toolchain for language version 8 is incompatible with the minimum supported language version.
BUILD SUCCESSFUL in 8s
There was a problem hiding this comment.
Thanks. with this change GitHub Action now passed.
However my original question remains. ./gradlew :sdks:java:container:agent:build on java8 still failing. My point is we don't want it to fail. User or IDE (especially true for IntelliJ) often invoke a single ./gradlew <command> like ./gradlew build instead of specifying module. Currently it works if only a single java8/11 installed. I don't think we want to break them.
| @@ -2690,7 +2502,7 @@ class BeamModulePlugin implements Plugin<Project> { | |||
| for (path in config.expansionProjectPaths) { | |||
| dependsOn project.project(path).shadowJar.getPath() | |||
| } | |||
| dependsOn ":sdks:java:container:$javaContainerSuffix:docker" | |||
There was a problem hiding this comment.
Here the docker file is consumed by Python test, where the docker java version is expected to be the same as the hosts' shell (PATH) java version, not the gradle project java version. Otherwise those test will fail due to cannot pull apache/beam_javaxx_sdk:2.66.0dev (due to it only built locally)
| def setupTask = project.tasks.register(config.name+"Setup", Exec) { | ||
| dependsOn ':sdks:java:container:'+javaContainerSuffix+':docker' |
| @@ -49,26 +49,6 @@ public void verifyCodeIsCompiledWithJava8() throws IOException { | |||
| assertEquals(v1_8, getByteCodeVersion(DoFn.class)); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Related to the comments above, we ought to have these tests exercised to make sure the tests are running on intended java version when specified
…mLanguageVersion instead of javaVersion
This refactor intends to replace manual toolchain management with Gradle's built-in toolchain management.
testJavaVersionis removed in favor ofjavaVersionwhich sets the default for all Java tasks and can be overridden on tasks usingjavaCompilerforJavaCompiletasks orjavaLauncherforJavaExecandTesttasks.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.