[GitHub] [thrift] ctubbsii commented on a diff in pull request #2606: THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8

[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