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

Support building with JDK 21 #234

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Support building with JDK 21 #234

merged 1 commit into from
Apr 12, 2024

Conversation

odenix
Copy link
Contributor

@odenix odenix commented Feb 22, 2024

JDK 21 is an LTS release that reached GA in September 2023.

@bioball
Copy link
Member

bioball commented Feb 22, 2024

I think we need to bump the version of google-java-format.

Error:

Caused by: java.lang.reflect.InvocationTargetException
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:118)
        at com.diffplug.spotless.java.GoogleJavaFormatStep$State.lambda$constructRemoveUnusedFunction$4(GoogleJavaFormatStep.java:211)
        at com.diffplug.spotless.java.GoogleJavaFormatStep$State.lambda$createFormat$1(GoogleJavaFormatStep.java:178)
        at com.diffplug.spotless.FormatterFunc.apply(FormatterFunc.java:32)
        at com.diffplug.spotless.FormatterStepImpl$Standard.format(FormatterStepImpl.java:82)
        at com.diffplug.spotless.FormatterStep$Strict.format(FormatterStep.java:88)
        at com.diffplug.spotless.Formatter.compute(Formatter.java:230)
        ... 119 more
Caused by: java.lang.NoSuchMethodError: 'com.sun.tools.javac.tree.JCTree com.sun.tools.javac.tree.JCTree$JCImport.getQualifiedIdentifier()'
        at com.google.googlejavaformat.java.RemoveUnusedImports.getSimpleName(RemoveUnusedImports.java:293)
        at com.google.googlejavaformat.java.RemoveUnusedImports.buildReplacements(RemoveUnusedImports.java:275)
        at com.google.googlejavaformat.java.RemoveUnusedImports.removeUnusedImports(RemoveUnusedImports.java:227)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)

Seems related to diffplug/spotless#1819

@odenix
Copy link
Contributor Author

odenix commented Feb 22, 2024

Fixed and tested locally.

Fun fact:

This release includes GraalVM native-image binaries for google-java-format for windows, linux, and mac.

@bioball
Copy link
Member

bioball commented Feb 22, 2024

Currently failing test (can you see the CI job?):

org.pkl.codegen.java.CliJavaCodeGeneratorTest

org.opentest4j.AssertionFailedError: 
expected: 
  "org.pkl.config.java.mapper.org.mod1\#Person=org.Mod1$Person
  org.pkl.config.java.mapper.org.mod1\#ModuleClass=org.Mod1"
 but was: 
  "#Java mappings for Pkl module `org.mod1`
  #Thu Feb 22 21:45:46 UTC 2024
  org.pkl.config.java.mapper.org.mod1\#ModuleClass=org.Mod1
  org.pkl.config.java.mapper.org.mod1\#Person=org.Mod1$Person
  "
	at [email protected]/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
	at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at app//org.pkl.codegen.java.CliJavaCodeGeneratorTest.assertContains(CliJavaCodeGeneratorTest.kt:181)
	at app//org.pkl.codegen.java.CliJavaCodeGeneratorTest.module inheritance(CliJavaCodeGeneratorTest.kt:100)
	at [email protected]/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:580)
	at app//org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727)
	at app//org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
	at app//org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:217)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:213)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:138)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:68)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1596)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at [email protected]/java.util.ArrayList.forEach(ArrayList.java:1596)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
	at app//org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at app//org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:107)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
	at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
	at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:53)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	at [email protected]/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:580)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at jdk.proxy1/jdk.proxy1.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)

@odenix
Copy link
Contributor Author

odenix commented Feb 22, 2024

Fixed the failing test. I can see the CI job. I just never know when you guys will trigger it. :-)

@odenix
Copy link
Contributor Author

odenix commented Feb 23, 2024

Looks like Gradle 7.5 doesn't support Java 21. Another reason to update Gradle. Strange that it worked on my machine.

PS: Could you omit --stacktrace (and ideally also --info) for PR builds? The build log is way too verbose.

@bioball
Copy link
Member

bioball commented Feb 23, 2024

Do you see the "tests" tab? That's what I use to filter out the noise.

Screenshot 2024-02-22 at 7 43 46 PM

Probably fine to get rid of --stacktrace from these PRBs though.

@odenix
Copy link
Contributor Author

odenix commented Feb 23, 2024

Ah I didn't realize what the Tests tab does. Getting rid of --stacktrace would help a lot.

@bioball
Copy link
Member

bioball commented Feb 23, 2024

On second thought, I think let's keep --stacktrace. It's verbose, but the test tools help, and it's extremely helpful the few times where we actually do need it.

@odenix
Copy link
Contributor Author

odenix commented Feb 23, 2024

It's still a poor experience, but OK. I don't recall the last time I ran a Gradle build with --stacktrace.

@odenix
Copy link
Contributor Author

odenix commented Feb 24, 2024

Rebased onto #245.

@odenix odenix changed the title Add CI job for JDK 21 Support building with JDK 21 Mar 21, 2024
@odenix
Copy link
Contributor Author

odenix commented Mar 21, 2024

@bioball Rebased and improved to fix most deprecation warnings

@odenix
Copy link
Contributor Author

odenix commented Mar 22, 2024

The only failing CI build is check-patch-file. Not sure what to do about this.

@bioball
Copy link
Member

bioball commented Mar 22, 2024

This means that there was a conflict when applying the graalVm23.patch file.

You can use git apply --reject patches/graalVm23.patch to apply the patch on a file-by-file basis to figure out which one failed to apply. That should point you to the code change that made the patch fail, and you can adjust the patch file accordingly.

If you're curious: we are using this patch file to bump the GraalVM version to 23 only for darwin/aarch64. We don't use Graal 23 otherwise because we need to support JDK11 (for now). Once we drop JDK11 support, we'll bump GraalVM to 23.x and the patch file will go away.

@odenix
Copy link
Contributor Author

odenix commented Mar 22, 2024

Sounds cumbersome. Is this the reason why Pkl's dependencies are so outdated?

I was planning to send a couple of PRs that gradually update all of Pkl's dependencies.
However, fixing the patch file every time doesn't seem feasible.
Would you accept one large PR that updates all/most dependencies at once, provided the build remains green?

For my understanding, what prevents you from staying compatible with JDK 11 and building native executables with JDK 17, without any further changes?

@odenix
Copy link
Contributor Author

odenix commented Mar 26, 2024

@bioball How can we move forward with updating Pkl's dependencies and keeping them up-to-date?
Are you aware of a legitimate reason why Pkl's native build still uses JDK11/GraalVM22?
If not, I'll try to move the native build to JDK17/GraalVM23 while keeping JDK11 compatibility for the Java build.
The decoupling of JDK versions seems desirable in any case.

@odenix
Copy link
Contributor Author

odenix commented Mar 26, 2024

Is the problem related to newer Truffle library versions requiring Java 17?

What's the status of moving to JDK 17? Do users that are still on JDK 11 really need the latest and greatest Pkl Java library version? Can't they stay on 0.25.x until they upgrade to JDK 17?

@bioball
Copy link
Member

bioball commented Mar 26, 2024

RE: Java 11: TBD. This is still something we are coming up with an answer for. But, let's not make any build changes until we have an answer here. If we end up shedding Java 11, there's no need to do any extra work. We can just apply the patch file, commit those changes, and move on.

Also, yeah, the issue is that GraalVM's tooling and SDKs dropped support for Java 11, but we needed to make sure that our Java libraries are still compatible. And, there's no reason we can't build the native executables themselves with Java 17.

@bioball
Copy link
Member

bioball commented Mar 27, 2024

@bioball How can we move forward with updating Pkl's dependencies and keeping them up-to-date?

Hold off on this and also #350 for now until we figure out our Java 11 situation

- Update google-java-format to a version compatible with JDK 21 and run "gw spotlessApply".
- Fix wrong test assumption
  JavaCodeGenerator writes a properties file using java.util.Properties,
  which doesn't guarantee order of entries.
- Fix most deprecation warnings
- Add CI job for JDK 21
@odenix
Copy link
Contributor Author

odenix commented Apr 12, 2024

Rebased and patch updated. Local build with JDK 21 succeeds. Ready to merge from my side.

Copy link
Member

@bioball bioball left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bioball bioball merged commit 90b461a into apple:main Apr 12, 2024
6 checks passed
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.

2 participants