[Gradle] Ensure Kotlin ecosystem plugin brings Kotlin Gradle plugin classes into build classpath#6296
Conversation
Code Owners
|
Egor Andreevich (Egorand)
left a comment
Egor Andreevich (Egorand)
left a comment
There was a problem hiding this comment.
Have concerns about how future-proof the approach is, but overall looks good!
/final
|
|
||
| dependencies { | ||
| commonApi(platform(project(":kotlin-gradle-plugins-bom"))) | ||
| commonApi(project(":kotlin-gradle-plugin")) |
There was a problem hiding this comment.
My main concern is that I believe this would break if kotlin-dsl adds another dependency governed by kotlin-gradle-plugins-bom, and we'll have to then explicitly add it here to avoid another version clash? Though if the possibility of kotlin-dsl acquiring another dependency is very low then I'm fine with this approach. I'd suggest adding a comment explaining why we're doing this!
A larger question I guess is whether we want the ecosystem plugin to enforce version alignment across subprojects (including buildSrc) of a project it's added to? I'm not familiar with how to properly do this from a settings plugin, but component metadata rules seems to be one way?
There was a problem hiding this comment.
Here is a build scan from the added test. As you see kotlin-dsl plugin still brings KGP and kotlin-sam-with-receiver plugins version 1.7.10 (Gradle Kotlin runtime) and it does not fail the build.
1.7.10 version here could be surprising but on the classloaders level it shoud use the snapshot one brought by settings plugin.
There was a problem hiding this comment.
A larger question I guess is whether we want the ecosystem plugin to enforce version alignment across subprojects (including buildSrc) of a project it's added to?
I would say - no. Included projects should be self-contained, and we should not try to align versions between them. Specifically, from use-case of dynamically added included builds.
There was a problem hiding this comment.
Here is a build scan from the added test. As you see kotlin-dsl plugin still brings KGP and kotlin-sam-with-receiver plugins version 1.7.10 (Gradle Kotlin runtime) and it does not fail the build.
1.7.10 version here could be surprising but on the classloaders level it shoud use the snapshot one brought by settings plugin.
Right, but I imagine it would still eventually use version 1.7.10 of kotlin-sam-with-receiver? My guess is that there's either no binary incompatibility between kotlin-sam-with-receiver:1.7.10 and kotlin-sam-with-receiver:2.4.255-SNAPSHOT, or kotlin-dsl doesn't really use any API from that plugin. If it did - we would've probably had to add an explicit dependency on kotlin-sam-with-receiver to the ecosystem plugin as well? Which demonstrates the larger problem: any Kotlin artifacts kotlin-dsl depends on, the ecosystem plugin should depend on as well.
There was a problem hiding this comment.
My guess is that there's either no binary incompatibility between kotlin-sam-with-receiver:1.7.10 and kotlin-sam-with-receiver:2.4.255-SNAPSHOT
There is definitely some binary incompatibility between these versions
|
/safe-merge |
|
Command rejected: Missing code owners approval - verify the check 'Code Owners Approval' is green. |
Yahor Berdnikau (Tapchicoma)
assigned
Cristian Garcia (CristianGM) and unassigned
Cristian Garcia (CristianGM)
Yahor Berdnikau (Tapchicoma)
requested a review
from Cristian Garcia (CristianGM)
|
/safe-merge |
kotlin-safemerge
Bot
force-pushed
the
yahor/kt-78294-fix-settings-plugin
branch
from
b46cf54 to
e950b91
Compare
|
Quality gate is triggered at https://buildserver.labs.intellij.net/build/977605352 — use this link to get full insight. Quality gate was triggered with the following revisions:
Triggered a retry attempt №1 out of 1. Quality gate failed. See https://buildserver.labs.intellij.net/build/977605352 to get full insight. |
kotlin-safemerge
Bot
dismissed
Cristian Garcia (CristianGM)’s stale review
New changes affect files owned by this reviewer. Re-review is required.
For internal PRs, review can be finalized by adding /final to the approving message.
|
/dry-run |
|
THIS IS A DRY RUN Quality gate is triggered at https://buildserver.labs.intellij.net/build/978394619 — use this link to get full insight. Quality gate was triggered with the following revisions:
Quality gate failed. See https://buildserver.labs.intellij.net/build/978394619 to get full insight. |
|
/safe-merge |
|
Autosquash aborted: Conflict detected in file(s): libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/testbase/testGradleBuildInjection.kt. Commit 'fixup! [Gradle] Fix build scan option in tests for newer Gradle versions' and fixup 'fixup! fixup! [Gradle] Ensure Kotlin ecosystem plugin brings Kotlin Gradle plugin classes into build classpath' both modify these files. Please rebase manually. |
Yahor Berdnikau (Tapchicoma)
force-pushed
the
yahor/kt-78294-fix-settings-plugin
branch
from
29d4596 to
4aef012
Compare
|
/safe-merge |
|
Quality gate is triggered at https://buildserver.labs.intellij.net/build/978615867 — use this link to get full insight. Quality gate was triggered with the following revisions:
Triggered a retry attempt №1 out of 1. |
|
/safe-merge |
|
Command rejected: A Coordinator build is already in progress for this pull request. Please wait for it to finish or cancel it with |
|
/cancel-coordinator |
|
/safe-merge |
|
Autosquash aborted: Conflict detected in file(s): libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/testbase/testGradleBuildInjection.kt. Commit '[Gradle] Fix build scan option in tests for newer Gradle versions' and fixup 'fixup! [Gradle] Ensure Kotlin ecosystem plugin brings Kotlin Gradle plugin classes into build classpath' both modify these files. Please rebase manually. |
…lasses into build classpath Due to Gradle classloaders hierarchy - depending only on Kotlin Gradle plugins, bom is not enough, as BOM is not applied to child classloaders. To fix it, ecosystem plugin should bring Kotlin Gradle plugin as direct dependency, so it will override any versions from child sub-classloaders. This change should be fine as now we allow users to just override Kotlin compiler version via 'kotlin.compilerVersion', so pinning KGP version should not be a problem for them. ^KT-78294 Verification Pending
Since Gradle 9.5 develocity plugin is used by default when `--scan` option is provided to the build.
Yahor Berdnikau (Tapchicoma)
force-pushed
the
yahor/kt-78294-fix-settings-plugin
branch
from
2b4ca99 to
3be13a0
Compare
|
/safe-merge |
|
Quality gate is triggered at https://buildserver.labs.intellij.net/build/978762649 — use this link to get full insight. Quality gate was triggered with the following revisions:
Triggered a retry attempt №1 out of 1. Quality gate failed. See https://buildserver.labs.intellij.net/build/978762649 to get full insight. |
4 participants
Due to Gradle classloaders hierarchy - depending only on Kotlin Gradle
plugins, bom is not enough, as BOM is not applied to child classloaders.
To fix it, ecosystem plugin should bring Kotlin Gradle plugin as direct
dependency, so it will override any versions from child sub-classloaders.
This change should be fine as now we allow users to just override Kotlin
compiler version via 'kotlin.compilerVersion', so pinning KGP version
should not be a problem for them.
^KT-78294 Verification Pending