THRIFT-5581: java lib to upgrade to gradle 7 and migrate from `maven` to `maven-publish` plugin #2601

jimexist · 2022-05-07T12:58:00Z

relies on THRIFT-5553: java library to use newer gradle API #2561 to be merged first

Did you create an Apache Jira 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.

Jens-G · 2022-05-07T13:59:12Z

move off maven

because ...

jimexist · 2022-05-07T16:31:49Z

move off maven

because ...

I had to correct my typing, sorry for the confusion... @Jens-G

jimexist · 2022-05-07T16:32:41Z

This is put to draft state because on xenial JDK 8 is used but somehow gradle 7.4+ does not quite work with Java 8. It'll at least require Java 11, which makes sense because it's the LTS version that's not at its end of life now.

ctubbsii · 2022-05-08T05:11:26Z

move off maven

because ...

@Jens-G "maven" is the name of the Gradle plugin used for the publishing of the libthrift.jar. That Gradle plugin was deprecated and replaced by a plugin called "maven-publish". Gradle 7 does not support the old plugin, so this transition needs to happen in order to support Gradle 7.

ctubbsii · 2022-05-08T05:18:19Z

This is put to draft state because on xenial JDK 8 is used but somehow gradle 7.4+ does not quite work with Java 8. It'll at least require Java 11, which makes sense because it's the LTS version that's not at its end of life now.

My personal preference would be to migrate the build to use JDK 11 at a minimum. The resulting libthrift.jar could still be used with Java 8, if the appropriate --release 8 flag is placed on javac. In my experience, JDK 11 is even better than JDK 8 at enforcing the Java 8 language rules and results in byte code that more reliably runs on a variety of Java 8 runtimes anyway. But, I don't know if you'd get resistance to this, because some people don't like building and testing with a different JDK version than the Java runtime the jar is targeting. I don't know how to set --release 8 for Gradle, but it can't be too hard.

lib/java/Makefile.am

lib/java/gradle/environment.gradle

ctubbsii · 2022-05-08T05:27:23Z

Looking at this changeset makes me just hate Gradle so much. There's just so much tooling that I don't understand, and some of it seems pretty low-level. I prefer Maven over Gradle. I know XML sucks, but I think its lifecycle concept is simpler, and the conventions are more concise. The direction these changes are going is fine, though. I'm learning Gradle by reviewing these. Maybe I won't hate it as much as I understand it more. But, I understand Makefiles, and I still hate them... so maybe I still will. 🤷 😺

jimexist · 2022-05-08T05:47:11Z

Looking at this changeset makes me just hate Gradle so much. There's just so much tooling that I don't understand, and some of it seems pretty low-level. I prefer Maven over Gradle. I know XML sucks, but I think its lifecycle concept is simpler, and the conventions are more concise. The direction these changes are going is fine, though. I'm learning Gradle by reviewing these. Maybe I won't hate it as much as I understand it more. But, I understand Makefiles, and I still hate them... so maybe I still will. 🤷 😺

I'm totally with you on e.g. the unencessity of breaking changes needed to upgrade from 6.x to 7.x and all the migration from maven to maven-publish are just friction with no obvious gain. on the other hand there are good chances happening in gradle land, e.g. using .kts i.e. kotlin which has better tooling and typing support than groovy. Nowadays i don't use mvn as much but it's a more mature and popular choice so i'm okay with learning and using either one. currently libthrift uses gradle so be it (unless someone wants to make a chance).

lib/java/build.gradle

lib/java/gradle/publishing.gradle

build/docker/ubuntu-xenial/Dockerfile

fix typo

ctubbsii · 2022-05-09T02:22:18Z

I saw Travis fail with:

BUILD SUCCESSFUL in 45s
6 actionable tasks: 6 executed
ninja: build stopped: subcommand failed.
The command "build/docker/run.sh" exited with 1.

No idea what the problem is.

jimexist · 2022-05-09T02:28:32Z

I saw Travis fail with:

BUILD SUCCESSFUL in 45s
6 actionable tasks: 6 executed
ninja: build stopped: subcommand failed.
The command "build/docker/run.sh" exited with 1.

No idea what the problem is.

the error was further above. search for ECONNRESET - it's a flaky download connection reset, i guess rebuilding would help. but that's only limited to nodejs which i haven't touched in this pull request.

ctubbsii · 2022-05-09T02:50:15Z

build/docker/ubuntu-xenial/Dockerfile

+`# Open JDK 11 - we cannot install openjdk-11-jdk using apt-get because xenial has only openjdk-8-jdk` \
+      wget https://download.java.net/openjdk/jdk11/ri/openjdk-11+28_linux-x64_bin.tar.gz -O /tmp/openjdk-11+28_linux-x64_bin.tar.gz && \
+      (echo "3784cfc4670f0d4c5482604c7c513beb1a92b005f569df9bf100e8bef6610f2e  /tmp/openjdk-11+28_linux-x64_bin.tar.gz" | sha256sum -c -) && \
+      tar -C /usr/local -xzf /tmp/openjdk-11+28_linux-x64_bin.tar.gz && \


Xenial is old. It's probably time to drop it soon, but this is a reasonable temporary workaround.

ctubbsii · 2022-05-09T02:58:19Z

@jimexist This looks good to me, but I'm curious what the user experience is when trying to build with JDK 8. Will it fail in incomprehensible ways, or will the requirement to use JDK 11 be made obvious? For Maven, the maven-enforcer-plugin can provide a meaningful error message when the minimum Java version is not used.

jimexist · 2022-05-09T03:01:42Z

@jimexist This looks good to me, but I'm curious what the user experience is when trying to build with JDK 8. Will it fail in incomprehensible ways, or will the requirement to use JDK 11 be made obvious? For Maven, the maven-enforcer-plugin can provide a meaningful error message when the minimum Java version is not used.

In that case gradle will complain saying that gradle 7.4.2 needs Java 11.

I also put a step in https://github.com/apache/thrift/pull/2602/files where the Java 11 built artifact is run using Java 8 during cross test to make sure

jimexist · 2022-05-09T03:31:48Z

FYI i think JDK 8 support is still with Gradle 7:

build log

root@5ec15a799389:/opt/thrift/src/lib/java# ./gradlew
bash: ./gradlew: No such file or directory
root@5ec15a799389:/opt/thrift/src/lib/java# java -version
openjdk version "1.8.0_332"
OpenJDK Runtime Environment (build 1.8.0_332-b09)
OpenJDK 64-Bit Server VM (build 25.332-b09, mixed mode)
root@5ec15a799389:/opt/thrift/src/lib/java# ./gradlew assemble
Downloading https://services.gradle.org/distributions/gradle-7.4.2-bin.zip
...........10%...........20%...........30%...........40%...........50%...........60%...........70%...........80%...........90%...........100%

Welcome to Gradle 7.4.2!

Here are the highlights of this release:
 - Aggregated test and JaCoCo reports
 - Marking additional test source directories as tests in IntelliJ
 - Support for Adoptium JDKs in Java toolchains

For more details see https://docs.gradle.org/7.4.2/release-notes.html

Starting a Gradle Daemon (subsequent builds will be faster)

> Task :compileJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :javadoc
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/EnumCache.java:51: warning: no @return
  public TEnum get(Class<? extends TEnum> enumClass, int value) {
               ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/ThriftMetadata.java:112: warning: no @param for level
    protected String getIndent(int level) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/ThriftMetadata.java:112: warning: no @return
    protected String getIndent(int level) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/ThriftMetadata.java:117: warning: no @param for sb
    protected void append(StringBuilder sb, String format, Object... args) {
                   ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/ThriftMetadata.java:117: warning: no @param for format
    protected void append(StringBuilder sb, String format, Object... args) {
                   ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/ThriftMetadata.java:117: warning: no @param for args
    protected void append(StringBuilder sb, String format, Object... args) {
                   ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/ThriftMetadata.java:122: warning: no @return
    protected String getName() {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:37: warning: no @param for obj
  public static void checkNotNull(Object obj, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:37: warning: no @param for argName
  public static void checkNotNull(Object obj, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:42: warning: no @param for value
  public static void checkPositiveInteger(long value, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:42: warning: no @param for argName
  public static void checkPositiveInteger(long value, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:47: warning: no @param for value
  public static void checkNotNegative(long value, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:47: warning: no @param for argName
  public static void checkNotNegative(long value, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:59: warning: no @param for isValid
  public static void checkValid(boolean isValid, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:59: warning: no @param for argName
  public static void checkValid(boolean isValid, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:64: warning: no @param for isValid
  public static void checkValid(boolean isValid, String argName, String validValues) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:64: warning: no @param for argName
  public static void checkValid(boolean isValid, String argName, String validValues) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:64: warning: no @param for validValues
  public static void checkValid(boolean isValid, String argName, String validValues) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:69: warning: no @param for arg
  public static void checkNotNullAndNotEmpty(String arg, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:69: warning: no @param for argName
  public static void checkNotNullAndNotEmpty(String arg, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:75: warning: no @param for <T>
  public static <T> void checkNotNullAndNotEmpty(T[] array, String argName) {
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:75: warning: no @param for array
  public static <T> void checkNotNullAndNotEmpty(T[] array, String argName) {
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:75: warning: no @param for argName
  public static <T> void checkNotNullAndNotEmpty(T[] array, String argName) {
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:81: warning: no @param for array
  public static void checkNotNullAndNotEmpty(byte[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:81: warning: no @param for argName
  public static void checkNotNullAndNotEmpty(byte[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:87: warning: no @param for array
  public static void checkNotNullAndNotEmpty(short[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:87: warning: no @param for argName
  public static void checkNotNullAndNotEmpty(short[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:93: warning: no @param for array
  public static void checkNotNullAndNotEmpty(int[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:93: warning: no @param for argName
  public static void checkNotNullAndNotEmpty(int[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:99: warning: no @param for array
  public static void checkNotNullAndNotEmpty(long[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:99: warning: no @param for argName
  public static void checkNotNullAndNotEmpty(long[] array, String argName) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:105: warning: no @param for <T>
  public static <T> void checkNotNullAndNotEmpty(Iterable<T> iter, String argName) {
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:105: warning: no @param for iter
  public static <T> void checkNotNullAndNotEmpty(Iterable<T> iter, String argName) {
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:105: warning: no @param for argName
  public static <T> void checkNotNullAndNotEmpty(Iterable<T> iter, String argName) {
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:112: warning: no @param for <T>
  public static <T> void checkNotNullAndNumberOfElements(
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:112: warning: no @param for collection
  public static <T> void checkNotNullAndNumberOfElements(
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:112: warning: no @param for numElements
  public static <T> void checkNotNullAndNumberOfElements(
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:112: warning: no @param for argName
  public static <T> void checkNotNullAndNumberOfElements(
                         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:124: warning: no @param for value1
  public static void checkValuesEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:124: warning: no @param for value1Name
  public static void checkValuesEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:124: warning: no @param for value2
  public static void checkValuesEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:124: warning: no @param for value2Name
  public static void checkValuesEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:136: warning: no @param for value1
  public static void checkIntegerMultiple(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:136: warning: no @param for value1Name
  public static void checkIntegerMultiple(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:136: warning: no @param for value2
  public static void checkIntegerMultiple(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:136: warning: no @param for value2Name
  public static void checkIntegerMultiple(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:148: warning: no @param for value1
  public static void checkGreater(long value1, String value1Name, long value2, String value2Name) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:148: warning: no @param for value1Name
  public static void checkGreater(long value1, String value1Name, long value2, String value2Name) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:148: warning: no @param for value2
  public static void checkGreater(long value1, String value1Name, long value2, String value2Name) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:148: warning: no @param for value2Name
  public static void checkGreater(long value1, String value1Name, long value2, String value2Name) {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:159: warning: no @param for value1
  public static void checkGreaterOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:159: warning: no @param for value1Name
  public static void checkGreaterOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:159: warning: no @param for value2
  public static void checkGreaterOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:159: warning: no @param for value2Name
  public static void checkGreaterOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:171: warning: no @param for value1
  public static void checkLessOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:171: warning: no @param for value1Name
  public static void checkLessOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:171: warning: no @param for value2
  public static void checkLessOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:171: warning: no @param for value2Name
  public static void checkLessOrEqual(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:183: warning: no @param for value
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:183: warning: no @param for valueName
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:183: warning: no @param for minValueInclusive
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:183: warning: no @param for maxValueInclusive
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:195: warning: no @param for value
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:195: warning: no @param for valueName
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:195: warning: no @param for minValueInclusive
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/partial/Validate.java:195: warning: no @param for maxValueInclusive
  public static void checkWithinRange(
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/TDeserializer.java:85: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TDeserializer(
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/TDeserializer.java:109: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TDeserializer(
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TEndpointTransport.java:56: warning: no description for @param
   * @param newSize
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TEndpointTransport.java:58: warning: no @throws for org.apache.thrift.transport.TTransportException
  protected void resetConsumedMessageSize(long newSize) throws TTransportException {
                 ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TEndpointTransport.java:78: warning: no description for @param
   * @param size
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TEndpointTransport.java:90: warning: no description for @param
   * @param numBytes
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TEndpointTransport.java:100: warning: no description for @param
   * @param numBytes
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TEndpointTransport.java:102: warning: no @throws for org.apache.thrift.transport.TTransportException
  protected void countConsumedMessageBytes(long numBytes) throws TTransportException {
                 ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/AutoExpandingBufferWriteTransport.java:42: warning: no @throws for org.apache.thrift.transport.TTransportException
  public AutoExpandingBufferWriteTransport(
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TFileTransport.java:572: warning: no @param for args
  public static void main(String[] args) throws Exception {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TFileTransport.java:572: warning: no @throws for java.lang.Exception
  public static void main(String[] args) throws Exception {
                     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TTransportFactory.java:35: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TTransport getTransport(TTransport trans) throws TTransportException {
                    ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:57: warning: no @throws for org.apache.thrift.transport.TTransportException
  protected TIOStreamTransport() throws TTransportException {
            ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:76: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TIOStreamTransport(InputStream is) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:122: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TIOStreamTransport(InputStream is, OutputStream os) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:97: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TIOStreamTransport(OutputStream os) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:49: warning: no @param for config
  protected TIOStreamTransport(TConfiguration config) throws TTransportException {
            ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:49: warning: no @throws for org.apache.thrift.transport.TTransportException
  protected TIOStreamTransport(TConfiguration config) throws TTransportException {
            ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:64: warning: no description for @param
   * @param config
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:67: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TIOStreamTransport(TConfiguration config, InputStream is) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:105: warning: no description for @param
   * @param config
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:109: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TIOStreamTransport(TConfiguration config, InputStream is, OutputStream os)
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:84: warning: no description for @param
   * @param config
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TIOStreamTransport.java:87: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TIOStreamTransport(TConfiguration config, OutputStream os) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingServerSocket.java:56: warning: no @param for port
  public TNonblockingServerSocket(int port) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingServerSocket.java:56: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TNonblockingServerSocket(int port) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingServerSocket.java:61: warning: no @param for port
  public TNonblockingServerSocket(int port, int clientTimeout) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingServerSocket.java:61: warning: no @param for clientTimeout
  public TNonblockingServerSocket(int port, int clientTimeout) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingServerSocket.java:61: warning: no @throws for org.apache.thrift.transport.TTransportException
  public TNonblockingServerSocket(int port, int clientTimeout) throws TTransportException {
         ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingSocket.java:99: warning: no description for @param
   * @param selector
     ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingSocket.java:121: warning: no @return
  public SocketChannel getSocketChannel() {
                       ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingTransport.java:39: warning: no @return
  public abstract boolean startConnect() throws IOException;
                          ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingTransport.java:39: warning: no @throws for java.io.IOException
  public abstract boolean startConnect() throws IOException;
                          ^
/opt/thrift/src/lib/java/src/main/java/org/apache/thrift/transport/TNonblockingTransport.java:46: warning: no @return
  public abstract boolean finishConnect() throws IOException;
                          ^
100 warnings

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.4.2/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1m 27s
6 actionable tasks: 6 executed

jimexist · 2022-05-09T03:33:59Z

FYI i think JDK 8 support is still with Gradle 7:

build log

the need for java 8 is possibly due to kotlin's unspecified dependencies, for which i've pinned down in https://github.com/apache/thrift/pull/2602/files#diff-fac297e377f3c9d2ebbc3e82419e0c9d7a6d15e741acdfaf62c4485b8e228b83R37-R42

maybe after merging that i'll try to revert the upgrade of xenial's java 8 => 11 changes (maybe in later PR), or not, since xenial is a bit old.

i'm trying to add focal in:

THRIFT-5574: add ubuntu focal build #2528 or even jammy but maybe not today, since they both have python2 dropped, which in turn relies on
THRIFT-5537: Remove python2 #2588

jimexist · 2022-05-09T03:37:23Z

confirmed that the build failure in Java 8 was due to kolin's ktfmt which depends on gradle api that's Java 11+ only:

build log

root@5ec15a799389:/opt/thrift/src/lib/kotlin# ../java/gradlew assemble

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project 'libthrift-kotlin'.
> Could not determine the dependencies of null.
   > Could not resolve all task dependencies for configuration ':classpath'.
      > Could not resolve com.ncorti.ktfmt.gradle:plugin:0.8.0.
        Required by:
            project : > com.ncorti.ktfmt.gradle:com.ncorti.ktfmt.gradle.gradle.plugin:0.8.0
         > No matching variant of com.ncorti.ktfmt.gradle:plugin:0.8.0 was found. The consumer was configured to find a runtime of a library compatible with Java 8, packaged as a jar, and its dependencies declared externally, as well as attribute 'org.gradle.plugin.api-version' with value '7.4.2' but:
             - Variant 'apiElements' capability com.ncorti.ktfmt.gradle:plugin:0.8.0 declares a library, packaged as a jar, and its dependencies declared externally:
                 - Incompatible because this component declares an API of a component compatible with Java 11 and the consumer needed a runtime of a component compatible with Java 8
                 - Other compatible attribute:
                     - Doesn't say anything about org.gradle.plugin.api-version (required '7.4.2')
             - Variant 'runtimeElements' capability com.ncorti.ktfmt.gradle:plugin:0.8.0 declares a runtime of a library, packaged as a jar, and its dependencies declared externally:
                 - Incompatible because this component declares a component compatible with Java 11 and the consumer needed a component compatible with Java 8
                 - Other compatible attribute:
                     - Doesn't say anything about org.gradle.plugin.api-version (required '7.4.2')

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.4.2/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 37s

ctubbsii · 2022-05-09T03:39:54Z

@jimexist I fixed the merge conflicts, and also bumped the spotless version, and formatted the kotlin and java files using Gradle 7.4.2. I'll let CI finish its checks, but I did notice that:

there are some deprecated stuff in use here that will be removed in Gradle 8 (which is not out yet, but will be nice to be prepared for) -- could be fixed as part of this PR,
some Gradle plugins use banned APIs that are removed/hidden in Java 17 (this might require waiting on upstream plugin developers, or disabling some plugins if Java 17 is used) -- could be a follow-on PR, and
there's a lot of warnings spamming the console for both lib/java and lib/kotlin -- could be a follow-on PR.

As a side note: I'm really happy to be helping review your changes. I'm learning a lot from the exercise, and I'm impressed with the quality and pace of your contributions. I've been feeling guilty about being added as a committer, but not really having much time to contribute. But, all your work has kept me interested, and contributing to the project, even if it's limited to the Java and CI changes. So, thanks for contributing so that I can contribute. 😺

ctubbsii · 2022-05-09T03:43:52Z

confirmed that the build failure in Java 8 was due to kolin's ktfmt which depends on gradle api that's Java 11+ only:

Maybe using that plugin can be wrapped around a conditional? If so, then could go ahead and revert the Xenial changes here.
it'd be nice to make the choice to require JDK 11 a conscious decision for later, rather than as a side-effect of the Gradle update now.

jimexist · 2022-05-09T04:08:05Z

confirmed that the build failure in Java 8 was due to kolin's ktfmt which depends on gradle api that's Java 11+ only:

Maybe using that plugin can be wrapped around a conditional? If so, then could go ahead and revert the Xenial changes here. it'd be nice to make the choice to require JDK 11 a conscious decision for later, rather than as a side-effect of the Gradle update now.

yes let me try to put a if guard around ktfmt and revert the changes of upgrading java to xenial docker image

jimexist · 2022-05-09T04:11:26Z

@jimexist I fixed the merge conflicts, and also bumped the spotless version, and formatted the kotlin and java files using Gradle 7.4.2. I'll let CI finish its checks, but I did notice that:

there are some deprecated stuff in use here that will be removed in Gradle 8 (which is not out yet, but will be nice to be prepared for) -- could be fixed as part of this PR,

some Gradle plugins use banned APIs that are removed/hidden in Java 17 (this might require waiting on upstream plugin developers, or disabling some plugins if Java 17 is used) -- could be a follow-on PR, and

there's a lot of warnings spamming the console for both lib/java and lib/kotlin -- could be a follow-on PR.

As a side note: I'm really happy to be helping review your changes. I'm learning a lot from the exercise, and I'm impressed with the quality and pace of your contributions. I've been feeling guilty about being added as a committer, but not really having much time to contribute. But, all your work has kept me interested, and contributing to the project, even if it's limited to the Java and CI changes. So, thanks for contributing so that I can contribute. 😺

thanks for reviewing my pull request and helping with formatting!

i've fixed the deprecations
i guess we can wait a bit for gradle 7.5 to be out for which they will possibly fix them
i think you meant javadoc warnings?

thanks for the kind words as well and i'm also learning a lot along the way. i think there's no shame and pressure on being a committer and the amount of actual contributions you make, as long as it's net positive then it's all good, also i totally agree that reviewing code and commenting is also very good part of contributions. also a side note - i don't even know how to become a committer (there's no documented process yet) but that won't stop me from committing code either way.

ctubbsii · 2022-05-09T07:41:28Z

i don't even know how to become a committer (there's no documented process yet) but that won't stop me from committing code either way.

At the ASF, each project's PMC nominates and votes on new committers (and new PMC members) in secret on their private mailing lists. Each project is different, and each PMC member might be different in their criteria for nominating somebody. I'm not a PMC member for this project, so I don't know how it typically happens here. Personally, I think you're probably doing all the right things to warrant consideration, but it's up to the PMC.

ctubbsii · 2022-05-09T07:50:05Z

3. i think you meant javadoc warnings?

Mostly, yes. Most of the javadoc warnings can probably be taken care of with -Xdoclint:all,-missing (it's not really a problem if an unimportant method is missing a javadoc, but if one exists, then all checks should be enabled).
I thought I saw a few warnings on the kotlin side, but I don't remember what they were.

In any case, these don't have to be part of this PR.

ctubbsii · 2022-05-09T09:19:55Z

It looks like the conditional configuration isn't working. When the plugin isn't applied, then the type safe ktfmt configuration block that calls kotlinLangStyle() fails with a compilation error. I can't figure out how to conditionally apply the configuration... I don't think any type-safe way will work, and I don't understand the non-type-safe ways discussed in the docs.

jimexist · 2022-05-09T10:17:22Z

It looks like the conditional configuration isn't working. When the plugin isn't applied, then the type safe ktfmt configuration block that calls kotlinLangStyle() fails with a compilation error. I can't figure out how to conditionally apply the configuration... I don't think any type-safe way will work, and I don't understand the non-type-safe ways discussed in the docs.

for now i'm just configuring the plugin in both versions but only conditionally apply it with Java 11+ so there's no conflict. hopefully this can solve the issue.

ctubbsii · 2022-05-09T18:10:08Z

for now i'm just configuring the plugin in both versions but only conditionally apply it with Java 11+ so there's no conflict. hopefully this can solve the issue.

This seems to work for the CI tests (gradle assemble seems to work fine), but manually running gradle build or gradle ktfmtFormat using JDK 8 won't work still. That's probably okay.

ctubbsii · 2022-05-10T00:00:00Z

Another Travis thing is failing. This time, it's because it couldn't download something from NuGet, unrelated to these changes. I look forward to more things being done in GH Actions. It will be much easier to re-run only the failed CI tasks that way, and in only a few minutes, instead of having it queued for hours and hours. All the relevant CI checks related to these checks seem to be passing just fine, though.

So, I think this is ready to merge.

@Jens-G since you made a comment earlier, did you want to take another look, or express any comments or objections before this is merged?

jimexist · 2022-05-10T00:03:04Z

It will be much easier to re-run only the failed CI tasks that way, and in only a few minutes, instead of having it queued for hours and hours. All the relevant CI checks related to these checks seem to be passing just fine, though.

for anyone interested in reviewing GitHub action related pull requests:

jimexist · 2022-05-10T00:04:21Z

did you want to take another look, or express any comments or objections before this is merged?

one thing i didn't test myself, nor on CI, is actual publishing, because my lack of access. but i've tested publishing locally and the content seems fine:

.jar
-sources.jar
-javadoc.jar
pom file
without the shadow jar

the files can also be found in the artifacts in the GitHub action run: https://github.com/apache/thrift/actions/runs/2295929764

jimexist · 2022-05-11T14:10:44Z

#2601

hi @mitumaruto i'm not sure i understand your comment nor suggested changes, can you elaborate?

Jiayu Liu and others added 7 commits April 28, 2022 15:26

use newer gradle API

897aca1

Merge branch 'master' into use-newer-gradle-api

a151083

Merge branch 'master' into use-newer-gradle-api

94aac3b

Merge branch 'master' into use-newer-gradle-api

a0f09ba

migrate to gradle 7

3dc7cad

upgrade gradle version

021a4ec

upgrade kotlin

3e7d648

jimexist changed the title ~~java lib to upgrade to gradle 7 and move off maven~~ java lib to upgrade to gradle 7 and migrate from maven to maven-publish plugin May 7, 2022

jimexist mentioned this pull request May 7, 2022

THRIFT-5553: java library to use newer gradle API #2561

Merged

5 tasks

ctubbsii reviewed May 8, 2022

View reviewed changes

lib/java/Makefile.am Show resolved Hide resolved

ctubbsii reviewed May 8, 2022

View reviewed changes

lib/java/gradle/environment.gradle Outdated Show resolved Hide resolved

Merge branch 'master' into gradle-7

0c32e5c

jimexist commented May 8, 2022

View reviewed changes

lib/java/build.gradle Show resolved Hide resolved

jimexist commented May 8, 2022

View reviewed changes

lib/java/gradle/publishing.gradle Outdated Show resolved Hide resolved

jimexist and others added 4 commits May 8, 2022 13:50

Update lib/java/gradle/publishing.gradle

c066ec7

add make install for java

41e2591

install jdk 11 for xenial

a4430af

remove links per comment

59c9511

jimexist commented May 8, 2022

View reviewed changes

build/docker/ubuntu-xenial/Dockerfile Outdated Show resolved Hide resolved

Update build/docker/ubuntu-xenial/Dockerfile

63b9bc3

fix typo

jimexist marked this pull request as ready for review May 8, 2022 14:31

jimexist changed the title ~~java lib to upgrade to gradle 7 and migrate from maven to maven-publish plugin~~ THRIFT-5581: java lib to upgrade to gradle 7 and migrate from maven to maven-publish plugin May 8, 2022

ctubbsii reviewed May 9, 2022

View reviewed changes

Merge branch 'master' into gradle-7

4a77c36

Fix formatting

c129e64

Jiayu Liu added 2 commits May 9, 2022 11:55

fix gralde 8 deprecation version

c498f70

revert xenial java upgrade and use guard for ktfmt

62852e1

add guard to ktfmt as well

bd25822

fix ktfmt build

2620cdc

Merge branch 'master' into gradle-7

73c606c

ctubbsii approved these changes May 9, 2022

View reviewed changes

Merge branch 'master' into gradle-7

89b767d

ctubbsii merged commit 5b15838 into apache:master May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-5581: java lib to upgrade to gradle 7 and migrate from `maven` to `maven-publish` plugin #2601

THRIFT-5581: java lib to upgrade to gradle 7 and migrate from `maven` to `maven-publish` plugin #2601

jimexist commented May 7, 2022 •

edited

Loading

Jens-G commented May 7, 2022 •

edited

Loading

jimexist commented May 7, 2022

jimexist commented May 7, 2022

ctubbsii commented May 8, 2022

ctubbsii commented May 8, 2022

ctubbsii commented May 8, 2022

jimexist commented May 8, 2022 •

edited

Loading

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

ctubbsii May 9, 2022

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

jimexist commented May 9, 2022

jimexist commented May 9, 2022 •

edited

Loading

jimexist commented May 9, 2022 •

edited

Loading

ctubbsii commented May 9, 2022

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

jimexist commented May 9, 2022

ctubbsii commented May 9, 2022

ctubbsii commented May 9, 2022

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

ctubbsii commented May 9, 2022

ctubbsii commented May 10, 2022

jimexist commented May 10, 2022

jimexist commented May 10, 2022 •

edited

Loading

jimexist commented May 11, 2022

THRIFT-5581: java lib to upgrade to gradle 7 and migrate from maven to maven-publish plugin #2601

THRIFT-5581: java lib to upgrade to gradle 7 and migrate from maven to maven-publish plugin #2601

Conversation

jimexist commented May 7, 2022 • edited Loading

Jens-G commented May 7, 2022 • edited Loading

jimexist commented May 7, 2022

jimexist commented May 7, 2022

ctubbsii commented May 8, 2022

ctubbsii commented May 8, 2022

ctubbsii commented May 8, 2022

jimexist commented May 8, 2022 • edited Loading

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

ctubbsii May 9, 2022

Choose a reason for hiding this comment

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

jimexist commented May 9, 2022

jimexist commented May 9, 2022 • edited Loading

jimexist commented May 9, 2022 • edited Loading

ctubbsii commented May 9, 2022

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

jimexist commented May 9, 2022

ctubbsii commented May 9, 2022

ctubbsii commented May 9, 2022

ctubbsii commented May 9, 2022

jimexist commented May 9, 2022

ctubbsii commented May 9, 2022

ctubbsii commented May 10, 2022

jimexist commented May 10, 2022

jimexist commented May 10, 2022 • edited Loading

jimexist commented May 11, 2022

THRIFT-5581: java lib to upgrade to gradle 7 and migrate from `maven` to `maven-publish` plugin #2601

THRIFT-5581: java lib to upgrade to gradle 7 and migrate from `maven` to `maven-publish` plugin #2601

jimexist commented May 7, 2022 •

edited

Loading

Jens-G commented May 7, 2022 •

edited

Loading

jimexist commented May 8, 2022 •

edited

Loading

jimexist commented May 9, 2022 •

edited

Loading

jimexist commented May 9, 2022 •

edited

Loading

jimexist commented May 10, 2022 •

edited

Loading