fix(plugin): shade the plugin's ksrpc/ktor/coroutines runtime into the jar (#194) - #200
Conversation
…e jar (#194) Any MULTI-PROJECT consumer of 0.3.4 died on its first `kplusplusSync` with `NoSuchMethodError: Job.invokeOnCompletion$default`. Gradle gives each project's buildscript its own classloader whose parent is the root's, and loading is parent-first: a build that declares the Kotlin plugin only at the root has kotlin-compiler-runner's kotlinx-coroutines 1.8.0 on that PARENT, so a subproject applying kplusplus resolves coroutines 1.11.0 into its child loader and never sees it — ktor-io then calls a method 1.9+ moved onto the `Job` interface and 1.8.0 keeps in `Job$DefaultImpls`. Reachable since #185 put ksrpc (hence ktor + coroutines) on the plugin's runtime classpath for the first time. Option 1 of the issue: SHADE. The plugin jar now bundles ksrpc, ktor-io, kotlinx coroutines/serialization/io and jnanoid with their packages relocated under `com.monkopedia.kplusplus.compiler.shaded`, so nothing a parent classloader owns can shadow them and the plugin runs on the versions it was compiled against. Nothing left to relocate is published: the POM now declares only the Kotlin runtime pieces Gradle/KGP already supply (stdlib, gradle-plugin-api, reflect), which also retires the org.jetbrains:annotations `strictly 13.0` exclusions. Also adds samples/multiproject — the canary this class of failure needed. Every other sample is single-project, where both plugins land in one `plugins { }` block and one classloader, which is exactly why this shipped. Wired into the release local dry run (step 4). The root `plugins { id(...) apply false }` workaround from #193 STAYS for now, with its comment rewritten to say why: this repo self-hosts one release behind, on the already-published (unshaded) 0.3.4, so the workaround is load-bearing until the pluginManagement pin moves to 0.3.5. Verified removable by publishing this build to mavenLocal and running the whole gate without it.
|
Reviewed: Posting the central verification first, before deeper analysis. Root workaround: still present — and that is correct, not a defect
I independently verified the removability claim rather than taking it on trust:
So the removability claim checks out end-to-end: same repro, same fix, verified by me Published-consumer test —
|
Final verdict: APPROVE — MERGEReviewed: Bytecode-level confirmation of relocationWent one step past listing jar entries: extracted
Full test gate — exact match to claimed counts (summed from XML, not console)
|
monkopedia-reviewer
left a comment
monkopedia-reviewer
left a comment
There was a problem hiding this comment.
Approved — see verdict comments. Reviewed: dd8515e
One line — `version` in the root `gradle.properties`. That is the whole change, and it is the direct payoff of #192/#202: the five other version declarations (three per-module `version = "..."` lines and the hand-written `PLUGIN_VERSION` literal in plugin source) no longer exist. During 0.3.4 prep the same operation touched six places, five were bumped, and the sixth was caught only by a reviewer's grep. The three consumed pins (root `settings.gradle.kts`, `samples/minimal`, `samples/multiproject/bindings`) DELIBERATELY stay at 0.3.4. The repo self-hosts one release behind, so they can only move once 0.3.5 exists on the Portal. `verifyConsumedPluginPins` (also from #202) was run and accepts this as a legal one-release lag. ## What 0.3.5 ships * **#194 / #200 — the reason for this release.** 0.3.4 is BROKEN for multi-project consumers: the plugin picked up whatever kotlinx-coroutines the root buildscript classloader owned (1.8.0 via kotlin-compiler-runner) rather than the 1.11.0 it resolves, and ktor-io calls a method coroutines 1.9+ moved onto the interface -> `NoSuchMethodError: Job.invokeOnCompletion$default`. The plugin's ksrpc/ktor/coroutines runtime is now shaded and relocated under `com.monkopedia.kplusplus.compiler.shaded`, so it is immune to the buildscript classloader's contents. `samples/multiproject` is a new canary reproducing the exact topology that shipped broken. * **#196 / #199** — drop-ledger diagnostics: structurally-expected drops (DEDUP, the operator== capability gate) demoted to INFO so the capped WARNING budget carries actionable entries. Totals still reported per-severity. * **#195 / #197** — `samples/minimal:nativeTest` repaired (it had not compiled since the context-parameter migration) plus the repo's first PR-triggered CI. * **#198 / #201** — that CI now asserts a non-zero test count from the JUnit XML and uploads it; previously it went green whether 3 tests ran or none. * **#192 / #202** — this release's own prep, made a one-line operation. ## Verification Full gate re-run on main @ `a65d066` immediately before the bump: krapper 304 / 0 featuregen 195 / 0 cppfixture 2 / 0 noncopyableDeterminismCheck green BUILD SUCCESSFUL in 4m 39s (34/34 tasks executed) `-p compiler check` green at 0.3.5, including `apiCheck` and the new `verifyConsumedPluginPins`. ## Follow-up, immediately after this publishes The post-release migration is one change: bump the three consumed pins 0.3.4 -> 0.3.5 AND delete the root `build.gradle.kts` `apply false` workaround (#193/#194), whose comment names exactly this condition. It cannot go earlier — `release.yml` builds `:krapper` on main to cut the release, and until 0.3.5 exists the pin cannot move off the unshaded 0.3.4. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NLWsoRAe7f1RdQrYJ2ZXqm
Both problems found in review; neither changes the document's conclusions. **§7 was stale, and wrong in its recommendation.** It was drafted while #194 was open and framed it as a live blocker needing a choice between two remedies, recommending option 2 (isolated-classloader Gradle worker). But #194 was CLOSED before this doc was committed — fixed via option 1 (shade/relocate, PR #200, `0a5a705`) and shipped in v0.3.5 — and this branch's own base is two commits after that fix. So the section recommended the option we did not take, against a blocker that no longer existed. Rewritten to record the outcome, and the outcome is better for #186 than what §7 originally proposed: shading REMOVES the version conflict rather than isolating the plugin from it, so a persistent connection holding that stack open across builds is no more exposed than a per-task one. The superseded recommendation is preserved in a collapsed block, because the reasoning that turned out wrong is worth keeping — it optimised for giving the daemon handle a stable home and treated the conflict as something to work around. Propagated: the L2 layer note, B5's and B9's "blocked on #194" cells (B5 now depends only on B4 + probe P3), risk R5 (narrowed to the real residual — ksrpc on the *kotlinc* plugin classpath, where shading the Gradle plugin does not help), probe P7 (mostly done: `samples/multiproject` landed with #200 and was verified to fail against the real released 0.3.4), and open question §12.3 (struck — no longer a question). **One bad citation.** §1.1 cited `compiler/gradle/build.gradle.kts:76-84` for the "both sides compile the same source" claim; that range does not support it at any commit checked. The actual mechanism is `:144-147`, which adds `../../krapper/src/commonMain/kotlin` and `../../krapper_model/src/commonMain/kotlin` as `srcDir`s of the plugin's `main` source set, with the rationale comment at `:104-107`. **Minor line nits**, also from review: `Parsing.kt` 97/103/134 -> 98/104/135, and `DropLedger.kt` 67 -> 78. The six load-bearing citations the review prioritised all checked out exactly, including both halves of the "no live Clang AST" finding (the `memScoped` wrapping and zero bound Sema symbols) and the faithful reading of `broad-forcing-resolve.md:210-224`. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NLWsoRAe7f1RdQrYJ2ZXqm
* docs: the live krapper service + FIR client design (#186 brick 3) Design only; no production change. Written against main @ 34fb7a3 after bricks 1 (#184) and 2 (#185) landed. This was accidentally swept into the 0.3.5 release-prep commit by a `git add -A` and caught in review of #203 — a release commit that claimed "one line" while carrying 704 unrelated lines. Split out here, which is where it belonged. The document's three load-bearing findings CORRECT the framing the issue was opened with, so they are worth stating in the commit as well as the file: 1. **There is no live Clang AST today, anywhere.** `CppParser.parse` wraps the whole parse in `memScoped`, so the `ASTUnit` — and `ASTContext`/`Sema` with it — is destroyed when the parse returns; only the pure-Kotlin `WrappedTU` survives, and `clang_slice.h` binds zero Sema symbols. "A persistent Clang instance HAS Sema" is not something persistence unlocks; it is new capability work that persistence makes affordable. Probe-gated. 2. **A compiler cannot materialize a binding mid-compilation.** Generated bindings enter the build as a source dir plus a cinterop klib, both frozen before `compileKotlinNative`; a FIR-synthesized class would have no backing native symbol. The compiler can ASK and REQUEST; the unit of materialization is the next sync. 3. **Lazy binding cannot be the default.** `broad-forcing-resolve.md:216` already rated bounding the closure "disqualifying as the primary fix" because it changes bound output, and featuregen depends on INCLUDE_MISSING's transitivity. So #10's two premises become ATTACKABLE, not dissolved — an honest downgrade from what was claimed when the direction was set. What the design proposes instead is a canonical binding index emitted by krapper and consumed by the FIR plugin as a compilation-start snapshot, with persistence, Sema and lazy binding as separate probe-gated bricks above it. §11 argues bricks B1-B3 are ~80% of the realizable value with no persistence, no socket and no Sema — and they fix a live defect: the FIR plugin re-derives krapper's KotlinWriter name mangling by hand (`CppVectorMapping.kt:248-268`) with nothing checking the two implementations against each other, so a generator change silently degrades to a spurious SYNC_REQUIRED on a binding that exists. §9 lists seven probes, each cheap and each allowed to come back negative and kill its brick. §12 carries four open questions for the owner. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NLWsoRAe7f1RdQrYJ2ZXqm * docs: correct the §7 #194 framing and one bad citation (review of #204) Both problems found in review; neither changes the document's conclusions. **§7 was stale, and wrong in its recommendation.** It was drafted while #194 was open and framed it as a live blocker needing a choice between two remedies, recommending option 2 (isolated-classloader Gradle worker). But #194 was CLOSED before this doc was committed — fixed via option 1 (shade/relocate, PR #200, `0a5a705`) and shipped in v0.3.5 — and this branch's own base is two commits after that fix. So the section recommended the option we did not take, against a blocker that no longer existed. Rewritten to record the outcome, and the outcome is better for #186 than what §7 originally proposed: shading REMOVES the version conflict rather than isolating the plugin from it, so a persistent connection holding that stack open across builds is no more exposed than a per-task one. The superseded recommendation is preserved in a collapsed block, because the reasoning that turned out wrong is worth keeping — it optimised for giving the daemon handle a stable home and treated the conflict as something to work around. Propagated: the L2 layer note, B5's and B9's "blocked on #194" cells (B5 now depends only on B4 + probe P3), risk R5 (narrowed to the real residual — ksrpc on the *kotlinc* plugin classpath, where shading the Gradle plugin does not help), probe P7 (mostly done: `samples/multiproject` landed with #200 and was verified to fail against the real released 0.3.4), and open question §12.3 (struck — no longer a question). **One bad citation.** §1.1 cited `compiler/gradle/build.gradle.kts:76-84` for the "both sides compile the same source" claim; that range does not support it at any commit checked. The actual mechanism is `:144-147`, which adds `../../krapper/src/commonMain/kotlin` and `../../krapper_model/src/commonMain/kotlin` as `srcDir`s of the plugin's `main` source set, with the rationale comment at `:104-107`. **Minor line nits**, also from review: `Parsing.kt` 97/103/134 -> 98/104/135, and `DropLedger.kt` 67 -> 78. The six load-bearing citations the review prioritised all checked out exactly, including both halves of the "no live Clang AST" finding (the `memScoped` wrapping and zero bound Sema symbols) and the faithful reading of `broad-forcing-resolve.md:210-224`. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NLWsoRAe7f1RdQrYJ2ZXqm --------- Co-authored-by: Monkopedia <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]>
2 participants
Fixes #194.
Which option, and why
Option 1 — shading. The plugin jar now bundles its runtime (ksrpc, ktor-io, kotlinx
coroutines/serialization/io, jnanoid) with every package relocated under
com.monkopedia.kplusplus.compiler.shaded. Nothing a parent classloader owns can shadow arelocated name, so the plugin runs on the exact versions it was compiled against no matter
what the root buildscript classpath happens to hold. No reflection / service-loader obstacle
turned up (details below), so option 2 (isolated-classloader worker) was not needed.
Concretely, in
compiler/gradle/build.gradle.kts:implementationto a newshadedconfiguration thatcompileOnlyextends — compiled against the real coordinates, shipped as relocatedbytecode;
shadowJartakes the main jar's name (archiveClassifier = "", plainjardisabled), andthe shadow plugin already makes it the artifact of the
javacomponent, so it is whatvanniktech publishes, what the Plugin Portal uploads, and what an
includeBuildconsumersubstitutes to;
Gradle and KGP supply them on the parent classloader, and relocating them is meaningless.
The Kotlin three moved to shadow's
shadowbucket, which is what still reaches the POM;mergeServiceFiles()+DuplicatesStrategy.INCLUDEso a service registration can't bedropped before a transformer sees it;
META-INF/*.SF|DSA|RSAandmodule-info.classareexcluded.
Details worth knowing:
_COROUTINEis relocated too. kotlinx-coroutines ships real classes with real methods inthat root-level package for stack-trace recovery; leaving them unrelocated would have
reproduced the exact hazard being fixed, one package over.
META-INF/servicesentries exist in any bundled jar (coroutines 1.11 no longer shipsthem) — verified by listing all resolved jars.
mergeServiceFiles()is kept as a guard forfuture bumps, not because it currently does anything.
.kotlin_modulefiles are rewritten by shadow'sKotlinModuleMetadataTransformer(whichrenames them
*.shadow.kotlin_module; the extension is what the compiler matches on).rules are inherited down
extendsFrom, andcompileOnlyextendsshaded— aconfiguration-level
exclude(group = "org.jetbrains.kotlin")stripped the Kotlin API off ourown compile classpath (caught immediately, as a compile failure).
kotlin-stdlib,kotlin-gradle-plugin-apiandkotlin-reflect. That also retires theorg.jetbrains:annotationsstrictly 13.0exclusions from [arch] ksrpc service interface between the compiler and krapper (typed diagnostics, no stdout-scrape) #185 — the conflict they worked around cannot arise from dependencies that
are no longer declared.
Reproduced before, gone after
Before — the released 0.3.4 plugin, root workaround stripped,
:krapper:kplusplusSync:The new sample reproduces it standalone against the released plugin, which is the better
demonstration —
samples/multiproject,:bindings:kplusplusSync, same signature.After — this build published to mavenLocal with the tool bundled (the release local dry
run):
samples/multiproject :bindings:kplusplusSync→cpp front-end generated 2 Kotlin file(s); no diagnostics, and:bindings:buildcompiles the generated bindings.Then, on the repo itself with the root workaround deleted and the shaded plugin resolved
from mavenLocal:
:krapper:kplusplusSyncdiff -rqover the wholebuild/krapped-cpptree, both modules, incl. the.defs):krapper:nativeTest:featuregen:nativeTest:cppfixture:nativeTest:krapper:noncopyableDeterminismCheckktlintCheck --rerun-tasks-p compiler build(incl.apiCheck)samples/minimal kplusplusSync(from-published)The published-shape jar was checked directly: the bundled
krapperbinary is still atcom/monkopedia/kplusplus/tools/linuxX64/krapperinside it, no unrelocatedkotlinx/,io/ktor/,_COROUTINE/,kotlin/ororg/class survives, and there are no duplicate zipentries.
The root workaround: why it is still there
It is one line away from gone, and this PR proves the fix, but deleting it now would make
mainunbuildable. The repo self-hosts one release behind:settings.gradle.ktspins theconsumed plugin to the published 0.3.4, whose jar is the unshaded one.
:krappercan onlyregenerate its own bindings with the tool binary bundled in that released jar, so the pin
cannot move to 0.3.5 before 0.3.5 exists — and the release workflow itself builds
:krapper:linkReleaseExecutableKlinkeronmain, so a pin pointing at an unpublished versionwould deadlock the cut.
So the workaround stays, with its comment rewritten to say exactly that and to name its own
deletion condition:
The removal is verified, just not committed: the full gate above ran with that line deleted,
against this build published to mavenLocal. Post-0.3.5 the pin bump and this line's deletion
are the same one-line follow-up. Flagging it explicitly rather than shipping a red
main.samples/multiproject
A root that declares the Kotlin plugin and a
:bindingssubproject that applies kplusplus toone header-only, std-free C++ struct. That is the whole sample — what it binds does not matter;
being two projects with KGP at the root is the test.
samples/v8andsamples/minimalareboth single-project, which is precisely why #194 shipped.
It resolves the plugin by coordinate with
mavenLocal()first (same model assamples/minimal), so it exercises the real published shape. It gets no CI workflow of itsown — unlike
samples/minimalit cannot run against the released plugin, since the pluginunder test is the point — and is instead step 4 of the release local dry run in
docs/releasing.md.Also
docs/getting-started.mdgains a short multi-project note (apply the plugin where thebindings live; the 0.3.4-only workaround, and that upgrading is the real fix).
samples/minimal'sMain.ktdrift — that is samples/minimal:nativeTest does not compile — property assignment vs generated accessor #195, fixed infix(samples): repair samples/minimal:nativeTest and guard it in CI (#195) #197, which landed while this was in flight and is included in the rebase.
~/.m2was restored afterwards: the locally published 0.3.4 used for verification has beenremoved, so a reviewer reproducing 0.3.4: NoSuchMethodError in multi-project consumers — plugin classloader shadows coroutines to 1.8.0 #194 against the real released 0.3.4 is not masked by it.