-
Notifications
You must be signed in to change notification settings - Fork 41
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
Bump Kotlin version to 2.0.20 #255
Conversation
qurbonzoda
commented
Sep 2, 2024
- Bump Kotlin version to 2.0.20
- Drop obsolete tests
- Remove IR designation for JS target declarations
- Remove redundant dependsOn edges in runtime module
- Bump the minimum supported Kotlin version to 2.0.0 and verify it through tests
// The test uses Kotlin 1.9.24 as previous versions | ||
// would append the --experimental-wasm-gc flag causing run failures. | ||
val runner = project( | ||
"wasm-gc-non-experimental/wasm-nodejs", true, kotlinVersion = "1.9.24" |
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.
Since the supported Kotlin versions are not compatible with NodeJs versions earlier than 22.0.0, I am not sure if this use case is still viable: #213 (comment).
If it is, the code that adds the --experimental-wasm-gc
flag to NodeJsExec
should be preserved.
However, this test does not seem to make much sense in any case, since the supported Kotlin versions use NodeJs 22.0.0 or newer, and setting the nodeVersion
to an earlier version will cause the test project to fail.
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.
@adam-enko Could you please help me understand if the use case mentioned is still viable with Kotlin 2.0.20?
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 tried to build a 2.0.20-based project with NodeJS 21.0.0-v8-canary-*
and after a few a options added, everything continue working the same way it does with v22.
I don't know why one would do that in their projects, but it works.
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.
Ah, exactly. Kotlin/WasmJs requires a NodeJs version that supports the new Wasm GC.
In the mentioned use case, the NodeJs version is synced with the one used in Electron.
Hopefully, Electron or any other respectable tools do not ship with canary releases (?)
Anyways, it sounds like if we can drop --experimental-wasm-gc
for NodeJs.
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.
There are some small things to tidy up.
Please add the |
|
||
dependsOn(linkTask) | ||
|
||
val executableFile = linkTask.outputFile.get() | ||
val executableFile = linkTask.get().outputFile.get() |
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.
.get()
will eagerly configure the task and the value of outputFile
, and this should be avoided.
It'd probably work instead like this:
onlyIf { linkTask.get().outputFile.get().exists() }
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.
Could you please help me with the other use of executableFile
below?
Should it be linkTask.configure { this.executable.set(it.outputFile) }
?
This would also require changing NativeBenchmarkExec.executable
type to Property<File>
.
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.
Ah I see... yes, NativeBenchmarkExec.executable
should be updated to be compatible with the Provider API. Do you want do that now, or in another PR?
(BTW RegularFileProperty
or DirectoryProperty
is preferred over Provider<File>
.)
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 will do it in a separate PR.
It succeeds in Kotlin 2.0 as all JS targets use IR backend now. See https://youtrack.jetbrains.com/issue/KFC-113/Sunset-Old-JS-BE-and-make-JS-IR-BE-default
Starting from Kotlin 2.0 all JS targets use IR backend.
- store the minimal supported Gradle version in libs.versions.toml - check that the README is up-to-date in the CheckReadmeTask - add integration tests for checking the supported Kotlin version, and logged warning
The Kotlin/WasmJs in the min supported Kotlin (2.0.0) does not support NodeJs < 22.0.0
…ve benchmark compilation
In Kotlin 2.0.20 the workaround is no longer needed as it uses NodeJs 22.0.0+
b86152d
to
b78e50e
Compare