fix(samples): repair samples/minimal:nativeTest and guard it in CI (#195) - #197
Conversation
) `rect.origin = …` in Main.kt used property syntax for a field the generator now exposes as `origin()`/`_origin(v)` accessors (a by-value object getter needs a MemScope, so it can't be a Kotlin property) — a pre-existing drift from the context-parameter migration (#145/#146) that nothing ever caught, since samples/minimal was only ever exercised as far as kplusplusSync. - Fix Main.kt to use the accessor form the generator actually emits (verified against the generated build/krapped-cpp/src/geo_Rect.kt). - Add src/nativeTest/kotlin/GeometryTest.kt exercising every accessor kind the header produces (scalar var properties, MemScope-context accessors, a plain const-ref method), so a shape drift fails to compile, not just to run. - Wire `:nativeTest` into a new samples/minimal-scoped CI workflow (.github/workflows/samples-minimal.yml) and into the release local dry run (docs/releasing.md), since samples/minimal is a from-published consumer and can only ever catch generator drift at its own next build. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NLWsoRAe7f1RdQrYJ2ZXqm
|
Reviewing PR #197 (Reviewed SHA so far: 64be5d5). Measured test count (independent — the CI console log itself shows no per-test names/count, and no test-report artifact was uploaded, so a green checkmark alone would NOT have been trusted here): 3/3 executed, 0 skipped, 0 failures — matches the PR's claim exactly, from my own local build (LLVM 22 / clang++ 22.1.6) run against the actual :nativeTest task, not just BUILD SUCCESSFUL. Also confirmed CI run 30477106930 on this exact head SHA (branch fix/195-minimal-sample) triggered on pull_request and genuinely executed the pipeline: kplusplusSync ran, compileKotlinNative/compileTestKotlinNative/linkDebugTestNative/nativeTest all executed (none NO-SOURCE/UP-TO-DATE), 10 actionable tasks: 10 executed. Proceeding to independently reproduce the guard (reintroduce the rect.origin=... property-assignment bug, confirm red, revert, confirm green) and the rest of the checklist. |
monkopedia-reviewer
left a comment
There was a problem hiding this comment.
Reviewed: 64be5d5
Verdict: APPROVE + MERGE (tier-1, green, independently verified)
Measured test count (not trusting green alone)
CI's console log shows no per-test names/count (default K/N test-task output is terse, no
--info, no report artifact uploaded), so I built locally in my own worktree off this exact
SHA (LLVM 22 / clang++ 22.1.6 on PATH) and read the actual XML report:
build/test-results/nativeTest/TEST-nativeTest.GeometryTest.xml
tests="3" skipped="0" failures="0" errors="0"
rectExposesOriginAsAccessorsNotAProperty[native]
rectAreaAndCenterMatchTheGeometry[native]
pointDistanceToIsAPlainConstRefMethod[native]
3/3 executed, matches the PR's claim exactly. Also confirmed via --rerun-tasks (no
UP-TO-DATE/NO-SOURCE masking anything): 10 actionable tasks: 10 executed, kplusplusSync ran,
compileKotlinNative/compileTestKotlinNative/linkDebugTestNative/nativeTest all genuinely executed.
CI run 30477106930 (branch fix/195-minimal-sample, this head SHA) fired on pull_request as
expected and matches this same shape.
Guard reproduction (independent)
Reintroduced the exact original bug (rect.origin = Point().apply { x = 0.0; y = 0.0 } in place
of rect._origin(...)) and ran ./gradlew nativeTest --no-daemon:
> Task :compileKotlinNative FAILED
e: .../Main.kt:40:14 Function invocation 'origin()' expected.
e: .../Main.kt:40:14 Variable expected.
BUILD FAILED in 17s
Reverted (diff against pre-break content was a single line, exact inverse), re-ran fresh with
--rerun-tasks: BUILD SUCCESSFUL, 10/10 executed, 3/3 tests green. The guard has teeth.
Generated-shape claim verified against the real artifact
Read build/krapped-cpp/src/geo_Rect.kt and geo_Point.kt from my own local kplusplusSync
run: origin() is inline context(scope: MemScope) fun origin(): Point?, _origin(value) is a
plain setter fun, width/height are ordinary var properties, center() is also
context'd, and distanceTo on Point is a plain non-context'd method taking Point by value
(const-ref in C++). Exactly matches Main.kt/GeometryTest.kt's usage and the PR's description.
Test quality
GeometryTest.kt's three cases assert actual values (origin x/y round-trip, area, center
midpoint, distanceTo=5.0 on a 3-4-5 triangle) — not tautological, and would fail to compile
(not just assert wrong) against the original bug, since rect._origin(...) doesn't exist if the
generator ever reverts to a property. Good bidirectional coverage.
v8 audit spot-check
Confirmed Isolate::CreateParams::array_buffer_allocator (v8-isolate.h:255) is
ArrayBuffer::Allocator* — a raw pointer field, correctly still exposed as an ordinary var
property per the same migration rule. samples/v8/src/nativeMain/kotlin/Main.kt:87 uses
property syntax accordingly. Claim checks out; v8 itself remains unguarded by CI (correctly
flagged by the author as a separate, larger follow-up, not this PR's scope).
CI workflow
- Path-scoped to
samples/minimal/**+ itself, doesn't touch the expensive in-tree LLVM build —
appropriately cheap and independent ofrelease.yml. - No
-PenableClanganywhere (correctly dead/removed per the require-LLVM contract). - Installs LLVM 22 via apt.llvm.org and symlinks clang++/clang/llvm-config — matches the hard
LLVM-22 requirement. - Minor non-blocking nit: no
timeout-minuteson the job (defaults to GitHub's 360-minute cap).
Given the run takes ~2 minutes and the task is a plainnativeTestinvocation with no obvious
wedge vector, not worth blocking on, but worth adding opportunistically next touch. - Confirmed genuinely from-published: build.gradle.kts pins plugin
0.3.4, my local mavenLocal
cache only has up to0.3.2, and the build still succeeded — so resolution came from
mavenCentral/gradlePluginPortal for real, both locally and (necessarily, no local cache exists
on a fresh runner) in CI.
Scope / overlap
Diff touches only .github/workflows/samples-minimal.yml (new), docs/releasing.md,
samples/minimal/**. No overlap with compiler/**, root build.gradle.kts, or krapper/src/**
(the concurrent #194/#196 lanes).
No defects found. Merging.
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
The bug (reproduced)
samples/minimal:nativeTestfails to compile:Main.ktdidrect.origin = Point().apply { ... }— property-assignment syntax. ButRect.originis a
Point-valued field, and a by-value object getter needs aMemScopeto allocate thereturned
Pointinto, so the generator can't expose it as a Kotlin property (there's no way tothread a
MemScopethrough a property getter). It emitsorigin()(acontext(scope: MemScope) fun) /_origin(value)(a plain setter fun) instead — confirmed byreading the actual generated
build/krapped-cpp/src/geo_Rect.kt. This is correct generatorbehavior; the sample was stale. Pre-existing, caused by the context-parameter migration
(#145/#146), and invisible until now because
samples/minimalwas only ever exercised as far askplusplusSync, nevernativeTest.The fix
samples/minimal/src/nativeMain/kotlin/Main.kt:rect._origin(Point().apply { x = 0.0; y = 0.0 })instead of property assignment, plus a
rect.origin()read to demonstrate the getter shape too.Scalar fields (
width/height/x/y) are untouched — those stay ordinaryvarproperties,confirmed against the generated bindings.
./gradlew runReleaseExecutableKlinkerprints the expectedRect area: 6.0/Rect origin: (0.0, 0.0)/distance ... 5.0/center: (1.0, 1.5).The guard
samples/minimalis a from-published consumer (resolves the released plugin frommavenCentral/Plugin Portal, not the in-tree generator), so it can never catch a generated-shape
change at the moment the generator changes — only the next time this sample itself builds against
a new plugin version. So the guard is: make that next build fail loudly instead of silently.
src/nativeTest/kotlin/GeometryTest.kt— threekotlin.testcases exercising everyaccessor kind the header produces: scalar
varproperties, thecontext(scope: MemScope)get/
_setpair for the object-valuedoriginfield, the by-value-returningcenter()(also context'd), and the plain const-ref
distanceTo. Wiredkotlin("test")into thenativeTestsource set inbuild.gradle.kts..github/workflows/samples-minimal.yml: runs./gradlew nativeTestinsamples/minimalon any push/PR touchingsamples/minimal/**(there was previously noPR-triggered CI in this repo at all — only
release.ymlonrelease/workflow_dispatch).It's cheap: no in-tree LLVM build, just the published plugin + system clang++/LLVM 22 for
the runtime front-end, so it's independent of (and much faster than) the main self-hosting
build.
nativeTeststep to the release "local dry run" indocs/releasing.md, since thatchecklist already runs samples/minimal by hand before every cut.
Proof the guard actually catches drift: reintroduced the exact
rect.origin = …bugtemporarily and re-ran
./gradlew nativeTest:then reverted (
diffagainst the pre-break copy showed byte-identical) and re-ran clean —green, 3/3 tests pass:
Also ran a full
./gradlew clean nativeTest runReleaseExecutableKlinker --rerun-tasksto confirmnothing regressed end to end.
samples/v8
Checked whether
samples/v8has the same rot (source-level audit only — did not run a freshkplusplusSync/build; no generated-bindings output existed to diff against, and v8 is muchlarger, so this was read-only per the task instructions):
samples/v8/src/nativeMain/kotlin/Main.kthas exactly one struct-field access(
createParams.array_buffer_allocator = NewDefaultAllocator(), line 87). The underlying C++field (
v8-isolate.h:255) is a raw pointer (ArrayBuffer::Allocator*), which — per thesame migration rule that broke
originhere — correctly stays an ordinaryvarproperty(only by-value object getters need the
context(MemScope)treatment). Everything else in thefile is already call-style (
.reference(),.Int32Value(context),.FromJust(), etc.), anda couple of in-file comments/issue references ([krapper_gen] Local<PolymorphicBase>.reference() returns an empty marker Api interface — base instance methods unreachable #107, and an older
_reference()→reference()rename) show it's been kept in sync with codegen shape changes before.
nativeTest, no fresh generated output to compare against — same "never exercised beyond amanual run" rot pattern samples/minimal had), but there's no property-vs-accessor mismatch
visible at the source level. Not fixing anything there in this PR; flagging in case a
samples/v8:nativeTestguard is wanted as a follow-up (would be a larger, separate changegiven v8's size and
fixup{}complexity).Scope
Touched only
samples/minimal/**, a new.github/workflows/samples-minimal.yml, anddocs/releasing.md(the release checklist that already references samples/minimal) — nothingunder
compiler/**, the rootbuild.gradle.kts, or krapper's drop-ledger source, per theconcurrent-work lanes for this cycle.