bgfx: manifest-driven codegen-splice for bgfx-desktop (epic #386 Phase 3/5) - #396
Merged
Conversation
Productionizes the sokol-desktop POC (c9aeec3) into a landable, manifest- presence-gated splice for bgfx-desktop. - Port manifest_splice.zig onto main; gate is now MANIFEST PRESENCE (a backend opts in by shipping backend.manifest.zon + being a desktop target), replacing the POC's LABELLE_POC_MANIFEST env var. @TagName stays only to LOCATE the package (documented Phase-5 name->registry seam). - Support loop_style=.loop (bgfx-desktop while(!shouldQuit) loop) in addition to the POC's .callback (sokol). paramValue handles bgfx's gui_enabled spelling of the imgui-only predicate alongside sokol's with_imgui. - Ship backends/bgfx/backend.manifest.zon (loop, templates/desktop.txt) + build_fragments/{backend_dep,link}.txt extracted verbatim from the embedded backend_bgfx/link_bgfx template sections. Desktop-only: wasm/ios/android stay on the enum path (bgfx-android NDK ordering is non-declarative). Verified: byte-identical main.zig + build.zig vs the enum path; generated build.zig/main.zig pass zig ast-check; negative-control sentinel proves the splice is active; zig build test green (enum path intact for every other backend x platform).
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a ChangesManifest-driven backend splice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a manifest-driven backend splice for pluggable backends (RFC #378), allowing the assembler to dynamically resolve run-loop styles and build.zig fragments from a backend manifest file instead of hardcoding enum branches for desktop targets. Feedback on the changes suggests using standard logging (std.log.warn) instead of std.debug.print for user-facing warnings, declaring the global mutable loop_style_override as threadlocal to ensure thread safety in concurrent contexts, and replacing a fixed-size buffer formatting with std.fmt.allocPrint in root.zig to prevent potential panics from unexpectedly long directory names.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+141
to
+144
| }) catch |err| { | ||
| std.debug.print("labelle-assembler: failed to parse backend.manifest.zon at {s}: {any}\n", .{ manifest_path, err }); | ||
| return error.BackendManifestParseError; | ||
| }; |
There was a problem hiding this comment.
In CLI tools, standard logging facilities (such as std.log.warn) should be used for user-facing warnings rather than debug-specific print functions like std.debug.print.
}) catch |err| {
std.log.warn("failed to parse backend.manifest.zon at {s}: {any}", .{ manifest_path, err });
return error.BackendManifestParseError;
};
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
Comment on lines
+1021
to
+1024
| return std.Io.Dir.cwd().readFileAlloc(config.globalIo(), tmpl_path, allocator, .limited(64 * 1024)) catch |err| { | ||
| std.debug.print("labelle: could not read manifest template '{s}': {any}\n", .{ tmpl_path, err }); | ||
| return error.TemplateNotFound; | ||
| }; |
There was a problem hiding this comment.
In CLI tools, standard logging facilities (such as std.log.warn) should be used for user-facing warnings rather than debug-specific print functions like std.debug.print.
return std.Io.Dir.cwd().readFileAlloc(config.globalIo(), tmpl_path, allocator, .limited(64 * 1024)) catch |err| {
std.log.warn("could not read manifest template '{s}': {any}", .{ tmpl_path, err });
return error.TemplateNotFound;
};
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
| /// refactor can thread it as a proper argument. Null keeps the enum path | ||
| /// verbatim. For bgfx-desktop the manifest declares `.loop`, so the resolved | ||
| /// `use_callback_lifecycle` is false — identical to the enum path. | ||
| pub var loop_style_override: ?manifest_splice.BackendManifest.LoopStyle = null; |
There was a problem hiding this comment.
To prevent data races and ensure thread safety in multi-threaded or concurrent contexts (such as parallel test execution), declare the global mutable state variable as threadlocal.
pub threadlocal var loop_style_override: ?manifest_splice.BackendManifest.LoopStyle = null;
References
- When using static variables for caching (such as memoizing pointers per type) in a multi-threaded or concurrent context, declare the cache variables as 'threadlocal' to prevent data races and ensure thread safety.
Comment on lines
+1015
to
+1017
| var sub_buf: [128]u8 = undefined; | ||
| const sub = std.fmt.bufPrint(&sub_buf, "backends/{s}", .{m.dir_name}) catch unreachable; | ||
| const backend_path = try cache.resolveBundledPackage(allocator, cfg.labelle_version, cfg.assembler_version, game_dir, sub); |
There was a problem hiding this comment.
Using a fixed-size buffer of 128 bytes with catch unreachable on a runtime-parsed string (m.dir_name) can lead to a hard panic if the directory name is unexpectedly long. Using std.fmt.allocPrint is safer and more robust.
const sub = try std.fmt.allocPrint(allocator, "backends/{s}", .{m.dir_name});
defer allocator.free(sub);
const backend_path = try cache.resolveBundledPackage(allocator, cfg.labelle_version, cfg.assembler_version, game_dir, sub);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/codegen/manifest_splice.zig (1)
151-157: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winResolve fragments from manifest data, not the enum tag.
Line 153 re-enters
backendPackageDir, so fragment loading still depends on@tagName(cfg.backend)after the manifest has been parsed. Ifdir_nameever diverges from the enum tag, the manifest can load but its fragments are read from the wrong package. Pass the resolved package root throughloadManifest, or resolve fromm.dir_namebefore reading fragments.🤖 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 151 - 157, The fragment loader in readFragment still derives the backend package path from backendPackageDir and cfg.backend, which bypasses the manifest-resolved directory name. Update loadManifest and the fragment-reading flow so the resolved package root comes from the manifest data (m.dir_name or an equivalent resolved root) and is passed into readFragment, instead of re-resolving via the enum tag.
🤖 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/codegen/main_template.zig`:
- Around line 36-46: The loop-style override is currently stored in public
module-global mutable state, which can leak between concurrent or nested runs.
Remove the global `loop_style_override` usage in `src/codegen/main_template.zig`
and thread the override explicitly through `generateMainZigFromTemplate` as an
option/argument, then update the call site in `root.zig` to pass and clear it
locally so lifecycle selection stays scoped per generation.
In `@src/codegen/manifest_splice.zig`:
- Around line 165-203: The param lookup in paramValue currently falls back to an
empty string for unrecognized manifest params, which hides typos and drift;
change it to return an error for unknown names instead of "" and propagate that
failure through renderFragmentWithParams. Use the existing paramValue and
renderFragmentWithParams flow in manifest_splice.zig so any unexpected manifest
parameter causes codegen to fail immediately rather than rendering invalid
fragments.
In `@src/root.zig`:
- Around line 1015-1017: The backend path construction in the code that uses
bufPrint for `backends/{s}` is using a fixed 128-byte stack buffer, which can
fail if `m.dir_name` from `backend.manifest.zon` is longer than expected.
Replace the temporary `sub_buf`/`bufPrint` flow with an allocator-backed string
from `allocPrint` in this backend resolution path, and make sure the resulting
value is handled consistently when calling `cache.resolveBundledPackage`.
---
Nitpick comments:
In `@src/codegen/manifest_splice.zig`:
- Around line 151-157: The fragment loader in readFragment still derives the
backend package path from backendPackageDir and cfg.backend, which bypasses the
manifest-resolved directory name. Update loadManifest and the fragment-reading
flow so the resolved package root comes from the manifest data (m.dir_name or an
equivalent resolved root) and is passed into readFragment, instead of
re-resolving via the enum tag.
🪄 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: baef09bc-d530-4f27-bdab-0d6acb0cdd3b
📒 Files selected for processing (7)
backends/bgfx/backend.manifest.zonbackends/bgfx/build_fragments/backend_dep.txtbackends/bgfx/build_fragments/link.txtsrc/build_files.zigsrc/codegen/main_template.zigsrc/codegen/manifest_splice.zigsrc/root.zig
Comment on lines
+36
to
+46
| /// Manifest-driven run-loop splice (pluggable-backends RFC, assembler#378). | ||
| /// When non-null, the run-loop style was resolved from the backend manifest's | ||
| /// `loop_style` field and OVERRIDES the enum-based `use_callback_lifecycle` | ||
| /// selection in `generateMainZigFromTemplate`. `root.zig` sets this | ||
| /// immediately before the call and clears it after (so the override is scoped | ||
| /// to that one generation). Module-level rather than a positional param on the | ||
| /// ~130-arg generator purely to keep the splice's diff localized — a later | ||
| /// refactor can thread it as a proper argument. Null keeps the enum path | ||
| /// verbatim. For bgfx-desktop the manifest declares `.loop`, so the resolved | ||
| /// `use_callback_lifecycle` is false — identical to the enum path. | ||
| pub var loop_style_override: ?manifest_splice.BackendManifest.LoopStyle = null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Thread the loop-style override through the generator instead of using global state.
loop_style_override is public module-global mutable state, so concurrent or nested generateMainZigFromTemplate calls can cross-contaminate lifecycle selection. Pass the override as an explicit option/argument instead.
Also applies to: 987-997
🤖 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/main_template.zig` around lines 36 - 46, The loop-style override
is currently stored in public module-global mutable state, which can leak
between concurrent or nested runs. Remove the global `loop_style_override` usage
in `src/codegen/main_template.zig` and thread the override explicitly through
`generateMainZigFromTemplate` as an option/argument, then update the call site
in `root.zig` to pass and clear it locally so lifecycle selection stays scoped
per generation.
Comment on lines
+165
to
+203
| fn paramValue(name: []const u8, cfg: ProjectConfig) []const u8 { | ||
| if (std.mem.eql(u8, name, "with_imgui") or std.mem.eql(u8, name, "gui_enabled")) { | ||
| // sokol spells this `with_imgui`, bgfx spells it `gui_enabled`; both are | ||
| // the same imgui-only predicate (true iff the resolved gui plugin is | ||
| // imgui). Two names, one computation — matches both enum branches. | ||
| return if (cfg.resolved_gui) |gui| | ||
| (if (std.mem.eql(u8, gui.name, "imgui")) "true" else "false") | ||
| else | ||
| "false"; | ||
| } | ||
| if (std.mem.eql(u8, name, "gamepad_enabled")) { | ||
| return if (cfg.gamepad == .auto) "true" else "false"; | ||
| } | ||
| if (std.mem.eql(u8, name, "gamepad_hidapi")) { | ||
| return if (cfg.gamepad_hidapi) "true" else "false"; | ||
| } | ||
| return ""; | ||
| } | ||
|
|
||
| /// Render the fragment against the manifest-declared param set using the | ||
| /// dynamic template engine (runtime string map — the param list is data, not a | ||
| /// comptime struct). Equivalent to the enum path's | ||
| /// `tpl.renderSection(.., .{ .with_imgui = .., .. })`. | ||
| fn renderFragmentWithParams( | ||
| allocator: std.mem.Allocator, | ||
| fragment: []const u8, | ||
| param_names: []const []const u8, | ||
| cfg: ProjectConfig, | ||
| w: anytype, | ||
| ) !void { | ||
| var data = tpl.TemplateData{ | ||
| .scalars = std.StringHashMap([]const u8).init(allocator), | ||
| .lists = std.StringHashMap([]const tpl.ListItem).init(allocator), | ||
| }; | ||
| defer data.scalars.deinit(); | ||
| defer data.lists.deinit(); | ||
| for (param_names) |name| { | ||
| try data.scalars.put(name, paramValue(name, cfg)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail on unknown manifest params instead of rendering an empty value.
Line 181 makes manifest typos silently substitute "", which can generate invalid or semantically wrong build.zig fragments. Return an error for unknown params so schema/fragment drift fails at codegen time.
Proposed fix
-fn paramValue(name: []const u8, cfg: ProjectConfig) []const u8 {
+fn paramValue(name: []const u8, cfg: ProjectConfig) ![]const u8 {
if (std.mem.eql(u8, name, "with_imgui") or std.mem.eql(u8, name, "gui_enabled")) {
// sokol spells this `with_imgui`, bgfx spells it `gui_enabled`; both are
// the same imgui-only predicate (true iff the resolved gui plugin is
// imgui). Two names, one computation — matches both enum branches.
@@
if (std.mem.eql(u8, name, "gamepad_hidapi")) {
return if (cfg.gamepad_hidapi) "true" else "false";
}
- return "";
+ std.debug.print("labelle-assembler: unknown backend manifest param `{s}`\n", .{name});
+ return error.UnknownBackendManifestParam;
}
@@
defer data.lists.deinit();
for (param_names) |name| {
- try data.scalars.put(name, paramValue(name, cfg));
+ try data.scalars.put(name, try paramValue(name, cfg));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn paramValue(name: []const u8, cfg: ProjectConfig) []const u8 { | |
| if (std.mem.eql(u8, name, "with_imgui") or std.mem.eql(u8, name, "gui_enabled")) { | |
| // sokol spells this `with_imgui`, bgfx spells it `gui_enabled`; both are | |
| // the same imgui-only predicate (true iff the resolved gui plugin is | |
| // imgui). Two names, one computation — matches both enum branches. | |
| return if (cfg.resolved_gui) |gui| | |
| (if (std.mem.eql(u8, gui.name, "imgui")) "true" else "false") | |
| else | |
| "false"; | |
| } | |
| if (std.mem.eql(u8, name, "gamepad_enabled")) { | |
| return if (cfg.gamepad == .auto) "true" else "false"; | |
| } | |
| if (std.mem.eql(u8, name, "gamepad_hidapi")) { | |
| return if (cfg.gamepad_hidapi) "true" else "false"; | |
| } | |
| return ""; | |
| } | |
| /// Render the fragment against the manifest-declared param set using the | |
| /// dynamic template engine (runtime string map — the param list is data, not a | |
| /// comptime struct). Equivalent to the enum path's | |
| /// `tpl.renderSection(.., .{ .with_imgui = .., .. })`. | |
| fn renderFragmentWithParams( | |
| allocator: std.mem.Allocator, | |
| fragment: []const u8, | |
| param_names: []const []const u8, | |
| cfg: ProjectConfig, | |
| w: anytype, | |
| ) !void { | |
| var data = tpl.TemplateData{ | |
| .scalars = std.StringHashMap([]const u8).init(allocator), | |
| .lists = std.StringHashMap([]const tpl.ListItem).init(allocator), | |
| }; | |
| defer data.scalars.deinit(); | |
| defer data.lists.deinit(); | |
| for (param_names) |name| { | |
| try data.scalars.put(name, paramValue(name, cfg)); | |
| } | |
| fn paramValue(name: []const u8, cfg: ProjectConfig) ![]const u8 { | |
| if (std.mem.eql(u8, name, "with_imgui") or std.mem.eql(u8, name, "gui_enabled")) { | |
| // sokol spells this `with_imgui`, bgfx spells it `gui_enabled`; both are | |
| // the same imgui-only predicate (true iff the resolved gui plugin is | |
| // imgui). Two names, one computation — matches both enum branches. | |
| return if (cfg.resolved_gui) |gui| | |
| (if (std.mem.eql(u8, gui.name, "imgui")) "true" else "false") | |
| else | |
| "false"; | |
| } | |
| if (std.mem.eql(u8, name, "gamepad_enabled")) { | |
| return if (cfg.gamepad == .auto) "true" else "false"; | |
| } | |
| if (std.mem.eql(u8, name, "gamepad_hidapi")) { | |
| return if (cfg.gamepad_hidapi) "true" else "false"; | |
| } | |
| std.debug.print("labelle-assembler: unknown backend manifest param `{s}`\n", .{name}); | |
| return error.UnknownBackendManifestParam; | |
| } | |
| /// Render the fragment against the manifest-declared param set using the | |
| /// dynamic template engine (runtime string map — the param list is data, not a | |
| /// comptime struct). Equivalent to the enum path's | |
| /// `tpl.renderSection(.., .{ .with_imgui = .., .. })`. | |
| fn renderFragmentWithParams( | |
| allocator: std.mem.Allocator, | |
| fragment: []const u8, | |
| param_names: []const []const u8, | |
| cfg: ProjectConfig, | |
| w: anytype, | |
| ) !void { | |
| var data = tpl.TemplateData{ | |
| .scalars = std.StringHashMap([]const u8).init(allocator), | |
| .lists = std.StringHashMap([]const tpl.ListItem).init(allocator), | |
| }; | |
| defer data.scalars.deinit(); | |
| defer data.lists.deinit(); | |
| for (param_names) |name| { | |
| try data.scalars.put(name, try paramValue(name, cfg)); | |
| } |
🤖 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 165 - 203, The param lookup in
paramValue currently falls back to an empty string for unrecognized manifest
params, which hides typos and drift; change it to return an error for unknown
names instead of "" and propagate that failure through renderFragmentWithParams.
Use the existing paramValue and renderFragmentWithParams flow in
manifest_splice.zig so any unexpected manifest parameter causes codegen to fail
immediately rather than rendering invalid fragments.
- std.debug.print → std.log.warn for the two user-facing manifest warnings (CLI logging convention) - loop_style_override → threadlocal (no cross-thread race under parallel tests) - backends/<dir> path: allocPrint instead of a fixed 128-byte buffer + catch unreachable (m.dir_name is a runtime-parsed manifest field). All Gemini.
This was referenced Jun 28, 2026
This was referenced Jun 28, 2026
Merged
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
…step 2) (#426) * fix(wgpu): reset quit/timing state in initWindow (close→reopen) quit_requested + last_frame_time persisted across a window close→reopen: a prior requestQuit would close the new window immediately, and the first frameDuration would be a huge time-since-old-baseline. Reset both at initWindow (same fix raylib got in #411). CodeRabbit/gemini on #424. * feat(wgpu): add backend.manifest.zon — manifest-splice codegen (#386 step 2) wgpu step 2 of out-of-tree extraction (after window conformance #424). Presence of backends/wgpu/backend.manifest.zon opts the wgpu DESKTOP build into the manifest-splice path (manifest_splice.zig) instead of the enum `switch (cfg.backend)` sections in build_zig.txt — exactly as bgfx did in #396. Loop-style, desktop-only (wgpu has no wasm/android target). No params: the fragments take no gamepad/gui toggles and wgpu pulls no shared gamepad sub-package, so unlike bgfx there's no core-diamond override. build_fragments/{backend_dep,link}.txt are the verbatim .backend_wgpu / .link_wgpu section bodies. Output is BYTE-IDENTICAL to the enum path (diffed a generated baseline: 0 differences). `zig build test` green (golden suites unchanged). This is the step that lets wgpu's build sections travel WITH the package on extraction.
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Productionizes the throwaway sokol splice POC (RFC #378 rev 15) into a real, landable manifest-driven path for bgfx-desktop — and extends it to the loop model the POC never exercised (it did sokol/callback).
What
backends/bgfx/backend.manifest.zon— declares as DATA whatswitch(cfg.backend) => .bgfxhardcodes for desktop:loop_style = .loop, the desktop template, the externalizedbackend_dep/linkbuild fragments + their params (gamepad_enabled/gamepad_hidapi/gui_enabled).build_fragments/{backend_dep,link}.txt— thebackend_bgfx/link_bgfxsections extracted verbatim from the assembler'sbuild_zigtemplate (viatpl.getSection→ byte-identity).manifest_splice.zig(ported from POCc9aeec3) — resolves the run-loop style + build fragments from the manifest, no=> .bgfxbranch in the splice logic.Productionized vs the POC
LABELLE_POC_MANIFEST=1: a backend opts in by shipping abackend.manifest.zon; the splice takes over iff the manifest exists AND the target is desktop. bgfx-desktop opts in; raylib/sdl/wgpu/null (no manifest) + bgfx wasm/ios/android (NDK link ordering stays non-declarative) all stay on the enum path, untouched.@tagNamejust locates the package to read its manifest — documented in-code as the Phase-5 name→registry seam (registry not built here).The headline result
The loop model fit the same manifest shape — no new fields, only a
gui_enabled/with_imguiparam alias. One manifest schema now covers both a callback backend (sokol) and a loop backend (bgfx). That's the encouraging signal for Phase 5.Verified
main.zig(27708 B) +build.zig(7964 B) identical between manifest path and enum path (zero diffs); both passzig ast-check.build_fragments/link.txtappears in the manifest-pathbuild.zigbut NOT the enum-path one — the splice genuinely reads the fragments.zig build testgreen (enum path intact for every other backend × platform).Caveat (not a splice defect)
A full
zig buildof the generated game fails atdecoded.compressed— an engine source/codegen version skew in the checked-out sibling repo (mid-refactor), outside the assembler. Because outputs are byte-identical between paths, the enum path fails identically; the splice contributes nothing.Summary by CodeRabbit