-
Notifications
You must be signed in to change notification settings - Fork 123
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
fix: fix kokoro windows java8 ci #2573
fix: fix kokoro windows java8 ci #2573
Conversation
Warning: This pull request is touching the following templated files:
|
set JAVA8_HOME=%JAVA_HOME:"=% | ||
choco install -y openjdk11 | ||
set JAVA11_HOME=C:\Program Files\Eclipse Adoptium\jdk-11.0.18.10-hotspot\ | ||
|
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.
@olavloite, @arpan14, @rajatbhatta
We generally update these jdk versions to latest manually, whenever a windows build fail. But do we need these explicitly getting installed and set JAVA11_HOME?
The build.sh file already sets these JAVA8_HOME
and JAVA11_HOME
Lines 53 to 54 in 08af671
if [ -n "${JAVA8_HOME}" ]; then | |
setJava "${JAVA8_HOME}" |
Lines 28 to 29 in 08af671
if [ -n "${JAVA11_HOME}" ]; then | |
setJava "${JAVA11_HOME}" |
I compared the windows build by removing and having this and it seems it is able to capture the latest JDK by default.
With out above lines
https://screenshot.googleplex.com/8k9CiGKsgEknz8a
With above lines after updating to latest version
https://screenshot.googleplex.com/xBRpe9UXnmXjAPw
Can you please give your views here?
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.
CC: @mpeddada1 for their inputs on this..
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 have no idea why that is explicitly being installed here. But the outcome of the build seems to indicate that we don't need it.
Also; Our JDBC driver build setup also does not include it: https://github.com/googleapis/java-spanner-jdbc/blob/main/.kokoro/build.bat
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 can provide a bit of context here. The graal-sdk dependency, which is used to provide native image configurations requires a minimum version of JDK 11 otherwise it results in a compilation error:
bad class file: C:\Users\kbuilder\.m2\repository\org\graalvm\sdk\graal-sdk\22.3.0\graal-sdk-22.3.0.jar(org/graalvm/nativeimage/hosted/Feature.class)
[ERROR] class file has wrong version 55.0, should be 52.0
Since we still need to verify against JDK 8 for some jobs, this change was made to compile tests against JDK 11 but run them against JDK 8. This logic requires both JAVA11_HOME and JAVA8_HOME env variables to be provided so when they initially weren't included in the Windows java 8 job, we started seeing the compilation error mentioned above (#2205). IIRC, the setJava
method only assigns the PATH
to the JAVA8_HOME
value but doesn't contain any logic to compute it.
@harshachinta in the screenshot, I see that the windows tests are running in JDK 11. Are we no longer validating with JDK 8? If we're only testing with JDK 11 then we should no longer need these lines.
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.
@harshachinta Any thoughts on the last question?
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.
@mpeddada1
I see that we are running tests on JDK 8 and JDK 11.
Line 26 in fbf1df9
# Kokoro integration test uses both JDK 11 and JDK 8. GraalVM dependencies |
Running on JDK 11 - https://screenshot.googleplex.com/87wspyzBrPNqRJE
Running on JDK 8 - https://screenshot.googleplex.com/uFos4MuHKEBYXNg
Note:
However, I observed that the JDK 8 and JDK 11 is already set but we are overwriting them by setting them again in the build.bat file.
java-spanner/.kokoro/build.bat
Line 20 in fbf1df9
set JAVA11_HOME=C:\Program Files\Eclipse Adoptium\jdk-11.0.18.10-hotspot\ |
From the below screenshot the environment variables JAVA8_HOME
and JAVA11_HOME
are already set.
https://screenshot.googleplex.com/7AUaPVjtWkS3ups
We are doing redundant work of downloading them again and in most cases leads to version mismatch due to the old version being hardcoded in the build.bat file
I think we no longer need these and can remove it.
@olavloite @mpeddada1 Can you please let me know your views on this?
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 for the detailed explanation @harshachinta! re I see that we are running tests on JDK 8 and JDK 11.
and the environment variables JAVA8_HOME and JAVA11_HOME are already set
: given this observation, it looks like we no longer need these environment variables.
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 both @harshachinta and @mpeddada1! SGTM
* chore: fix kokoro windows java8 ci * chore: update version * chore: update file * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Windows build fails due to incorrect JAVA11_HOME env. This PR corrects it to latest version to fix the failing window builds.