fix(windows): preserve parent env for run flags + extract astcenc via System32 bsdtar - #267
Conversation
…eenshot/--profile The env-injecting run flags strip the entire parent environment on Windows, so the child `zig build` fails with `AppDataDirUnavailable` (no LOCALAPPDATA). `buildEnvironWithExtra` switched on `@TypeOf(block)` and assumed Windows uses a `WindowsBlock`, but Zig 0.16 represents the inherited environment as a *global* (PEB-backed) block — so it hit the "nothing to snapshot" branch and produced an extras-only env, dropping PATH/LOCALAPPDATA/etc. Use `std.process.Environ.createMap`, which is the platform-correct snapshot: it reads the PEB on Windows, `environ` on POSIX, and the WASI environ API on WASI. Verified on Windows: `labelle run --screenshot` now writes a valid PNG and self-exits instead of failing the build step.
`labelle astc` extracted the astcenc release zip with a bare `tar -xf`.
That resolves to System32 bsdtar in PowerShell/cmd (fine), but when
labelle runs from a Git Bash / MSYS / Cygwin shell the environment's GNU
`tar` shadows it on PATH — and GNU tar cannot read a zip ("This does not
look like a tar archive"), so the extract fails only for those users.
Resolve bsdtar by absolute path (<SystemRoot>\System32\tar.exe, with a
bare-`tar` fallback when SystemRoot is unset) so the libarchive-backed
system tar is always used regardless of PATH shadowing. Verified a full
download -> extract -> convert from a Git Bash shell.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates Windows extraction and environment inheritance, and adds ChangesWindows Environment and Tar Fixes
Test project Y-axis settings
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 improves Windows compatibility in two areas. First, it resolves the absolute path to the system tar.exe using environment variables (SystemRoot or windir) to prevent Git Bash or MSYS GNU tar from shadowing it and failing to extract zip files. Second, it replaces custom environment mapping in runner.zig with environ.createMap to ensure the parent environment is correctly preserved on Windows. The reviewer feedback suggests improving error handling in the new tar resolution logic by propagating OutOfMemory errors immediately rather than catching them and falling back to alternative options.
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.
| // extract would fail only for those users. The absolute path always hits the | ||
| // libarchive-backed system tar. Falls back to bare `tar` if SystemRoot is unset. | ||
| const extracted = if (builtin.os.tag == .windows) blk: { | ||
| const tar_exe = windowsTarPath(allocator) catch try allocator.dupe(u8, "tar"); |
There was a problem hiding this comment.
If windowsTarPath fails due to error.OutOfMemory, it is best practice in Zig to propagate the OOM error immediately rather than catching it and attempting to fall back to "tar". Masking OutOfMemory can lead to secondary failures or unexpected behavior.
const tar_exe = windowsTarPath(allocator) catch |err| switch (err) {
error.OutOfMemory => return error.OutOfMemory,
else => try allocator.dupe(u8, "tar"),
};
| const root = env.getAlloc(allocator, "SystemRoot") catch | ||
| try env.getAlloc(allocator, "windir"); |
There was a problem hiding this comment.
Similarly, if env.getAlloc(allocator, "SystemRoot") fails with error.OutOfMemory, we should propagate the OOM error immediately instead of attempting to query "windir".
const root = env.getAlloc(allocator, "SystemRoot") catch |err| switch (err) {
error.OutOfMemory => return error.OutOfMemory,
else => try env.getAlloc(allocator, "windir"),
};
Address review feedback: the bare-`tar` and `windir` fallbacks exist only for a missing SystemRoot/windir env var, so they should not also swallow `error.OutOfMemory`. Switch on the error and re-raise OOM immediately in both `ensure`'s tar resolution and `windowsTarPath`.
|
Addressed both Gemini suggestions in a79f3b0 — |
The assembler now enforces the y-axis epic (engine#640): a project.labelle without .y_axis fails generate with MissingYAxis. This fixture predates that; declare .up to preserve its existing bottom-origin behavior (the migration default for pre-existing games). Fixes the Versions Integration Test red-bar (present on main too, not introduced by this PR).
The assembler enforces the y-axis epic at generate time; the imgui-anchor, nuklear-plugin, and gui-plugin fixtures predate it. Declare .up (preserve bottom-origin) so the Versions Integration Test's generate steps pass — same fix as plugin-manifest-test in the prior commit.
There was a problem hiding this comment.
Pull request overview
This PR fixes two Windows-specific regressions discovered while validating v1.50.0 on Windows 11 / Zig 0.16: (1) labelle run env-injecting flags now preserve the parent environment, and (2) labelle astc extraction is made robust when running under Git Bash/MSYS where GNU tar can shadow Windows’ bsdtar.
Changes:
- Update
buildEnvironWithExtrato snapshot the parent environment viastd.process.Environ.createMap, preserving PATH/LOCALAPPDATA on Windows. - Pin Windows ASTC encoder extraction to
<SystemRoot>\System32\tar.exe(bsdtar) with a fallback to baretarwhen SystemRoot/windir are unavailable.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/cli/runner.zig | Uses Environ.createMap to correctly snapshot the inherited environment across platforms (fixes Windows env stripping). |
| src/astc/astcenc_bin.zig | Uses an absolute System32 tar.exe on Windows to avoid Git Bash/MSYS GNU tar incompatibility with zip extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const tar_exe = windowsTarPath(allocator) catch |err| switch (err) { | ||
| // OOM is fatal — don't mask it behind the bare-`tar` fallback, | ||
| // which only exists for a missing SystemRoot/windir. | ||
| error.OutOfMemory => return error.OutOfMemory, | ||
| else => try allocator.dupe(u8, "tar"), | ||
| }; |
| const root = env.getAlloc(allocator, "SystemRoot") catch |err| switch (err) { | ||
| // OOM is fatal — only fall back to the `windir` alias when SystemRoot | ||
| // is genuinely absent, not when the allocation itself failed. | ||
| error.OutOfMemory => return error.OutOfMemory, | ||
| else => try env.getAlloc(allocator, "windir"), | ||
| }; |
Both fallbacks (SystemRoot→windir, and windowsTarPath→bare tar) used a catch-all `else` that masked any non-OOM error. Replace with the explicit two-case form matching envVarOwnedOptional (android_sdk.zig): fall back only on EnvironmentVariableMissing/InvalidWtf8 (the 'effectively unset' cases), and propagate everything else (OOM + any future error) instead of silently running whatever `tar` is on PATH. Addresses Copilot review on #267.
|
Addressed the two Copilot findings on the Windows tar-path fallbacks ( Both used a catch-all catch |err| switch (err) {
error.EnvironmentVariableMissing, error.InvalidWtf8 => <fallback>,
else => return err, // OOM + any future error propagate
};So the fallback fires only for the genuinely unset/invalid env cases, and OOM (or any future error in the set) propagates instead of silently running whatever |
Summary
Two Windows fixes found while validating the v1.50.0 changes on Windows 11 (Zig 0.16). Both are independent; each is its own commit.
1.
fix(run): parent environment stripped on Windows for env-injecting flagsThe new direct-run path injects env vars for
--headless,--scene,--screenshot, and--profileviabuildEnvironWithExtra, which is meant to be "parent env + extras". On Windows it instead produced an extras-only environment, so the childzig buildlostLOCALAPPDATA/PATHand failed with:Root cause: the helper switched on
@TypeOf(block)and assumed the inherited env is aWindowsBlock, but Zig 0.16 represents it as a global (PEB-backed) block — hitting the "nothing to snapshot" branch. Fixed by usingstd.process.Environ.createMap, the platform-correct snapshot (PEB on Windows,environon POSIX, WASI environ API on WASI).2.
fix(astc):labelle astcextraction fails under Git Bash/MSYSExtraction used a bare
tar -xfto read the astcenc release zip. In PowerShell/cmd this is System32 bsdtar (reads zips, fine), but when labelle runs from a Git Bash / MSYS / Cygwin shell, that shell's GNUtarshadows it onPATHand cannot read a zip (This does not look like a tar archive). Pinned the Windows extract to absolute<SystemRoot>\System32\tar.exe(with a bare-tarfallback).Testing
zig build test— 294/294 pass.labelle run --screenshot=... --after=1swrites a valid 800×600 PNG and self-exits (previously:AppDataDirUnavailable).labelle run --timeoutconfirmed still kills cleanly with no orphan.labelle astcfull download → extract → convert from a Git Bash shell produces a valid.astc(correct0x5CA1AB13magic, 8×8 blocks).Note (not addressed here)
DebugAllocator reports a benign
TimeoutStateleak (runner.zigrunZigInheritWithEnv) when a game self-exits before--timeoutfires — the detached watchdog thread still holds its refcount and the process exits before it wakes. It's a deliberate trade-off of the UAF fix in #265 and harmless; flagging only for visibility.