docs(#453): implementation design — build-graph manifest v2 + build hook - #456
Conversation
Pre-implementation review doc for issue #453 item 3, the capstone of the backend-agnostic epic (#386). Pins the concrete v2 BackendManifest field surface, the declarative-vs-hook split (verified against the real sokol fragments + build_zig.txt sections), the generic core-diamond graph walk, migration ordering, byte-equivalence differential test, and a PR breakdown. No code — design only. 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! |
📝 WalkthroughWalkthroughAdds a design document proposing a "manifest v2" build-graph interface replacing v1 raw-Zig-text fragment splicing. It defines a typed ChangesManifest v2 Build-Graph Design
Sequence Diagram(s)sequenceDiagram
participant Assembler
participant BackendManifestV2
participant BuildHook
participant BuildZig
Assembler->>BackendManifestV2: read typed manifest data
Assembler->>BuildHook: invoke pre_wire for DependencyOptions
BuildHook-->>Assembler: return dependency option flags
Assembler->>BuildHook: invoke post_wire after generic wiring
BuildHook-->>BuildZig: perform residual platform wiring
Assembler->>BuildZig: generate build graph
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 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: 1
🤖 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 `@docs/design/manifest-v2-build-graph.md`:
- Around line 209-223: The `BackendManifestV2` shape currently makes
`manifest_version` mandatory, which prevents old v1 manifests from being parsed
before the version gate can route them to the legacy path. Update the v2
manifest loading flow so `loadManifest` or the `BackendManifestV2` definition
can read the version safely first, either by making `manifest_version`
optional/defaulted or by doing a small pre-parse to inspect the version before
full parsing. Keep the gating logic in the manifest version check so v1
manifests still follow the existing splice path.
🪄 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: ad7954d6-2ff7-4a4c-9cbd-88c0d245dc67
📒 Files selected for processing (1)
docs/design/manifest-v2-build-graph.md
| The v2 `BackendManifest` supersedes the v1 struct in `manifest_splice.zig:52`–`81`. | ||
| It is a ZON file (`backend.manifest.zon`), parsed with `std.zon.parse.fromSliceAlloc` | ||
| exactly as `loadManifest` does today (`manifest_splice.zig:170`–`188`), and gated | ||
| on `.manifest_version` (§6). | ||
|
|
||
| ### Concrete field surface | ||
|
|
||
| ```zig | ||
| /// v2 build-graph manifest. Parsed from `backends/<dir>/backend.manifest.zon`. | ||
| /// Every field is DATA the assembler wires generically — no backend enum tag | ||
| /// appears, and no field is raw Zig source (that was the v1 fragment mistake). | ||
| pub const BackendManifestV2 = struct { | ||
| /// Gate. v2 fields are only read when this is >= 2. v1 manifests | ||
| /// (no field / == 1) stay on the v1 splice + enum path (§6). | ||
| manifest_version: u8, // = 2 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Make v1 manifests readable before the version gate.
BackendManifestV2 requires manifest_version, but the migration plan says existing v1 manifests have no field and should stay on the old path. A direct parse into this struct will fail before the >= 2 check runs, so the compatibility story does not work as written. Make the version field optional/defaulted, or parse the version in a tiny pre-pass first.
Also applies to: 612-618
🤖 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 `@docs/design/manifest-v2-build-graph.md` around lines 209 - 223, The
`BackendManifestV2` shape currently makes `manifest_version` mandatory, which
prevents old v1 manifests from being parsed before the version gate can route
them to the legacy path. Update the v2 manifest loading flow so `loadManifest`
or the `BackendManifestV2` definition can read the version safely first, either
by making `manifest_version` optional/defaulted or by doing a small pre-parse to
inspect the version before full parsing. Keep the gating logic in the manifest
version check so v1 manifests still follow the existing splice path.
![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: ed99c00141
ℹ️ 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".
| /// Cross-compile triple. `.native` for desktop; a triple string for | ||
| /// mobile/wasm. Replaces the hand-written resolveTargetQuery blocks. | ||
| target: Target, // .native | .{ .triple = "aarch64-linux-android" } |
There was a problem hiding this comment.
For iOS and Android this cannot be a static manifest triple: the current template chooses simulator vs device and Android ABI from -Ddevice, -Demulator, -Dandroid_arch, and host arch before any b.dependency calls. With only .native/.triple here and the hook running either for options or after generic wiring, v2 would always use the hard-coded manifest target for dependency resolution, dropping simulator/emulator/all-ABI builds and failing the promised 0-diff migration.
Useful? React with 👍 / 👎.
| if (std.mem.eql(u8, key, "labelle-core") or std.mem.eql(u8, key, "labelle_core")) { | ||
| overrideImport(root, key, core_mod); // preserve the existing key spelling | ||
| } else { | ||
| unifyCoreDiamond(entry.value_ptr.*, core_mod, visited); // recurse into sub-packages |
There was a problem hiding this comment.
When this replaces the fixed .deps diamond, the walk only rewrites labelle-core/labelle_core imports; it never preserves the current overrideImport(engine_mod, "labelle-gfx", gfx_mod). In any v2 build where engine resolves its own gfx module, this recurses into that module instead of replacing the import, leaving two labelle-gfx instances across the app/engine seam.
Useful? React with 👍 / 👎.
| /// C archives the provider ships. Generic link pass calls | ||
| /// `linkLibrary(backend_dep.artifact(name))` for each; applies `.pic`. | ||
| artifacts: []const ArtifactDecl, |
There was a problem hiding this comment.
A single top-level artifact list that the generic pass links for every active platform cannot represent the current bgfx graph: desktop links both bgfx and glfw, while Android exposes only bgfx and has no glfw artifact. A v2 bgfx manifest would either omit glfw on desktop or call backend_dep.artifact("glfw") on Android, so artifact presence and attributes such as PIC need platform scoping or filters.
Useful? React with 👍 / 👎.
| root_module: *std.Build.Module, | ||
|
|
||
| /// The root compile artifact (exe or dynamic lib), so post_wire can call | ||
| /// linkLibrary / setLibCFile / addLibraryPath. post_wire only. | ||
| root_artifact: *std.Build.Step.Compile, |
There was a problem hiding this comment.
pre_wire is called before b.dependency and before the generated exe/lib root is created, but these fields are non-optional in the same HookContext. Implementations would have to pass undefined post-only pointers into third-party hook code, so use separate pre/post context types or make the post-only fields nullable and unavailable during pre_wire.
Useful? React with 👍 / 👎.
| `manifest_version >= 2` (parsed first, before any v2 field is read — the parser | ||
| already tolerates this via `.ignore_unknown_fields = true`, | ||
| `manifest_splice.zig:183`). A v1 manifest (`backends/sokol/backend.manifest.zon` |
There was a problem hiding this comment.
This migration step relies on .ignore_unknown_fields to keep existing v1 manifests on the old path, but that option does not make missing required v2 fields acceptable. If PR1 parses the current sokol manifest as BackendManifestV2, it lacks manifest_version, id, modules, artifacts, etc. and will fail instead of falling back; parse a minimal version header first, then parse the full v2 schema only when the version is >= 2.
Useful? React with 👍 / 👎.
| /// Per-platform, per-OS system libraries → `linkSystemLibrary`. | ||
| system_libs: SystemLibs, | ||
|
|
||
| /// Per-platform Apple frameworks → `linkFramework` (Zig distinguishes | ||
| /// these from system libs). |
There was a problem hiding this comment.
The schema captures system libraries and frameworks, but it has no field for the current mobile root_module.link_libc = true assignments in ios_link, android_link, and android_link_bgfx. For iOS/Android backends with C/C++ artifacts, a v2 build generated only from these fields will omit the libc link mode even if it still lists the framework/system-library names.
Useful? React with 👍 / 👎.
| 3. **May only construct build-graph nodes** (`b.dependency`, `linkLibrary`, | ||
| `linkSystemLibrary`, `linkFramework`, `addSystemIncludePath`, `addLibraryPath`, | ||
| `setLibCFile`, `addWriteFiles`, module wiring) **and manifest-modeled | ||
| shell-outs** (emcc/xcrun/NDK probe). **No arbitrary filesystem access outside | ||
| the provider's own package, no network.** A backend that needs more than the |
There was a problem hiding this comment.
This still hands an arbitrary provider-authored Zig hook the full *std.Build and relies on contract/review to prevent filesystem, process, or network side effects. For third-party backends there is no review boundary, so a consumer build can execute exactly the arbitrary behavior the “constrained hook” is meant to rule out; the hook needs a mechanically limited API or the design should treat it as trusted build code.
Useful? React with 👍 / 👎.
| `BackendManifestV2` and the generic walk/link/hook machinery. Gate strictly on | ||
| `manifest_version >= 2` (parsed first, before any v2 field is read — the parser | ||
| already tolerates this via `.ignore_unknown_fields = true`, | ||
| `manifest_splice.zig:183`). A v1 manifest (`backends/sokol/backend.manifest.zon` |
There was a problem hiding this comment.
Gating on manifest_version >= 2 means an older assembler will accept a future v3 declarative manifest and silently ignore any unknown fields; the hook ABI check only catches manifests that actually have a hook. To avoid generating an incomplete build graph for hookless future manifests, the gate should reject versions above the assembler's supported maximum.
Useful? React with 👍 / 👎.
| | **4** | sokol **android** (residual a: NDK sysroot + libc.txt in `post_wire`); add cell; 0-diff. | Med | M | | ||
| | **5** | sokol **ios** (residual b: xcrun in `post_wire`, `-Ddevice`); 0-diff. | Med | S | | ||
| | **6** | sokol **wasm** (residual c: emcc `emLinkStep`); 0-diff. | Med | M | | ||
| | **7** | Shared platform-packager delegation for `.platforms[p].package` (apk/web) — factor `.android_package`/`.wasm_footer` out of the template into a packager the manifest references. | Med | M | |
There was a problem hiding this comment.
The Android and WASM conversions are scheduled before the shared packager PR, but the v2 path described earlier delegates .platforms[p].package to that packager to replace .android_package and .wasm_footer. As ordered, the first Android/WASM v2 PRs cannot produce the promised complete 0-diff output unless they keep using enum template sections or implement the packager earlier.
Useful? React with 👍 / 👎.
| const sdk = getIosSdkPath(b, if (ctx.ios_device_mode) "iphoneos" else "iphonesimulator") orelse @panic("no iOS SDK"); | ||
| // configureSdkPaths / addExeSdkPaths … | ||
| }, | ||
| .wasm => { /* emccStep / emLinkStep on ctx.root_artifact */ }, |
There was a problem hiding this comment.
A WASM hook that runs emccStep/emLinkStep still needs any root build.zig.zon dependencies it calls via b.dependency, such as the current generated emsdk dependency for sokol wasm. The v2 manifest only describes build.zig wiring, so the hook can be generated successfully but then fail at zig build because the root project never declared the build-time dependency the hook tries to resolve.
Useful? React with 👍 / 👎.
chatgpt-codex raised 8 findings on PR #459 not covered by the #456 round. Verified each against the real code and folded the valid ones in: - root_build_deps now carry required resolution (url+hash/path/builtin); emsdk is a pinned template section, not name-synthesizable (#2) - bgfx-android android_app extra module requires root_alias="backend_app" (#3) - hookless mobile uses an assembler-owned default resolve_target; the backend-agnostic resolver means .resolved does not force a hook (#4) - build_hook must be a dedicated backend.hook.zig, not the provider build.zig (top-level @import("sokol") re-exports don't resolve in the root package) (#5) - android_target_sdk is required for Android; post_wire panics instead of the silent orelse 34 fallback (#6) - golden gate strengthened for hook-bearing cells: snapshot hook source and/or run the hook against a fixture *std.Build (#7) - carried v1 .capabilities forward into the v2 schema so opting into v2 doesn't bypass capability negotiation (.id was already present) (#8) Finding #1 (dep-option removal) was already resolved by ea24373 (base = universal options, per-platform = appends, no subtractive form) — recorded, not re-edited. Added a "PR #459 corrections" section documenting each. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
* docs(#453): fold PR #456 review findings into manifest-v2 design Revise the build-graph manifest v2 design doc to address the substantive coderabbitai/chatgpt-codex findings that PR #456 merged without incorporating, so the #453 item-3 implementation does not inherit the design flaws. Central corrections (all verified against the real code): - Dependency options are declarative (DepOption name + closed ValueSource predicate set), NOT a runtime pre_wire hook returning []Flag — a b.dependency options literal needs comptime-known field names. pre_wire/DependencyOptions deleted. - Target selection is a pre-dependency resolve_target phase (iOS device/sim + SDK, Android ABI) resolved from -Ddevice/-Demulator/-Dandroid_arch + host, not a static .triple; iOS SDK now computed before plugin b.dependency calls. - Core-diamond walk carries a gfx_mod singleton so it preserves engine->gfx. - Header-first bounded version parse (v1 stays readable, > SUPPORTED rejected). - Preserve backend_* import aliases; per-platform loop_style/artifacts/link_libc; root_build_deps for the emsdk wasm hook; android_target_sdk into HookContext. - Hook reframed as trusted build code (not mechanically sandboxable). - Byte-identical gate -> one desktop anchor + golden snapshots; packager PR moved before Android/wasm conversions. Docs-only. Adds a "Review corrections (PR #456)" summary section. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw * docs(#453): specify dep_options merge semantics + fix code-span spacing Address CodeRabbit findings on PR #459. Major — dep_options merge is now defined precisely: a comptime, name-keyed fold of per-platform entries over the base (override on collision, append otherwise), with NO subtractive form (an empty per-platform list inherits the base unchanged). Grounded in the v1 paramValue/param_names mechanism (manifest_splice.zig): the merge is a codegen-time operation on the NAME set, and because each platform's b.dependency literal is emitted independently, a name absent from a platform's folded set is simply never written. Corrected the sokol worked example to match build_zig.txt ground truth (:94/:124/:535/:763): with_imgui is the only base option (common to all four platforms); gamepad_* is a desktop-only append; dont_link_system_libs is an ios/android append (android's was missing). This dissolves the empty-wasm-list "drop" conflict — wasm forwards only with_imgui because gamepad_* was never in the base, not by removing it. Updated the "Review corrections" section to note the clarification. Minor — fixed inline code spans that wrapped across lines (MD038): the version gate spans (`< 1 or > SUPPORTED`, `2 <= v <= SUPPORTED_MANIFEST_VERSION`, `v > SUPPORTED`) and the `switch (target.result.os.tag)` span now sit on single lines. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw * docs(#453): fold PR #459 review findings into manifest-v2 design chatgpt-codex raised 8 findings on PR #459 not covered by the #456 round. Verified each against the real code and folded the valid ones in: - root_build_deps now carry required resolution (url+hash/path/builtin); emsdk is a pinned template section, not name-synthesizable (#2) - bgfx-android android_app extra module requires root_alias="backend_app" (#3) - hookless mobile uses an assembler-owned default resolve_target; the backend-agnostic resolver means .resolved does not force a hook (#4) - build_hook must be a dedicated backend.hook.zig, not the provider build.zig (top-level @import("sokol") re-exports don't resolve in the root package) (#5) - android_target_sdk is required for Android; post_wire panics instead of the silent orelse 34 fallback (#6) - golden gate strengthened for hook-bearing cells: snapshot hook source and/or run the hook against a fixture *std.Build (#7) - carried v1 .capabilities forward into the v2 schema so opting into v2 doesn't bypass capability negotiation (.id was already present) (#8) Finding #1 (dep-option removal) was already resolved by ea24373 (base = universal options, per-platform = appends, no subtractive form) — recorded, not re-edited. Added a "PR #459 corrections" section documenting each. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
1 participant
Implementation design document for item 3 of #453 (the capstone of the backend-agnostic epic #386). The shape is accepted in
RFC-PLUGGABLE-BACKENDS.md; this doc pins the concrete field surface, migration ordering, and byte-equivalence strategy before any coding.docs/design/manifest-v2-build-graph.md(743 lines), grounded in the actual code.Key decisions
b.dependencyoption flags. Two corrections to the original prediction:.picis declarative (manifest, not hook), and the per-OSswitch (target.result.os.tag)framework/syslib blocks are declarative (per-OS keys). GUI bridge is GUI-plugin-owned, out of scope.BackendManifestV2:.manifest_version,.modules,.artifacts(+.pic),.system_libs(per-OS),.frameworks,.platforms, optional.build_hook— with a full worked sokol rewrite.HookContext/DependencyOptionsreusingplugin_manifest.zig's gate discipline;pre_wire → DependencyOptions(flat flag list so third parties add flags without patching the assembler),post_wirefor NDK/xcrun/emcc, 4 constraints spelled out.labelle-core/labelle_coreoverride sites +unifyGfxSubpackageCoreinto one visited-set walk — flagged as highest regression risk (CI-cross-compile-only failures).manifest_version >= 2; sokol fixture first; desktop→android→ios→wasm; delete the enum path last.build.zigdiff, 0-diff per conversion PR.This is a design doc for review — no code changes. Ref #453, #386.
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit