Add Java 17 to docker image for self-hosted runners #29992
Conversation
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
|
build-and-version-runner failing "No credentials detected, skipping authentication". However, the same action on other workflow always pass. Found this action was run on |
|
The failure is because its using pull_request instead of pull_request_target so the secret isn't available. I'll put up a fix in a moment. |
|
#29995 should fix the issue. |
Definitely pro-adding stuff to the image if we're going to use it often |
|
@kennknowles if you pull in the latest from master this should work now |
| @@ -41,6 +41,9 @@ RUN curl -OL https://cdn.azul.com/zulu/bin/zulu8.70.0.23-ca-jdk8.0.372-linux_x64 | |||
| tar -C /usr/local -xzf zulu8.70.0.23-ca-jdk8.0.372-linux_x64.tar.gz && \ | |||
| rm zulu8.70.0.23-ca-jdk8.0.372-linux_x64.tar.gz && \ | |||
| mv /usr/local/zulu8.70.0.23-ca-jdk8.0.372-linux_x64 /usr/local/java | |||
| RUN curl -OL https://cdn.azul.com/zulu/bin/zulu17.46.19-ca-jdk17.0.9-linux_x64.tar.gz && \ | |||
| tar -C /usr/local -xzf https://cdn.azul.com/zulu/bin/zulu17.46.19-ca-jdk17.0.9-linux_x64.tar.gz && \ | |||
| rm zulu17.46.19-ca-jdk17.0.9-linux_x64.tar.gz | |||
There was a problem hiding this comment.
Do we need something like mv /usr/local/zulu8.70.0.23-ca-jdk8.0.372-linux_x64 /usr/local/java above in this line as well so that this is available on PATH rather than the current directory?
There was a problem hiding this comment.
I'm not sure how this works, but this is a thing:
% ./gradlew -q javaToolchains
+ Options
| Auto-detection: Enabled
| Auto-download: Enabled
+ Azul Zulu JDK 11.0.21+9-LTS
| Location: /Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home
| Language Version: 11
| Vendor: Azul Zulu
| Architecture: x86_64
| Is JDK: true
| Detected by: MacOS java_home
+ Azul Zulu JDK 20.0.2+9
| Location: /Library/Java/JavaVirtualMachines/zulu-20.jdk/Contents/Home
| Language Version: 20
| Vendor: Azul Zulu
| Architecture: x86_64
| Is JDK: true
| Detected by: MacOS java_home
+ Azul Zulu JDK 21.0.1+12-LTS
| Location: /Library/Java/JavaVirtualMachines/zulu-21.jdk/Contents/Home
| Language Version: 21
| Vendor: Azul Zulu
| Architecture: x86_64
| Is JDK: true
| Detected by: Current JVM
+ Oracle JDK 1.8.0_221-b11
| Location: /Library/Java/JavaVirtualMachines/jdk1.8.0_221.jdk/Contents/Home
| Language Version: 8
| Vendor: Oracle
| Architecture: x86_64
| Is JDK: true
| Detected by: MacOS java_home
+ Oracle JRE 1.8.0_221-b11
| Location: /Library/Internet Plug-Ins/JavaAppletPlugin.plugin/Contents/Home
| Language Version: 8
| Vendor: Oracle
| Architecture: x86_64
| Is JDK: false
| Detected by: MacOS java_home
Which makes me think it may be that we just select them directly. Or just set JAVA_HOME directly to wherever we unpack it, etc.
There was a problem hiding this comment.
I know setup-java will setup toolchains like this. I'd expect if Java was already on the path it wouldn't reinstall it (though I don't know for sure) and would just set up the toolchain. You could take a look at the current java 8 runs in the setup-java step and see what's happening. Throwing it into a random folder probably will not help us by itself though.
There was a problem hiding this comment.
I don't have strong opinions on that. Mostly I expect that we should set up the java version explicitly in all cases. Maybe there's a "normal" way to set things up so that by specifying version "17" you get it to resolve to zulu17.46.19-ca-jdk17.0.9
There was a problem hiding this comment.
I know setup-java will setup toolchains like this. I'd expect if Java was already on the path it wouldn't reinstall it (though I don't know for sure) and would just set up the toolchain.
Currently the java environment is an optional parameter of the setup-environment-action action. If not provided, the action runs on this pre-installed java version, e.g.
. If provided, it will use the (back of the list) configured java version as the default java command, e.g.:There was a problem hiding this comment.
This looks to be potentially a totally different distribution?
I presume that action can be invoked repeatedly with multiple versions and they each get installed into some non-overlapping places.
There was a problem hiding this comment.
Which is to say that what I'm trying to do on the image is just make it idempotent w.r.t. actions/setup-java and I would love your advice on the best way to do that. Probably having this script in concordance with actions/setup-java is more of a goal than having it match the code it is copy/pasted from.
There was a problem hiding this comment.
I presume that action can be invoked repeatedly with multiple versions and they each get installed into some non-overlapping places.
Correct
Probably having this script in concordance with actions/setup-java is more of a goal than having it match the code it is copy/pasted from.
Yeah, that's right. Looking at https://github.com/actions/setup-java/blob/7a445ee88d4e23b52c33fdc7601e40278616c7f8/src/setup-java.ts#L71 the key thing is can we find it in the tool-cache - https://github.com/actions/setup-java/blob/7a445ee88d4e23b52c33fdc7601e40278616c7f8/src/distributions/base-installer.ts#L49
We're not actually doing that correctly with our other versions either. Looking at https://github.com/apache/beam/actions/runs/7585699321/job/20662163515 maybe it just doesn't matter though. It takes all of 6 seconds to download/extract Java 11. So I guess we don't need this, the primary value here is just making sure we have some ok default version of Java downloaded + available on the path so that every job doesn't need to specify a java-version.
I guess my take is that this just isn't actually worth it until we commit to making Java 17 the default, at which point we should just replace the java 8 download/extraction
There was a problem hiding this comment.
FWIW I expected this to have a bigger impact on build times, but it just doesn't appear to be the case.
There was a problem hiding this comment.
OK if it only takes 6 seconds then this isn't a prerequisite for moving ahead with version upgrades.
To be clear, this was a step in the process of migrating to 17 as the default for CI, and from there bumping up the minimum version for build so that we could keep up with build tools that have dropped Java 8 support. Aka I think we can commit.
kennknowles
force-pushed
the
java17-docker
branch
from
c5eb203
to
e2b2c11
Compare
|
I wasn't sure if it was necessary to rebase for the GHA re-run to get the changes from #29995 or if the merge commit would be used, but I rebased and force pushed anyhow. |
|
OK the failure is a silly copy/paste problem - one moment |
kennknowles
force-pushed
the
java17-docker
branch
from
e2b2c11
to
610d6e0
Compare
Any commit is fine, but the merge commit needed to be regenerated (GHA will always reuse the old one on workflow retries) |
|
I'd probably decouple "default for CI" from "is already installed" so that we can toggle default for CI just by editing a version somewhere. |
This should make Java 17 available for java jobs to use without an extra download step. Then we can migrate them and remove Java 8 from the image. The goal is to build with gradle and plugins using this version of Java, and probably javac and all that also using this version but targeting Java 8 platform.
Feedback welcome on the tradeoff between preloading this stuff onto the image vs downloading dynamically (as we seem to do for non-default versions), how to migrate to it being default, etc.
Particularly I might eliminate the concept of it being symlinked to /usr/local/java and always use explicit versions when it comes to the filesystem layer.
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.