fix(codegen): let a tag-matched external backend use the enum path on non-desktop (#386) - #417
Merged
Merged
Conversation
) The backend-dep section rejected EVERY external backend that didn't reach the manifest splice (`else if (cfg.isExternal()) return ExternalBackendNeedsManifest`). But the splice is DESKTOP-ONLY, so on Android (and other non-desktop targets) an external backend has no splice — and the guard wrongly rejected it. For an external backend whose package name MATCHES its enum tag — e.g. the extracted bgfx selected via `.backend = .bgfx` (the enum-as-shorthand resolves to a package while preserving the tag, #386 Phase 6c) — the enum `switch (cfg.backend)` is the correct codegen on non-desktop: the android sections pull the backend from `b.dependency("labelle_<tag>")`, which resolves to the fetched package. Only a backend named ONLY by string (no matching tag, `cfg.backend` at its `.raylib` default) must still hard-error, since the switch would emit the wrong (raylib) codegen. Gate the reject on `cfg.backendName() != @TagName(cfg.backend)`. This is the second of the two flip-blockers (the first was the render.zig callback guard, #416). VALIDATED ON-DEVICE (Galaxy Tab A7 / Android 12): Flying Platform built against the out-of-tree labelle-bgfx package — generate → NDK cross-compile → APK → install → launch — runs crash-free, and behaves IDENTICALLY to the bundled bgfx (both render black under the local-assembler-vs-released-engine version skew the build warns about; that black is NOT the extraction — it reproduces with bundled bgfx too). The extraction introduces no on-device regression. Tests: a tag-matched external bgfx on android uses the enum path (no error, emits the android backend section); a string-only external backend still errors.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesExternal backend enum-path fallthrough fix
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 updates the build file generation logic in src/build_files.zig to allow external backends whose package names match their enum tags to fall through to the built-in backend switch on non-desktop targets, and adds corresponding unit tests. Feedback highlights a missed guard in the desktop link section (around line 583) that still uses the unconditional cfg.isExternal() check and should be updated to match the new logic.
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.
| // external backend reaches the splice branch. Guard so an external | ||
| // backend never falls into the built-in `switch (cfg.backend)` (which | ||
| // reads a meaningless enum tag for a backend named only by string). | ||
| } else if (cfg.isExternal() and !std.mem.eql(u8, cfg.backendName(), @tagName(cfg.backend))) { |
There was a problem hiding this comment.
While this condition correctly allows tag-matched external backends to fall through to the built-in switch when the manifest splice doesn't run (e.g., on non-desktop or when project_dir is null), a corresponding guard in the desktop link section at line 583 was missed and still uses the unconditional cfg.isExternal() check:\n\nzig\n} else if (cfg.isExternal()) {\n return error.ExternalBackendNeedsManifest;\n}\n\n\nThis will cause tag-matched external backends on desktop to fail with ExternalBackendNeedsManifest when project_dir is null (such as in tests or dry-runs), even though they should be allowed to fall through to the built-in link switch. Please update the guard at line 583 to match this one:\n\nzig\n} else if (cfg.isExternal() and !std.mem.eql(u8, cfg.backendName(), @tagName(cfg.backend))) {\n
Contributor
Author
There was a problem hiding this comment.
Fixed — the desktop link guard (line ~589) now routes through the same new externalUsesEnumPath(cfg) helper as the backend-dep guard, so the two can't drift. (It's the desktop exe path, where the helper is false, so an external backend still errors there — but they're now symmetric.)
![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 `@src/build_files.zig`:
- Around line 244-256: The fallthrough in the backend selection logic is too
broad: it now lets all tag-matched external backends reach the enum path, but
the WASM-specific codegen in the later backend sections still treats
cfg.isExternal() as a hard skip. Narrow the exception in the backend switch so
it only applies to the validated Android path, or propagate the same tag-matched
external allowance through the WASM handling in build_files.zig where the
cfg.isExternal() guards control the wasm_emsdk_* and link_*_wasm sections.
🪄 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: 10b144a4-956b-4d9a-a709-7401e12239bd
📒 Files selected for processing (2)
src/build_files.zigtest/build_zig_tests.zig
…oderabbit/gemini) Two bot findings on the fallthrough guard: - coderabbit: it was too broad — a tag-matched external on wasm would pass here but miss the wasm-specific sections. Scope the exception to the validated ANDROID path via a new externalUsesEnumPath(cfg) helper (android + name==tag). - gemini: the desktop LINK guard (line ~589) still used a bare cfg.isExternal(). Route it through the SAME helper so the two guards can't drift (it's the desktop exe path, so the helper is false there and external still errors). Tests: tag-matched external on wasm now errors (locks the android-only scope); the android tag-matched + string-only cases unchanged.
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
… (#418) Pluggable-backends Phases 5+6 (epic #386): the assembler can now FETCH, verify, and build out-of-tree backend packages, and bgfx is extracted to its own repo. - #409 6a: remote .backend_package cache-fetch (fetched like a plugin) - #410 6b: contract-verify an external backend (assertBackend/Window/Input) - #411: conform raylib + null windows to the canonical window contract - #412 Phase 5: enum-as-shorthand resolution (a built-in tag can resolve to a package) - #413 6c: bgfx extracted -> github.com/labelle-toolkit/labelle-bgfx; opt-in CI-verified - #414: select the GUI bridge by backend name, not the enum (external backends) - #415: labelle-bgfx v0.2.0 — gamepad sources extracted to their own packages - #416/#417: the two flip-blockers (callback-external guard; android enum-fallthrough) Opt-in today via .backend_package; built-in .backend = .bgfx still ships bundled (the default-flip is a follow-up gated on this release). Built-in backends are byte-identical. External bgfx validated on-device (Galaxy Tab A7): builds, runs crash-free, behaves identically to bundled.
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
…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).
apotema
added a commit
that referenced
this pull request
Jun 30, 2026
…lt (#386 Phase 6c) (#419) * feat(config): flip .backend = .bgfx to the extracted package by default (#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). * fix(cache): fetch/validate the EFFECTIVE backend package (covers the 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. * doc: update builtinProvider comment — bgfx is extracted, not 'all bundled' (gemini)
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.
The second (and final) flip-blocker — found by on-device validation
Validating Flying Platform against the out-of-tree
labelle-bgfxon a real Galaxy Tab A7 surfaced this:generate --platform=androidfailed withExternalBackendNeedsManifest.Root cause: the backend-dep section rejected every external backend that didn't reach the manifest splice (
else if (cfg.isExternal()) return ExternalBackendNeedsManifest). But the splice is desktop-only — so on Android an external backend has no splice, and the guard wrongly rejected it instead of falling through to the enum path.Fix
An external backend whose package name matches its enum tag — e.g. extracted bgfx via
.backend = .bgfx(the enum-as-shorthand resolves to a package while preserving the tag) — should use the enumswitch (cfg.backend)on non-desktop targets. That's the correct codegen: the android sections pull the backend fromb.dependency("labelle_<tag>"), which resolves to the fetched package. Only a backend named only by string (no matching tag,cfg.backendat its.raylibdefault) must still hard-error, since the switch would emit raylib codegen.Gate the reject on
cfg.backendName() != @tagName(cfg.backend). This is the second of the two flip-blockers (the first was the render.zig callback guard, #416).Validated on-device (Galaxy Tab A7, Android 12)
Flying Platform built against the out-of-tree
labelle-bgfx: generate → NDK cross-compile → APK → install → launch → runs crash-free. The externallibgame.soloads, NativeActivity + GL context come up, no crash.It behaves identically to bundled bgfx — both render black under the version skew this setup forces (local main assembler vs FP's released engine 1.65.0; the build prints
cli 1.50.0 / gfx 1.16.1 may be incompatible with core 1.21.0). That black reproduces with bundled bgfx too, so it is not the extraction — a clean render check needs a version-matched assembler release (which ships the extraction). The extraction introduces no on-device regression.Tests
ExternalBackendNeedsManifest).zig build testgreen.Net effect on the flip
With #416 + this, both flip-blockers are resolved. The default
.backend = .bgfx→ external flip is now codegen-ready on desktop and android; the remaining gate is cutting an assembler release (so the version skew goes away) + the test-harness updates for the flipped default + an on-device render confirmation against that release.Summary by CodeRabbit