Repository navigation
feat(#461): manifest-v2 PR10 — bgfx all platforms (last backend converted) - #471
Conversation
Convert bgfx, the last and hardest backend, to the manifest-v2 build graph (epic #453 item 3, PR 10, design §8). bgfx is the first backend needing BOTH per-platform loop_style (desktop .loop, android .callback) AND a platform-only extra_modules with a non-default root_alias (android's android_app → backend_app), both derived from ONE manifest. - backends/bgfx_v2/backend.manifest.v2.zon: faithful v2 translation of the real labelle-bgfx v1 manifest + enum backend_bgfx/link_bgfx/backend_bgfx_android/ android_link_bgfx sections. Desktop: .loop, .native, bgfx+glfw artifacts, base gui_enabled + desktop gamepad_* dep_options. Android: .callback, .resolved, .apk, pic, link_libc, bgfx artifact, android_app extra module aliased to backend_app, NDK system libs, build_hook. - backends/bgfx_v2/backend.hook.zig: std-only android resolve_target (ABI) + post_wire (bgfx-Android NDK residual: addLibraryPath + libc.txt; no artifact addSystemIncludePath since bgfx C++ TUs are backend-wired). android_target_sdk required (panics on null). Compiled as a test target in build.zig. - manifest_v2_splice: extend the android backend-dep emitter to declare + walk PlatformEntry.extra_modules under their root_alias (the backend_app alias — the load-bearing correctness bit, design §3 review-correction #3). - Golden cells: test/goldens/bgfx_v2_{desktop,android}.build.zig + assertions (AST-valid; desktop hookless + bgfx/glfw on exe AND test_root; android hook phases + backend_app alias + generic unifyCoreDiamond + apk packaging). Per-platform loop_style is already schema-supported (PlatformEntry); the emitter extension needed was extra_modules root_alias. sokol desktop byte anchor + all prior goldens unchanged; v1/enum path unchanged (v2 is opt-in). zig build test exit 0. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
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 (8)
📝 WalkthroughWalkthroughAdds a bgfx v2 backend manifest and an Android-focused build hook module (target resolution, NDK sysroot detection, libc.txt generation), extends manifest_v2_splice codegen to wire Android extra modules and unifyCoreDiamond, adds a bgfx_hook_tests build target, and adds desktop/android golden build.zig fixtures with corresponding tests and helpers. Changesbgfx v2 manifest, hook, and codegen
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Build as std.Build
participant Hook as backend.hook.zig
participant NDK as Android NDK/SDK
Build->>Hook: resolve_target(ctx)
Hook->>Hook: selectAndroidArch(host_arch, emulator, arch_opt)
Hook-->>Build: ResolvedTargetInfo
Build->>Hook: post_wire(ctx)
Hook->>Hook: requireAndroidSdk(ctx)
Hook->>NDK: detect NDK sysroot (ANDROID_NDK_HOME/ANDROID_HOME)
Hook->>Hook: libcTxt(include_dir, sys_include_dir, crt_dir)
Hook-->>Build: attach android-libc.txt to root .so artifact
Possibly related issues
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 implements the manifest-v2 migration for the bgfx backend, introducing a dedicated build hook (backend.hook.zig) and a v2 manifest (backend.manifest.v2.zon) to handle both desktop and android platforms. It also updates the codegen splice logic to support platform-only extra modules and adds corresponding golden tests. Feedback on the changes includes correcting the usage of std.ArrayList to std.ArrayListUnmanaged in the NDK detection logic, and preferring standard logging (std.log.err) over std.debug.print for user-facing errors.
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.
| const ndk_dir = b.pathJoin(&.{ home, "ndk" }); | ||
| var dir = std.Io.Dir.cwd().openDir(io, ndk_dir, .{ .iterate = true }) catch return null; | ||
| defer dir.close(io); | ||
| var candidates: std.ArrayList(NdkCandidate) = .empty; |
There was a problem hiding this comment.
The std.ArrayList type is a managed array list that stores its allocator internally and does not support .empty initialization. Its deinit and append methods do not accept an allocator argument. Since you are initializing it with .empty and passing b.allocator to deinit and append, you should use std.ArrayListUnmanaged instead.
var candidates: std.ArrayListUnmanaged(NdkCandidate) = .empty;
References
- In Zig 0.16, the deinit API for an unmanaged ArrayList requires passing the allocator.
| const emulator_mode = b.option(bool, "emulator", "Build for Android emulator (x86_64 on Intel Mac, arm64 on Apple Silicon)") orelse false; | ||
| const android_arch_opt = b.option([]const u8, "android_arch", "Android target arch (arm64|x86_64). Overrides -Demulator when set."); | ||
| const android_arch = selectAndroidArch(b.graph.host.result.cpu.arch, emulator_mode, android_arch_opt) catch { | ||
| std.debug.print("build.zig: unknown -Dandroid_arch value (expected arm64 or x86_64)\n", .{}); |
There was a problem hiding this comment.
In CLI tools (including build scripts and hooks), standard logging facilities like std.log.err should be preferred over debug-specific print functions like std.debug.print for user-facing warnings and errors. Note that std.log functions automatically append a newline, so the trailing \n should be omitted.
std.log.err("build.zig: unknown -Dandroid_arch value (expected arm64 or x86_64)", .{});
References
- In CLI tools, use standard logging facilities (e.g., std.log.warn) or write to stderr for user-facing warnings, rather than using debug-specific print functions (e.g., std.debug.print).
|
Both gemini findings evaluated and declined (with reasons):
CodeRabbit found no issues. Merging. |
PR 10 of the manifest-v2 epic (#461, #453 item 3). Converts bgfx — the last and hardest backend — across desktop + android from a single manifest. With this, all 6 backends are v2-modeled.
bgfx v2 fixture (
backends/bgfx_v2/backend.manifest.v2.zon)Faithful translation of the real labelle-bgfx v1 + enum sections.
id="labelle.bgfx", capabilities{android, surface_loss, compressed_textures, screenshots, gamepad_polling, raw_gui_adapter}.loop_style=.loop, artifactsbgfx+glfw, basedep_options gui_enabled+ desktop-onlygamepad_enabled/gamepad_hidapi, hookless.loop_style=.callback,target=.resolved,package=.apk,pic,link_libc, artifactbgfx, theandroid_appextra module withroot_alias="backend_app",system_libs.android={android,log,EGL,GLESv3,m,dl,mediandk,aaudio},build_hook.bgfx hook (
backends/bgfx_v2/backend.hook.zig)Std-only. Android
resolve_target(ABI) +post_wire(bgfx-Android NDK residual:addLibraryPath+ libc.txt). Unlike sokol, noaddSystemIncludePathon the artifact (bgfx/bx/bimg C++ are NDK-wired by the backend's own build.zig).ctx.android_target_sdkrequired (noorelse 34). Compiled as a test target.Emitter extension (the only new codegen)
renderAndroidBackendDepV2now declaresextra_modulesunder theirroot_alias(const backend_app = backend_dep.module("android_app");) + walks them withunifyCoreDiamond. Per-platformloop_stylewas already schema-supported (raylib desktop.loop/wasm.callbackproved it) — bgfx just adds android.callback.Goldens + the load-bearing alias
bgfx_v2_desktop.build.zig— hookless, generic walk,bgfx+glfwon both exe and test_root.bgfx_v2_android.build.zig— imports the hook,resolve_target+post_wire,const backend_app = backend_dep.module("android_app")published as.{ .name = "backend_app", .module = backend_app }(asserted;backend_android_appnever appears), generic diamond overbackend_app+backend_input, declarative NDK syslibs, apk via the packager.android_app→backend_appalias (§3 review finding — wrong alias breaks the NativeActivity shell import).Core-diamond coverage
Desktop
sdl_gamepad+ androidandroid_gamepadare transitive sub-packages ofbackend_input; the genericunifyCoreDiamondwalk recurses into them (no per-backend override site). Android also coversbackend_input's directlabelle-core(#310) + thebackend_appshell.sokol desktop byte anchor + ALL prior goldens (sokol android/ios/wasm, null, wgpu, raylib, sdl) unchanged; v1/enum unchanged (v2 opt-in).
zig build test+zig buildexit 0.Ref #461, #453.
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit
New Features
Bug Fixes
Tests