THRIFT-5699: java lib and build tool chain: gradle 8.0.2 #2779

jimexist · 2023-04-09T07:57:46Z

upgrade gradle to version 8
~~upgrade toolchain java version to 19~~
upgrade shadow plugin due to incompatible gradle dependency

note this does not change built java library version which is pinned to 11

Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
Did you squash your changes to a single commit? (not required, but preferred)
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

fishy · 2023-04-09T18:13:07Z

.github/workflows/build.yml

    steps:
      - uses: actions/checkout@v3

      - uses: actions/setup-java@v3
        with:
          distribution: temurin
-          java-version: 17
+          java-version: 19


19 is not an LTS version, we should stick with LTS versions?

for maintainability i think we should stick with 11 for released jars (which is guaranteed using -release flag). i trust JDK's capability of down-compiling with newer Java versions.

for build tool chain (CI) i think it is okay to move along with newest stable release.

the -release flag was introduced by @ctubbsii

my main concern is that someone accidentally introduced some code that requires feature not available in LTS versions, but if that's already guaranteed by -release flag then I think this is fine.

the other concern is that the non-LTS versions all have a relatively much shorter support lifespan so we need to update those to supported versions much more frequently. For example, according to https://www.oracle.com/java/technologies/java-se-support-roadmap.html 19 is already out of support?

we can move to java 21 in September but atm i think it is fine given there's no much of a support needed, given the compiled .class files are in Java 11. The only support needed is in the build system in this repo.

out of support would also mean that it won't get security fixes, so if a security bug is discovered in jdk 19 now the fix would only land on 20 and LTS versions, not 19, I assume? But maybe Oracle's "sustaining support" means it would still get security fixes?

if we are not using any new features introduced since 17 (the latest LTS version right now), I'm not sure why do we need to use java 19 here?

this is to check if we are prepared to run build/tests with upcoming releases.
I would recommend to use 20 here

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.

fishy · 2023-04-11T20:54:30Z

.github/workflows/build.yml

    steps:
      - uses: actions/checkout@v3

      - uses: actions/setup-java@v3
        with:
          distribution: temurin
-          java-version: 17
+          java-version: 19


out of support would also mean that it won't get security fixes, so if a security bug is discovered in jdk 19 now the fix would only land on 20 and LTS versions, not 19, I assume? But maybe Oracle's "sustaining support" means it would still get security fixes?

if we are not using any new features introduced since 17 (the latest LTS version right now), I'm not sure why do we need to use java 19 here?

slachiewicz · 2023-04-11T22:24:12Z

doc/install/README.md

@@ -28,8 +28,8 @@ These are only required if you choose to build the libraries for the given langu
    * zlib (optional)
    * Qt (optional)
 * Java
-    * Java 17
-    * Gradle 7.6
+    * Java 19


please leave 11/17 here

slachiewicz · 2023-04-11T22:25:14Z

lib/java/gradle/unitTests.gradle

@@ -65,7 +65,7 @@ test {
        outputs.upToDateWhen { false }
    }

-    // This is required for Mockito to run under Java 17
+    // This is required for Mockito to run under Java 19


leave as "17+"

slachiewicz · 2023-04-11T22:29:33Z

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)


so with this definition - java defined with github actions setup is onoy used for Gradle process and not to build/run tests?

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?

ctubbsii · 2023-04-11T23:29:23Z

.github/workflows/build.yml

    steps:
      - uses: actions/checkout@v3

      - uses: actions/setup-java@v3
        with:
          distribution: temurin
-          java-version: 17
+          java-version: 19


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.

ctubbsii · 2023-04-11T23:31:54Z

.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


Could omit the version here, so it doesn't need to be updated so often. Just say:

Suggested change

# here we intentionally use an older version so that we also verify java 19 compiles to it

# intentionally use Java 11 here to ensure that the newer JDK builds for Java 11

ctubbsii · 2023-04-11T23:36:09Z

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)


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?

jimexist · 2023-04-12T13:47:35Z

@fishy @ctubbsii java version rolled back to 17

Fokko · 2023-04-14T08:40:41Z

Nice, thanks for picking this up @jimexist

jimexist added 4 commits April 9, 2023 15:55

use gradle 8.0.2

87eca8c

use java 19

c57b962

update shasum

e62e049

fix org.gradle.api.tasks.bundling.Jar

f818979

Jens-G added the java Pull requests that update Java code label Apr 9, 2023

update shadow plugin

a3f89f8

jimexist requested review from fishy and ctubbsii April 9, 2023 17:07

fishy reviewed Apr 9, 2023

View reviewed changes

fishy approved these changes Apr 11, 2023

View reviewed changes

slachiewicz reviewed Apr 11, 2023

View reviewed changes

ctubbsii reviewed Apr 11, 2023

View reviewed changes

jimexist added 2 commits April 12, 2023 21:44

roll back to use java 17

abaeb38

update doc

628f33c

jimexist changed the title ~~java lib and build tool chain: gradle 8.0.2 and JDK 19~~ java lib and build tool chain: gradle 8.0.2 Apr 12, 2023

jimexist changed the title ~~java lib and build tool chain: gradle 8.0.2~~ THRIFT-5699: java lib and build tool chain: gradle 8.0.2 Apr 12, 2023

jimexist mentioned this pull request Apr 13, 2023

THRIFT-5671: return dependencies to POM #2782

Merged

5 tasks

ctubbsii approved these changes Apr 13, 2023

View reviewed changes

jimexist merged commit be73a57 into apache:master Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-5699: java lib and build tool chain: gradle 8.0.2 #2779

THRIFT-5699: java lib and build tool chain: gradle 8.0.2 #2779

jimexist commented Apr 9, 2023 •

edited

Loading

fishy Apr 9, 2023

jimexist Apr 10, 2023 •

edited

Loading

jimexist Apr 10, 2023

fishy Apr 10, 2023 •

edited

Loading

jimexist Apr 11, 2023 •

edited

Loading

fishy Apr 11, 2023

slachiewicz Apr 11, 2023

ctubbsii Apr 11, 2023

fishy Apr 11, 2023

slachiewicz Apr 11, 2023

slachiewicz Apr 11, 2023

slachiewicz Apr 11, 2023

ctubbsii Apr 11, 2023

ctubbsii Apr 11, 2023

ctubbsii Apr 11, 2023

ctubbsii Apr 11, 2023

jimexist commented Apr 12, 2023

Fokko commented Apr 14, 2023

	# here we intentionally use an older version so that we also verify java 19 compiles to it
	# intentionally use Java 11 here to ensure that the newer JDK builds for Java 11

THRIFT-5699: java lib and build tool chain: gradle 8.0.2 #2779

THRIFT-5699: java lib and build tool chain: gradle 8.0.2 #2779

Conversation

jimexist commented Apr 9, 2023 • edited Loading

Choose a reason for hiding this comment

jimexist Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

jimexist Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimexist commented Apr 12, 2023

Fokko commented Apr 14, 2023

jimexist commented Apr 9, 2023 •

edited

Loading

jimexist Apr 10, 2023 •

edited

Loading

fishy Apr 10, 2023 •

edited

Loading

jimexist Apr 11, 2023 •

edited

Loading