Repository navigation
feat(#461): manifest-v2 PR5 — sokol android (first hook-bearing conversion) - #466
Merged
Merged
Conversation
…rsion) 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
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThis PR adds Android support for the sokol backend v2 hook flow, routes v2 manifest-driven Android codegen through the generator and splice modules, adds hook-focused tests, and validates the generated Android build script with golden and regression coverage. ChangesAndroid v2 hook and codegen
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Generator as build_files.zig
participant Splice as manifest_v2_splice.zig
participant Hook as backend.hook.zig
participant NDK as Android NDK
Generator->>Splice: renderAndroidHeaderV2 / renderAndroidDepsDeclsV2
Generator->>Splice: renderBackendDepSectionV2(ANDROID)
Splice->>Hook: resolve_target(b, ctx)
Hook-->>Splice: ResolvedTargetInfo
Generator->>Splice: renderLinkSectionV2(ANDROID)
Splice->>Hook: post_wire(b, ctx)
Hook->>NDK: selectGreatestValidNdk(...)
Hook-->>Splice: android-libc.txt, include/lib paths
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fe10ae2a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try w.print( | ||
| \\const std = @import("std"); | ||
| \\ | ||
| \\const backend_build_hook = @import("{s}"); |
There was a problem hiding this comment.
When the v2 Android path is exercised (backend_manifest_name opt-in), this generated import requires a sibling backend_build_hook.zig, but the generator only writes the generated build.zig/build.zig.zon and never copies the manifest's build_hook file to that name. As a result, the generated v2 Android project fails at import resolution before resolve_target or post_wire can run.
Useful? React with 👍 / 👎.
![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
🤖 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 `@backends/sokol/backend.hook.zig`:
- Around line 202-220: The NDK selection logic in post_wire currently picks the
lexicographically greatest directory before validating it, so a stray or partial
install can cause the wrong candidate to win. Update the directory scan that
builds latest to validate each directory’s sysroot path (using ndk_dir,
ndkHostTag(), and the sysroot join) before treating it as the best candidate,
and only keep candidates that actually exist. If a newer-looking directory is
invalid, continue scanning so an older valid NDK can still be selected instead
of failing later with “Could not find Android NDK.”
In `@src/build_files.zig`:
- Around line 306-310: The Android build header is missing the target alias when
promoted scripts are present, so `emitPromotedScriptModules` can emit `.target =
target` without a definition. Update the `build_files.zig` Android branch to
also emit `android_target_alias` when promoted scripts exist, alongside the
existing `cfg.plugins`, `cfg.ecs`, and `cfg.hasGui()` conditions. Use the
`emitPromotedScriptModules` and `android_target_alias` symbols to locate the
affected logic and ensure generated build.zig always defines `target` for
Android.
🪄 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: 56640456-8148-4387-a823-96aed5464e9c
📒 Files selected for processing (7)
backends/sokol/backend.hook.zigbuild.zigsrc/build_files.zigsrc/codegen/manifest_splice.zigsrc/codegen/manifest_v2_splice.zigtest/build_zig_tests.zigtest/goldens/sokol_android_v2.build.zig
… 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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/root.zig (1)
83-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inline import breaks the file's "imports at top" convention.
PromotedScriptis re-exported via a fresh inline@import("codegen/scan.zig")rather than reusing a top-level module alias like every other import in this file (lines 6-24). Purely cosmetic — no functional impact.♻️ Optional cleanup
+const scan = `@import`("codegen/scan.zig"); const gui_resolve = `@import`("gui_resolve.zig"); pub const app_icon = `@import`("app_icon.zig"); @@ pub const backend_build_hook_name = manifest_v2_splice.hook_import_name; -pub const PromotedScript = `@import`("codegen/scan.zig").PromotedScript; +pub const PromotedScript = scan.PromotedScript;🤖 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 83 - 88, The PromotedScript re-export in root.zig uses a fresh inline `@import`, which breaks the file’s existing “imports at top” convention. Add a top-level module alias for codegen/scan.zig alongside the other imports in this file, then re-export PromotedScript through that alias so the import pattern stays consistent with the rest of root.zig.
🤖 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/root.zig`:
- Around line 83-88: The PromotedScript re-export in root.zig uses a fresh
inline `@import`, which breaks the file’s existing “imports at top” convention.
Add a top-level module alias for codegen/scan.zig alongside the other imports in
this file, then re-export PromotedScript through that alias so the import
pattern stays consistent with the rest of root.zig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7fb5155-2260-4e75-8ad1-12d7196cad6e
📒 Files selected for processing (5)
backends/sokol/backend.hook.zigsrc/build_files.zigsrc/codegen/manifest_v2_splice.zigsrc/root.zigtest/build_zig_tests.zig
🚧 Files skipped from review as they are similar to previous changes (3)
- src/build_files.zig
- src/codegen/manifest_v2_splice.zig
- backends/sokol/backend.hook.zig
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.
PR 5 of the manifest-v2 epic (#461, #453 item 3). Converts sokol android to v2 — the first conversion exercising
resolve_target+post_wire+ the packager apk. Gate = a golden cell (residuals live in the hook, so not a pure byte-anchor per §7).Hook (
backends/sokol/backend.hook.zig)resolve_target— android ABI from-Demulator/-Dandroid_arch+ host →resolveTargetQuery(.{cpu_arch, .linux, .android}). Runs beforeb.dependency.post_wire— the §2(a) residual: NDK sysroot detection, system include paths,addLibraryPath(usr/lib/<triple>/<api>),libc.txtviasetLibCFile.ctx.android_target_sdkis REQUIRED — panics if null, noorelse 34(§4 review correction fix: Android build template — libc.txt, addLibrary, Apple Silicon emulator #6).selectAndroidArch/ndkArchTriple/requireAndroidSdk/libcTxt) unit-tested (7 tests).Golden + hook gates (§7)
test/goldens/sokol_android_v2.build.zig— byte-equality vs generated output + AST-validity (0 parse errors) + hook-boundary asserts (importsbackend_build_hook, callsresolve_target/post_wirewithandroid_target_sdk = 34, uses the genericunifyCoreDiamondloop, no inline NDK/unifyGfxSubpackageCore).build.zig— "run the hook in the gate": typechecksresolve_target/post_wireagainst realstd.Build+ runs the pure-helper tests.Intended enum-vs-v2 diffs (documented)
The v2 android build.zig deliberately differs from the enum path: inline NDK/target/libc.txt move into the hook; the unrolled override diamond +
unifyGfxSubpackageCorebecome the genericunifyCoreDiamondloop (§5); apk packaging delegates to the shared packager (byte-identical to.android_package). Hence a golden cell, not a byte anchor.Invariants
build_zig.txtzero diff;manifest_splice's only change relaxesmanifestPathEnabledfor the explicit v2 opt-in on non-desktop (a v1 manifest on non-desktop still uses the enum path).zig build testexit 0 (verified identical to main — the 12 stderr "failed command" lines are pre-existingexpectErrornegative tests, net-neutral).zig buildexit 0.Note: physically staging
backend_build_hook.zignext to the generated build.zig is deferred to when the v2 route is promoted to the productiongeneratepath (v2 is gated-dark/test-only today per §6).Ref #461, #453.
https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
Summary by CodeRabbit
targetis defined correctly when promoted scripts are enabled.