feat(#461): manifest-v2 PR3 — wire v2 desktop codegen + sokol byte anchor (0 diff) - #464
Conversation
…te anchor (PR 3) Wires the v2 build-graph codegen path behind the header-first version gate, converts the retained sokol fixture's DESKTOP cell to a v2 manifest, and proves the critical byte anchor (§7): the v2-generated build.zig is BYTE-IDENTICAL (0 diff) to the enum/v1-splice path for sokol-desktop. What landed: - src/codegen/manifest_v2_splice.zig: v2 desktop codegen. Generates the b.dependency literal (dep name + base↔per-platform dep_options merge, each value from the closed ValueSource predicate set), the provider .module(...) decls (honoring root_alias, default backend_<name>), the .artifact(...) decls, and the linkLibrary/framework wiring — all from typed manifest data. - src/build_files.zig: gated dispatch. New opt-in BuildZigOptions .backend_manifest_name; null keeps the PRODUCTION path 100% unchanged (manifest_splice.loadManifest → v1/enum, byte-for-byte). When set, the named manifest is header-first parsed (manifest_v2.parseManifest) and dispatched: v1/field-less → v1 splice; manifest_version >= 2 → v2 desktop codegen. - src/codegen/manifest_v2.zig: loadNamedManifest (load + version-dispatch a named manifest file from the resolved backend package). - backends/sokol/backend.manifest.v2.zon: the v2 manifest (faithful to §3), SEPARATE from the retained v1 backend.manifest.zon (kept intact — other tests depend on it). PR 3 wires desktop only; android/ios/wasm authored for later PRs. - backends/sokol/backend.hook.zig: the dedicated hook (empty desktop post_wire; NOT invoked by the desktop path). - test/build_zig_tests.zig: the byte-anchor differential test across the cfg matrix the dep_options predicates branch on (gamepad auto/off, hidapi on/off, +plugins/zig_ecs). Verified live: a 1-byte manifest change flips it to fail. Byte anchor: 0 diff. The desktop cell UNROLLS the core-diamond overrides (the verbatim residual mirrors the v1 fragment) per §7's "unrolling the walk's output for the desktop case", so the generic core_diamond.generated_walk_zig is intentionally NOT spliced on this path — splicing a walk call + helper def instead of the unrolled if-blocks would change the text and break 0-diff. The generic walk stays validated (PR 2 tests + emitCoreDiamondWalk drift guard) and is the mechanism for the golden-snapshot cells (PR 5+), whose gate is a reviewed golden, not byte-identity. v1/enum byte-unchanged: the version gate only activates for manifest_version >= 2, reached solely via the opt-in .backend_manifest_name; the field-less production backend.manifest.zon stays v1. All pre-existing generation/golden tests pass. zig build: exit 0. zig build test: exit 0 (byte anchor 0 diff + all prior tests). Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThis PR adds a manifest-v2 backend path for sokol: a new v2 manifest and hook, named-manifest loading, desktop v2 splice generation, wiring in build code, and tests that compare v1 and v2 output. ChangesManifest v2 backend build path
Estimated code review effort: 4 (Complex) | ~55 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/codegen/manifest_v2_splice.zig (1)
260-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
bufArrayList.
bufis never written to or read — onlyawfeedsemitCoreDiamondWalk. It's a leftover from a prior buffer-based version.♻️ Proposed cleanup
- var buf: std.ArrayList(u8) = .empty; - defer buf.deinit(testing.allocator); var aw: std.Io.Writer.Allocating = .init(testing.allocator); defer aw.deinit();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codegen/manifest_v2_splice.zig` around lines 260 - 261, Remove the unused buf ArrayList in manifest_v2_splice.zig: the local buf is never read or written and only aw is passed into emitCoreDiamondWalk. Delete the buf declaration and its defer deinit cleanup, and keep the surrounding logic in the function that builds the walker/emit path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/build_files.zig`:
- Around line 158-169: `backend_manifest_name` is not being honored by the
external-manifest gate, so backends that only ship the alternate manifest file
never reach `loadNamedManifest`. Update the manifest decision flow in
`requireManifestIfExternal` and `manifestPathEnabled` to key off the requested
filename when `backend_manifest_name` is set, while preserving the existing
legacy `backend.manifest.zon` behavior when it is null. Use the existing symbols
`backend_manifest_name`, `requireManifestIfExternal`, `manifestPathEnabled`, and
`loadNamedManifest` to route the check to the selected manifest path.
In `@test/build_zig_tests.zig`:
- Around line 64-67: The anchor test description and name claim it covers
plugins + zig_ecs, but the `expectV2MatchesEnum` call only passes `.ecs =
.zig_ecs` and never enables plugins. Update the test setup in `test "byte
anchor: v2 desktop == enum/v1, with plugins + zig_ecs (shared regions
unaffected)"` so it actually includes a plugin entry in the config, or rename
the test/comment to match the current `expectV2MatchesEnum` coverage if plugins
are not meant to be exercised here.
---
Nitpick comments:
In `@src/codegen/manifest_v2_splice.zig`:
- Around line 260-261: Remove the unused buf ArrayList in
manifest_v2_splice.zig: the local buf is never read or written and only aw is
passed into emitCoreDiamondWalk. Delete the buf declaration and its defer deinit
cleanup, and keep the surrounding logic in the function that builds the
walker/emit path unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e358787e-777b-42ce-a379-b62a621f665c
📒 Files selected for processing (8)
backends/sokol/backend.hook.zigbackends/sokol/backend.manifest.v2.zonsrc/build_files.zigsrc/codegen/manifest_v2.zigsrc/codegen/manifest_v2_splice.zigsrc/root.zigtest/build_zig_tests.zigtest/helpers.zig
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d3c45215d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // PRODUCTION path — unchanged: read `backend.manifest.zon` (v1) directly. | ||
| splice_manifest = try manifest_splice.loadManifest(allocator, cfg, opts.project_dir.?); |
There was a problem hiding this comment.
When a backend is migrated in place by putting manifest_version = 2 in the normal backend.manifest.zon file, production callers leave backend_manifest_name null, so this branch still invokes the v1-only manifest_splice.loadManifest. That parser requires v1 fields like loop_style/build_fragments and will reject the v2 shape before the new renderBackendDepSectionV2 path can run, making canonical v2 manifests unusable outside the test-only alternate filename path.
Useful? React with 👍 / 👎.
| // Anchor residual: the framework switch (from `.frameworks.desktop`) + prose. | ||
| try w.writeAll(anchor_link_body); |
There was a problem hiding this comment.
For any v2 desktop manifest whose framework/system-library requirements differ from sokol's, this generic v2 link path ignores m.frameworks/m.system_libs and appends the hard-coded sokol IOSurface/CoreFoundation block instead. That means future or external v2 backends can declare required link metadata in the manifest but still generate a build.zig that omits it (and adds sokol-specific Darwin frameworks).
Useful? React with 👍 / 👎.
| // TAIL: the two unrolled core-diamond `overrideImport` `if`-blocks + prose. | ||
| // Anchor residual — §7 "unrolling the walk's output for the desktop case". | ||
| try w.writeAll(anchor_tail_overrides); |
There was a problem hiding this comment.
In a v2 desktop manifest that does not declare the default backend_input variable (for example an explicit root_alias, or a non-sokol provider without an input module), this unconditional tail emits hard-coded sokol gamepad overrides referencing backend_input/sdl_gamepad. The preceding module declarations are manifest-driven, so this can make otherwise valid v2 manifests generate a build.zig that does not compile or rewrites the wrong backend graph.
Useful? React with 👍 / 👎.
| // GENERATED: provider module decls under their root alias. | ||
| for (m.modules) |mod| { | ||
| const alias = try moduleAlias(allocator, mod); | ||
| defer allocator.free(alias); | ||
| try w.print(" const {s} = backend_dep.module(\"{s}\");\n", .{ alias, mod.name }); | ||
| } |
There was a problem hiding this comment.
For a v2 desktop manifest that uses the schema’s .platforms.desktop.extra_modules, this loop only emits the top-level m.modules declarations and never binds the platform-scoped modules. Any generated root or hook wiring that expects those extra module aliases will then reference variables that were never declared, even though the manifest declared them in the supported platform entry.
Useful? React with 👍 / 👎.
…ugin/v2-only anchor coverage PR #464 review findings. Finding 1 (Major): the manifest gate (`requireManifestIfExternal` / `manifestPathEnabled` / `manifestExists`) hardcoded the legacy `backend.manifest.zon`, so a backend shipping ONLY `backend.manifest.v2.zon` was wrongly treated as manifest-less and never reached `loadNamedManifest`. The three functions now take a `manifest_name: ?[]const u8` (null → the legacy name, exposed as `LEGACY_MANIFEST_NAME`), and `build_files.zig` keys BOTH the external-manifest requirement and the desktop gate off `opts.backend_manifest_name`. root.zig's production generators pass `null` (byte-unchanged). Added a `backends/sokol_v2only` fixture (v2 manifest only, no legacy sibling) + a manifest_splice unit test and a build_zig_tests byte anchor proving a v2-only backend now reaches v2 codegen and is byte-identical to the dual-manifest output. Finding 2 (Minor): the "plugins + zig_ecs" byte anchor set no plugins, so the plugin-wiring / core-diamond-override region was untested. Added a real plugin entry (labelle-pathfinding) so the anchor exercises the plugin path; still 0-diff. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codegen/manifest_splice.zig (1)
244-275: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPlumb the requested manifest filename into provider contract loading
loadProviderManifeststill hardcodesbackend.manifest.zon, butsrc/root.zig:363uses it on every target for identity/capability enforcement. A backend shipping onlybackend.manifest.v2.zonwill fall back to derived identity and an empty capability set. Pass the requested manifest name through here, or route this path throughmanifest_v2.loadNamedManifestas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codegen/manifest_splice.zig` around lines 244 - 275, `loadProviderManifest` is still hardcoded to read `backend.manifest.zon`, so it misses backends that only ship the requested manifest variant. Update the provider contract loading path in `loadProviderManifest` to accept the manifest filename from the caller or reuse the same named-manifest resolution used by `manifest_v2.loadNamedManifest`. Make sure the identity/capability parsing path in `ProviderManifest` continues to work for both legacy and v2 manifest names without falling back incorrectly.
🧹 Nitpick comments (1)
src/codegen/manifest_splice.zig (1)
438-472: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared constant for the v2 manifest filename.
LEGACY_MANIFEST_NAMEis exported as a constant, but the v2 counterpart ("backend.manifest.v2.zon") is repeated as a literal here (lines 452, 466) and presumably elsewhere. A shared exported constant (likely already defined inmanifest_v2.zig) would avoid drift if the filename ever changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codegen/manifest_splice.zig` around lines 438 - 472, The test and related manifest handling use the v2 filename as a hardcoded literal, while the legacy name already has a shared constant. Update the `requireManifestIfExternal` test to reference the exported v2 manifest constant from `manifest_v2.zig` instead of repeating `"backend.manifest.v2.zon"`, and use that same symbol anywhere else in the diff where the v2 filename is duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/codegen/manifest_splice.zig`:
- Around line 244-275: `loadProviderManifest` is still hardcoded to read
`backend.manifest.zon`, so it misses backends that only ship the requested
manifest variant. Update the provider contract loading path in
`loadProviderManifest` to accept the manifest filename from the caller or reuse
the same named-manifest resolution used by `manifest_v2.loadNamedManifest`. Make
sure the identity/capability parsing path in `ProviderManifest` continues to
work for both legacy and v2 manifest names without falling back incorrectly.
---
Nitpick comments:
In `@src/codegen/manifest_splice.zig`:
- Around line 438-472: The test and related manifest handling use the v2
filename as a hardcoded literal, while the legacy name already has a shared
constant. Update the `requireManifestIfExternal` test to reference the exported
v2 manifest constant from `manifest_v2.zig` instead of repeating
`"backend.manifest.v2.zon"`, and use that same symbol anywhere else in the diff
where the v2 filename is duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e17cdcdc-c496-48cf-832a-f2391df77564
📒 Files selected for processing (6)
backends/sokol_v2only/backend.manifest.v2.zonsrc/build_files.zigsrc/codegen/manifest_splice.zigsrc/root.zigtest/build_zig_tests.zigtest/helpers.zig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17395f0421
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // GENERATED: artifact decls for this platform (the var name IS the artifact | ||
| // name, matching the enum path: `const sokol_clib = backend_dep.artifact(...)`). | ||
| for (desktop.artifacts) |art| { | ||
| try w.print(" const {s} = backend_dep.artifact(\"{s}\");\n", .{ art.name, art.name }); |
There was a problem hiding this comment.
For a v2 desktop backend whose artifact name is not a Zig identifier (for example a package artifact named foo-lib, which is valid as a string passed to artifact(...)), this emits const foo-lib = ... and the generated build.zig becomes syntactically invalid before it can link the artifact. Since the schema only stores the provider artifact name and has no separate variable alias, the codegen needs to sanitize or introduce an alias instead of reusing art.name as the local identifier.
Useful? React with 👍 / 👎.
| // target, .optimize = optimize, <merged dep_options> });` | ||
| const opts = try mergeDepOptions(allocator, m.dep_options, desktop.dep_options); | ||
| defer allocator.free(opts); | ||
| try w.print(" const backend_dep = b.dependency(\"{s}\", .{{ .target = target, .optimize = optimize", .{m.dep_name}); |
There was a problem hiding this comment.
When a v2 manifest's .dep_name differs from the assembler's labelle_{cfg.backendName()} convention, the generated build.zig asks for this manifest-provided key but generateBuildZigZon/deps_linker still emit the backend dependency under the cfg-derived key. The new sokol_v2only fixture has exactly that shape (backend_package.name = "sokol_v2only", manifest .dep_name = "labelle_sokol"), so a full generated project would stage .labelle_sokol_v2only while this line calls b.dependency("labelle_sokol", ...), producing a missing dependency at build time. Either the zon/deps emission needs to use the v2 manifest dep name too, or the manifest should be validated to match the cfg-derived key.
Useful? React with 👍 / 👎.
1 participant
PR 3 of the manifest-v2 epic (#461, #453 item 3). Wires the v2 desktop codegen path behind the version gate and proves the sokol-desktop byte anchor: 0 diff vs the enum/v1 splice.
Byte anchor — proven live
The v2-generated sokol-desktop
build.zigis byte-identical to the enum/v1 baseline across the cfg matrix (gamepad auto/off, hidapi on/off, +plugins/zig_ecs). Verified non-vacuous: a 1-byte v2-manifest change (labelle_sokol→labelle_sokolX) flipszig build testto exit 1 with a readable diff; reverting restores 0.Wired
src/codegen/manifest_v2_splice.zig(new): v2 desktop codegen —b.dependencyliteral (dep name + base↔per-platformdep_optionsmerge, values from the closedValueSourcepredicate), provider.module(...)decls honoringroot_alias(defaultbackend_<name>),.artifact(...), linkLibrary/framework wiring — all from typedBackendManifestV2.src/build_files.zig: opt-inBuildZigOptions.backend_manifest_name. Null ⇒ production path 100% unchanged (v1/enum). Set ⇒ header-first parse + dispatch (manifest_version >= 2→ v2 codegen).backends/sokol/backend.manifest.v2.zon(new, per §3) +backend.hook.zig(new, empty desktoppost_wire) — the retained v1backend.manifest.zonis untouched (other tests depend on it); shipped labelle-sokol repo not touched.Walk-splice reconciliation (documented deviation)
Splicing the generic
core_diamond.generated_walk_zigwould change the text vs the enum baseline's unrolledoverrideImportblocks + backend-specific comments, breaking the 0-diff gate. Per design §7 (loop-vs-unrolled), the desktop byte anchor emits the unrolled overrides (byte-exact); the generic walk stays validated (PR 2 tests + a newemitCoreDiamondWalkdrift-guard) and is the mechanism for the golden-snapshot cells (PR 5+), whose gate is a reviewed golden, not byte-identity.v1/enum unchanged
Gate activates only for
manifest_version >= 2via the opt-in field; the field-less production manifest stays v1. All pre-existing generation/golden tests pass;zig build test+zig buildexit 0.Ref #461, #453.
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit