Update build to use Gradle toolchains and modern JDK #3562
Conversation
lwasyl
force-pushed
the
wasyl/update-build
branch
from
b7dd803
to
b1d645e
Compare
| @@ -26,7 +26,7 @@ jobs: | |||
| uses: actions/setup-java@v3 | |||
| with: | |||
| distribution: 'temurin' | |||
| java-version: '8' | |||
| java-version: '17' | |||
There was a problem hiding this comment.
The JDK used to run the build is no longer determining the target artifacts bytecode, so it's safe to use a more recent JDK.
This could easily be '20' as well, but I decided to use an LTS version until I get feedback that more recent versions are OK too. Fwiw I've been running JDK20 for a long time without issues (and JDK19 and 18 before that). Personally I'd suggest to always use the newest JDK for both CI and for the toolchains
There was a problem hiding this comment.
I think staying on LTS versions makes sense for this project. I dont think we would bother to bump these versions every 6 months, so it's likely we would end up on an unsupported version. Besides, I don't think we gain much from being on latest JDK?
There was a problem hiding this comment.
I agree. Compatibility is more important than anything else so lets stick to the LTS 17. We can go to 21 when that's out.
There was a problem hiding this comment.
Besides, I don't think we gain much from being on latest JDK?
Generally, you do — newer JDKs bring performance improvements and bug fixes. Some are really noticeable (e.g. a metaspace leak that was transparently handled by Gradle but incurred massive memory cost, see https://youtrack.jetbrains.com/issue/KT-55831) but there are also smaller performance fixes, improvements to GC algorithms etc. This may not matter for CI, but this is related to:
Compatibility is more important than anything else so lets stick to the LTS 17
This PR is specifically fixing incompatibility with JDK20, which is stable and is a valid JDK to run builds with, but Kotest build was failing. So building with JDK17 on CI is not guaranteeing further compatibility with JDK20. It's of course valid to say that Kotest is only compatible with JDK17, but then you would force your consumers to use an old JDK (note that this PR had to change some tests too, which means the consumers face the same issues). So if anything, it'd make sense to test the build on both LTS and current JDKs.
Same goes for toolchains, using newer JDK gives performance improvements for the cost of updating a number in couple of places every half a year. Of course that's your decision (I'm just happy I can now have deterministic output regardless of JDK I use to run the build). I just wanted to point out new JDKs do have benefits and using LTS doesn't solve any compatibility issues in a world where developers use non-LTS releases. Personally I'd say it's more reasonable to expect people to use most recent stable releases of software, even if LTS exists. In case of Java I think LTS releases make much more sense for something like servers where updating runtime is problematic
| @@ -25,6 +25,8 @@ kotlin.native.ignoreDisabledTargets=true | |||
| systemProp.kotlin.native.disableCompilerDaemon=true | |||
| org.gradle.configureondemand=false | |||
| kotlin.mpp.stability.nowarn=true | |||
| # https://youtrack.jetbrains.com/issue/KT-58063 (should be fixed in Kotlin 1.9) | |||
| org.gradle.configuration-cache=false | |||
There was a problem hiding this comment.
Configuration cache is now stable, so it's reasonable that people will enable it in ~/.gradle/gradle.properties. Unfortunately, some multiplatform tasks are not yet compatible with it
kotest-framework/kotest-framework-multiplatform-plugin-gradle/build.gradle.kts
Show resolved
Hide resolved
buildSrc/build.gradle.kts
Outdated
| } | ||
|
|
||
| tasks.withType<KotlinCompile>().configureEach { | ||
| compilerOptions.jvmTarget.set(JvmTarget.JVM_1_8) |
There was a problem hiding this comment.
Btw if the reason to use JDK8 bytecode is android, then you should know Android supports JDK11 libraries for quite some time now. So unless there are other reasons to generate Java 8 bytecode, this can probably be bumped to 11 too
lwasyl
force-pushed
the
wasyl/update-build
branch
4 times, most recently
from
5b5b3b9
to
cc36ca9
Compare
lwasyl
force-pushed
the
wasyl/update-build
branch
from
cc36ca9
to
394a710
Compare
We need to use a JVM 17+ since buildSrc sets toolchain to 17. We should probably make sure to either use kotest-action everywhere, or ditch it. PR builds differ from master builds due to this, which is why #3562 built without issues.
I've checked out Kotest repository and couldn't build it because I'm using JDK20. This PR is a suggestion on how this could be fixed: now contributors can run the build using any JDK they want, and Gradle will download JDK used for the build itself. This ensures all developers and CI will use the same JDK to build the project, and the resulting artifacts will always target the same bytecode