fix: Android build template — libc.txt, addLibrary, Apple Silicon emulator - #6
Conversation
…lator - Replace addSharedLibrary with addLibrary + .linkage = .dynamic (Zig 0.15.2) - Generate android-libc.txt pointing at NDK sysroot so Zig can resolve Android libc headers (Zig doesn't bundle Android libc unlike gnu/musl) - Detect host arch for --emulator: Apple Silicon uses arm64-v8a images, Intel Macs use x86_64 images
PR SummaryMedium Risk Overview Improves Adds generation of an Reviewed by Cursor Bugbot for commit ca5cba1. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the Zig build script for Android development by adding dynamic support for emulators on different host architectures (Intel Mac vs. Apple Silicon). It now automatically resolves the correct emulator architecture (x86_64 or aarch64) and adjusts NDK pathing accordingly. The changes also include setting the library linkage to dynamic and introducing a mechanism to configure Zig's libc paths to the Android NDK sysroot, which is essential for resolving standard C headers. A review comment suggests using b.fmt instead of std.mem.concat for constructing the libc.txt content, which would improve readability and align with more idiomatic Zig build script practices.
| const libc_content = std.mem.concat(b.allocator, u8, &.{ | ||
| "include_dir=", b.pathJoin(&.{ ndk_sysroot, "usr/include" }), "\n", | ||
| "sys_include_dir=", b.pathJoin(&.{ ndk_sysroot, "usr/include", ndk_arch_triple }), "\n", | ||
| "crt_dir=", b.pathJoin(&.{ ndk_sysroot, "usr/lib", ndk_arch_triple, "{{target_sdk_version}}" }), "\n", | ||
| "msvc_lib_dir=\n", | ||
| "kernel32_lib_dir=\n", | ||
| "gcc_dir=\n", | ||
| }) catch @panic("OOM"); |
There was a problem hiding this comment.
Using b.fmt is more idiomatic and readable for constructing multi-line strings in Zig build scripts compared to std.mem.concat. It also simplifies the code by removing the need for explicit error handling with catch @panic("OOM"), as b.fmt uses the build allocator which panics on failure by default.
const libc_content = b.fmt(
\\include_dir={s}
\\sys_include_dir={s}
\\crt_dir={s}
\\msvc_lib_dir=
\\kernel32_lib_dir=
\\gcc_dir=
\\
, .{
b.pathJoin(&.{ ndk_sysroot, "usr/include" }),
b.pathJoin(&.{ ndk_sysroot, "usr/include", ndk_arch_triple }),
b.pathJoin(&.{ ndk_sysroot, "usr/lib", ndk_arch_triple, "{{target_sdk_version}}" }),
});
There was a problem hiding this comment.
Pull request overview
Updates the generated Android build.zig template to stay compatible with newer Zig build APIs and to improve Android cross-compilation ergonomics (NDK libc headers + emulator ABI selection).
Changes:
- Replace
addSharedLibrarywithaddLibrary(.linkage = .dynamic)for Zig 0.15.2 compatibility. - Generate and set an
android-libc.txtso Zig can locate Android libc headers/crt files via the NDK sysroot. - Make
--emulatorpick ABI based on the host architecture (Apple Silicon → arm64-v8a, Intel → x86_64).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // On Apple Silicon the Android emulator runs ARM64 images; on Intel Macs | ||
| // it runs x86_64. Use the host arch to select the right ABI automatically. | ||
| const emulator_arch: std.Target.Cpu.Arch = switch (b.graph.host.result.cpu.arch) { | ||
| .aarch64 => .aarch64, | ||
| else => .x86_64, | ||
| }; |
There was a problem hiding this comment.
The new Apple Silicon emulator support relies on finding the NDK sysroot, but getAndroidNdkSysroot() uses ndkHostTag() which currently always returns "darwin-x86_64" for macOS. On Apple Silicon with a standard arm64 NDK install (.../prebuilt/darwin-arm64/...), this will fail to locate the sysroot, so --emulator won’t work even though emulator_arch selects .aarch64. Update ndkHostTag() to incorporate the host CPU arch (e.g., return darwin-arm64 when the build host arch is .aarch64, and keep darwin-x86_64 for Intel).
| // Zig does not bundle Android libc — provide a libc.txt that points the | ||
| // compiler at the NDK sysroot so Zig can resolve <stdio.h> and friends. | ||
| const libc_content = std.mem.concat(b.allocator, u8, &.{ | ||
| "include_dir=", b.pathJoin(&.{ ndk_sysroot, "usr/include" }), "\n", | ||
| "sys_include_dir=", b.pathJoin(&.{ ndk_sysroot, "usr/include", ndk_arch_triple }), "\n", | ||
| "crt_dir=", b.pathJoin(&.{ ndk_sysroot, "usr/lib", ndk_arch_triple, "{{target_sdk_version}}" }), "\n", | ||
| "msvc_lib_dir=\n", | ||
| "kernel32_lib_dir=\n", | ||
| "gcc_dir=\n", | ||
| }) catch @panic("OOM"); | ||
| const android_libc = b.addWriteFiles(); | ||
| lib.setLibCFile(android_libc.add("android-libc.txt", libc_content)); |
There was a problem hiding this comment.
This change adds non-trivial Android-specific build logic (switching to addLibrary(.linkage = .dynamic) and generating/setting android-libc.txt for NDK sysroot headers), but the current test suite doesn’t appear to generate build.zig for .platform = .android and assert these key strings/sections. Adding a generator test that builds an Android config and checks for addLibrary + .linkage = .dynamic and setLibCFile("android-libc.txt", ...) would help prevent regressions across Zig/NDK updates.
chatgpt-codex raised 8 findings on PR #459 not covered by the #456 round. Verified each against the real code and folded the valid ones in: - root_build_deps now carry required resolution (url+hash/path/builtin); emsdk is a pinned template section, not name-synthesizable (#2) - bgfx-android android_app extra module requires root_alias="backend_app" (#3) - hookless mobile uses an assembler-owned default resolve_target; the backend-agnostic resolver means .resolved does not force a hook (#4) - build_hook must be a dedicated backend.hook.zig, not the provider build.zig (top-level @import("sokol") re-exports don't resolve in the root package) (#5) - android_target_sdk is required for Android; post_wire panics instead of the silent orelse 34 fallback (#6) - golden gate strengthened for hook-bearing cells: snapshot hook source and/or run the hook against a fixture *std.Build (#7) - carried v1 .capabilities forward into the v2 schema so opting into v2 doesn't bypass capability negotiation (.id was already present) (#8) Finding #1 (dep-option removal) was already resolved by ea24373 (base = universal options, per-platform = appends, no subtractive form) — recorded, not re-edited. Added a "PR #459 corrections" section documenting each. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
* docs(#453): fold PR #456 review findings into manifest-v2 design Revise the build-graph manifest v2 design doc to address the substantive coderabbitai/chatgpt-codex findings that PR #456 merged without incorporating, so the #453 item-3 implementation does not inherit the design flaws. Central corrections (all verified against the real code): - Dependency options are declarative (DepOption name + closed ValueSource predicate set), NOT a runtime pre_wire hook returning []Flag — a b.dependency options literal needs comptime-known field names. pre_wire/DependencyOptions deleted. - Target selection is a pre-dependency resolve_target phase (iOS device/sim + SDK, Android ABI) resolved from -Ddevice/-Demulator/-Dandroid_arch + host, not a static .triple; iOS SDK now computed before plugin b.dependency calls. - Core-diamond walk carries a gfx_mod singleton so it preserves engine->gfx. - Header-first bounded version parse (v1 stays readable, > SUPPORTED rejected). - Preserve backend_* import aliases; per-platform loop_style/artifacts/link_libc; root_build_deps for the emsdk wasm hook; android_target_sdk into HookContext. - Hook reframed as trusted build code (not mechanically sandboxable). - Byte-identical gate -> one desktop anchor + golden snapshots; packager PR moved before Android/wasm conversions. Docs-only. Adds a "Review corrections (PR #456)" summary section. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw * docs(#453): specify dep_options merge semantics + fix code-span spacing Address CodeRabbit findings on PR #459. Major — dep_options merge is now defined precisely: a comptime, name-keyed fold of per-platform entries over the base (override on collision, append otherwise), with NO subtractive form (an empty per-platform list inherits the base unchanged). Grounded in the v1 paramValue/param_names mechanism (manifest_splice.zig): the merge is a codegen-time operation on the NAME set, and because each platform's b.dependency literal is emitted independently, a name absent from a platform's folded set is simply never written. Corrected the sokol worked example to match build_zig.txt ground truth (:94/:124/:535/:763): with_imgui is the only base option (common to all four platforms); gamepad_* is a desktop-only append; dont_link_system_libs is an ios/android append (android's was missing). This dissolves the empty-wasm-list "drop" conflict — wasm forwards only with_imgui because gamepad_* was never in the base, not by removing it. Updated the "Review corrections" section to note the clarification. Minor — fixed inline code spans that wrapped across lines (MD038): the version gate spans (`< 1 or > SUPPORTED`, `2 <= v <= SUPPORTED_MANIFEST_VERSION`, `v > SUPPORTED`) and the `switch (target.result.os.tag)` span now sit on single lines. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw * docs(#453): fold PR #459 review findings into manifest-v2 design chatgpt-codex raised 8 findings on PR #459 not covered by the #456 round. Verified each against the real code and folded the valid ones in: - root_build_deps now carry required resolution (url+hash/path/builtin); emsdk is a pinned template section, not name-synthesizable (#2) - bgfx-android android_app extra module requires root_alias="backend_app" (#3) - hookless mobile uses an assembler-owned default resolve_target; the backend-agnostic resolver means .resolved does not force a hook (#4) - build_hook must be a dedicated backend.hook.zig, not the provider build.zig (top-level @import("sokol") re-exports don't resolve in the root package) (#5) - android_target_sdk is required for Android; post_wire panics instead of the silent orelse 34 fallback (#6) - golden gate strengthened for hook-bearing cells: snapshot hook source and/or run the hook against a fixture *std.Build (#7) - carried v1 .capabilities forward into the v2 schema so opting into v2 doesn't bypass capability negotiation (.id was already present) (#8) Finding #1 (dep-option removal) was already resolved by ea24373 (base = universal options, per-platform = appends, no subtractive form) — recorded, not re-edited. Added a "PR #459 corrections" section documenting each. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
…rsion) (#466) * feat(#453): manifest-v2 PR5 — sokol android (first hook-bearing conversion) Convert sokol Android to the manifest-v2 build-graph path — the first cell that exercises the build hook (design §4). This is a GOLDEN cell, not a byte-anchor: the residual moved into the imported hook and the unrolled core-diamond overrides became the generic `unifyCoreDiamond` loop, so the generated text legitimately differs from the enum path (§7). What lands: - backend.hook.zig: real `resolve_target` (android ABI from -Demulator/ -Dandroid_arch + host) and `post_wire` (NDK sysroot detection + addSystemIncludePath/addLibraryPath + libc.txt). `android_target_sdk` is REQUIRED — the hook PANICS on null, never `orelse 34` (§4 correction #6). Pure decision helpers (arch select, NDK triple, required-SDK, libc.txt body) are unit-tested; the whole file is compiled as a test target so the residual typechecks against the real std.Build API. - manifest_v2_splice.zig: android emitters — header (imports the hook + calls resolve_target before any b.dependency), core/gfx/engine dep decls, generic b.dependency + modules + artifacts + .pic, the generic core-diamond walk CALLS (§5), and the link section (declarative linkLibrary/linkSystemLibrary/ link_libc + the post_wire hook call). renderBackendDepSectionV2 / renderLinkSectionV2 now dispatch by platform (desktop byte anchor unchanged). - build_files.zig: wire the v2 android path (header/deps/backend-dep/link/ package/footer + emit the walk def). Manifest loading hoisted above header emission so the android header can branch on v2. - manifest_splice.zig: relax `manifestPathEnabled` so the explicit v2 opt-in (`backend_manifest_name`) enables the manifest path on non-desktop targets; a v1 manifest on a non-desktop target still falls back to the enum path. - Golden cell: test/goldens/sokol_android_v2.build.zig + comparison, AST validity, and hook-boundary assertions. Intended enum-vs-v2 diffs (documented): the inline NDK detection / target resolution / libc.txt move into the imported hook; the unrolled overrideImport diamond + unifyGfxSubpackageCore become the generic `unifyCoreDiamond` loop. The APK packaging delegates to the shared packager (byte-identical to `.android_package`). Desktop byte anchor stays 0-diff; v1/enum path and the embedded template are untouched. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw * fix(#453): PR #466 review — undefined target w/ promoted scripts, NDK validation, stage v2 hook Finding 1 (CRITICAL): the android/ios `target` alias (`const target = <platform>_target;`) was emitted only under `plugins|ecs|gui`, but `emitPromotedScriptModules` unconditionally references `.target = target`. A game with promoted (FlowNodes-bearing) scripts and no plugins/ECS/GUI produced an undefined `target`. The alias guard now also fires on `promoted_scripts.len > 0`. This guard is SHARED by both the v2 and enum android routes, so the fix covers BOTH; applied the same fix to the ios guard for parity. Finding 2 (Minor): `getAndroidNdkSysroot` picked the lexicographically-greatest NDK dir and only checked its sysroot AFTER, so a stray/partial install could shadow a valid older NDK and panic. Now collects each candidate with whether its sysroot exists and picks the greatest VALID one via the new pure, unit-tested `selectGreatestValidNdk` helper. Finding 3 (P2): the generated v2 android build.zig `@import`s `backend_build_hook.zig`, but nothing staged it. Added `stageBackendBuildHook` (re-exported from the generator) which copies the manifest's `build_hook` file next to the generated build.zig under that name, mirroring the other sibling writes. Tests: hook unit test for the stray-NDK shadow case; regression tests asserting both v2 and enum android define `target` with promoted scripts + no plugins/ECS/GUI (and AST-parse); a staging test asserting the fixture hook bytes land at `backend_build_hook.zig`. Desktop byte anchor and android golden unchanged (0-diff). `zig build` + `zig build test` green. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary
addSharedLibrarywithaddLibrary(.linkage = .dynamic)—addSharedLibrarywas removed in Zig 0.15.2android-libc.txtpointing at the NDK sysroot so Zig can resolve Android libc headers (Zig doesn't bundle Android libc unlike gnu/musl)--emulatorflag now auto-detects host arch: Apple Silicon →arm64-v8a, Intel Mac →x86_64Test plan
labelle android buildcompiles withoutaddSharedLibraryerror--emulatorgenerates correct ABI target on both Apple Silicon and Intel Macs