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

chore: support for kotlin 2.x #147

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sgammon
Copy link

@sgammon sgammon commented Feb 14, 2024

Summary

Initial changes to support Kotlin 2.x.x beta.

Changelog

  • chore: changes to support kotlin 2.x
  • chore: changes to support newer serialization plugin
  • chore: upgrade gradle → 8.6
  • chore: upgrade detekt → latest

- chore: changes to support kotlin `2.x`
- chore: changes to support newer serialization plugin
- chore: upgrade gradle → `8.6`
- chore: upgrade detekt → latest

Signed-off-by: Sam Gammon <[email protected]>
@@ -102,7 +102,7 @@ subprojects {
localDirectory = [email protected]("src/main/kotlin")

remoteUrl =
URL("https://github.com/JetBrains-Research/reflekt/tree/master/using-embedded-kotlin/${[email protected]}/src/main/kotlin/")
uri("https://github.com/JetBrains-Research/reflekt/tree/master/using-embedded-kotlin/${[email protected]}/src/main/kotlin/").toURL()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it relevant? :/

Copy link
Author

Choose a reason for hiding this comment

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

Fixes a build warning, I can roll it back if you want

Comment on lines +23 to +39
kotlin {
compilerOptions {
apiVersion = ktTarget
languageVersion = ktTarget
freeCompilerArgs = freeCompilerArgs.get().plus(ktCompilerArgs)
}
}

afterEvaluate {
tasks.withType(KotlinCompile::class).configureEach {
kotlinOptions {
apiVersion = ktTarget.version
languageVersion = ktTarget.version
freeCompilerArgs = freeCompilerArgs.plus(ktCompilerArgs)
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary duplication?

Copy link
Author

Choose a reason for hiding this comment

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

Hopefully this can replace the duplication in other modules, but I can roll back this project if you'd rather express these flags in each module.

@@ -1,2 +1,4 @@
kotlin.code.style=official
org.gradle.jvmargs=-XX:MaxMetaspaceSize=512m
org.gradle.caching=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's useless for local builds. Build cache is good for CI, because on CI, you can grab the caches after build to reuse them in the next run.

Suggested change
org.gradle.caching=true

@@ -1,2 +1,4 @@
kotlin.code.style=official
org.gradle.jvmargs=-XX:MaxMetaspaceSize=512m
org.gradle.caching=true
org.gradle.parallel=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's true by default.

Suggested change
org.gradle.parallel=true

import org.jetbrains.kotlin.gradle.dsl.KotlinVersion
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

val libs = the<LibrariesForLibs>()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused val.

includeBuild("using-embedded-kotlin")

enableFeaturePreview("STABLE_CONFIGURATION_CACHE")
enableFeaturePreview("TYPESAFE_PROJECT_ACCESSORS")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feature preview enabled, but never used.

includeBuild("using-embedded-kotlin")

enableFeaturePreview("STABLE_CONFIGURATION_CACHE")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please explain how this preview helps.

Copy link
Author

Choose a reason for hiding this comment

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

I added this because it improves the configuration cache experience, at least for me; doesn't need to be in this PR, though. I'll remove it.


dependencies {
implementation(libs.plugin.kotlin)
implementation(files(libs.javaClass.superclass.protectionDomain.codeSource.location))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is going on here?

Copy link
Author

Choose a reason for hiding this comment

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

This is a fix from the Gradle team which makes Version Catalog symbols visible within the precompiled build script (the convention plugin). I can add a comment which explains it because otherwise it's pretty mysterious.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why can't this logic be just in a couple of build files? buildSrc and convention plugins were avoided for clarity, preventing mix-up of classpathes and build performance.

Copy link
Author

Choose a reason for hiding this comment

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

I've structured this as a single convention plugin, so that compiler flags are consistent across modules. To use Kotlin 2.0.0 beta, we'll need to pass at least some compiler flags to solve OptIns and allow unstable outputs.

I don't think the flags need to be kept after Kotlin 2.0.0 release, so I can add them to each module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make it so. If we had more modules, of course, a convention plugin would be necessary.

Comment on lines +17 to +20
"-Xskip-prerelease-check",
"-Xsuppress-version-warnings",
"-Xallow-unstable-dependencies",
"-opt-in=org.jetbrains.kotlin.ir.symbols.UnsafeDuringIrConstructionAPI",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need all these compiler keys now, considering that everything was fine?

Copy link
Author

Choose a reason for hiding this comment

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

Some of these are needed for Kotlin 2.0, in particular the final two. I will check if the first two are needed.

@sgammon
Copy link
Author

sgammon commented Feb 14, 2024

@CommanderTvis thank you for the quick review, sorry for the mess 😅 we are trying this downstream to see if it works as expected.

@sgammon
Copy link
Author

sgammon commented Feb 14, 2024

With regard to the NoSuchMethodError, here is the exception silenced by that code:

java.lang.NoSuchMethodError: 'void com.intellij.openapi.util.io.NioFiles.deleteRecursively(java.nio.file.Path)'
	at org.jetbrains.kotlin.test.services.impl.TemporaryDirectoryManagerImpl.cleanupTemporaryDirectories(TemporaryDirectoryManagerImpl.kt:45)
	at org.jetbrains.kotlin.test.TestRunner.runTest(TestRunner.kt:32)

I remember now. This deleteRecursively function on com.intellij.openapi.util.io.NioFiles is called by AbstractKotlinCompilerTest.runTest. It seems the Open API dependency for IntelliJ may need to be upgraded, but I don't see where it is being pulled in. Do you have any ideas? Full stacktrace below.

Stacktrace
java.lang.NoSuchMethodError: 'void com.intellij.openapi.util.io.NioFiles.deleteRecursively(java.nio.file.Path)'
	at org.jetbrains.kotlin.test.services.impl.TemporaryDirectoryManagerImpl.cleanupTemporaryDirectories(TemporaryDirectoryManagerImpl.kt:45)
	at org.jetbrains.kotlin.test.TestRunner.runTest(TestRunner.kt:32)
	at org.jetbrains.kotlin.test.TestRunner.runTest$default(TestRunner.kt:27)
	at org.jetbrains.kotlin.test.runners.AbstractKotlinCompilerTest.runTest(AbstractKotlinCompilerTest.kt:88)
	at org.jetbrains.reflekt.plugin.compiler.runners.general.ReflektWithLibraryTestGenerated$Classes$Classes_1.testClasses_1(ReflektWithLibraryTestGenerated.java:46)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.concurrent.RecursiveAction.exec(RecursiveAction.java:194)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.tryRemoveAndExec(ForkJoinPool.java:1351)
	at java.base/java.util.concurrent.ForkJoinTask.awaitDone(ForkJoinTask.java:422)
	at java.base/java.util.concurrent.ForkJoinTask.join(ForkJoinTask.java:651)
	at java.base/java.util.concurrent.RecursiveAction.exec(RecursiveAction.java:194)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.tryRemoveAndExec(ForkJoinPool.java:1351)
	at java.base/java.util.concurrent.ForkJoinTask.awaitDone(ForkJoinTask.java:422)
	at java.base/java.util.concurrent.ForkJoinTask.join(ForkJoinTask.java:651)
	at java.base/java.util.concurrent.RecursiveAction.exec(RecursiveAction.java:194)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.tryRemoveAndExec(ForkJoinPool.java:1351)
	at java.base/java.util.concurrent.ForkJoinTask.awaitDone(ForkJoinTask.java:422)
	at java.base/java.util.concurrent.ForkJoinTask.join(ForkJoinTask.java:651)
	at java.base/java.util.concurrent.RecursiveAction.exec(RecursiveAction.java:194)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1312)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1843)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1808)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:188)

It was being suppressed because it's deleting temporary files, otherwise the tests seem to pass.

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