Adds a Zig stdlib compatibility layer so zpdf builds on both Zig 0.15.x and Zig 0.16.0.#8
Adds a Zig stdlib compatibility layer so zpdf builds on both Zig 0.15.x and Zig 0.16.0.#8trvon wants to merge 3 commits intoLulzx:mainfrom
Conversation
…hanges, including ArrayList writers, process entrypoints, file I/O, timers, and child process execution
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new compatibility module ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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
🧹 Nitpick comments (1)
src/bench.zig (1)
127-133: Return value fromrunIgnoredis discarded—consider checking for execution failures.The exit code returned by
compat.runIgnoredis assigned to_and discarded. Ifmutoolexits with a non-zero code (indicating a failure), the benchmark will still report timing results that may be misleading. Consider checking the exit code for validity.Proposed fix
fn benchMutool(allocator: std.mem.Allocator, pdf_path: []const u8) !f64 { const start = compat.nanoTimestamp(); - _ = try compat.runIgnored(&.{ "mutool", "draw", "-F", "txt", "-o", "/dev/null", pdf_path }, allocator); + const exit_code = try compat.runIgnored(&.{ "mutool", "draw", "-F", "txt", "-o", "/dev/null", pdf_path }, allocator); + if (exit_code != 0) return error.MutoolFailed; return `@floatFromInt`(compat.nanoTimestamp() - start); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bench.zig` around lines 127 - 133, The benchmark discards the exit code from compat.runIgnored in benchMutool so failures of the mutool process are ignored; change the call to capture the return value (e.g., exitCode := try compat.runIgnored(..., allocator)), then check if exitCode != 0 and propagate or return an error (or log/report the non-zero exit) instead of ignoring it so the benchmark doesn't report timings for failed runs; update benchMutool to handle non-zero exit codes from compat.runIgnored accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/compat.zig`:
- Around line 227-247: In runIgnored replace the direct access to term.Exited
(from the comptime `@hasDecl` branch where you call child.spawnAndWait()) with a
safe match like the other branch: inspect the returned termination value (the
variable term) using a switch on its variants, return the exit code when term is
.Exited and return a fallback (e.g., 255) for non-exit cases (e.g., .Signaled),
so accessing term.Exited cannot panic; update the branch using
child.spawnAndWait() to mirror the switch logic used in the other branch.
- Around line 168-200: In mmapFileReadOnlyCwd fix the incorrect prot argument in
the else branch: replace the struct literal used for the prot parameter with the
integer constant from std.posix.PROT (use std.posix.PROT.READ, matching the if
branch) so the call to std.posix.mmap in the else branch uses the same prot type
as the if branch; update the mmap invocation that currently passes .{ .READ =
true } to pass std.posix.PROT.READ and keep the rest of the parameters (flags,
file.handle, offset) unchanged.
---
Nitpick comments:
In `@src/bench.zig`:
- Around line 127-133: The benchmark discards the exit code from
compat.runIgnored in benchMutool so failures of the mutool process are ignored;
change the call to capture the return value (e.g., exitCode := try
compat.runIgnored(..., allocator)), then check if exitCode != 0 and propagate or
return an error (or log/report the non-zero exit) instead of ignoring it so the
benchmark doesn't report timings for failed runs; update benchMutool to handle
non-zero exit codes from compat.runIgnored accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61c541e8-6787-4d32-b14c-35f5ad7701a1
📒 Files selected for processing (11)
build.ziggen_test.zigsrc/bench.zigsrc/capi.zigsrc/compat.zigsrc/encoding.zigsrc/integration_test.zigsrc/main.zigsrc/root.zigsrc/testpdf.zigsrc/wapi.zig
| pub fn mmapFileReadOnlyCwd(allocator: std.mem.Allocator, path: []const u8) ![]align(std.heap.page_size_min) u8 { | ||
| if (comptime has_legacy_fs_file) { | ||
| const file = try std.fs.cwd().openFile(path, .{}); | ||
| defer file.close(); | ||
|
|
||
| const stat = try file.stat(); | ||
| return std.posix.mmap( | ||
| null, | ||
| stat.size, | ||
| std.posix.PROT.READ, | ||
| .{ .TYPE = .PRIVATE }, | ||
| file.handle, | ||
| 0, | ||
| ); | ||
| } else { | ||
| var threaded: std.Io.Threaded = .init(allocator, .{}); | ||
| defer threaded.deinit(); | ||
| const io = threaded.io(); | ||
|
|
||
| const file = try std.Io.Dir.cwd().openFile(io, path, .{}); | ||
| defer file.close(io); | ||
|
|
||
| const stat = try file.stat(io); | ||
| return std.posix.mmap( | ||
| null, | ||
| stat.size, | ||
| .{ .READ = true }, | ||
| .{ .TYPE = .PRIVATE }, | ||
| file.handle, | ||
| 0, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the code compiles by examining how PROT is used elsewhere in Zig stdlib or the codebase
rg -n "posix\.PROT" --type zig -C2Repository: Lulzx/zpdf
Length of output: 283
🏁 Script executed:
sed -n '190,200p' src/compat.zigRepository: Lulzx/zpdf
Length of output: 291
🏁 Script executed:
rg -n "mmap" --type zig -B2 -A6Repository: Lulzx/zpdf
Length of output: 2927
🏁 Script executed:
rg -n "PROT\s*=" --type zig -B2 -A5Repository: Lulzx/zpdf
Length of output: 36
🏁 Script executed:
rg -n "fn mmap" --type zig -B2 -A10Repository: Lulzx/zpdf
Length of output: 722
🏁 Script executed:
# Check for any type definitions or struct definitions related to PROT
rg -n "PROT|prot" --type zig | head -30Repository: Lulzx/zpdf
Length of output: 106
🌐 Web query:
Zig std.posix.mmap prot parameter documentation
💡 Result:
The prot parameter of std.posix.mmap specifies the memory access protection for the mapped region. It is a u32 (or c_uint) bitfield with standard POSIX constants available in std.posix.PROT, which are re-exported from the underlying system definitions (e.g., std.c.PROT or OS-specific like std.os.linux.PROT). Common constants: - std.posix.PROT.READ (0x1): read access - std.posix.PROT.WRITE (0x2): write access - std.posix.PROT_EXEC (0x4): execute access - std.posix.PROT_NONE (0x0): no access These can be bitwise OR'd together (e.g., std.posix.PROT.READ | std.posix.PROT.WRITE). The mmap function signature is approximately: pub fn mmap(addr: ?[]align(std.mem.page_size) u8, length: usize, prot: std.posix.PROT, flags: std.posix.MAP.Flags, fd: std.posix.fd_t, offset: u64) MmapError![]align(std.mem.page_size) u8 std.posix.mmap is a cross-platform POSIX wrapper that lowers to libc or direct syscalls, providing the standard mmap(2) semantics where prot controls page protections enforced by the OS. Note: On Linux, equivalent constants exist as std.os.linux.PROT.READ etc., but std.posix.PROT is preferred for portability. Windows does not support POSIX mmap; use OS-specific APIs.
Citations:
- 1: mmap constants in stdlib ziglang/zig#12470
- 2: https://cookbook.ziglang.cc/01-02-mmap-file/
- 3: https://github.com/ziglang/zig/blob/master/lib/std/posix.zig
- 4: mmap() wrapper in std.os.linux has bitness-related portability issues that should be fairly straightforward to fix ziglang/zig#22464
- 5: https://ziglang.org/documentation/0.11.0/std/
🌐 Web query:
Zig std.posix.PROT type definition struct flag
💡 Result:
In Zig's standard library, std.posix.PROT is an alias for std.os.system.PROT, which provides the POSIX mmap protection flags as constants. It is not a struct but a namespace containing integer constants such as PROT_NONE, PROT_READ, PROT_WRITE, PROT_EXEC. These constants are used bitwise, e.g., std.posix.PROT.READ | std.posix.PROT.WRITE. The values are platform-specific but standardized across POSIX systems: - PROT_NONE = 0 - PROT_READ = 1 - PROT_WRITE = 2 - PROT_EXEC = 4 std.posix.PROT re-exports these from the underlying system definitions (e.g., std.os.linux.PROT on Linux), ensuring portability. Examples from usage: const ptr = try std.posix.mmap(null, length, std.posix.PROT.READ | std.posix.PROT.WRITE, .{ .TYPE = .SHARED }, fd, 0);
Citations:
- 1: https://github.com/ziglang/zig/blob/master/lib/std/posix.zig
- 2: https://cookbook.ziglang.cc/01-02-mmap-file/
- 3: mmap constants in stdlib ziglang/zig#12470
- 4: https://ratfactor.com/zig/stdlib-browseable2/os.zig.html
🏁 Script executed:
# Search for how flags/PROT types are typically defined in Zig
rg -n "TYPE.*PRIVATE" --type zig -B3 -A3Repository: Lulzx/zpdf
Length of output: 641
Fix compile error: incorrect prot argument syntax in else branch of mmapFileReadOnlyCwd.
Line 177 correctly uses std.posix.PROT.READ (an integer constant), but line 194 incorrectly uses .{ .READ = true } (a struct literal). The prot parameter to std.posix.mmap expects an integer constant from the std.posix.PROT namespace, not a struct. The else branch will fail to compile. Change line 194 to std.posix.PROT.READ to match line 177.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/compat.zig` around lines 168 - 200, In mmapFileReadOnlyCwd fix the
incorrect prot argument in the else branch: replace the struct literal used for
the prot parameter with the integer constant from std.posix.PROT (use
std.posix.PROT.READ, matching the if branch) so the call to std.posix.mmap in
the else branch uses the same prot type as the if branch; update the mmap
invocation that currently passes .{ .READ = true } to pass std.posix.PROT.READ
and keep the rest of the parameters (flags, file.handle, offset) unchanged.
There was a problem hiding this comment.
Pull request overview
Adds a Zig stdlib compatibility layer so zpdf can build on both Zig 0.15.x and Zig 0.16.0 by centralizing version-specific stdlib differences in src/compat.zig.
Changes:
- Introduces
src/compat.zigwith adapters for ArrayList writing, process entrypoint args handling, file I/O helpers, timestamps, and running child processes. - Updates CLI/tools/tests/C API/WASM API to use the compat adapters instead of version-specific stdlib calls.
- Adds
compat.zigunit tests to the buildteststep.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/compat.zig | New compatibility layer (ArrayList writer, fs/process/timing wrappers) + unit tests |
| src/root.zig | Routes file open/mmap and ArrayList writer usage through compat |
| src/main.zig | Uses compat main-entry wrapper + compat stdout/stderr/file/timestamp/process helpers |
| src/bench.zig | Uses compat main-entry wrapper + compat file size/timestamp/process helpers |
| src/testpdf.zig | Uses compat ArrayList writer adapter for PDF generation |
| src/integration_test.zig | Uses compat ArrayList writer adapter for extraction tests |
| src/encoding.zig | Uses compat ArrayList writer adapter in tests; removes stray whitespace |
| src/capi.zig | Uses compat ArrayList writer adapter for C API extraction |
| src/wapi.zig | Uses compat ArrayList writer adapter for WASM API extraction |
| gen_test.zig | Uses compat allocator + compat file helpers (and initializes Io when needed) |
| build.zig | Adds compat.zig unit test artifact to the test step |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| child.stderr_behavior = .Ignore; | ||
| child.stdout_behavior = .Ignore; | ||
| const term = try child.spawnAndWait(); | ||
| return term.Exited; |
There was a problem hiding this comment.
In the legacy branch, spawnAndWait() can return termination states other than normal exit (e.g., signal). Returning term.Exited without switching on the tag can trigger a safety trap or undefined behavior. Match the non-legacy branch by switching on term and mapping non-.Exited cases to a sentinel (e.g., 255).
| return term.Exited; | |
| return switch (term) { | |
| .Exited => |code| code, | |
| else => 255, | |
| }; |
| var buffer: std.ArrayList(u8) = .empty; | ||
| doc.extractText(@intCast(page_num), buffer.writer(wasm_allocator)) catch return null; | ||
| doc.extractText(@intCast(page_num), compat.arrayListWriter(&buffer, wasm_allocator)) catch return null; | ||
|
|
There was a problem hiding this comment.
buffer is never deinitialized on the error path (the catch return null after extractText). If extractText has already appended to the list, this leaks WASM heap memory. Consider adding errdefer buffer.deinit(wasm_allocator); immediately after initializing the ArrayList (and ensuring the empty-buffer early-return path also deinitializes if capacity was allocated).
| @@ -56,7 +57,7 @@ export fn zpdf_extract_page(handle: ?*ZpdfDocument, page_num: c_int, out_len: *u | |||
| if (page_num < 0) return null; | |||
|
|
|||
| var buffer: std.ArrayList(u8) = .empty; | |||
There was a problem hiding this comment.
buffer is not deinitialized if extractText fails (catch return null). If any bytes were appended before the error, this leaks memory from c_allocator. Add an errdefer buffer.deinit(c_allocator); after initialization (and keep the current toOwnedSlice ownership transfer on success).
| var buffer: std.ArrayList(u8) = .empty; | |
| var buffer: std.ArrayList(u8) = .empty; | |
| errdefer buffer.deinit(c_allocator); |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/capi.zig`:
- Around line 60-61: The current use of errdefer for buffer.deinit(c_allocator)
won't run when doc.extractText(`@intCast`(page_num),
compat.arrayListWriter(&buffer, c_allocator)) exits via "catch return null"
because the function returns an optional, not an error union; replace errdefer
with a plain defer so buffer.deinit(c_allocator) always runs on normal returns
as well as errors, ensuring the allocation is freed even when extractText
triggers "catch return null". Ensure the defer is placed immediately after
buffer is initialized (before calling doc.extractText) so it always applies.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 438d3f5b-7bf9-48f0-98b9-e0e2f07dc444
📒 Files selected for processing (3)
src/capi.zigsrc/compat.zigsrc/wapi.zig
🚧 Files skipped from review as they are similar to previous changes (2)
- src/wapi.zig
- src/compat.zig
This PR keeps zpdf’s public API unchanged unchanged and routes version-specific stdlib differences through
src/compat.zig.Summary by CodeRabbit
New Features
Tests
Refactor