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

Jump to bottom

ci: using JDK 11+ to compile and JDK 8 to run junit (8) #1870

Merged
merged 8 commits into from
May 25, 2022
Merged

ci: using JDK 11+ to compile and JDK 8 to run junit (8) #1870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ba89167
ci: using JDK 11 to compile and JDK 8 to run junit (8)
suztomo May 5, 2022
a18055c
ci: release build to use JDK 11
suztomo May 5, 2022
37f7eb2
Merge remote-tracking branch 'origin/main' into java8-and-java11
suztomo May 20, 2022
100f41e
build: configuring owlbot.py to exclusion
suztomo May 20, 2022
e42a572
ci: windows build for JDK 11 and 8
suztomo May 20, 2022
ac77054
investigating windows environment variables
suztomo May 20, 2022
52a1214
shell: bash for windows
suztomo May 20, 2022
ec67faf
removed debug output
suztomo May 20, 2022
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
32 changes: 30 additions & 2 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
strategy:
fail-fast: false
matrix:
java: [8, 11, 17]
java: [11, 17]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this impact any of the existing test runs against java8? units-java8 job should cover the unit tests, but on fusion, I can see java8-samples as another job.

cc @thiagotnunes

Copy link
Member Author

@suztomo suztomo May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Kokoro builds, I have modified java8 container to have JAVA11_HOME environment variable in googleapis/testing-infra-docker#211.

suztomo@suztomo:~$ docker run --rm -it --entrypoint bash gcr.io/cloud-devrel-kokoro-resources/java8
Unable to find image 'gcr.io/cloud-devrel-kokoro-resources/java8:latest' locally
latest: Pulling from cloud-devrel-kokoro-resources/java8
...
Digest: sha256:86a9e70f109b5efd16204dea51cf30aa2295bbbd4e980d2fd2172d2f7b2382e4
Status: Downloaded newer image for gcr.io/cloud-devrel-kokoro-resources/java8:latest


root@c4a6f4994f6c:/workspace# 
root@c4a6f4994f6c:/workspace# printenv
PYENV_SHELL=bash
HOSTNAME=c4a6f4994f6c
JAVA_HOME=/usr/local/openjdk-8
JAVA8_HOME=/usr/local/openjdk-8
...
JAVA11_HOME=/usr/lib/jvm/java-11-openjdk-amd64
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suztomo We specify the docker image version for java8-samples here: https://github.com/googleapis/java-spanner/blob/main/.kokoro/nightly/java8-samples.cfg#L6

I think this will be alright, but double checking if we set the correct ENV variables in this container as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thiagotnunes Thank you for the info. Yes, that line value: "gcr.io/cloud-devrel-kokoro-resources/java8" matches the container image URL I have modified. It has JAVA8_HOME and JAVA11_HOME now. (I added the docker command invocation to my comment above)

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 info LGTM

steps:
- uses: actions/checkout@v3
- uses: actions/setup-java@v3
Expand All @@ -36,14 +36,42 @@ jobs:
- run: .kokoro/build.sh
env:
JOB_TYPE: test
units-java8:
name: "units (8)"
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-java@v3
with:
java-version: 8
distribution: zulu
- run: echo "JAVA8_HOME=${JAVA_HOME}" >> $GITHUB_ENV
shell: bash
- uses: actions/setup-java@v3
with:
java-version: 11
distribution: zulu
- run: echo "JAVA11_HOME=${JAVA_HOME}" >> $GITHUB_ENV
shell: bash
- run: .kokoro/build.sh
env:
JOB_TYPE: test
windows:
runs-on: windows-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-java@v3
with:
distribution: zulu
java-version: 8
distribution: zulu
- run: echo "JAVA8_HOME=${JAVA_HOME}" >> $GITHUB_ENV
shell: bash
- uses: actions/setup-java@v3
with:
java-version: 11
distribution: zulu
- run: echo "JAVA11_HOME=${JAVA_HOME}" >> $GITHUB_ENV
shell: bash
- run: java -version
- run: .kokoro/build.bat
env:
Expand Down
20 changes: 19 additions & 1 deletion .kokoro/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ cd ${scriptDir}/..
# include common functions
source ${scriptDir}/common.sh

