Repository navigation
feat(manifest-v2): lazy root build deps; lazy-safe emsdk preflight (fixes #823) - #824
Conversation
…#823) A Labelle wasm build fetched the emsdk package into the checkout even when `EMSDK` named a ready emsdk, partly because the generated `build.zig.zon` declares emsdk as an eager root dependency. - `RootBuildDep.lazy` (default false): when a backend manifest sets it, the generated build.zig.zon entry gets `.lazy = true` (for `.builtin` emsdk and `.remote` entries), so Zig fetches it only when a build script calls `b.lazyDependency`. labelle-bgfx sets it on its wasm emsdk root dep; its hook only asks for emsdk without a valid EMSDK. Older assemblers ignore the unknown field; manifests without it emit byte-identical zon. - The emitted `ensureEmsdkActivated` preflight resolves the package with `b.lazyDependency(...) orelse return` instead of `b.dependency`, which panics on a lazy entry. It works unchanged for eager entries. Goldens updated. Claude-Session: https://claude.ai/code/session_01Hpt1haxcboZPznpG1KqDRS
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Manifest option and dependency emission src/codegen/manifest_v2.zig, docs/design/manifest-v2-build-graph.md, src/codegen/manifest_v2_splice/common.zig |
RootBuildDep adds a default-false lazy option. The generator emits .lazy = true for lazy builtin and remote dependencies, and tests cover parsing, generated output, and malformed builtin sections. |
Lazy emsdk preflight src/codegen/emsdk_preflight.zig, src/build_files/build_zig.zig, test/goldens/*_wasm*.build.zig |
The generated preflight uses b.lazyDependency and returns when emsdk is unavailable. Its tests and WASM build goldens check or use lazy resolution. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant GeneratedBuildScript
participant EmsdkPreflight
participant ZigBuildRunner
GeneratedBuildScript->>EmsdkPreflight: Run emsdk activation preflight
EmsdkPreflight->>GeneratedBuildScript: Call b.lazyDependency("emsdk", .{})
GeneratedBuildScript-->>EmsdkPreflight: Return null when emsdk is unavailable
EmsdkPreflight-->>GeneratedBuildScript: Return without checking activation
ZigBuildRunner->>GeneratedBuildScript: Fetch dependency and rerun configuration
Merge Risk: 
fdf3e
-
No merge-blocking issue was identified. Backend hooks must follow the documented lazy-resolution contract.
Pre-merge checks: 
-
Check name Status Explanation Resolution Linked Issues check Issue #823requires the emsdk dependency to be lazy in the generated manifest and in labelle-audio, with header resolution from a validEMSDKand package fetch only as a fallback. This PR implements…Include reviewable labelle-audio changes that declare its emsdk dependency lazy, resolve headers from the valid EMSDKlayout, fetch the package only when that toolchain is unavailable, and add automated tests for those paths.-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: a new lazy option for root build dependencies in manifest v2, plus a lazy-safe emsdk preflight. The raw summary and objectives both describe these changes as the core … Out of Scope Changes check The changed schema, manifest emission, generated preflight, WASM goldens, tests, and design documentation all support the lazy emsdk dependency objective in #823. No unrelated change is established by…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Title check
-
Explanation
The title names the main change: a new lazy option for root build dependencies in manifest v2, plus a lazy-safe emsdk preflight. The raw summary and objectives both describe these changes as the core of the PR. The title also references issue
#823, which the objectives link to this work. The title is concise, specific, and single-sentence, so it meets the criteria.
Full details: Out of Scope Changes check
-
Explanation
The changed schema, manifest emission, generated preflight, WASM goldens, tests, and design documentation all support the lazy emsdk dependency objective in
#823. No unrelated change is established by the available evidence.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (8 skipped: 8 unsupported.)
-
Full details: Linked Issues check
-
Explanation
Issue
#823requires the emsdk dependency to be lazy in the generated manifest and in labelle-audio, with header resolution from a validEMSDKand package fetch only as a fallback. This PR implements the assembler part:RootBuildDep.lazyemits.lazy = truefor builtin and remote entries, and the generated preflight usesb.lazyDependency("emsdk", .{}). No labelle-audio implementation is present in the reviewed repository. References to related pull requests do not establish that code at this head. Theemsdk_preflightcode also does not provide labelle-audio header resolution.Resolution
Include reviewable labelle-audio changes that declare its emsdk dependency lazy, resolve headers from the valid
EMSDKlayout, fetch the package only when that toolchain is unavailable, and add automated tests for those paths.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the build at dawn
“Lazy emsdk,” it says, “wait on”
If the package is not there
The runner fetches it with care
Then the build resumes its song
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
apotema
left a comment
apotema
left a comment
There was a problem hiding this comment.
Codex, OpenAI LLM — review of fdf3e78141d5917ab9242d42661e28a093e60976.
No correctness findings in the lazy-dependency manifest/code-generation diff. I checked default compatibility (lazy = false), built-in and remote ZON emission, unchanged path dependencies, and the generated lazyDependency(... ) orelse return first-pass behavior. The eager and lazy golden fixtures cover the intended contract.
Validation: current-head GitHub CI run 41159616796 is successful. Local zig build test -j2 --summary all finished with 3,830 passes, 47 skips and one failure: a nested i18n compile reported Unexpected while reading Zig's own std/os/uefi/protocol/shell_parameters.zig. Re-running the unchanged failing test executable in isolation passed all 6 tests. I found no connection between that transient local file-read failure and this diff; I am not claiming the original full local run was green.
The partial-sysroot readiness issue reported on audio#12/bgfx#210/imgui#49 belongs to those SDK helpers, not this assembler diff. Keep that correction in the release train.
… inspector outlining (#826) Version-only release of #824 (manifest-v2 optional `.lazy` on root build deps; lazy-safe emsdk preflight, fixes #823) and #822 (debug entity inspector outlining, labelle-engine#922), part of labelle-toolkit/labelle-cli#528. Claude-Session: https://claude.ai/code/session_01Hpt1haxcboZPznpG1KqDRS
Fixes #823 (epic labelle-toolkit/labelle-cli#528).
This PR
RootBuildDep.lazy(manifest v2, default false): the generatedbuild.zig.zonentry gets.lazy = true(for.builtinemsdk and.remoteentries). Without the field the output is byte-identical. Older assemblers skip the unknown field.ensureEmsdkActivatedpreflight usesb.lazyDependency("emsdk", .{}) orelse return, becauseb.dependencypanics on a lazy entry. It behaves the same for eager entries. The three wasm goldens are updated..lazy, and that the preflight never callsb.dependency("emsdk"). The design doc documents the field.Tests
zig build test -j2on Windows: 3869 tests, all pass. The first run had 5 failures, all mine (my new test plus the three goldens); they were fixed before pushing.Why a valid
EMSDKstill installed a second emsdkBaseline from the box: GitLab job #133, FP
mainpipeline #30, imagewasm-builder 4.1.2-1, withEMSDK=/opt/emsdkinherited (the log says "labelle-web: emsdk from inherited EMSDK: /opt/emsdk"). Every bundle, cold and warm, still installed a second emsdk into.labelle/bgfx_wasm/zig-pkg/N-V-__8AAJl1DwBezhYo_VE6f53mPVm00R-Fk28NPW7P14EQ/. It downloadednode-v20.18.0-linux-x64.tar.xz(25,738,964 B, about 5 s) andwasm-binaries.tar.xz(301,247,292 B, about 42 s), then unpacked and activated them: about 45 to 60 s per bundle. emcc then ran from that zig-pkg emsdk, and Emscripten logged "config changed, clearing cache" between runs. Test-game bundle: 209 s cold, 151 s warm.What triggers it:
emsdk_source.resolve(Freshinit→generatefails: scaffolds "dev" versions, fetch maps them to a bogus "vdev" git ref #159/gui_resolve: support.packageand.urlGUI plugin references #163) treated an EMSDK as valid only if<EMSDK>/upstream/emscripten/cache/sysroot/includeexisted. The image keeps/opt/emsdkread-only without that directory and pointsEM_CACHEat/cache/..., so the build fell back to the package and ranemsdk install/activate lateston it (1,390,146,850 B unpacked). The hook's link and labelle-imgui's bridges use the same rule.build.zig.zon(assembler): emsdk is an eager root dependency, so it is always fetched (about 1 MB launcher).b.lazyDependency("emsdk")on wasm, and reads its sysroot from the package that bgfx installed..sokolentry pulls sokol-zig plus its eager emsdk 5.0.6 checkout and sokol-tools-bin, about 57 MB, even for a bgfx build.The fix: one rule, in four repos
.emscripten+upstream/emscripten/emcc.$EM_CACHE/sysroot/includewhenEM_CACHEis set, else<EMSDK>/upstream/emscripten/cache/sysroot/include. A cold one is filled at configure time withembuilder build sysroot(headers only, under a second), under that emsdk's own EM_CONFIG.b.lazyDependency) only when there is no valid EMSDK, and the root entry is declared.lazy = true.emsdk_source.zigis identical in labelle-bgfx, labelle-imgui (both bridges) and labelle-audio.EMSDK unset: unchanged. The package is fetched, installed and activated as before.
PRs:
Validation: Flying Platform in the WASM builder image
Local runs used Docker Desktop and the image
labelle-wasm-builder:local(amd64,EMSDK=/opt/emsdk,EM_CACHEon an ext4/cache), with steps run as UID 10001,--memory=14g, and FP ate90a638on a clean checkout each run.For the after runs, FP had local overrides that were never committed to FP:
local:;labelle_audiopointed at the audio branch commit's archive;LABELLE_ASSEMBLERset to this assembler branch, built for x86_64-linux.Before each EMSDK run, the emsdk (4.0.9 and 5.0.6), sokol and sokolshdc tarballs were removed from the Zig global cache, so anything that still needed them would have had to fetch them again.
EMSDK=/opt/emsdk, warm.labelle/**/zig-pkg.labelle/bgfx_wasm/zig-pkgtotallabelle bundle/opt/emsdk, sysroot underEM_CACHElabelle bundle--isolated --expect-threads yes) / fallback smoke (--expect-threads no)Box baseline, for comparison: 151 s warm, which includes the 45 to 60 s emsdk install.
The first after run recompiled the game because the bgfx, imgui, audio and assembler sources had changed (bundle 936 s). It had the same zero-emsdk numbers (network 156,722 B), plus Emscripten's one-time "config changed, clearing cache" as it moved from the zig-pkg emsdk to
/opt/emsdk.EMSDK unset (
env -u EMSDK, labelle-webemsdk.source = "package", emsdk purged from the global cache): unchanged behavior. Zig fetched the emsdk package during generation'szig build --list-stepsprobe, and labelle-web activated it in place ("activating fetched emsdk 4.0.9 in place"). The build installed it (1,390,216,762 B,upstream/+.emscripten; 1.36 GB received) and linked with it. The bundle and both smokes passed (bundle 553 s: Emscripten system libraries rebuilt for the other emsdk).The first package-mode run failed with labelle-web
NoFetchedEmsdk. bgfx'sbuildWasmreturned early on the pass before the lazy package was fetched, leaving nobgfxartifact, and the generated build panicked ("unable to find artifact 'bgfx'") before Zig could fetch. The root's eager emsdk entry had hidden this. Fixed in labelle-bgfx#210 (commitscd6eaec,332f1c6) and re-run green as above. The EMSDK-set rows are from a run with that final code.Release order
labelle_audiopin to 0.5.1 in labelle-bgfx#210 before it is released..lazytakes effect only with a bgfx manifest that sets it, and older assemblers ignore the field.https://claude.ai/code/session_01Hpt1haxcboZPznpG1KqDRS
Summary by CodeRabbit