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

Bump Kotlin version to 2.0.20 #255

Merged
merged 12 commits into from
Sep 10, 2024
Merged

Conversation

qurbonzoda
Copy link
Collaborator

  • 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"
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

README.md Show resolved Hide resolved
Copy link
Member

@adam-enko adam-enko left a 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.

@adam-enko
Copy link
Member

Please add the .kotlin/ directory to .gitignore

plugin/build.gradle Outdated Show resolved Hide resolved

dependsOn(linkTask)

val executableFile = linkTask.outputFile.get()
val executableFile = linkTask.get().outputFile.get()
Copy link
Member

@adam-enko adam-enko Sep 5, 2024

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

Copy link
Collaborator Author

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

Copy link
Member

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

Copy link
Collaborator Author

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.

qurbonzoda and others added 12 commits September 9, 2024 23:41
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
In Kotlin 2.0.20 the workaround is no longer needed as it uses NodeJs 22.0.0+
@qurbonzoda qurbonzoda merged commit ce7bca6 into master Sep 10, 2024
1 check passed
@qurbonzoda qurbonzoda deleted the qurbonzoda/bump-kotlin-version branch September 10, 2024 07:36
This pull request was closed.
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.

4 participants