config: open .backend to a named external package (#386 Phase 5 — open the resolver) - #398
Merged
Merged
Conversation
A backend can now be declared by string + package via the new
ProjectConfig.backend_package (?PluginDep) field, in addition to the
closed Backend enum. External backends are named + located through the
plugin-resolution infra (backend_registry.resolveBackendPackage →
cache.resolvePlugin) and generate exclusively via their backend.manifest.zon.
- config: add backend_package, isExternal(), extend backendName() to
return the package name for external backends. .backend = .<enum> is
unchanged (byte-identical built-in fast path).
- backend_registry: add resolveBackendPackage(cfg) — built-in →
resolveBundledPackage(backends/{name}); external → resolvePlugin().
Route manifest_splice.backendPackageDir, deps_linker backend dep, and
both root.loadBackendTemplate dir resolutions + the build.zig.zon dep
through it.
- gate behavioral switch (cfg.backend) sites on !isExternal()
(sdl/android gamepad sub-packages via stagesSdlGamepad/stagesAndroidGamepad;
per-backend build/link/wasm fragments) — external backends are
self-contained.
- external backends REQUIRE a manifest: requireManifestIfExternal raises
error.ExternalBackendNeedsManifest instead of falling to the .raylib
enum path.
- tests: external-stub routing, name-keyed lookup, needs-manifest error,
gamepad-skip gating. zig build test green (682/686, 4 pre-existing skips).
📝 WalkthroughWalkthroughAdds external backend package support by introducing ChangesExternal Backend Open-Config Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 support for external graphics backends (open-config) in the assembler, allowing backends to be resolved via the plugin infrastructure rather than being restricted to the built-in closed enum. It adds a backend_package field to ProjectConfig, routes backend path resolution through a new resolveBackendPackage function, and enforces that external backends must provide a backend.manifest.zon file. Built-in-specific logic, such as gamepad sub-package staging and WASM emsdk helpers, is gated off for external backends. Feedback on the changes highlights a potential memory leak in createDepsLinks within src/deps_linker.zig if an error occurs after dependencies have been appended, suggesting an errdefer block to clean up allocated strings.
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.
| // Location seam: built-in → bundled `backends/{name}` slot; external | ||
| // → the plugin checkout (`resolvePlugin`). The zon/link names follow | ||
| // the same `labelle_{name}` / `labelle-{name}` convention either way. | ||
| const backend_path = try backend_registry.resolveBackendPackage(allocator, cfg, project_dir); |
There was a problem hiding this comment.
There is a potential memory leak in createDepsLinks if any error occurs after some dependencies have already been successfully appended to deps (for example, if a subsequent dependency resolution fails, or if directory creation/hardlinking fails).
Since deps is a std.ArrayListUnmanaged (or similar) and its entries contain allocated strings (zon_name, link_name, abs_path), returning an error from this function prevents the caller from receiving the list and freeing those entries via freeDepEntries.
To prevent memory leaks on error paths, consider adding an errdefer block at the beginning of createDepsLinks to clean up the already appended entries and deinit the list:
errdefer {
for (deps.items) |dep| {
allocator.free(dep.zon_name);
allocator.free(dep.link_name);
allocator.free(dep.abs_path);
}
deps.deinit(allocator);
}createDepsLinks builds up and returns it via toOwnedSlice; on any error return the caller never gets the list, so the appended entries' strings leaked. Added a function-level errdefer that frees the appended entries + deinits the list backing — fires only on error (success path's toOwnedSlice leaves deps empty, so it's a no-op). The PR's new external error paths (ExternalBackendNeeds- Manifest, resolvePlugin failure) made this more reachable. Complements the per-entry errdefers in the backend-dep block (no double-free). (Gemini)
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/root.zig (1)
948-957: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
backend_packagewhen forcing the tests target to.null.Line 949 no longer fully switches the tests target to the null backend:
isExternal()remains true if the original config hadbackend_package, so.labelle/testscan still require/resolve the external backend instead of using the backend-agnostic null path.Suggested fix
var cfg = cfg_in; cfg.backend = .null; + cfg.backend_package = null; // Force the host platform too. For wasm/ios/android projects, leaving🤖 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/root.zig` around lines 948 - 957, The tests-target override in generate should fully switch to the null backend, not just set cfg.backend on the copied config. Update the logic around generate and the tests-target setup so backend_package is cleared alongside cfg.backend = .null, ensuring isExternal() cannot stay true and .labelle/tests uses the backend-agnostic null path instead of resolving an external backend.
🤖 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/manifest_splice.zig`:
- Around line 112-123: The external-backend branch in manifest_splice.zig is
collapsing all manifestExists failures into ExternalBackendNeedsManifest, which
hides real resolution/allocation/access errors for invalid local paths or
plugin/cache lookups. Update the manifest probing flow around manifestExists and
the backend manifest check so external backends only return
ExternalBackendNeedsManifest when the manifest is truly absent, while preserving
and propagating any underlying error from resolution or allocation failures in
the manifestExists path.
In `@src/deps_linker.zig`:
- Around line 94-99: Local external backends staged via
backend_registry.resolveBackendPackage are not included in the later ZON
path-rewrite pass, so any relative .path deps can still point at the original
checkout after staging. Update the rewrite logic in deps_linker.zig to treat
these staged backend deps the same way as cfg.plugins and GUI deps, using the
backend_info/backend_path entry added to deps so their relative paths are
rewritten against the staged .labelle/deps/labelle-<backend> location.
---
Outside diff comments:
In `@src/root.zig`:
- Around line 948-957: The tests-target override in generate should fully switch
to the null backend, not just set cfg.backend on the copied config. Update the
logic around generate and the tests-target setup so backend_package is cleared
alongside cfg.backend = .null, ensuring isExternal() cannot stay true and
.labelle/tests uses the backend-agnostic null path instead of resolving an
external backend.
🪄 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: b7efa552-83ce-40cf-a286-91f05d850264
📒 Files selected for processing (6)
src/backend_registry.zigsrc/build_files.zigsrc/codegen/manifest_splice.zigsrc/config.zigsrc/deps_linker.zigsrc/root.zig
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/deps_linker.zig`:
- Around line 69-72: The errdefer cleanup in deps_linker.zig is double-freeing
the ArrayList backing storage because freeDepEntries() already frees deps.items
before deps.deinit(allocator) runs. Update the cleanup around freeDepEntries and
deps.deinit so the errdefer only frees the individual entry fields directly,
then deinitializes the deps ArrayList without freeing the slice twice.
🪄 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: b12707a3-ed15-47ff-92b9-5bf3b6471285
📒 Files selected for processing (1)
src/deps_linker.zig
Comment on lines
+69
to
+72
| errdefer { | ||
| freeDepEntries(allocator, deps.items); | ||
| deps.deinit(allocator); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the cleanup ownership pattern in this file.
sed -n '63,72p;316,324p' src/deps_linker.zig
rg -n 'freeDepEntries\(allocator, deps\.items\)|deps\.deinit\(allocator\)' src/deps_linker.zigRepository: labelle-toolkit/labelle-assembler
Length of output: 1130
🏁 Script executed:
#!/bin/bash
# Inspect the surrounding function and ArrayList ownership transitions.
sed -n '1,120p' src/deps_linker.zig
sed -n '300,340p' src/deps_linker.zigRepository: labelle-toolkit/labelle-assembler
Length of output: 8289
Avoid freeing deps.items here. freeDepEntries() already frees the slice, so calling it on the ArrayList backing store and then deps.deinit() double-frees the same allocation. Free the entry fields directly in this errdefer, then deinit the list.
🤖 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/deps_linker.zig` around lines 69 - 72, The errdefer cleanup in
deps_linker.zig is double-freeing the ArrayList backing storage because
freeDepEntries() already frees deps.items before deps.deinit(allocator) runs.
Update the cleanup around freeDepEntries and deps.deinit so the errdefer only
frees the individual entry fields directly, then deinitializes the deps
ArrayList without freeing the slice twice.
…th deps Two external-backend correctness gaps from review (CodeRabbit): - requireManifestIfExternal probed via manifestExists, which swallows ALL errors for built-in fallback probing -> a broken local: path / resolve failure / OOM became a misleading ExternalBackendNeedsManifest. Now resolves + accesses the manifest path directly: only FileNotFound -> ExternalBackendNeedsManifest; any other error propagates as the real config failure. - A local external backend is staged into .labelle/deps/labelle-<name> like a plugin, but the .path rewrite loop only processed cfg.plugins/GUI -> its own relative .path deps stayed anchored to the original checkout. Added the same re-anchoring (worktree vs main-checkout) for a local backend_package. (The Gemini re-flag of the createDepsLinks leak is the same one fixed by the prior commit's function-level errdefer.)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/deps_linker.zig (1)
303-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
rewriteLocalDepand the backend resolver here.This block duplicates
rewriteLocalDep’s path anchoring flow and callscache.resolvePlugindirectly even though backend package lookup sites are meant to route throughbackend_registry.resolveBackendPackage. A smaller version keeps the resolver seam centralized and avoids drift.♻️ Proposed refactor
if (cfg.backend_package) |bp| blk: { if (!bp.isLocal()) break :blk; const link_name = try std.fmt.allocPrint(allocator, "labelle-{s}", .{bp.name}); defer allocator.free(link_name); - const dest = try std.fs.path.join(allocator, &.{ deps_dir, link_name }); - defer allocator.free(dest); - - const backend_path = try cache.resolvePlugin(allocator, bp, project_dir); + const backend_path = try backend_registry.resolveBackendPackage(allocator, cfg, project_dir); defer allocator.free(backend_path); - const abs_src = cwd.realPathFileAlloc(io, backend_path, allocator) catch break :blk; - defer allocator.free(abs_src); - - const abs_dest = cwd.realPathFileAlloc(io, dest, allocator) catch break :blk; - defer allocator.free(abs_dest); - - const resolution_src = if (try firstPathDepResolvesInWorktree(allocator, abs_src)) - try allocator.dupe(u8, abs_src) - else - try cache.toMainCheckoutPath(allocator, abs_src, project_dir); - defer allocator.free(resolution_src); - - try rewriteZonPaths(allocator, resolution_src, abs_dest); + try rewriteLocalDep(allocator, cwd, backend_path, deps_dir, link_name, project_dir); }🤖 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/deps_linker.zig` around lines 303 - 335, The local backend anchoring block duplicates the existing `rewriteLocalDep` flow and bypasses the centralized backend lookup seam. Refactor this branch to reuse `rewriteLocalDep` for the path re-anchoring logic, and replace the direct `cache.resolvePlugin` call with `backend_registry.resolveBackendPackage` so backend resolution stays centralized. Keep the same `cfg.backend_package`, `bp.isLocal()`, `cache`, and `rewriteZonPaths` behavior, but route through the shared helper/resolver instead of inlining the steps here.
🤖 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/deps_linker.zig`:
- Around line 303-335: The local backend anchoring block duplicates the existing
`rewriteLocalDep` flow and bypasses the centralized backend lookup seam.
Refactor this branch to reuse `rewriteLocalDep` for the path re-anchoring logic,
and replace the direct `cache.resolvePlugin` call with
`backend_registry.resolveBackendPackage` so backend resolution stays
centralized. Keep the same `cfg.backend_package`, `bp.isLocal()`, `cache`, and
`rewriteZonPaths` behavior, but route through the shared helper/resolver instead
of inlining the steps here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bd65bdd-2add-4bc6-b1fa-9a6296612f26
📒 Files selected for processing (2)
src/codegen/manifest_splice.zigsrc/deps_linker.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/codegen/manifest_splice.zig
This was referenced Jun 28, 2026
apotema
added a commit
that referenced
this pull request
Jun 29, 2026
* feat(cache): remote backend_package cache-fetch (#386 Phase 6a) A non-local `.backend_package` (a github-style `.repo`) is now fetched into the assembler's package cache the same way a plugin / regular dep is, then resolved + built. Local (`local:`/`@libs`) external backends already worked; this closes the remote gap so .backend_package = .{ .name = "foo", .repo = "github.com/org/foo-backend", .version = "1.0.0" } fetches + builds an out-of-tree backend with no per-backend assembler changes. Because `backend_package` is a `PluginDep`, the resolver (#398) and the deps-linker staging already treat a fetched backend as a self-contained cache dir — the only gaps were the cache layer's validate/fetch entry points: - cache/resolve.zig `validateCache`: report a non-local backend_package as `backend <name> <version>` when absent (via `isPluginCached`). Local backends short-circuit to cached; built-ins have no backend_package, so the byte-identical built-in path is untouched. - cache_cmd.zig `ensureCache` + `fetchBackendWithFallback`: fetch a missing remote backend via the existing `fetchPlugin` clone (monorepo-sibling local-copy fallback for backend authors). An unreachable/missing `.repo` gives a clear, backend-named error. Also fixes a codegen gate the headless-fixture e2e surfaced: an external backend leaves `cfg.backend` at its `.raylib` enum DEFAULT, so the `is_raylib_desktop` preview-readback gate misfired and spliced raylib's PBO async-readback (`window.preview_pbo.*`) into a game whose backend window module has no such surface. Gated on `!cfg.isExternal()` — built-ins unchanged. Tests: validateCache local-vs-remote backend reporting; a preview-readback regression proving an external backend takes the no-readback path while a real raylib build still emits it. Verified end-to-end against a real out-of-tree fixture (github.com/labelle-toolkit/labelle-nullfixture-backend@0.1.0): clean LABELLE_HOME → `install` fetches it into the cache → `generate` resolves it → `zig build` → runs to a bounded frame count and exits cleanly. Generated main.zig/build.zig are equivalent to the built-in `null` enum path modulo the backend dep name. * review: harden external-backend paths (PR #409 bot findings) - render.zig: fail fast on a CALLBACK-style external backend instead of silently mis-wiring it. `cfg.backend` sits at its `.raylib` default for any external backend, so a callback external (use_callback_lifecycle=true) fell through the `cfg.backend == .sokol`/bgfx-android checks into the raylib-wasm callback branch and inherited raylib preview wiring. Only loop-style externals are wired today (#386); reject callback ones with a clear, project-level error (`error.ExternalCallbackBackendUnsupported`). Log gated on !is_test (matches env.zig — the test runner fails on any logged error). (coderabbitai) - cache_cmd.zig: guard a non-local backend with an empty `.repo`/`.version` before fetch — otherwise resolvePlugin probes a bogus `plugins///` slot and the clone degrades to `git clone --branch "" https://.git`. Clear `error.InvalidBackendPackage` instead. (Copilot) - resolve.zig test: make the remote-missing validateCache test hermetic — point LABELLE_HOME at a guaranteed-absent dir (static PosixBlock; skipped on Windows) so the cache probe can't be swayed by the caller's real ~/.labelle/packages. Now asserts the backend is the SOLE missing entry. (coderabbitai) - test: regression for the callback-external rejection (platform=.wasm forces the callback lifecycle).
This was referenced Jun 29, 2026
Merged
Merged
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 headline Phase-5 step: a backend can now be named by string + package, not just the closed
Backendenum. After this, a manifest-shipping external backend can be named, located, and drive codegen — the enum becomes a fast-path shorthand for the built-ins.Additive, non-breaking
.backend = .bgfx(nobackend_package) is the built-in path, byte-identical to before. The new escape hatch:Changes
config:backend_package: ?PluginDep = null(reuses the plugin{name, repo, version}+local:/@libs/github resolution — no new repo code) +isExternal();backendName()returns the package name for external, the enum tag otherwise.backend_registry.resolveBackendPackage: external →cache.resolvePlugin; built-in →resolveBundledPackage(registry.subpath)(identical to before). Routed every backend-package location site (manifest_splice, deps_linker, loadBackendTemplate) through it.switch(cfg.backend)sites gated on!isExternal()— external backends are self-contained (their ownbuild.zig.zondeclares deps), so they skip built-in-specific staging (sdl_gamepad/android_gamepad, per-backend fragments). Factored into unit-tested predicatesstagesSdlGamepad/stagesAndroidGamepad.requireManifestIfExternal→error.ExternalBackendNeedsManifestif absent, so external never silently falls to the.raylibenum path.Verified
build_zig_tests/build_zig_zon_tests/main_zig_tests) assert exactbuild.zig/build.zig.zon/main.zigfor raylib-desktop + bgfx-desktop — all pass unchanged.resolveBackendPackageroutes alocal:external offbackends/,lookupresolves the name with no enum tag, external-without-manifest errors, gamepad switches skipped for external.zig build test: 682/686 (4 pre-existing skips), +6 new.Next (Phase 6, honest verdict)
This opens the name/location/codegen-selection layer. Two things still block an actual third-party repo producing a running game: (a) cache population —
ensureCache/validateCachedon't yet fetch abackend_packagelike a plugin (onlylocal:/@libsresolve to a real dir today); (b) contract verification — the external backend's modules must satisfy the engine's render/window/input contracts the built-ins assert viacore.assertBackend/assertWindow/assertInput.Summary by CodeRabbit
.zonpath entries.backend.manifest.zonwhen needed, preventing incomplete builds.