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

Use JVM Toolchain #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

StefMa
Copy link

@StefMa StefMa commented Mar 7, 2024

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description, Motivation and Context

This PR moves the JVM Version selection from the machine to the build time.
With that change, Gradle makes sure that the code will be compiled with the configured JVM ersion (21) from the configured vendor (Corretto/Amazon).
There is "no need" to install a specific JVM version on CI or your dev machine.
You only need "a" JVM installation that can start Gradle.
Gradle will detect that the configuration say it should compile the code with Java 21/Corretto, it will install the respective toolchain and use that to compile the code and run tests with that.

More information can also be found here:
https://docs.gradle.org/8.6/userguide/toolchains.html

πŸ§ͺ How Has This Been Tested?

Remove your locally installed JVM version (21/Corretto).
Then use another JVM version and kick of Gradle (./gradlew check).
Wihin the configuration phase you will see an output like this:
Screenshot 2024-03-07 at 9 02 37β€―AM
Indicating that the requested toolchain (was donwloaded) and now installing on the machine for later usage.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@hartmut-co-uk
Copy link
Contributor

Thanks, @StefMa, for submitting this PR!
I'll need to look closer at the gradle docs. But at first glance, this seems like a valid and useful addition.

@hartmut-co-uk hartmut-co-uk self-assigned this Mar 9, 2024
@StefMa
Copy link
Author

StefMa commented May 25, 2024

Hey @hartmut-co-uk, any chance to have a look at this?

Copy link
Contributor

@hartmut-co-uk hartmut-co-uk left a comment

Choose a reason for hiding this comment

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

Thanks @StefMa for picking this up again! I'm happy with this feature.

There's one detail that worries me - the volume of gradle dependencies cached jumps from 170MB to 356MB (due to the JDK...). So the data volumes written and read from cache is higher, also the JDK is downloaded with each and every workflow run.
But maybe that's generally a misconfiguration here?

Thank you very much for the contribution and apologies for the delay! πŸ’š
Let's wrap this up quickly!

Comment on lines +28 to +29
languageVersion.set(JavaLanguageVersion.of(21))
vendor.set(JvmVendorSpec.AMAZON)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
languageVersion.set(JavaLanguageVersion.of(21))
vendor.set(JvmVendorSpec.AMAZON)
languageVersion = JavaLanguageVersion.of(21)
vendor = JvmVendorSpec.AMAZON

Cosmetic only, but I think it would align better with the DSL used in this file in other places.

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

Successfully merging this pull request may close these issues.

None yet

2 participants