[GitHub] [thrift] ctubbsii commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1163423549


##########
.github/workflows/build.yml:
##########
@@ -431,7 +431,7 @@ jobs:
       - uses: actions/setup-java@v3
         with:
           distribution: temurin
-          # here we intentionally use an older version so that we also verify java 17 compiles to it
+          # here we intentionally use an older version so that we also verify java 19 compiles to it

Review Comment:
   Could omit the version here, so it doesn't need to be updated so often. Just say:
   
   ```suggestion
             # intentionally use Java 11 here to ensure that the newer JDK builds for Java 11
   ```



##########
lib/java/gradle/sourceConfiguration.gradle:
##########
@@ -31,7 +31,7 @@
 // also a runtime CI that's based on Java 11 to ensure that.
 java {
     toolchain {
-        languageVersion = JavaLanguageVersion.of(17)
+        languageVersion = JavaLanguageVersion.of(19)

Review Comment:
   I think this forces developers to build using JDK 19. I would prefer not to do that. I think many developers are perfectly content to build using the latest LTS, which I think is still 17. The build should still work with later versions, but I don't think the build should require anything later than 17. I'm not very experienced with Gradle, but I think that's what this does.
   
   Can we bump to the newest Gradle without forcing devs to develop using newer than JDK 17?



##########
.github/workflows/build.yml:
##########
@@ -137,14 +137,14 @@ jobs:
     needs: compiler
     runs-on: ubuntu-20.04
     env:
-      GRADLE_VERSION: "7.6"
+      GRADLE_VERSION: "8.0.2"
     steps:
       - uses: actions/checkout@v3
 
       - uses: actions/setup-java@v3
         with:
           distribution: temurin
-          java-version: 17
+          java-version: 19

Review Comment:
   I don't see any reason to test with the newer JDK that's available. But, I would suggest prioritizing testing against a JDK that developers are more likely to be using... like the current LTS. If we want to build against both, it's pretty easy to set up a Matrix build in GitHub Actions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org