-
Notifications
You must be signed in to change notification settings - Fork 288
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
JDK 21 toolchain #876
base: main
Are you sure you want to change the base?
JDK 21 toolchain #876
Conversation
8c2a05f
to
dc45756
Compare
pkl-core/src/test/files/LanguageSnippetTests/input/errors/analyzeInvalidHttpModule.inert
Outdated
Show resolved
Hide resolved
This is great, thanks, Sam! I will do a review of this by next week. FYI: Pkl today supports Java 21. The range enabled here is Java 22+ (updated your PR title). |
This comment was marked as outdated.
This comment was marked as outdated.
@sgammon feel free to extract the toolchain support into another PR. Indeed, my PR is quited dated. I will close it... |
@StefMa roger, no worries :) I am a well known offender in that regard |
dc45756
to
1982880
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
Also, general note: we'll need to set up our CI to use Java 21 to compile (for various compileJava
tasks), and ideally to test with all of JDK 17/21/22+. I actually think toolchains probably makes sense for this, so, I'd be okay with the toolchain changes coming back into this PR.
With toolchains, maybe the version of Java used for tasks.test
can come from a build flag.
pkl-core/src/main/resources/META-INF/native-image/org.pkl-lang/native-image.properties
Outdated
Show resolved
Hide resolved
1982880
to
a314e92
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
f9daed3
to
263588b
Compare
Some quick benchmarks as a sanity check. Not a huge improvement but no regressions either, which is what I was worried about. These are benchmarks using the native binary, compared with the Summary
Speed
FootprintOn disk
In-memory
Full result at
|
8bfd605
to
d087ea6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
61b5e30
to
65d0ce3
Compare
@bioball I've rebased this against latest |
Sounds great! |
8ad9dd5
to
465337a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
465337a
to
5f98585
Compare
This comment was marked as outdated.
This comment was marked as outdated.
69750d3
to
d6a8801
Compare
pkl-core/src/test/kotlin/org/pkl/core/LanguageSnippetTestsEngine.kt
Outdated
Show resolved
Hide resolved
d6a8801
to
e85b259
Compare
This comment was marked as resolved.
This comment was marked as resolved.
e85b259
to
8d9c22c
Compare
// Extra JPMS modules forced onto the module path via `--add-modules` in some cases. | ||
private val jpmsAddModules = arrayOf("jdk.unsupported") | ||
|
||
// Formats `jpmsExports` for use in JAR manifest attributes. | ||
val jpmsExportsForJarManifest: String by lazy { | ||
jpmsExports.joinToString(" ") { it.substringBefore("=") } | ||
} | ||
|
||
// Formats `jpmsExports` for use on the command line with `--add-exports`. | ||
val jpmsExportsForAddExportsFlags: Collection<String> by lazy { | ||
jpmsExports.map { "--add-exports=$it" } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to list the full suite of JPMS settings within BuildInfo
, because it's much easier to use them throughout the codebase and avoid error-prone repetition; instead of rendering things in each project, jpmsExportsForJarManifest
provides a JAR manifest value, and jpmsExportsForAddExportsFlags
provides command-line flags.
This way, things are guaranteed to remain consistent across projects, and the actual set of JPMS exports can be made a private
implementation detail.
val jdkTestRange: Collection<JavaLanguageVersion> by lazy { | ||
JavaVersionRange.inclusive(jdkTestFloor, jdkTestCeiling).filter { version -> | ||
// unless we are instructed to test all JDKs, tests only include LTS releases and | ||
// versions above the toolchain version. | ||
testAllJdks || (JavaVersionRange.isLTS(version) || version >= jdkToolchainVersion) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filtering for JDK testing based on flags like testAllJdks
now applies to all multi-JDK-related tasks, instead of just tests. Things like testStartJavaExecutable
now respect these flags, where previously they did not.
// In CI, this defaults to `true` to catch potential cross-JDK compat regressions or other bugs. | ||
// In local dev, this defaults to `false` to speed up the build and reduce contributor load. | ||
System.getProperty("pklMultiJdkTesting")?.toBoolean() ?: isCiBuild | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of disabling pklMultiJdkTesting
by default, I've kept it enabled in CI, and disabled by default in local dev.
// Setup `testJavaExecutable` tasks for multi-JDK testing. | ||
val testJavaExecutableOnOtherJdks = | ||
if (buildInfo.multiJdkTesting) { | ||
buildInfo.multiJdkTestingWith(testJavaExecutable) | ||
} else { | ||
emptyList() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like multi-JDK tests, testJavaExecutableOnJdk*
and friends now only run as dependencies of check
iff (we are in ci) OR (the local dev has activated multi-JDK testing)
.
// 0.28 Preparing for JDK21 toolchains revealed that `testStartJavaExecutable` may pass, even though | ||
// the evaluator fails. To catch this, we need to test the evaluator. We render the CircleCI config | ||
// as a realistic test of the fat JAR. | ||
val testEvalJavaExecutable by | ||
setupJavaExecutableRun("testEvalJavaExecutable", evalTestFlags) { useRootDirAndSuppressOutput() } | ||
|
||
// Run the same evaluator tests on all configured JDK test versions. | ||
val testEvalJavaExecutableOnOtherJdks = | ||
buildInfo.jdkTestRange.map { jdkTarget -> | ||
setupJavaExecutableRun( | ||
"testEvalJavaExecutableJdk${jdkTarget.asInt()}", | ||
evalTestFlags, | ||
serviceOf<JavaToolchainService>().launcherFor { languageVersion = jdkTarget }, | ||
) { | ||
useRootDirAndSuppressOutput() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new tests effectively run:
jpkl eval ./.circleci/config.pkl > /dev/null
... to make sure that fat JARs run as expected end-to-end. This change is inspired by the testStartJavaExecutableJdk17
passing even though GraalVM 17 ultimately refused to load the evaluator.
Same as all other multi-JDK tests, jpkl eval ...
will be run on the toolchain JDK only unless multi-JDK testing is active.
- feat: support for jvm21+ toolchain - feat: support for gradle toolchains - feat: pass `-PnativeArch=native` to build with `-march=native` - test: multi-jdk testing support - test: support for `jvm-test-suite` plugin - test: add tasks to run `jpkl eval` on multiple jdks - test: make jdk exec tests respect multi-jdk flags and ranges - fix: remove mrjar classes at >jvm17 from fatjars - fix: use jdk21 to run the tests (needed for `Unsafe.ensureInitialized`) - fix: truffle svm dependency is required after graalvm `24.0.0` - fix: warnings for gvm flag usage, renamed truffle svm macro - fix: build with `--add-modules=jdk.unsupported` where needed - fix: don't use `gu` tool for modern graalvm versions - fix: catch `Throwable` instead of deprecated-for-removal `ThreadDeath` - chore: buildinfo changes for JVM targets, toolchains - chore: enforce testing at exactly jdk21 - chore: enforce build tooling at jdk21+ - chore: bump graalvm/truffle libs → `24.1.2` - chore: toolchains for `buildSrc` Signed-off-by: Sam Gammon <[email protected]>
- chore: update to jdk21 in circle jobs - chore: re-gen circle jobs Signed-off-by: Sam Gammon <[email protected]>
8d9c22c
to
a14ef41
Compare
@bioball This is ready for final review again :) I've left a self-review detailing the cleanups I've applied. Thank you for your patience while I figured out that GVM 17 bug. |
Summary
This PR updates dependencies and build process for Pkl to bring GraalVM and Truffle support up to their latest versions. Today, Pkl builds against Java 17+, and requires Java 17+ to run.
After merging this PR, Pkl would build against Java 21, test against Java 21, and impose no change to lib consumers (i.e. still only requiring Java 17+ to run or transitively compile against Pkl).
Test downstream here (updated), via Github Actions. On that fork PR, the change set is expressed in full, with fixups.
PR Tree
native-image
builds #914 (split out into independent PR)kotlinc
#920 (split out into independent PR)Rationale
Gradle Toolchains: Uses Gradle's toolchains feature to provision and use different versions of the JDK.
JDK 21 Toolchain: Builds (and tests) occur under JDK 21, with bytecode targeting JVM 17. Build bytecode adopts JVM 21 to accelerate build tooling with no change to lib consumers.
Truffle SVM Dependency: Certain superclasses used by Pkl (notably,
AbstractTruffleException
andTruffleFeature
) have moved to the neworg.graalvm.nativeimage:truffle-runtime-svm
coordinate.Multi-JDK testing: Adds stronger guarantees for backward compatibility with regard to Pkl and JVM execution (i.e.
java -jar ...
with a fat JAR). Tests against versions 18-23 with test suites andjava -jar ...
executions.Known Issues
pkl-gradle
)--add-exports=...
Pre-merge Checklist
Changelog
-PnativeArch=native
to build with-march=native
jvm-test-suite
pluginjpkl eval
on multiple jdksUnsafe.ensureInitialized
)24.0.0
--add-modules=jdk.unsupported
where neededgu
tool for modern graalvm versionsThrowable
instead of deprecated-for-removalThreadDeath
24.1.2
buildSrc