function setJava() {
export JAVA_HOME=$1
export PATH=${JAVA_HOME}/bin:$PATH
}

# units-java8 uses both JDK 11 and JDK 8. GraalVM dependencies require JDK 11 to
# compile the classes touching GraalVM classes.
if [ ! -z "${JAVA11_HOME}" ]; then
setJava "${JAVA11_HOME}"
Comment on lines +33 to +34
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JAVA11_HOME is now available in the container image for Kokoro:

#1870 (comment)

fi

# Print out Maven & Java version
mvn -version
echo ${JOB_TYPE}
Expand All @@ -42,12 +53,19 @@ if [[ ! -z "${GOOGLE_APPLICATION_CREDENTIALS}" && "${GOOGLE_APPLICATION_CREDENTI
export GOOGLE_APPLICATION_CREDENTIALS=$(realpath ${KOKORO_GFILE_DIR}/${GOOGLE_APPLICATION_CREDENTIALS})
fi

# units-java8 uses both JDK 11 and JDK 8. We ensure the generated class files
# are compatible with Java 8 when running tests.
if [ ! -z "${JAVA8_HOME}" ]; then
setJava "${JAVA8_HOME}"
Comment on lines +58 to +59
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JAVA8_HOME is now available in the container image for Kokoro:

#1870 (comment)

mvn -version
fi

RETURN_CODE=0
set +e

case ${JOB_TYPE} in
test)
mvn test -B \
mvn test -B -V \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we need the version of mvn for - does it tie to native image compilation in some way?

Copy link
Member Author

@suztomo suztomo May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clarifies which versions of JDK it's running. We're running JDK 11 to compile and JDK 8 to run tests. (It's nice-to-have)

-Dclirr.skip=true \
-Denforcer.skip=true \
-Djava.net.preferIPv4Stack=true
Expand Down
2 changes: 1 addition & 1 deletion .kokoro/release/common.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ build_file: "java-spanner/.kokoro/trampoline.sh"
# Configure the docker image for kokoro-trampoline.
env_vars: {
key: "TRAMPOLINE_IMAGE"
value: "gcr.io/cloud-devrel-kokoro-resources/java8"
value: "gcr.io/cloud-devrel-kokoro-resources/java11"
}

before_action {
Expand Down
2 changes: 2 additions & 0 deletions owlbot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
".kokoro/presubmit/java8-samples.cfg",
".kokoro/presubmit/java11-samples.cfg",
".kokoro/presubmit/samples.cfg",
".kokoro/release/common.cfg",
"samples/install-without-bom/pom.xml",
"samples/snapshot/pom.xml",
"samples/snippets/pom.xml",
Expand All @@ -40,6 +41,7 @@
".github/release-please.yml",
".github/blunderbuss.yml",
".github/workflows/samples.yaml",
".github/workflows/ci.yaml",
".kokoro/build.sh",
]
)
13 changes: 13 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,17 @@
</plugins>
</reporting>

<profiles>
<profile>
<!-- JDK 9+ has the "release" option to ensure the generated bytecode is
compatible with Java 8. -->
<id>compiler-release-8</id>
Comment on lines +228 to +231
Copy link
Member Author

@suztomo suztomo May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This profile may be appropriate to be defined in the google-cloud-shared-config (the parent pom of this file).

The google-cloud-shared-config defines compiler option:

            <source>1.8</source>
            <target>1.8</target>

https://github.com/googleapis/java-shared-config/blob/3f84da04d9df8d0a114915e8faa8dc2e682e7cb5/pom.xml#L175

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added todo: googleapis/java-shared-config#477

<activation>
<jdk>[9,]</jdk>
</activation>
<properties>
<maven.compiler.release>8</maven.compiler.release>
</properties>
</profile>
</profiles>
</project>