You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Jimexist (via GitHub)" <gi...@apache.org> on 2023/04/09 07:57:48 UTC
[GitHub] [thrift] Jimexist opened a new pull request, #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Jimexist opened a new pull request, #2779:
URL: https://github.com/apache/thrift/pull/2779
<!-- Explain the changes in the pull request below: -->
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), 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.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1162166841
##########
.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:
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 no longer supported?
##########
.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:
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?
--
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
[GitHub] [thrift] Jimexist commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1161388525
##########
.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:
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.
--
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
[GitHub] [thrift] slachiewicz commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1163390284
##########
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
Review Comment:
please leave 11/17 here
--
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
[GitHub] [thrift] fishy commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1161322478
##########
.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:
19 is not an LTS version, we should stick with LTS versions?
--
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
[GitHub] [thrift] Jimexist commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1161388525
##########
.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:
for maintainability i think we should stick with 11 (which is guaranteed using `-release` flag). i trust JDK's capability of down-compiling with newer Java versions.
for build tool chain i think it is okay to move along with newest stable release.
--
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
[GitHub] [thrift] ctubbsii commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "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
[GitHub] [thrift] slachiewicz commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1163390867
##########
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
Review Comment:
leave as "17+"
--
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
[GitHub] [thrift] Jimexist commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1162381303
##########
.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:
we can move the 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.
--
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
[GitHub] [thrift] Jimexist merged pull request #2779: THRIFT-5699: java lib and build tool chain: gradle 8.0.2
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist merged PR #2779:
URL: https://github.com/apache/thrift/pull/2779
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1162381303
##########
.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:
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.
--
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
[GitHub] [thrift] slachiewicz commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1163389431
##########
.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:
this is to check if we are prepared to run build/tests with upcoming releases.
I would recommend to use 20 here
--
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
[GitHub] [thrift] Fokko commented on pull request #2779: THRIFT-5699: java lib and build tool chain: gradle 8.0.2
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2779:
URL: https://github.com/apache/thrift/pull/2779#issuecomment-1508150624
Nice, thanks for picking this up @Jimexist
--
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
[GitHub] [thrift] fishy commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1163322147
##########
.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:
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 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
[GitHub] [thrift] Jimexist commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1161388834
##########
.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:
the `-release` flag was introduced by @ctubbsii
--
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
[GitHub] [thrift] slachiewicz commented on a diff in pull request #2779: java lib and build tool chain: gradle 8.0.2 and JDK 19
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz commented on code in PR #2779:
URL: https://github.com/apache/thrift/pull/2779#discussion_r1163393104
##########
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:
so with this definition - java defined with github actions setup is onoy used for Gradle process and not to build/run tests?
--
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
[GitHub] [thrift] Jimexist commented on pull request #2779: THRIFT-5699: java lib and build tool chain: gradle 8.0.2
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2779:
URL: https://github.com/apache/thrift/pull/2779#issuecomment-1505310288
@fishy @ctubbsii java version rolled back to 17
--
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