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

Move the logic from koltin repo atomicfu gradle plugin to the gradle plugin in the library. #406

Merged
merged 18 commits into from
Mar 20, 2024

Conversation

mvicsokolova
Copy link
Collaborator

@mvicsokolova mvicsokolova commented Feb 29, 2024

This is a reproducer for the problem #399

According to our last solution (#386), atomicfu-gradle-plugin provides a transitive atomicfu compiler plugin dependency and relies on the gradle version resolution. So, we provide compiler plugin with the lowest version and expect it to be resolved into the highest version present on the user's classpath.

runtimeOnly "org.jetbrains.kotlin:atomicfu:1.6.21"

But this does not work for the project configuration presented in this PR. This is a minimized version of an MPP project created by a MPP Wizard, which uses libs.versions.toml file as a version catalog.

How to reproduce the failure:

cd integration-testing/examples/compiler-plugin-resolution-bug
gradle clean build

The build will fail with the error:

* Where:
Build file '/Users/Maria.Sokolova/IdeaProjects/kotlinx-atomicfu-new/integration-testing/examples/compiler-plugin-resolution-bug/shared/build.gradle.kts' line: 16

* What went wrong:
org/jetbrains/kotlinx/atomicfu/gradle/AtomicfuKotlinGradleSubplugin$AtomicfuKotlinGradleExtension
> org.jetbrains.kotlinx.atomicfu.gradle.AtomicfuKotlinGradleSubplugin$AtomicfuKotlinGradleExtension

NOTE: if contents of compiler-plugin-resolution-bug/build.gradle.kts are removed, the build completes successfully:

plugins {
    // this is necessary to avoid the plugins to be loaded multiple times
    // in each subproject's classloader
    alias(libs.plugins.kotlinMultiplatform) apply false
}

@mvicsokolova mvicsokolova force-pushed the compiler-plugin-dependency branch from a0877cd to 1c1b02b Compare March 1, 2024 09:33
@mvicsokolova mvicsokolova marked this pull request as ready for review March 5, 2024 13:26
@mvicsokolova
Copy link
Collaborator Author

The last commit suggests an alternative solution to the problem of 2 atomicfu gradle plugins (#370):
the logic of the gradle plugin from the Kotlin repo (org.jetbrains.kotlinx.atomicfu.gradle.AtomicfuKotlinGradleSubplugin) that applies atomicfu compiler plugin transformations may be moved to the library starting from Kotlin 1.9.0.
It's possible because the sources of the compiler plugin are being published since Kotlin 1.9.0: kotlin-atomicfu-compiler-plugin-embeddable

This will solve the problem of Kotlin version resolution "before" kotlinx-atomicfu plugin application.

The current solution works in overall, but still has several issues to be solved:

  1. As the kotlin.native.version may be overriden by a user and differ from the KGP version, the Native part of the compiler plugin should be applied with kotlin version equal tokotlin.native.version. For now, the Native part of the compiler plugin is separated from the JVM and JS parts but kotlin.native.version is not extracted.
  2. Till this moment, we didn't pay attention to the fact that KGP version may differ from the version of Kotlin compiler. In atomicfu-gradle-plugin we extract KGP version. Though there is no public API for that yet (I'll attach the issue here).
  3. A test should be added for the case if kotlinx-atomicfu is applied to the project that uses Kotlin version older than 1.9.0

@mvicsokolova
Copy link
Collaborator Author

mvicsokolova commented Mar 6, 2024

As the problems 1 and 2 are not critical now, and there were no actual user cases of version difference, I think it's reasonable to postpone their solution (#408) until we have public API to retrieve Kotlin compiler and Kotlin native compiler versions. Here is the corresponding ticket: KT-66384.

I'll remove the separation of JVM/JS and Native compiler plugins and use KGP version to get the compiler plugin artifact, as it was done before.

}
}
}
error("You are applying `kotlinx-atomicfu` plugin of version 0.23.3 or newer. " +
Copy link
Collaborator Author

@mvicsokolova mvicsokolova Mar 7, 2024

Choose a reason for hiding this comment

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

Previously, in case of KGP version < 1.9.0, I asked users to add org.jetbrains.kotlin:atomicfu dependency manually to the buildscript. However, this caused further problems, that already were solved in higher Kotlin versions.

E.g. the problem below would occur if you try to apply the current kotlinx-atomicfu version with Kotlin 1.8.20 and just add the kotlin:atomicfu:1.8.20 dependency manually (this problem was solved in 0.23.0 and Kotlin 1.9.10)

e: Could not find ".gradle/caches/modules-2/files-2.1/org.jetbrains.kotlinx/atomicfu-linuxarm64/0.23.2/7b4270560c2723dc55b1d3c8925b7b93201c437b/atomicfu.klib" in [IdeaProjects/kotlinx-atomicfu-new/integration-testing/examples/mpp-sample, .konan/klib, .konan/kotlin-native-prebuilt-macos-aarch64-1.8.20/klib/common, .konan/kotlin-native-prebuilt-macos-aarch64-1.8.20/klib/platform/linux_arm64]

So, I decided to write this error message about incompatibility instead.
I've checked that the sample mpp project can be built with kotlinx-atomicfu 0.22.0 and Kotlin down to 1.6.0, though I believe these are very marginal use cases.

@fzhinkin WDYT? 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

version 0.23.3 or newer

Can we print the exact version here?

I think it should be enough to:

  • mention that Kotlin version should be upgraded
  • add a document (like readme) describing an alternative solution for those who can't upgrade the Kotlin for some reason

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've added a note about downgrading atomicfu version as an alternative solution in the README. And in the message left just the recommendation to upgrade the Kotlin version.

@mvicsokolova mvicsokolova changed the title WIP: Compiler plugin is not resolved Move the logic from koltin repo atomicfu gradle plugin to the gradle plugin in the library. Mar 7, 2024
@mvicsokolova mvicsokolova requested a review from fzhinkin March 7, 2024 11:24
}
}
}
error("You are applying `kotlinx-atomicfu` plugin of version 0.23.3 or newer. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

version 0.23.3 or newer

Can we print the exact version here?

I think it should be enough to:

  • mention that Kotlin version should be upgraded
  • add a document (like readme) describing an alternative solution for those who can't upgrade the Kotlin for some reason

@mvicsokolova mvicsokolova requested a review from fzhinkin March 13, 2024 12:53
README.md Outdated Show resolved Hide resolved
…/gradle/AtomicfuKotlinCompilerPluginInternal.kt

Co-authored-by: Filipp Zhinkin <filipp.zhinkin@jetbrains.com>
README.md Show resolved Hide resolved

* Gradle `7.0` or newer

* Kotlin `1.7.0` or newer
* Kotlin `1.9.0` or newer
Copy link
Contributor

Choose a reason for hiding this comment

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

## Atomicfu compiler plugin sections seems to be obsolete as it describes how the plugin should be configured for older versions.

As there's a suggestion to use an older atomicfu version with older Kotlin version, it does make sense to move everything related to older versions to a dedicated page (or leave it here, but stress that it only applies to older atomicfu versions).

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've fixed this section, added a note about Kotlin version requirements for the current atomicfu version and moved old configurations to the hidden section.

Copy link
Contributor

@fzhinkin fzhinkin left a comment

Choose a reason for hiding this comment

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

LGTM

@mvicsokolova mvicsokolova merged commit 387c3db into develop Mar 20, 2024
2 checks passed
@mvicsokolova mvicsokolova deleted the compiler-plugin-dependency branch March 20, 2024 14:38
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