feat(config): flip .backend = .bgfx to the extracted package by default (#386 Phase 6c) - #419
Merged
Conversation
…lt (#386 Phase 6c) `builtinProvider(.bgfx)` now returns the labelle-bgfx provider package (github.com/labelle-toolkit/labelle-bgfx v0.2.0), so `.backend = .bgfx` transparently fetches + builds the OUT-OF-TREE backend instead of the bundled `backends/bgfx` slot. bgfx is the first backend fully external by default. The enum-as-shorthand preserves the `.bgfx` tag, so every platform path keeps working: desktop via the manifest splice, android via the enum sections (which pull from `b.dependency("labelle_bgfx")` → fetched package). Both flip-blockers are already fixed (#416 callback guard, #417 enum-fallthrough). Test rework (bgfx is now external, not bundled): - backend_registry: the all-bundled guard now expects bgfx external (tag preserved, named "bgfx"); the "not external" case uses raylib (still bundled). - deps_linker: stagesSdlGamepad/stagesAndroidGamepad no longer stage siblings for bgfx (external = self-contained, carries its own gamepad packages). - build_zig: the bgfx-artifacts test overrides to the in-tree package via a local path + project_dir (desktop splice); the exe-naming tests switch to raylib (they test naming, not bgfx); the core-import loop drops bgfx. Existing projects on older assembler releases are unaffected (they pin those versions). On-device (Galaxy Tab A7) external bgfx builds + runs crash-free and behaves identically to bundled. NOTE: a clean-render confirmation against a VERSION-MATCHED stack is the recommended final check before this ships in a release (FP's current pins have a gfx/core mismatch that black-screens BOTH bundled and external — not the flip).
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changesbgfx extracted as external backend
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 extracts the bgfx backend into an external package, updating the project configuration to resolve .bgfx to its external repository and adjusting tests across backend_registry.zig, deps_linker.zig, and build_zig_tests.zig to reflect its new external, self-contained status. Feedback was provided to update an outdated comment block in src/config.zig that incorrectly states no backends have been extracted yet.
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.
| // .repo = "github.com/labelle-toolkit/labelle-bgfx", | ||
| // .version = "X.Y.Z" }, | ||
| .raylib, .sokol, .sdl, .bgfx, .wgpu, .null => null, | ||
| .bgfx => .{ .name = "bgfx", .repo = "github.com/labelle-toolkit/labelle-bgfx", .version = "0.2.0" }, |
There was a problem hiding this comment.
The comment block directly above this line (lines 613–617) states that 'No backend has been extracted yet — all ship bundled.' This is now outdated and incorrect because bgfx has been extracted to an external package in this pull request. Please update or remove that comment block to prevent confusion for future developers.
Contributor
Author
There was a problem hiding this comment.
Fixed — the comment now says bgfx is extracted (resolves to the labelle-bgfx provider) and the rest are still bundled, instead of the stale 'no backend extracted yet'.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/backend_registry.zig (1)
231-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the extracted provider metadata in this regression test.
Right now this only proves that
.bgfxis “some” external provider named"bgfx". If the repo or pinned tag drifts fromsrc/config.zig, these tests still pass and the break moves to fetch/build time. Assert the exactrepoandversionhere too.Suggested test hardening
if (cfg.backend == .bgfx) { try std.testing.expect(cfg.isExternal()); - try std.testing.expect(cfg.effectiveBackendPackage() != null); + const dep = cfg.effectiveBackendPackage().?; try std.testing.expectEqualStrings("bgfx", cfg.backendName()); + try std.testing.expectEqualStrings("github.com/labelle-toolkit/labelle-bgfx", dep.repo); + try std.testing.expectEqualStrings("0.2.0", dep.version); } else { try std.testing.expect(!cfg.isExternal()); try std.testing.expect(cfg.effectiveBackendPackage() == null); try std.testing.expectEqualStrings(f.name, cfg.backendName()); }🤖 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/backend_registry.zig` around lines 231 - 248, The bgfx regression test in the `enum-as-shorthand` case only checks that `.bgfx` is external and named `"bgfx"`, so it can miss drift in the pinned provider metadata. Strengthen the `cfg` assertions in this test to also verify the exact extracted backend package fields from `effectiveBackendPackage()`, specifically the expected `repo` and `version` values that match `src/config.zig`, while keeping the existing checks for `isExternal()` and `backendName()`.src/config.zig (1)
618-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
builtinProvidercontract comment in the same change.This branch makes the nearby “Every built-in maps to
nulltoday” note false, so the docs now describe the opposite behavior for.bgfx. Please refresh that comment here so future extractions/tests don’t inherit stale assumptions.🤖 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/config.zig` around lines 618 - 619, Update the stale builtinProvider contract comment near the .bgfx mapping so it no longer says every built-in maps to null; the current builtinProvider behavior now returns a real provider for .bgfx while the other built-ins still map to null. Refresh the comment in this area to accurately describe the mixed behavior, keeping it aligned with the builtinProvider and built-in mapping cases so future tests/extractions do not rely on outdated assumptions.test/build_zig_tests.zig (1)
110-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep one desktop test on the shorthand
.bgfxpath.This case now forces
backend_package, andeffectiveBackendPackage()makes that win overbuiltinProvider(.bgfx). If the new defaultlabelle-bgfxmapping regresses on desktop, this test still passes because it only exercises the local override. Please add a companion desktop test that leavesbackend_packageunset and verifies the shorthand path resolveslabelle_bgfxcorrectly.
🤖 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.
Nitpick comments:
In `@src/backend_registry.zig`:
- Around line 231-248: The bgfx regression test in the `enum-as-shorthand` case
only checks that `.bgfx` is external and named `"bgfx"`, so it can miss drift in
the pinned provider metadata. Strengthen the `cfg` assertions in this test to
also verify the exact extracted backend package fields from
`effectiveBackendPackage()`, specifically the expected `repo` and `version`
values that match `src/config.zig`, while keeping the existing checks for
`isExternal()` and `backendName()`.
In `@src/config.zig`:
- Around line 618-619: Update the stale builtinProvider contract comment near
the .bgfx mapping so it no longer says every built-in maps to null; the current
builtinProvider behavior now returns a real provider for .bgfx while the other
built-ins still map to null. Refresh the comment in this area to accurately
describe the mixed behavior, keeping it aligned with the builtinProvider and
built-in mapping cases so future tests/extractions do not rely on outdated
assumptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dedd289-9ab6-41db-a90a-dcc4a48c0899
📒 Files selected for processing (4)
src/backend_registry.zigsrc/config.zigsrc/deps_linker.zigtest/build_zig_tests.zig
…flip) validateCache + ensureCache checked the raw `.backend_package` field, which is null for a flipped built-in (`.backend = .bgfx` resolves via builtinProvider, not an explicit package). So a flipped backend was never fetched — ensureCache said 'all cached' and generate then failed staging with ExternalBackendNeedsManifest. Route both through `cfg.effectiveBackendPackage()` (the same accessor resolveBackendPackage uses) so an extracted built-in is fetched like any external backend. Caught by the bgfx Android .so cross-build CI job once #419 flipped bgfx; locally masked by the monorepo sibling-fallback + explicit-config paths.
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
…hase 6c) bgfx is now resolved out-of-tree (`.backend = .bgfx` → the labelle-bgfx provider, flipped in #419 and on-device-validated), so the in-tree `backends/bgfx/` slot is dead weight. Delete it (~the whole src/templates/libs/manifest tree) and the coverage that built it in-tree: - Remove the `bgfx-build` CI job (`cd backends/bgfx && zig build test` + the standalone example). The bgfx backend's unit tests now run in labelle-bgfx's own CI (labelle-bgfx#1); its desktop+android builds are still exercised here via the examples-integration `bgfx-external` step and the `bgfx-android-build` job (both fetch the package). - `bgfx-android-build` cache key now hashes the example + config (it pinned the deleted `backends/bgfx/build.zig.zon`). - Drop the desktop bgfx-artifacts unit test (no in-tree package to resolve; coverage moved to labelle-bgfx + the splice tests) and the bgfx case from the in-tree-template preview regression list. Other backends keep their bundled slots; only bgfx leaves. First backend fully out of the assembler bundle.
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
…hase 6c) (#420) bgfx is now resolved out-of-tree (`.backend = .bgfx` → the labelle-bgfx provider, flipped in #419 and on-device-validated), so the in-tree `backends/bgfx/` slot is dead weight. Delete it (~the whole src/templates/libs/manifest tree) and the coverage that built it in-tree: - Remove the `bgfx-build` CI job (`cd backends/bgfx && zig build test` + the standalone example). The bgfx backend's unit tests now run in labelle-bgfx's own CI (labelle-bgfx#1); its desktop+android builds are still exercised here via the examples-integration `bgfx-external` step and the `bgfx-android-build` job (both fetch the package). - `bgfx-android-build` cache key now hashes the example + config (it pinned the deleted `backends/bgfx/build.zig.zon`). - Drop the desktop bgfx-artifacts unit test (no in-tree package to resolve; coverage moved to labelle-bgfx + the splice tests) and the bgfx case from the in-tree-template preview regression list. Other backends keep their bundled slots; only bgfx leaves. First backend fully out of the assembler bundle.
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
) * chore(slim): remove the bundled bgfx backend — it's extracted (#386 Phase 6c) bgfx is now resolved out-of-tree (`.backend = .bgfx` → the labelle-bgfx provider, flipped in #419 and on-device-validated), so the in-tree `backends/bgfx/` slot is dead weight. Delete it (~the whole src/templates/libs/manifest tree) and the coverage that built it in-tree: - Remove the `bgfx-build` CI job (`cd backends/bgfx && zig build test` + the standalone example). The bgfx backend's unit tests now run in labelle-bgfx's own CI (labelle-bgfx#1); its desktop+android builds are still exercised here via the examples-integration `bgfx-external` step and the `bgfx-android-build` job (both fetch the package). - `bgfx-android-build` cache key now hashes the example + config (it pinned the deleted `backends/bgfx/build.zig.zon`). - Drop the desktop bgfx-artifacts unit test (no in-tree package to resolve; coverage moved to labelle-bgfx + the splice tests) and the bgfx case from the in-tree-template preview regression list. Other backends keep their bundled slots; only bgfx leaves. First backend fully out of the assembler bundle. * fix(stage): skip zig-pkg/.labelle when hardlinking a local package hardlinkTree skipped .zig-cache/zig-out/.git but copied zig-pkg/ and .labelle/. Staging a local backend package that's a real repo checkout (e.g. a backend repo shipping an example inside itself) then walked: - zig-pkg/<hash>/upstream/.../node_modules/... — deep enough that copyFile hit NameTooLong, which crashed staging (and tripped an invalid free on the way out). - .labelle/ — the in-repo example's own generated output, copied back into the stage. Neither is part of a package's importable modules, so skip both alongside the existing build/VCS dirs. Unblocks examples that live inside a backend package repo (labelle-bgfx/examples/*) resolving the backend via its local checkout.
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
…lt (#386 step 4) (#428) builtinProvider(.wgpu) → labelle-wgpu 0.1.0, so `.backend = .wgpu` now fetches the OUT-OF-TREE backend by default — wgpu is the second default-external backend (after bgfx #419). Desktop-only, loop-style, no android/callback complications. Test rework (same class as the bgfx flip): - backend_registry enum-as-shorthand: wgpu joins bgfx as external (tag preserved). - build_zig "links wgpu glfw artifact": point at a local wgpu pkg + project_dir so the manifest splice resolves in-tree (it now resolves external by default). - build_zig no-backend_input-override loop: drop wgpu (now external, exercised by the external tests) — only the bundled null backend remains in that loop. The cache fetch-path (effectiveBackendPackage in validate/ensureCache) + the externalUsesEnumPath guard already landed with the bgfx flip, so wgpu inherits them.
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
…lt (#386 step 4) (#431) * feat(config): flip .backend = .null to the extracted package by default (#386 step 4) builtinProvider(.null) → labelle-null 0.1.0 — null is the third default-external backend (after bgfx #419, wgpu #428). Pure-Zig, zero deps, empty link fragment. Notable: generateTestsTarget (#83) forces .backend=.null for every project's `zig build test`, so that universal test target now fetches labelle-null (cached, pure-Zig — the no-system-libs property still holds). The null headless + plugin-controllers examples + the is_tests_target codegen now resolve external null. Test rework (same class as the bgfx/wgpu flips): backend_registry enum-shorthand (null joins bgfx/wgpu external), and the 4 build_zig `.backend=.null` tests (null-wires-modules, no-override, the two is_tests_target tests) now point at a local null pkg + project_dir so the manifest splice resolves. Inherited the cache fetch-path + externalUsesEnumPath from the earlier flips. * fix(cache): fetch the tests-target null backend for every project The flip surfaced this (CI: a raylib project's tests-target generate failed ExternalBackendNeedsManifest): generateTestsTarget (#83) forces .backend=.null for EVERY project's `zig build test`, but validateCache/ensureCache only considered the project's OWN backend — so external labelle-null was never reported missing or fetched for a non-null project. Account for the tests-target null in both (dedup when the project already IS null). No-op while null is bundled. * test(cache): account for the tests-target null in validateCache external tests The two external-backend validateCache tests now also see the tests-target's external null in the missing set (every project validates null post-flip). Assert the project backend specifically (local stub) + expect both fakebackend AND null for the remote case.
This was referenced Jun 30, 2026
13 tasks
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.
bgfx becomes the first default-external backend
config.builtinProvider(.bgfx)now returns thelabelle-bgfxprovider (v0.2.0), so.backend = .bgfxtransparently fetches + builds the out-of-tree backend instead of the bundledbackends/bgfxslot. The enum-as-shorthand preserves the.bgfxtag, so every platform keeps working: desktop via the manifest splice, android via the enum sections (which pull fromb.dependency("labelle_bgfx")→ the fetched package). Both flip-blockers are already fixed (#416, #417).Real CI validation of the flipped android path
The bgfx Android .so cross-build job builds
examples/bgfx-android(.backend = .bgfx) — so on this PR it cross-compiles the flipped external bgfx for Android end-to-end. Green = the flip's android path works through CI.Test rework (bgfx is now external, not bundled)
"bgfx"); the "not external" case uses raylib.stagesSdlGamepad/stagesAndroidGamepadno longer stage siblings for bgfx (external = self-contained, carries its own gamepad packages).project_dir(desktop splice); exe-naming tests switch to raylib; the core-import loop drops bgfx.Compatibility & validation
Next (Phase 6c continued)
With the flip proven, the bundled
backends/bgfxbecomes removable (slim step), and the same pattern repeats for the remaining backends (each needs a manifest first; sdl/wgpu also need window-contract conformance).Summary by CodeRabbit
bgfxis resolved as an external backend package (instead of bundled built-in behavior), including updated cache validation and cache fetch logic driven by the effective backend package.bgfxno longer stages sibling SDL/Android assets that bundled backends would inherit.bgfxexternal resolution.bgfxextraction behavior, including tightened “no core import” coverage and backend configuration changes for naming expectations.