Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Conversation

@suztomo
Copy link
Member

@suztomo suztomo commented May 4, 2022

  • Java 8 build: compiling the code in JDK 11 and running tests in JDK 8.
  • Release build: compiling the code in JDK 11.

@suztomo suztomo requested a review from a team May 4, 2022 18:50
@suztomo suztomo requested review from a team as code owners May 4, 2022 18:50
@suztomo suztomo marked this pull request as draft May 4, 2022 18:50
@suztomo suztomo force-pushed the java11-java8-build branch from d548a55 to 1068273 Compare May 4, 2022 18:51
@suztomo suztomo force-pushed the java11-java8-build branch from 1068273 to 20da203 Compare May 4, 2022 18:52
@suztomo suztomo added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 4, 2022
@suztomo suztomo force-pushed the java11-java8-build branch 2 times, most recently from 088b709 to 96b5ca2 Compare May 4, 2022 19:19
@suztomo suztomo force-pushed the java11-java8-build branch from 96b5ca2 to c4c6999 Compare May 4, 2022 19:20
Comment on lines 25 to 35
- uses: actions/checkout@v3
- uses: actions/setup-java@v3
with:
java-version: 8
distribution: zulu
- run: echo "JAVA8_HOME=${JAVA_HOME}" >> $GITHUB_ENV
- uses: actions/setup-java@v3
with:
java-version: 11
distribution: zulu
- run: echo "JAVA11_HOME=${JAVA_HOME}" >> $GITHUB_ENV
Copy link
Member Author

Choose a reason for hiding this comment

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

This job now requires 2 JDKs.

.kokoro/build.sh Outdated
echo "Compiling using Java:"
java -version
echo
./gradlew compileJava compileTestJava
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't run tests.

.kokoro/build.sh Outdated
Comment on lines 47 to 48
./gradlew build publishToMavenLocal \
--exclude-task compileJava --exclude-task compileTestJava
Copy link
Member Author

Choose a reason for hiding this comment

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

This runs tests without re-compiling.

build.gradle Outdated
Comment on lines 107 to 108
// This flag is only available in JDK 9+
options.compilerArgs.addAll(['--release', '8'])
Copy link
Member Author

Choose a reason for hiding this comment

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

This flag ensures the generated bytecode is compatible with Java 8. (targetCompatibility = 1.8 is not sufficient)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this a source code comment?

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

maven.javax_annotation_javax_annotation_api=javax.annotation:javax.annotation-api:1.3.2
maven.org_graalvm_nativeimage_svm=org.graalvm.nativeimage:svm:22.0.0.2
maven.org_graalvm_sdk=org.graalvm.sdk:graal-sdk:21.3.2
maven.org_graalvm_sdk=org.graalvm.sdk:graal-sdk:22.1.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the dependency that was failing Java 8 build #1669

Copy link
Contributor

Choose a reason for hiding this comment

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

Not actionable: Given the new behavior introduced in 22.1.0, this might cause some of our Kokoro GraalVM builds to be temporarily red.

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed to remove this. This was a test to confirm the build passes. Reverted in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah got it, thanks.

@suztomo suztomo force-pushed the java11-java8-build branch from ae4f9a0 to a377d1d Compare May 4, 2022 19:51
@suztomo
Copy link
Member Author

suztomo commented May 4, 2022

@mpeddada1 @meltsufin It worked. Building the modules in JDK 11 and running tests in Java 8.

ci / units (8) (pull_request) Successful in 2m

@mpeddada1
Copy link
Contributor

That's wonderful news!

@suztomo suztomo removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 10, 2022
@suztomo suztomo marked this pull request as ready for review May 10, 2022 15:34
@suztomo suztomo added the automerge Merge the pull request once unit tests and other checks pass. label May 10, 2022
@suztomo suztomo changed the title ci: java8 build with java11 compilation feat: java8 build with java11 compilation May 10, 2022
@suztomo suztomo changed the title feat: java8 build with java11 compilation ci: java8 build with java11 compilation May 10, 2022
build.gradle Outdated
Comment on lines 107 to 108
// This flag is only available in JDK 9+
options.compilerArgs.addAll(['--release', '8'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this a source code comment?

maven.javax_annotation_javax_annotation_api=javax.annotation:javax.annotation-api:1.3.2
maven.org_graalvm_nativeimage_svm=org.graalvm.nativeimage:svm:22.0.0.2
maven.org_graalvm_sdk=org.graalvm.sdk:graal-sdk:21.3.2
maven.org_graalvm_sdk=org.graalvm.sdk:graal-sdk:22.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Not actionable: Given the new behavior introduced in 22.1.0, this might cause some of our Kokoro GraalVM builds to be temporarily red.

Copy link
Contributor

@mpeddada1 mpeddada1 left a comment

Choose a reason for hiding this comment

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

LGTM. Added a few minor comments!

.kokoro/build.sh Outdated
export PATH=${JAVA_HOME}/bin:$PATH
}

# GraalVM dependencies require to compiling the classes in JDK 11 or higher
Copy link
Member

Choose a reason for hiding this comment

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

Should there be an explicit check that JDK 11 or higher and JDK 8 are configured, and fail otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added source code comment here about how it fails in Java 8.

@suztomo suztomo requested a review from meltsufin May 10, 2022 16:27
@suztomo suztomo force-pushed the java11-java8-build branch from 6eb83ad to 1e21d5b Compare May 10, 2022 17:19
@suztomo suztomo force-pushed the java11-java8-build branch from 1e21d5b to 636c5e5 Compare May 10, 2022 17:22
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

My concern is that if you intend to test with Java 8, but forget to configure it correctly, you won't see any failure. Maybe you can add an argument to the script that specifies compilation JDK and runtime JVM version, so that you can validate that the script actually does what's intended by the caller.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@suztomo
Copy link
Member Author

suztomo commented May 10, 2022

@meltsufin PTAL. Added a check to confirm it's using JDK 8 for running the Java 8 test.

cd24fe5

@suztomo suztomo requested a review from meltsufin May 10, 2022 19:12
@suztomo suztomo merged commit 4483643 into googleapis:main May 10, 2022
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 10, 2022
@suztomo suztomo deleted the java11-java8-build branch May 10, 2022 20:31
@XN137 XN137 mentioned this pull request Jul 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants