Resolve the gate's zig from the build, not one contributor's home directory - #74
Conversation
…ectory
resolveZigBin listed /home/maci/.local/bin/zig and /home/maci/.zvm/0.16.0/zig
and nothing else, so on any other machine it returned null and runZig fell back
to a bare "zig". Every gate runs with cwd set to a staging or temp directory,
and that argv[0] is not found from there: on macOS the fmt and ast-check gates
failed at spawn, before zig ever saw the code they were meant to check.
Two of this file's own tests failed on a clean checkout for the same reason
("astCheckGate fails on a syntax error with a precise diagnostic" and "fmtGate
catches unformatted code and formatFiles fixes it"), which is how a red gate
became the normal state of the suite.
build.zig now passes b.graph.zig_exe through build_options. The interpreter that
built the binary is the right version by construction and is an absolute path,
which is the property that actually matters here. The two fixed paths stay
behind it for a binary whose build cache has since been cleared, and both are
skipped unless absolute.
zig build test: 135/135 steps, 416/418 passed, 2 skipped.
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe build now passes the active Zig executable path to gate checks. Gate checks prefer this path, ignore empty or relative paths, retain absolute fallbacks, and verify that the selected path is executable. ChangesZig executable resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
main reached the same two red tests from the other side, with skipIfNoSpawnableZig: turn "zig could not be spawned" into a skip rather than a failure. Both are kept. That helper is the backstop for a machine with no usable compiler; resolving zig_exe from the build is what stops the condition arising, so on any machine that still has the build's compiler the gates now run instead of skipping. zig build / tools / test all green: 135/135 steps, 416/418 passed, 2 skipped.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/gate/checks.zig (1)
699-710: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the build-provided candidate was selected.
The test passes when any fixed fallback is executable. It does not prove that
build_options.zig_exereachedresolveZigBin. A regression can therefore pass on a machine with/home/maci/.local/bin/zig. Assert equality withbuild_options.zig_exe; add a separate fallback test if fallback behavior also needs coverage.Suggested assertion
- try std.testing.expect(bin[0] == '/'); + try std.testing.expect(build_options.zig_exe.len > 0); + try std.testing.expectEqualStrings(build_options.zig_exe, bin);🤖 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/gate/checks.zig` around lines 699 - 710, Update the test “resolveZigBin finds the zig that built this binary” to assert that the returned path equals the build-provided build_options.zig_exe value, rather than only checking that it is absolute. Preserve the existing allocation and cleanup flow; add separate fallback coverage only if needed elsewhere.
🤖 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/gate/checks.zig`:
- Around line 690-693: Update the path filter in the gate check around
build_options.zig_exe to use std.fs.path.isAbsolute(k) instead of checking k[0]
against '/'. Preserve the empty-path skip and subsequent
std.Io.Dir.accessAbsolute validation so POSIX, drive-letter, and UNC absolute
paths are accepted.
---
Nitpick comments:
In `@src/gate/checks.zig`:
- Around line 699-710: Update the test “resolveZigBin finds the zig that built
this binary” to assert that the returned path equals the build-provided
build_options.zig_exe value, rather than only checking that it is absolute.
Preserve the existing allocation and cleanup flow; add separate fallback
coverage only if needed elsewhere.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f34b4597-5de4-437b-8e29-62ad520ce6a3
📒 Files selected for processing (2)
build.zigsrc/gate/checks.zig
| // Relative, or empty on a build that predates the option: `cwd` is not | ||
| // this process's, so it could not be resolved against anything useful. | ||
| if (k.len == 0 or k[0] != '/') continue; | ||
| std.Io.Dir.accessAbsolute(io, k, .{ .execute = true }) catch continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant source ---'
sed -n '1,40p' src/gate/checks.zig
sed -n '650,725p' src/gate/checks.zig
printf '%s\n' '--- target selection ---'
sed -n '1,70p' build.zig
printf '%s\n' '--- path handling and option definitions ---'
rg -n -C 3 'zig_exe|resolveZigBin|accessAbsolute|isAbsolute|absolute' src build.zig
printf '%s\n' '--- tracked Zig/toolchain metadata ---'
git ls-files | rg '(^|/)(build\.zig|build\.zig\.zon|.*\.zig)$' | head -80
command -v zig || true
zig version 2>/dev/null || trueRepository: maci0/clanker
Length of output: 38190
🌐 Web query:
Zig 0.16.0 std.fs.path.isAbsolute Windows drive UNC path source
💡 Result:
In Zig 0.16.0, std.fs.path.isAbsolute correctly identifies Windows UNC paths as absolute [1][2]. The standard library's path handling logic was significantly improved in recent versions to treat UNC paths (e.g., \server\share) as a distinct and fully supported absolute path type [3][4][5]. Internally, std.fs.path.isAbsolute on Windows delegates to isAbsoluteWindows, which relies on windows.getWin32PathType to categorize the path [2]. This categorization specifically classifies.unc_absolute as one of the types that returns true for being absolute [2]. Key technical details: - Mechanism: The isAbsoluteWindowsImpl function evaluates paths based on their Win32 path type [2]. The.unc_absolute variant is explicitly included in the set of absolute path types (.drive_absolute,.unc_absolute,.local_device,.root_local_device) [2]. - Evolution: Earlier versions of Zig had limited support for complex Windows path formats, which occasionally caused issues or panics when encountering UNC paths [6]. These have been resolved in modern versions (including 0.16.0) by implementing a more robust parser that correctly handles the various Windows path formats, including UNC [3][4][7]. - API usage: You can use std.fs.path.isAbsolute(path) to reliably check if a path is absolute on Windows, and it will return true for valid UNC paths [1][2].
Citations:
- 1: https://github.com/ziglang/zig/blob/master/lib/std/fs/path.zig
- 2: https://git.medv.io/zig/blob/master/lib/std/fs/path.zig.html
- 3: Teach
std.fs.pathabout the wonderful world of Windows paths ziglang/zig#25993 - 4: Windows: Support UNC, rooted, drive relative, and namespaced/device paths ziglang/zig#15768
- 5: https://git.medv.io/zig/commit/59b8bed222137061d74002a40544b5ea30eab666.html
- 6: Unreachable code reached when using UNC paths on Windows ziglang/zig#8205
- 7: https://ziglang.org/download/0.16.0/release-notes.html
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import urllib.request
url = "https://raw.githubusercontent.com/ziglang/zig/0.16.0/lib/std/fs/path.zig"
text = urllib.request.urlopen(url, timeout=20).read().decode()
needles = (
"pub fn isAbsolute(",
"fn isAbsoluteWindows",
".drive_absolute",
".unc_absolute",
)
missing = [needle for needle in needles if needle not in text]
print("zig 0.16.0 path.zig fetched:", len(text), "bytes")
print("required symbols present:", not missing, missing)
for needle in needles:
pos = text.find(needle)
if pos >= 0:
start = text.rfind("\n", 0, max(0, pos - 500))
end = text.find("\n", pos + 900)
print(f"\n--- {needle} ---")
print(text[start:end if end >= 0 else pos + 1200])
PY
printf '%s\n' '--- runZig call path ---'
sed -n '735,790p' src/gate/checks.zig
printf '%s\n' '--- exact build target block ---'
sed -n '25,48p' build.zigRepository: maci0/clanker
Length of output: 2437
Use a platform-aware absolute-path check.
When the native target is Windows, build_options.zig_exe can be a drive-letter or UNC path. The k[0] != '/' check skips it and prevents the gates from using the configured Zig executable. Replace it with std.fs.path.isAbsolute(k) for Zig 0.16.0.
🤖 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/gate/checks.zig` around lines 690 - 693, Update the path filter in the
gate check around build_options.zig_exe to use std.fs.path.isAbsolute(k) instead
of checking k[0] against '/'. Preserve the empty-path skip and subsequent
std.Io.Dir.accessAbsolute validation so POSIX, drive-letter, and UNC absolute
paths are accepted.
Source: Coding guidelines
1 participant
resolveZigBin listed /home/maci/.local/bin/zig and /home/maci/.zvm/0.16.0/zig
and nothing else, so on any other machine it returned null and runZig fell back
to a bare "zig". Every gate runs with cwd set to a staging or temp directory,
and that argv[0] is not found from there: on macOS the fmt and ast-check gates
failed at spawn, before zig ever saw the code they were meant to check.
Two of this file's own tests failed on a clean checkout for the same reason
("astCheckGate fails on a syntax error with a precise diagnostic" and "fmtGate
catches unformatted code and formatFiles fixes it"), which is how a red gate
became the normal state of the suite.
build.zig now passes b.graph.zig_exe through build_options. The interpreter that
built the binary is the right version by construction and is an absolute path,
which is the property that actually matters here. The two fixed paths stay
behind it for a binary whose build cache has since been cleared, and both are
skipped unless absolute.
zig build test: 135/135 steps, 416/418 passed, 2 skipped.
Summary by CodeRabbit
Chores
Tests