You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/05/13 01:38:01 UTC
[GitHub] [thrift] Jimexist opened a new pull request, #2606: use gradle toolchain to specify Java 11 with --release 8
Jimexist opened a new pull request, #2606:
URL: https://github.com/apache/thrift/pull/2606
<!-- Explain the changes in the pull request below: -->
use gradle toolchain to specify Java 11 with --release 8
<!-- 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? (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] ctubbsii commented on a diff in pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2606:
URL: https://github.com/apache/thrift/pull/2606#discussion_r874889116
##########
lib/java/gradle/codeQualityChecks.gradle:
##########
@@ -46,9 +42,7 @@ tasks.withType(Pmd) {
spotless {
java {
- target project.fileTree(project.rootDir) {
- include 'src/**/*.java'
- }
+ target 'src/*/java/**/*.java'
Review Comment:
Why not the simpler, `src/**/*.java`?
--
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 #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2606:
URL: https://github.com/apache/thrift/pull/2606#discussion_r871996671
##########
lib/java/src/crossTest/java/org/apache/thrift/test/TestClient.java:
##########
@@ -823,4 +808,10 @@ public static void main(String[] args) {
System.exit(returnCode);
}
+
+ private static byte[] getBytesData() {
+ ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(256);
+ IntStream.range(-128, 128).forEach(byteArrayOutputStream::write);
+ return byteArrayOutputStream.toByteArray();
+ }
Review Comment:
These seem unrelated, but it's a nice improvement. None of this requires Java 11, right?
##########
lib/java/gradle/sourceConfiguration.gradle:
##########
@@ -21,21 +21,28 @@
// ----------------------------------------------------------------------------
// Compiler configuration details
-// These two properties are still needed on JDK8, and possibly used directly by
-// plugins. However, the '--release' option added below makes these two
-// properties redundant when building with JDK9 or later.
-sourceCompatibility = '1.8'
-targetCompatibility = '1.8'
+// We are using Java 11 toolchain to compile.
+// This enables decoupling from the Java version that gradle runs, from
+// the actual JDK version for the project. For more details, see
+// https://docs.gradle.org/current/userguide/toolchains.html
+//
+// The '--release' option added below makes sure that even if we are using
+// the toolchain version > 8, the final artifact is at version 8. There is
+// also a runtime CI that's based on Java 8 to ensure that.
+java {
+ toolchain {
+ languageVersion = JavaLanguageVersion.of(11)
+ }
+}
-tasks.withType(JavaCompile) {
+tasks.withType(JavaCompile).configureEach {
options.encoding = 'UTF-8'
options.debug = true
options.deprecation = true
+ assert JavaVersion.current().isJava11Compatible(), "we are using Java 11 to compile, this is to ensure that"
Review Comment:
```suggestion
assert JavaVersion.current().isJava11Compatible(), "JDK 11 or higher is required to build"
```
--
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 #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2606:
URL: https://github.com/apache/thrift/pull/2606#issuecomment-1125796207
> > we don't need to update the xenial build, since using java toolchain means we are decoupling the JDK used to run gradle from the one used to compile Java. in the case of xenial, JDK8 is used to run gradle, which will download JDK 11 on its own before compiling Java.
>
> Ah, ok. That's convenient. I assumed it worked like Maven toolchains: detected but not installed.
yes there's a section in [here][1] describing the auto provisioning feature in case you are interested
[1]: https://docs.gradle.org/current/userguide/toolchains.html#sec:provisioning
--
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 #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
Jimexist commented on code in PR #2606:
URL: https://github.com/apache/thrift/pull/2606#discussion_r875392943
##########
lib/java/gradle/codeQualityChecks.gradle:
##########
@@ -46,9 +42,7 @@ tasks.withType(Pmd) {
spotless {
java {
- target project.fileTree(project.rootDir) {
- include 'src/**/*.java'
- }
+ target 'src/*/java/**/*.java'
Review Comment:
```suggestion
target 'src/**/*.java'
```
--
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 merged pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #2606:
URL: https://github.com/apache/thrift/pull/2606
--
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] nicolasb29 commented on pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by "nicolasb29 (via GitHub)" <gi...@apache.org>.
nicolasb29 commented on PR #2606:
URL: https://github.com/apache/thrift/pull/2606#issuecomment-1447940260
Hi,
It seems that the version 0.18 is compiled in Java 11 even if --release 8 is present :
- libthrift 0.18 :
javap -verbose org/apache/thrift/transport/THttpClient.class | grep -i major
major version: **55**
- libthrift 0.17 :
javap -verbose org/apache/thrift/transport/THttpClient.class | grep -i major
major version: **52**
https://javaalmanac.io/bytecode/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] ctubbsii commented on pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2606:
URL: https://github.com/apache/thrift/pull/2606#issuecomment-1125793112
> we don't need to update the xenial build, since using java toolchain means we are decoupling the JDK used to run gradle from the one used to compile Java. in the case of xenial, JDK8 is used to run gradle, which will download JDK 11 on its own before compiling Java.
Ah, ok. That's convenient. I assumed it worked like Maven toolchains: detected but not installed.
--
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 #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
Jimexist commented on code in PR #2606:
URL: https://github.com/apache/thrift/pull/2606#discussion_r872000962
##########
lib/java/src/crossTest/java/org/apache/thrift/test/TestClient.java:
##########
@@ -823,4 +808,10 @@ public static void main(String[] args) {
System.exit(returnCode);
}
+
+ private static byte[] getBytesData() {
+ ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(256);
+ IntStream.range(-128, 128).forEach(byteArrayOutputStream::write);
+ return byteArrayOutputStream.toByteArray();
+ }
Review Comment:
no, otherwise the cross-test CI in GitHub workflow will fail
--
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 #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
Jimexist commented on code in PR #2606:
URL: https://github.com/apache/thrift/pull/2606#discussion_r873048868
##########
lib/java/gradle/sourceConfiguration.gradle:
##########
@@ -21,21 +21,28 @@
// ----------------------------------------------------------------------------
// Compiler configuration details
-// These two properties are still needed on JDK8, and possibly used directly by
-// plugins. However, the '--release' option added below makes these two
-// properties redundant when building with JDK9 or later.
-sourceCompatibility = '1.8'
-targetCompatibility = '1.8'
+// We are using Java 11 toolchain to compile.
+// This enables decoupling from the Java version that gradle runs, from
+// the actual JDK version for the project. For more details, see
+// https://docs.gradle.org/current/userguide/toolchains.html
+//
+// The '--release' option added below makes sure that even if we are using
+// the toolchain version > 8, the final artifact is at version 8. There is
+// also a runtime CI that's based on Java 8 to ensure that.
+java {
+ toolchain {
+ languageVersion = JavaLanguageVersion.of(11)
+ }
+}
-tasks.withType(JavaCompile) {
+tasks.withType(JavaCompile).configureEach {
options.encoding = 'UTF-8'
options.debug = true
options.deprecation = true
+ assert JavaVersion.current().isJava11Compatible(), "JDK 11 or higher is required to build"
Review Comment:
i guess do we have to drop this assertion as the `current` refers to the one used by gradle not by toolchain.
```suggestion
```
--
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 #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
Jimexist commented on PR #2606:
URL: https://github.com/apache/thrift/pull/2606#issuecomment-1125657932
> Looks good to me, for the most part. I would suggest a slight change to the error message.
>
> Also, there are some conditionals in the Gradle build files that check for Java 11. Those can be removed now.
>
> We'd also need to remove the CI checks that run using JDK 8, like that the Ubuntu Xenial builds, unless you want to restore your previous workaround to force JDK 11 to be installed.
we don't need to update the xenial build, since using java toolchain means we are decoupling the JDK used to run gradle from the one used to compile Java. in the case of xenial, JDK8 is used to run gradle, which will download JDK 11 on its own before compiling Java.
--
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 pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
slachiewicz commented on PR #2606:
URL: https://github.com/apache/thrift/pull/2606#issuecomment-1130215309
why not split dependency updates or other code changes - not related to Gradle/Java update from real Gradle Java 11 upgrade?
--
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 pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2606:
URL: https://github.com/apache/thrift/pull/2606#issuecomment-1130394344
> why not split dependency updates or other code changes - not related to Gradle/Java update from real Gradle Java 11 upgrade?
If they are small and trivial, like these, I don't think it matters much. All of these are related to updating the build.
--
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 pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #2606:
URL: https://github.com/apache/thrift/pull/2606#issuecomment-1452368521
@nicolasb29 That's not a bug. Thrift moved to Java 11 in #2686 (THRIFT-5644)
--
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