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

Enable JS IR backend #1832

Merged
merged 4 commits into from
Mar 11, 2020
Merged

Enable JS IR backend #1832

merged 4 commits into from
Mar 11, 2020

Conversation

skuzmich
Copy link
Member

No description provided.

@skuzmich skuzmich requested a review from qwwdfsad February 27, 2020 12:25
@skuzmich skuzmich force-pushed the skuzmich/js-ir-1.4 branch from b26e921 to e961eb1 Compare March 2, 2020 14:16
gradle.properties Show resolved Hide resolved
gradle/compile-js-multiplatform.gradle Show resolved Hide resolved
}

sourceSets {
jsMain.dependencies {
api "org.jetbrains.kotlin:kotlin-stdlib-js:$kotlin_version"
api "org.jetbrains.kotlinx:atomicfu-js:$atomicfu_version"
Copy link
Member

Choose a reason for hiding this comment

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

dependency should be conditional and present only for IR backend

Copy link
Member Author

@skuzmich skuzmich Mar 4, 2020

Choose a reason for hiding this comment

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

It might be hard to enable it conditionally in this build script. @ilgonmic could you help?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that it should be not dependency in js source set but specific ir target compilation

js().irTarget?.compilations['main']?.dependencies {
   api "org.jetbrains.kotlinx:atomicfu-js:$atomicfu_version"
}


if (project.tasks.findByName('compileKotlinJsIr')) {
compileKotlinJsIr {
kotlinOptions.freeCompilerArgs += "-XXLanguage:-NewInference"
Copy link
Member

Choose a reason for hiding this comment

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

Please elaborate why NI is disabled

Copy link
Member

Choose a reason for hiding this comment

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

Same question regarding atomicfu

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted. It used to fail but now new inference is fixed for JS IR and disabled for classic backend.

@@ -5,6 +5,24 @@
apply plugin: 'kotlin-dce-js'
apply from: rootProject.file('gradle/node-js.gradle')

// Workaround resolving new Gradle metadata with kotlin2js
// TODO: Remove once plugin is fixed
Copy link
Member

Choose a reason for hiding this comment

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

...and how do we know that the plugin is fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Created issue

gradle/compile-js-multiplatform.gradle Show resolved Hide resolved

if (project.tasks.findByName('compileKotlinJsIr')) {
compileKotlinJsIr {
kotlinOptions.freeCompilerArgs += "-XXLanguage:-NewInference"
Copy link
Member

Choose a reason for hiding this comment

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

Same question regarding atomicfu

// disable metadata everywhere, but in native modules
if (type == 'maven' || type == 'metadata' || type == 'jvm' || type == 'js') {
// disable metadata everywhere, but in native and js modules
if (type == 'maven' || type == 'metadata' || type == 'jvm') {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it going to break our releases with Kotlin 1.3.70? Do we really have to publish metadata if IR BE is not here?

Copy link
Member Author

@skuzmich skuzmich Mar 4, 2020

Choose a reason for hiding this comment

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

Do we really have to publish metadata if IR BE is not here?

We don't

Isn't it going to break our releases with Kotlin 1.3.70?

I assumed adding metadata wouldn't break stuff. @ilgonmic please take a look

Copy link
Contributor

Choose a reason for hiding this comment

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

Since Gradle 5.3 it can consume metadata for dependency from repo. So for this version range it should not break release.
But this check can be covered with plus one check on 1.4 release, and not publish metadata for 1.3.70 releases

gradle/test-mocha-js.gradle Show resolved Hide resolved
@skuzmich skuzmich force-pushed the skuzmich/js-ir-1.4 branch from 82112d6 to 0b98fd4 Compare March 4, 2020 12:58
@skuzmich skuzmich requested review from qwwdfsad and ilgonmic March 4, 2020 13:11
@qwwdfsad qwwdfsad force-pushed the develop branch 3 times, most recently from 4a49830 to aff8202 Compare March 10, 2020 17:27
@qwwdfsad qwwdfsad merged commit fa97af2 into develop Mar 11, 2020
@qwwdfsad qwwdfsad deleted the skuzmich/js-ir-1.4 branch March 11, 2020 15:56
qwwdfsad added a commit that referenced this pull request Mar 13, 2020
* Enable JS IR backend
* Workaround resolving Gradle metadata in kotlin2js plugin

Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
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.

3 participants