Repository navigation
feat(#461): manifest-v2 PR7 — sokol wasm (sokol now fully v2) - #468
Conversation
Converts sokol **wasm** to the v2 build-graph manifest, the last sokol platform (epic #453 item 3). wasm is the emcc residual (design §2 (c)): - Hook `post_wire` .wasm arm supplies residual (c) — the Emscripten emcc link step. The enum path reaches emcc via `@import("labelle_sokol").emLinkStep`, but the hook is std-only and cannot import the provider package, so `emLinkStep` is reconstructed in `backend.hook.zig` from ONLY `std.Build` + the emsdk dependency (resolved via `b.dependency("emsdk", .{})`). Because `post_wire` returns void it also owns the install/run wiring the enum `.wasm_footer` did (the enum `emcc_step` local cannot escape a void hook), so the wasm v2 path does NOT route packaging through the packager `.web` footer — the documented enum-vs-v2 boundary. No `resolve_target` (static wasm32-emscripten triple, design §3). - `.root_build_deps = emsdk` (`.builtin`) is emitted into the generated build.zig.zon via `emitRootBuildDepsV2` (design §3 RootBuildDep, review #459 finding 2); `.builtin` reuses the pinned `dep_emsdk` section so the emitted zon stays byte-identical to the enum path. - v2 wasm codegen: hook-importing header + inline static-triple target, deps decls without the unrolled diamond, backend-dep + generic `unifyCoreDiamond` walk, declarative `linkLibrary(sokol_clib)` + `post_wire` call, and a footer (build-fn close + helpers) + the walk def. Golden cell `test/goldens/sokol_wasm_v2.build.zig` + hook/zon asserts: imports the hook, post_wire does the emcc step, no inline emLinkStep / `@import("labelle_sokol")` in build.zig, generic walk, emsdk in the generated zon (and byte-identical to the enum zon). Pure decision (512 KB stack arg) + emitRootBuildDepsV2 unit-tested; wasm_footer_helpers drift-guarded against the live android_footer tail. Invariants hold: desktop byte anchor 0-diff; android + ios goldens unchanged; v1/enum path unchanged (v2 via opt-in `backend_manifest_name` only). `zig build test` and `zig build` both exit 0. 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! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds manifest-v2 WASM generation and hook wiring so wasm builds route through a reconstructed emcc link step, with v2-specific header/deps/footer emission, zon root-dependency handling, golden output coverage, and broken-manifest error-path tests. ChangesWASM v2 build hook and codegen
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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
🤖 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 1003-1006: The ZON generation path in loadNamedManifest is
swallowing v2 manifest load failures via catch null, which causes silent
fallback instead of a hard failure. Update the opts.backend_manifest_name
handling in src/build_files.zig so manifest_v2.loadNamedManifest errors are
propagated or reported during generation, and only proceed to the parsed branch
when the manifest loads successfully. Use the existing
manifest_v2.loadNamedManifest and the surrounding parsed/pd/name logic to locate
the change.
In `@src/codegen/manifest_v2_splice.zig`:
- Around line 875-881: The ZON emission in manifest_v2_splice.zig writes
dependency .remote and .path values with raw string interpolation, so special
characters can break the generated output. Update the printing in the dependency
emission logic to escape .url, .hash, and .path using the existing
writeZigString helper from idents.zig instead of direct {s} formatting. Keep the
change localized to the code path that formats remote and path dependencies so
all build-dep strings are emitted safely.
🪄 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: e2c92181-b42f-4773-9cd1-105830117327
📒 Files selected for processing (5)
backends/sokol/backend.hook.zigsrc/build_files.zigsrc/codegen/manifest_v2_splice.zigtest/build_zig_tests.zigtest/goldens/sokol_wasm_v2.build.zig
… escaping
Finding 1 (build_files.zig): generateBuildZigZon loaded the v2 manifest
with `catch null` and fell back to enum output on error, which could pair
a build.zig that resolved its hook deps against a v2 manifest with a
build.zig.zon that silently fell back — a divergent (broken) pair. Now
the zon path mirrors generateBuildZig: gate the load on
`manifestPathEnabled` (missing manifest → enum fallback in BOTH) and
propagate load errors with `try`. A v2 manifest that fails to load now
errors in BOTH generators, so they always agree (both succeed or both
error). New fixture backends/sokol_v2broken + two tests prove the pair.
Finding 2 (manifest_v2_splice.zig): emitRootBuildDepsV2 wrote url/hash/path
into the generated build.zig.zon with raw `{s}`; a value containing a quote
or backslash produced malformed ZON. Escape via std.zig.fmtString (`{f}`)
for url/hash/path (hash is hex/low-risk but escaped for consistency).
Added a test with quote+backslash values.
zig build + zig build test both exit 0 (incl. desktop byte anchor 0-diff,
android/ios/wasm goldens, emsdk-zon byte-identity, and the new tests).
Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
![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: 322e666e30
ℹ️ 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".
| " .{s} = .{{\n .path = \"{f}\",\n }},\n", | ||
| .{ dep.name, std.zig.fmtString(p) }, |
There was a problem hiding this comment.
When a v2 manifest uses a relative .root_build_deps path, this writes that path verbatim into the generated build.zig.zon. Zig resolves dependency .path values relative to the generated build root, not relative to the backend package that owns the manifest, so a backend-shipped helper dependency like tools/helper will point under .labelle/<target>/tools/helper and fail to resolve. Resolve relative paths against the backend package/manifest location and emit a rebased or absolute path instead.
Useful? React with 👍 / 👎.
| " .{s} = .{{\n .url = \"{f}\",\n .hash = \"{f}\",\n }},\n", | ||
| .{ dep.name, std.zig.fmtString(r.url), std.zig.fmtString(r.hash) }, |
There was a problem hiding this comment.
For manifests whose root_build_deps key is not a Zig identifier (for example a third-party build-time dep named emsdk-tool or foo.bar), interpolating dep.name into .{s} emits invalid ZON such as .emsdk-tool = .{ ... }. Since the schema treats name as data and b.dependency accepts a string key, quote or validate the generated field name (e.g. emit a quoted identifier) before writing the dependency entry.
Useful? React with 👍 / 👎.
| if (opts.backend_manifest_name) |name| { | ||
| if (manifest_splice.manifestPathEnabled(allocator, cfg, pd, name)) { | ||
| const parsed = try manifest_v2.loadNamedManifest(allocator, cfg, pd, name); |
There was a problem hiding this comment.
When backend_manifest_name is set for an external backend and that manifest is missing, generateBuildZig reports ExternalBackendNeedsManifest, but this ZON path only checks manifestPathEnabled and silently falls back to enum-style output. That leaves callers able to get a successful build.zig.zon for a configuration whose build.zig cannot be generated, exactly the divergent pair this block is trying to avoid. Mirror generateBuildZig by calling requireManifestIfExternal before treating a missing named manifest as fallback.
Useful? React with 👍 / 👎.
1 participant
PR 7 of the manifest-v2 epic (#461, #453 item 3). Converts sokol wasm — the last sokol platform. With this, sokol is fully v2 across desktop/android/ios/wasm.
wasm
post_wire(emcc) —backends/sokol/backend.hook.zigThe enum path uses
@import("labelle_sokol").emLinkStep, but the hook is compiled std-only (its own test target) and can't import the provider package. ReconstructedemLinkStepfaithfully using onlystd.Build+ the emsdk dependency (b.dependency("emsdk").path(...)locates emcc; sokol-zig'semLinkStepis itself purestd.Build), so the emitted emcc command line matches the enum.link_sokol_wasmflags (use_webgl2,release_use_closure=false,-sSTACK_SIZE=512KB). Noresolve_target(staticwasm32-emscriptentriple).root_build_deps→ generatedbuild.zig.zonemitRootBuildDepsV2: a v2 manifest'sroot_build_depsbecome generated zon deps by resolution —.builtin "emsdk"reuses the pinneddep_emsdksection (byte-identical to enum),.remoteemits url+hash,.patha path; unknown.builtinhard-errors. Enum path (backend_manifest_name==null) unchanged. (Addresses #459 review finding 2.)Golden
test/goldens/sokol_wasm_v2.build.zig+ asserts: importsbackend_build_hook,post_wire{.platform=.wasm}, no inlineemLinkStep/@import("labelle_sokol")/emcc_step, noresolve_target, genericunifyCoreDiamond,linkLibrary(sokol_clib); + 2 zon tests (emsdk present + byte-identical to enum zon).post_wire, notrenderPackageV2(.web)post_wirereturnsvoid, so it can't export theemcc_steplocal to a separate packager footer that references it for install/run. Sopost_wireowns wasm install/run. The.webpackager fixture (PR 4) remains for the enum path / future use but isn't invoked for v2 wasm. Reviewers: is this the intended model, or shouldHookContext/post_wirebe extended to hand the emcc step to the packager? Flagging for a call.Invariants
Desktop byte anchor 0-diff; android + ios goldens unchanged (
git statusclean); v1/enum unchanged (v2 gated on opt-in).zig build test+zig buildexit 0.Ref #461, #453.
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit