Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8 #2606

Merged
merged 14 commits into from
May 18, 2022

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented May 13, 2022

use gradle toolchain to specify Java 11 with --release 8

  • 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.

@jimexist jimexist changed the title use gradle toolchain to specify Java 11 with --release 8 THRIFT-5584: use gradle toolchain to specify Java 11 with --release 8 May 13, 2022
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>
@jimexist
Copy link
Member Author

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.

@ctubbsii
Copy link
Member

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.

@jimexist
Copy link
Member Author

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 describing the auto provisioning feature in case you are interested

@jimexist jimexist requested review from ctubbsii May 18, 2022 01:31
@slachiewicz
Copy link
Member

why not split dependency updates or other code changes - not related to Gradle/Java update from real Gradle Java 11 upgrade?

@ctubbsii
Copy link
Member

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.

@ctubbsii ctubbsii merged commit c4e96c7 into apache:master May 18, 2022
@jimexist jimexist deleted the use-gradle-toolchain branch May 19, 2022 00:21
@nicolasb29
Copy link
Contributor

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/

@ctubbsii
Copy link
Member

ctubbsii commented Mar 2, 2023

@nicolasb29 That's not a bug. Thrift moved to Java 11 in #2686 (THRIFT-5644)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants