Add Gradle toolchain support #262

StefMa · 2024-02-28T09:51:43Z

fixes #184

With that we can get rid of the java 11 check and the information around it.
Also, we can "on the fly" update the jdk version the project will build on.
We only need "a" java version installed to start Gradle.

As discussed in the ticket, I also had to update Gradle to version 7.6 which (seems) brings flaky tests.
But we can't use toolchains before that version.
So maybe this PR wil hang here until this is fixed 🙃

Questions / Todos

I guess this is not going to work anymore (or has to be adjusted)
👉 https://github.com/apple/pkl/blob/2499e2c4937fc228e49e94e41fdaac065c203168/.circleci/config.pkl#L98C7-L107

I guess this is not going to work anymore (or has to be adjusted)
👉

pkl/patches/graalVm23.patch

Lines 25 to 30 in 2499e2c

    
            java { 
        
           -  sourceCompatibility = JavaVersion.VERSION_11 
        
           -  targetCompatibility = JavaVersion.VERSION_11 
        
           +  sourceCompatibility = JavaVersion.VERSION_17 
        
           +  targetCompatibility = JavaVersion.VERSION_17 
        
            }

Let me know if there is more like these 🙃

odenix · 2024-02-28T18:28:44Z

In my opinion, it’s important that the Java version/vendor can be changed in a single place. For example, I often use this to reproduce a failed CI build.

StefMa · 2024-02-28T18:35:04Z

Any suggestion/recommendation how to do that?
Maybe an optional gradle property? 🤔

odenix · 2024-02-28T18:47:47Z

Perhaps you could add pklJvmProject.gradle.kts and apply it in the other files.

odenix · 2024-02-28T21:38:23Z

How about renaming DEFAULT_JVM_VERSION to JDK_VERSION? This val is what developers will want to find/edit when troubleshooting. I'd also add val JDK_VENDOR just in case.

I think the project property is only useful for command-line invocations, so it could be a system property.

Do your changes guarantee that source/targetCompatibility remains fixed at 11 (or soon 17) regardless of which JDK version is selected?

StefMa · 2024-02-29T14:23:54Z

Do your changes guarantee that source/targetCompatibility remains fixed at 11 (or soon 17) regardless of which JDK version is selected?

Yes, that is the case 👍
Shameless self advertisement: Read my blog about this topic 😁

How about renaming DEFAULT_JVM_VERSION to JDK_VERSION? This val is what developers will want to find/edit when troubleshooting.

Are you sure we wannt go with JDK instead of JVM? 🤔
The whole (Gradle) feature is named "JVM Toolchain".
Using JDK in the middle of somewhere feels a bit strange to me.

I'd also add val JDK_VENDOR just in case

We could do this, but not as a const as only primitves are allowed to be constants.
Using this without const looks a bit odd IMHO.

odenix · 2024-02-29T14:55:57Z

I think JDK_VERSION is correct, but mainly I'd like to get rid of DEFAULT.

const is only an optimization, but a vendor val isn't essential.

System property would be a small improvement because setting the project property in a build script will likely not work correctly.

StefMa · 2024-03-05T08:05:28Z

Seems we run into a few other Gradle 7x toolchain issues 🙈
gradle/gradle#22796
gradle/gradle#19382

Lets wait for #245

StefMa · 2024-03-05T09:00:24Z

Hey @bioball and/or @stackoverflow I might need your help here:

For clarification. The CI checks gradle-check-jdk11 and gradle-check-jdk17 are meant to be check if the code got compiled/works with Java 11 and Java 17, right?

If this is the case, then my changes are correct.
Because before we rely on the installed Java version on the machine.
Since Gradle handles this now, we have to put the Java version into the build script (using the system property PKL_JVM_VERSION).
For now I left using the respective Docker images (11 and 17), however, they are not required anymore.
Gradle toolchains make sure that the correct JDK will be installed before compiling anything.
We only require an java image that can "Start" Gradle 🙃

Can someone help me out to fix the graalVm23.patch file? 🙈

I tried various things, but non of the results look like it should (or maybe? 🤷 )
I tried to run updateDependencyLocks, but this seems to update too many updates.
I tried to run buildNative classes --update-locks org.graalvm.compiler:\*,org.graalvm.sdk:\*,org.graalvm.js:\*,org.graalvm.nativeimage:\*,org.graalvm.truffle:\*, but this seems not to update all of the dependencies even if I mentioning them 😭
The new patch file contains only 266 lines (instead of the orignal 530 lines).
For example, the whole stdlib/gradle.lockfile is not included in the patch at all.

So what I'm doing wrong?
How can I update it?
(Maybe these changes because of the Gradle 7.6 update? 🤷 I don't know)
This is the patchfile I end up with: https://pastebin.com/xFVt1XRa

…gurable

bioball · 2024-03-26T04:14:58Z

Hey @StefMa! Just getting around to looking at this.

To fix the patch file: you can apply the patch on a file-by-file basis using git apply --reject patches/graalVm23.patch. This will apply changes to files where there are no conflicts, and create .rej files for those where conflicts are found.

In this case, I think what you need to do is:

git apply --reject patches/graalVm23.patch
Delete all .rej files (we don't need those changes anymore thanks to this PR)
Create the new patch file
- git diff > patches/graalVm23.patch
Commit the new changed patch file
Cleanup; reset the rest of the un-staged changes

buildSrc/src/main/kotlin/JvmToolchain.kt

bioball · 2024-03-26T04:34:25Z

The CI tests are failing on task :pkl-config-java:generateTestConfigClasses for gradle-check-jdk17.

Cause:

Caused by: org.gradle.process.internal.ExecException: Process 'command '/usr/local/jdk-11.0.22/bin/java'' finished with non-zero exit value 1
        at org.gradle.process.internal.DefaultExecHandle$ExecResultImpl.assertNormalExitValue(DefaultExecHandle.java:431)
        at org.gradle.process.internal.DefaultJavaExecAction.execute(DefaultJavaExecAction.java:52)
        at org.gradle.api.tasks.JavaExec.exec(JavaExec.java:165)

I tried running this locally in docker using openjdk:11, and am seeing this:

> Task :pkl-config-java:generateTestConfigClasses FAILED
Error: LinkageError occurred while loading main class org.pkl.codegen.java.Main
        java.lang.UnsupportedClassVersionError: org/pkl/codegen/java/Main has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0

So, it seems like Gradle toolchains might affect built-in Java compile and run tasks, but for some reason it doesn't affect JavaExec. This looks related: gradle/gradle#16791

Seems like we can fix it by adding this to pklJavaLibrary.gradle.kts:

tasks.withType<JavaExec>().configureEach {
  javaLauncher.set(javaToolchains.launcherFor(java.toolchain))
}

buildSrc/src/main/kotlin/JvmToolchain.kt

netvl · 2024-04-02T18:32:03Z

buildSrc/build.gradle.kts

-      }
-    }
-  }
+  jvmToolchain(11)


I think it would it make sense to configure the global toolchain here as well, for consistency, via the java { toolchain { } } block.

The build.gradle.kts is only responsible for the code within buildSrc, right?
Since we are using Kotlin only here, why should we set the java.toolchain {}? 🤔

Or what do you mean by that?

Yes, you are correct that this build script is responsible for buildSrc.

In Gradle, the java plugin is always applied to JVM-based projects, even if the project uses some other language as the core of its implementation. Therefore, for consistency, it is considered idiomatic to use the java plugin methods to configure something first, and use language-specific methods to adjust the configuration, if necessary.

In the toolchain case, it is specified explicitly in Kotlin docs (see here) that the toolchain version in the java block will be used by Kotlin compilation tasks, therefore, it would be conventional to use the java block to set it up.

(BTW, it's curious that the docs state that setting the toolchain in the kotlin extension will also configure it for JavaCompile tasks; this is another reason to set it in the java block, since it is the built-in way to configure the toolchain, and their effects would be similar anyway)

StefMa · 2024-04-03T12:04:32Z

Thanks for you comment here @bioball !
I fixed it for the :pkl-config-java:generateTestConfigClasses tasks. See ad5df18
But now I'm running in another (same? similar?) issue for :pkl-cli:testStartJavaExecutable:

Error: LinkageError occurred while loading main class org.pkl.cli.Main
        java.lang.UnsupportedClassVersionError: org/pkl/cli/Main has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0

Setting the same code in the pklKotlinLibrary.gradle.kts doesn't change that.
Trying to setting the launcher in the custom task testJavaExecutable and testStartJavaExecutable (as well as changing from Exec to JavaExec) leads to another strange error:

Toolchain from executable property does not match toolchain from javaLauncher property.

I tried different things but run out of ideas here 🙈

netvl · 2024-04-11T17:09:24Z

buildSrc/src/main/kotlin/pklKotlinLibrary.gradle.kts

+kotlin.jvmToolchain {
+  languageVersion.set(jvmToolchainVersion)
+  vendor.set(jvmToolchainVendor)
+}


This should not be necessary because this plugin applies pklJavaLibrary, and the latter already applies the toolchain in the java {} block.

StefMa force-pushed the gradle-toolchain branch from ad2fe32 to fbc4556 Compare February 28, 2024 20:45

StefMa force-pushed the gradle-toolchain branch 2 times, most recently from d358b24 to 8bd04e2 Compare February 29, 2024 15:09

This comment was marked as resolved.

Sign in to view

StefMa force-pushed the gradle-toolchain branch 2 times, most recently from 8e59b6f to c240736 Compare February 29, 2024 19:00

StefMa force-pushed the gradle-toolchain branch 3 times, most recently from 55897f7 to c721eda Compare March 5, 2024 08:39

StefMa added 5 commits March 20, 2024 19:37

Add Gradle toolchain support

ff11f54

Move toolchain version and vendor to single place and make them confi…

0371c2f

…gurable

Update development.adoc

2921b89

Fix CI job

cffa8e0

Update CI config

5b2e95b

StefMa force-pushed the gradle-toolchain branch from c721eda to 5b2e95b Compare March 20, 2024 18:42

bioball reviewed Mar 26, 2024

View reviewed changes

buildSrc/src/main/kotlin/JvmToolchain.kt Outdated Show resolved Hide resolved

netvl reviewed Apr 2, 2024

View reviewed changes

buildSrc/src/main/kotlin/JvmToolchain.kt Outdated Show resolved Hide resolved

netvl reviewed Apr 2, 2024

View reviewed changes

StefMa added 2 commits April 3, 2024 13:06

Remove unnecessary conversion

8338c17

Set javaLauncher for JavaExec tasks

ad5df18

netvl reviewed Apr 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Gradle toolchain support #262

Add Gradle toolchain support #262

StefMa commented Feb 28, 2024 •

edited

Loading

odenix commented Feb 28, 2024

StefMa commented Feb 28, 2024

odenix commented Feb 28, 2024

odenix commented Feb 28, 2024 •

edited

Loading

StefMa commented Feb 29, 2024 •

edited

Loading

odenix commented Feb 29, 2024

This comment was marked as resolved.

StefMa commented Mar 5, 2024

StefMa commented Mar 5, 2024

bioball commented Mar 26, 2024

bioball commented Mar 26, 2024 •

edited

Loading

netvl Apr 2, 2024

StefMa Apr 3, 2024

netvl Apr 4, 2024

StefMa commented Apr 3, 2024

netvl Apr 11, 2024

	java {
	- sourceCompatibility = JavaVersion.VERSION_11
	- targetCompatibility = JavaVersion.VERSION_11
	+ sourceCompatibility = JavaVersion.VERSION_17
	+ targetCompatibility = JavaVersion.VERSION_17
	}

Add Gradle toolchain support #262

Are you sure you want to change the base?

Add Gradle toolchain support #262

Conversation

StefMa commented Feb 28, 2024 • edited Loading

odenix commented Feb 28, 2024

StefMa commented Feb 28, 2024

odenix commented Feb 28, 2024

odenix commented Feb 28, 2024 • edited Loading

StefMa commented Feb 29, 2024 • edited Loading

odenix commented Feb 29, 2024

This comment was marked as resolved.

StefMa commented Mar 5, 2024

StefMa commented Mar 5, 2024

bioball commented Mar 26, 2024

bioball commented Mar 26, 2024 • edited Loading

netvl Apr 2, 2024

Choose a reason for hiding this comment

StefMa Apr 3, 2024

Choose a reason for hiding this comment

netvl Apr 4, 2024

Choose a reason for hiding this comment

StefMa commented Apr 3, 2024

netvl Apr 11, 2024

Choose a reason for hiding this comment

StefMa commented Feb 28, 2024 •

edited

Loading

odenix commented Feb 28, 2024 •

edited

Loading

StefMa commented Feb 29, 2024 •

edited

Loading

bioball commented Mar 26, 2024 •

edited

Loading