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

Add Gradle toolchain support #262

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Feb 28, 2024

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

Let me know if there is more like these 🙃

@translatenix
Copy link
Contributor

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
Copy link
Contributor Author

StefMa commented Feb 28, 2024

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

@translatenix
Copy link
Contributor

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

@translatenix
Copy link
Contributor

translatenix commented Feb 28, 2024

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
Copy link
Contributor Author

StefMa commented Feb 29, 2024

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.

@translatenix
Copy link
Contributor

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 StefMa force-pushed the gradle-toolchain branch 2 times, most recently from d358b24 to 8bd04e2 Compare February 29, 2024 15:09
@StefMa

This comment was marked as resolved.

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

StefMa commented Mar 5, 2024

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

Lets wait for #245

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

StefMa commented Mar 5, 2024

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

  1. 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 🙃

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

@bioball
Copy link
Contributor

bioball commented Mar 26, 2024

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:

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

@bioball
Copy link
Contributor

bioball commented Mar 26, 2024

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))
}

}
}
}
jvmToolchain(11)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

StefMa commented Apr 3, 2024

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 🙈

Comment on lines +27 to +30
kotlin.jvmToolchain {
languageVersion.set(jvmToolchainVersion)
vendor.set(jvmToolchainVendor)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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.

Cosindering using Gradle Java toolchain to avoid Java 11 check
4 participants