Skip to content
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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 31, 2023
Merged

fix: fix kokoro windows java8 ci #2573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4e7523
chore: fix kokoro windows java8 ci
harshachinta Aug 7, 2023
6eea943
chore: update version
harshachinta Aug 7, 2023
ca52276
chore: update file
harshachinta Aug 7, 2023
9f44aae
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .kokoro/build.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,4 @@
:: downstream client libraries before they are released.
:: See documentation in type-shell-output.bat

set JAVA8_HOME=%JAVA_HOME:"=%
choco install -y openjdk11
set JAVA11_HOME=C:\Program Files\Eclipse Adoptium\jdk-11.0.18.10-hotspot\

Comment on lines -18 to -21
Copy link
Contributor Author

@harshachinta harshachinta Aug 7, 2023 •
edited
Loading

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

java-spanner/.kokoro/build.sh

Lines 53 to 54 in 08af671

if [ -n "${JAVA8_HOME}" ]; then
setJava "${JAVA8_HOME}"

java-spanner/.kokoro/build.sh

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?

Copy link
Contributor

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..

Copy link
Collaborator

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

Copy link
Contributor

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

java-spanner/.kokoro/build.sh

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?

Copy link
Contributor

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.

Copy link
Collaborator

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

"C:\Program Files\Git\bin\bash.exe" %~dp0build.sh
8 changes: 4 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,20 @@ If you are using Maven without the BOM, add this to your dependencies:
If you are using Gradle 5.x or later, add this to your dependencies:

```Groovy
implementation platform('com.google.cloud:libraries-bom:26.21.0')
implementation platform('com.google.cloud:libraries-bom:26.22.0')

implementation 'com.google.cloud:google-cloud-spanner'
```
If you are using Gradle without BOM, add this to your dependencies:

```Groovy
implementation 'com.google.cloud:google-cloud-spanner:6.44.0'
implementation 'com.google.cloud:google-cloud-spanner:6.45.3'
```

If you are using SBT, add this to your dependencies:

```Scala
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.44.0"
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.45.3"
```
<!-- {x-version-update-end} -->

Expand Down Expand Up @@ -430,7 +430,7 @@ Java is a registered trademark of Oracle and/or its affiliates.
[kokoro-badge-link-5]: http://storage.googleapis.com/cloud-devrel-public/java/badges/java-spanner/java11.html
[stability-image]: https://img.shields.io/badge/stability-stable-green
[maven-version-image]: https://img.shields.io/maven-central/v/com.google.cloud/google-cloud-spanner.svg
[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-spanner/6.44.0
[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-spanner/6.45.3
[authentication]: https://github.com/googleapis/google-cloud-java#authentication
[auth-scopes]: https://developers.google.com/identity/protocols/oauth2/scopes
[predefined-iam-roles]: https://cloud.google.com/iam/docs/understanding-roles#predefined_roles
Expand Down
Loading