Isolate usage of kotlin-metadata to workers and downgrade Kotlin version used by the plugin#1719
Isolate usage of kotlin-metadata to workers and downgrade Kotlin version used by the plugin#1719ljacomet wants to merge 8 commits into
Conversation
|
Moving back to draft as it looks like there is one test failure. |
|
Honestly just one test failure with all that is kind of impressive. |
|
I cannot reproduce the failure in isolation for now, but when running what runs on CI, I got a different error. My concern is that whatever I did causes some data corruption somewhere ... which I hope is a test artifact rather than a deeper bug. |
|
That test that failed on CI also passes locally for me. Hm, if only Gradle had ever implemented my request for free hosted Develocity for some OSS projects, we could do a build comparison of local vs CI 🤔 Edit: I wonder if this relates to the Java runtime? CI is pinned to 21 (zulu): whereas locally I am currently using I might try using zulu-21 and see if that changes anything. |
autonomousapps
left a comment
autonomousapps
left a comment
There was a problem hiding this comment.
Thanks for this. Left some comments. Not yet sure why the test is failing.
| # Keep on a release built with Kotlin <= 2.1 (0.18 is the latest; 0.20 breaks Gradle 8.x). See kotlin* note above. | ||
| kotlin-editor-core = "0.18" | ||
| kotlin-editor-relocated = "0.18.0" |
There was a problem hiding this comment.
This will probably break some tests. Why isn't the version-forcing mechanism from build-logic handling this issue?
| #moshix = "0.31.0" | ||
| okio = "3.16.4" | ||
| # Keep on a release built with Kotlin <= 2.1 (3.15.0 is the latest; 3.16+ breaks Gradle 8.x). See kotlin* note above. | ||
| okio = "3.15.0" |
There was a problem hiding this comment.
This downgrade is probably fine, but I have the same question about the version-forcing mechanism.
There was a problem hiding this comment.
Regarding this comment and the other one about version-forcing, here, you are right, it works in the sense that the plugin will use and depend on the Kotlin version defined in the catalog.
But my understanding is that this could still be an issue if the plugin ships with dependencies that use a Kotlin version that Gradle 8.11 will not be able to run. Gradle 8.11 bundles Kotlin 2.0.20.
If I understand Kotlin binary compatibility properly, anything built on Kotlin 2.2+ (assuming it does not target a lower language level) is a potential problem.
After all, this is the reason behind moving kotlin-metadata to a worker with class loader isolation.
So maybe both kotlin-editor and okio can stay in the versions they had before this PR, as I tried running Gradle 8.11 tests and could not see them fail. But the risk is that a later upgrade, without raising the plugin's minimum support Gradle version could re-introduce a similar issue.
I have to admit I do not understand why okio 3.16.4 does not causes that issue since version 3.16.0 bumped Kotlin to 2.2.0.
Let me know how you would want to proceed.
There was a problem hiding this comment.
Let's take a step back. This plugin has a functional testing suite that (nominally) runs against Gradle 8.11. Why isn't that showing any issues? If we can get the tests to fail in the scenario that led to this PR, then I'd have confidence with making changes relating to these dependencies.
There was a problem hiding this comment.
I had some Fable 5 usage to burn. 🤖
The suite passes on 8.11.1 because the failure is opt-in. Per @3flex's comment on #1671, the Kotlin DSL script compiler skips the metadata version check unless
org.gradle.kotlin.dsl.skipMetadataVersionCheck=false(or warnings-as-errors, which is how the gradle/gradle build hit it).This PR adds
KotlinDslMetadataVersionSpecin d244cec, which enables that property in a Kotlin DSL fixture. Cherry-picked onto currentmain, it fails on Gradle 8.11.1 with "Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 2.2.0, expected version is 2.0.0", flagging the plugin jar itself plus graph-support, variant-artifacts, kotlin-metadata-jvm 2.2.21, okio-jvm 3.16.4, and kotlin-editor-relocated 0.23.0. On this branch it passes on both 8.11.1 and latest. That also answers the earlier okio question: the compiler checks every classpath jar's module metadata whether or not the script references it, so the okio and kotlin-editor downgrades are required.(To verify the green leg from a direct checkout of this branch, cherry-pick 1174326 first; the branch predates that fix, and the functional-test publish otherwise fails on signing. CI is unaffected since it builds the PR merged with main.)
There was a problem hiding this comment.
I don't mind so much pinning okio (though I'm not happy about it), but the lowered version of kotlin-editor isn't acceptable. That library is what lets fixDependencies work for Kotlin DSL scripts, which is an area of active development. If keeping it at a higher level isn't breaking anything, then I'm fine rolling the dice on that. Maybe a follow-up PR could do something similar for kotlin-editor as is being done here for kotlin-metadata.
There was a problem hiding this comment.
Given the information above, I create a simple project on which I applied the DAG plugin.
- Latest released version causes the build to fail if
org.gradle.kotlin.dsl.skipMetadataVersionCheck=falseis set ingradle.properties - The version produced by this PR no longer causes the build to fail, with that property set.
- The version produced by this PR, with either
okioorkotlin-editorback to their original version, causes the build to fail as well with the property set.
So effectively, in order to get full Gradle [8.11,9.0[ support, okio must be pinned to a Kotlin 2.0.0 compatible version and so should kotlin-editor until it follows the same handling as here: move to worker execution with classloader isolation.
My recommendation: This PR goes in (if approved) with the okio pinning as that one will be required for ever AFAICT. But I undo the pinning of kotlin-editor. Then a later PR can attempt to move script rewrite to a worker action, effectively removing that dependency from the buildscript classpath. In the meantime, Gradle 8.x compatibility is only valid if metadata version check is turned off.
Alternatives: Neither okio nor kotlin-editor are pinned in this PR, or both are.
@autonomousapps The final decision is yours to make
There was a problem hiding this comment.
@ljacomet I agree with your recommendation. Let's pin okio, unpin kotin-editor, and merge this PR. And then a future PR can move kotlin-editor to isolated worker actions. I just checked the source code and the usage of kotlin-editor classes is fairly limited. It looks like RewriteTask currently isn't using the Worker API, but I don't see a reason it can't.
I'll also add org.gradle.kotlin.dsl.skipMetadataVersionCheck=false to all test fixtures.
There was a problem hiding this comment.
I actually really appreciate this. The whole exploding/exploded has been annoying me for a while. Too many intermediate model classes. Just haven't made time to simplify.
| c.description = "kotlin-metadata-jvm, supplied to classloader-isolated DAGP workers." | ||
| c.defaultDependencies { deps -> | ||
| val dep = project.dependencies.create( | ||
| "org.jetbrains.kotlin:kotlin-metadata-jvm:${BuildConfig.KOTLIN_METADATA_VERSION}" |
There was a problem hiding this comment.
I think I'm missing something. Can you explain how this can resolve to a higher version that the consumer might require? Does that happen automatically or would they have to explicitly configure via the dependencyAnalysisKotlinMetadata configuration?
There was a problem hiding this comment.
This probably needs also documentation.
The idea is that without upgrading this plugin, a project that bumps its Kotlin version to a newer one could also update this dependency and get analysis working - assuming the kotlin-metadata-jvm API is stable enough. But from my quick search, it should be.
|
I installed zulu-21 and ran the tests again, and they still all succeeded: |
Move the InMemoryCache build-service interaction out of the FindKotlinMagic and ExplodeJar work actions and into their tasks. The task snapshots the cache entries relevant to its artifacts into a file, the worker reads that seed and emits its cache misses to another file, and the task merges those back into the service after the work completes. This is behavior-preserving (cross-task reuse is retained; only concurrently-running sibling tasks no longer observe each other's in-flight results) and decouples the work actions from the build service, which is a prerequisite for running them under worker classloader isolation. The exploded-jar cache now stores the serializable ExplodedJar (keyed by artifact path) rather than the computed ExplodingJar.
Stop bundling kotlin-metadata-jvm on the plugin's runtime classpath (it becomes a compileOnly dependency) and instead supply it to the FindKotlinMagic, AbiAnalysis and ExplodeJar work actions via worker classLoaderIsolation. The library is resolved from the consuming build's repositories through a new 'dependencyAnalysisKotlinMetadata' resolvable configuration and added to the isolated worker classpath; its version is exposed via BuildConfig.KOTLIN_METADATA_VERSION. This keeps kotlin-metadata-jvm's (newer) Kotlin binary metadata off the classpath that Gradle's Kotlin DSL script compiler scans, while still allowing the worker code to use a newer metadata library than the rest of the plugin. The work actions read metadata only (KotlinClassMetadata.readLenient) and expose no metadata types across the worker boundary. Consuming builds with locked-down repositories must make org.jetbrains.kotlin:kotlin-metadata-jvm resolvable. Refs autonomousapps#1671.
Everything DAGP exposes on its plugin classpath must carry a Kotlin binary-metadata version readable by the oldest supported Gradle's Kotlin DSL script compiler (Gradle 8.11.1 ships Kotlin 2.0, which tolerates up to 2.1.x) when org.gradle.kotlin.dsl.skipMetadataVersionCheck=false. Bring the remaining 2.2.0-metadata sources down to <= 2.1: - kotlinLanguageVersion 2.2 -> 2.1 (DAGP's own classes; rewrites a 'when' guard in StandardTransform that required language version 2.2) - bundle kotlin-stdlib/kotlin-reflect at 2.1.21 via a new 'kotlinRuntime' version and KotlinConfigurer (compiler/KGP stays at 2.2.x for Gradle 9 compatibility) - okio 3.16.4 -> 3.15.0 (3.16+ is built with Kotlin 2.2) - kotlin-editor 0.20 -> 0.18 (0.20 is built with Kotlin 2.2) Also rename the 'kotlin' version to 'kotlinGradlePlugin', since it pins the Kotlin Gradle Plugin/compiler tooling (KGP, KMP plugin, kotlin-bom, the kotlinJvm plugin, and the version applied to test fixtures), not the bundled runtime libraries. Adds a functional-test regression that applies the plugin to a Kotlin DSL build with the metadata version check enabled, across the supported Gradle versions. Fixes autonomousapps#1671.
With the 'kotlin' version gone (renamed to kotlinGradlePlugin), there is no longer a 'libs.versions.kotlin' leaf to collide with, so the kotlin-editor versions can use the conventional dotted names and the naming workaround/comment can be dropped.
…pply time. Split the former `of()` factory into `register()` (called once from ProjectPlugin.apply after the path-filter check) and a now-trivial `of()` that simply does `configurations.named(...)`. This removes the idempotency guard and the `findByName` probe — configurations are registered exactly once per analysed project, before any task registration happens.
Move the `newCacheEntries.getAndDelete()` calls to the top of ExplodeJarWorkAction and FindKotlinMagicTask.Action so that all output files are cleared before any work begins. This ensures a failed or interrupted action leaves no stale outputs from a prior run.
ljacomet
force-pushed
the
kotlin-metadata-gradle8
branch
from
d215a0f to
c4401e3
Compare
|
I've addressed some comments, but not all. However this is rebased, let's see what happens with tests. |
|
Looks like the same failure as before: https://scans.gradle.com/s/obkd2ypbdmt74 Here's the artifact download URL: https://github.com/autonomousapps/dependency-analysis-gradle-plugin/actions/runs/26915202104/artifacts/7397914732 It appears to have worked exactly as desired. That zip contains some directories, You can optionally simplify the failing spec to run just the combination of things that is failing, then run: ./gradlew :fT --tests "ClassifiersSpec.transitive classifier dependencies do not lead to wrong advice*"And then you'll see output in |
…oject identity leak The exploded-jar cache is keyed by file path. Since kotlin-metadata was isolated to workers, the cached value is ExplodedJar, which embeds the dependency's coordinates. When one physical file is resolved under different coordinates by different projects (e.g. a classifier/capability variant), a build-scoped cache hit returned the first consumer's coordinates, mis-attributing the dependency and producing wrong advice. This surfaced as the ClassifiersSpec "transitive classifier" failures, which only reproduce under low worker counts (hence CI-only). Reuse the cached file-content analysis on a hit, but rebind to the current artifact's coordinates. Adds a JarExploder regression test.
…an instantiate it Gradle cannot instantiate a WorkAction whose execute() is an interface default method. Under kotlinLanguageVersion 2.1 (jvm-default=disable) interface bodies compile via DefaultImpls, leaving execute() abstract in the interface bytecode, so class decoration fails with "Cannot have abstract method Action.execute()". This surfaced as the PublicTypeUsageSpec failure (Gradle 9.5.1, AGP 9.2.0), which CI never reached before because the ClassifiersSpec failure short-circuited the job. Making Action an abstract class (like every other work action) compiles execute() onto the class.
|
I'm not sure how you all feel about AI-assisted contributions, but I had some Claude tokens to burn. If interested, it reproduced the test failure and came up with a fix: ljacomet#1. |
|
Thanks @ianbrandt, I've merged your PR on my branch. I still need to answer questions around the dependency version-forcing mechanism. |
|
Thanks for all your work on this issue, @ljacomet. I'm glad the PR was helpful. |
|
@ljacomet Just curious if you've had a chance to look at this again recently? |
|
@rvandermeulen, It's easy to miss, but I believe the last status change is that Louis has an open question threaded above: #1719 (comment). |
|
Hey, I've added a comment based on the additional information. Let me know how I should proceed. Once a decision is made, I can rebase and mark it ready for review again in case a positive outcome is agreed upon. |
4 participants
Turns out that while the base idea for the fix was right, the devil is in the details.
I attempted to split the commit logically to help the review:
If those changes are too impactful - there is a slight risk of a performance regression with the changes to how caching works - the only alternative is to drop support for older Gradle versions. But this will be a never ending race because of the need for the plugin to understand the latest Kotlin metadata.