Repository navigation
chore: v0.34.1 hotfix — sokol screenshot bmp.zig + gl extern leak (#222) - #223
Conversation
… 1+3) Two link-breaking regressions slipped through 0.34.0's CI because none of the existing build matrix exercises a fully-linked end-to-end consumer on macOS native (Metal-only) hardware. Both are fixed here; the third bug in #222 (sokol-zig fork export of _sapp_metal_get_current_drawable) is being handled separately in the fork and will land in a follow-up that flips the new `fork_exports_drawable` comptime gate in screenshot/metal.zig. Issue 1 — bmp.zig used std.fs.cwd().createFile, which Zig 0.16 removed. Replace with libc fopen/fwrite/fclose via a shared `writeBytesViaLibc` helper, matching the same pattern PR #218 already used in gfx/texture.zig and audio/legacy.zig for the legacy path-based loaders. The window module gains `link_libc = true` so the libc surface is on the link line — libc was already pulled in by the gfx/audio modules for stb_image / stb_vorbis, so this adds no new system dep. Issue 3 — screenshot/gl.zig declared `extern fn glReadPixels` (et al.) at module scope. The comptime gate in window.readbackGL prevents the *call* from running on Darwin, but the extern declarations themselves leak into the linker's symbol table as unresolved references because the module is still part of the analysis graph. On macOS native, the OpenGL framework isn't linked, so those references fail. Move the extern decls inside the function body (so they only elaborate when `readback` is compiled) and add an early-return on Darwin so the body is dead-stripped before the extern is reached. Issue 2 (mitigation only) — screenshot/metal.zig had the same module-scope extern leak shape for `sapp_metal_get_current_drawable`. The fork patch that exports the symbol isn't in this PR, but we gate the extern declaration the same way (inside the function, behind a `fork_exports_drawable: bool = false` comptime flag) and add a clear stub log message so the binary links cleanly on macOS Metal until the fork patch lands. Follow-up PR flips the flag to true. Verification: - `zig build test` from assembler root: green. - `zig build` from backends/sokol/example: green. - flying-platform-labelle `labelle run --timeout=3s` against a locally-installed 0.34.1 binary on macOS aarch64 (Metal): builds + links cleanly, launches, runs the loading→playing transition, hits the 3s timeout. The screenshot path is still broken on Metal (waiting on Issue 2) but the build itself works, which is what FP needed.
PR SummaryMedium Risk Overview The labelle-toolkit/sokol-zig fork is bumped to BMP output no longer uses
Reviewed by Cursor Bugbot for commit bda9a7b. Bugbot is set up for automated code reviews on this repo. Configure here. |
![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 introduces changes to handle screenshot writing and readback mechanisms under Zig 0.16. It links libc to write BMP files via libc file operations, and scopes OpenGL and Metal extern declarations inside function bodies to prevent undefined-symbol errors at link time on platforms where those backends are unavailable. The review feedback suggests passing the existing allocator to the libc-based file writer instead of hardcoding std.heap.page_allocator, and recommends using extern "c" fn instead of extern fn when declaring external C functions.
| fn writeBytesViaLibc(path: []const u8, data: []const u8) !void { | ||
| const path_z = try std.heap.page_allocator.dupeZ(u8, path); | ||
| defer std.heap.page_allocator.free(path_z); | ||
| const fp = std.c.fopen(path_z.ptr, "wb") orelse return error.FileWriteFailed; | ||
| defer _ = std.c.fclose(fp); | ||
| if (std.c.fwrite(data.ptr, 1, data.len, fp) != data.len) return error.FileWriteFailed; | ||
| } |
There was a problem hiding this comment.
Instead of hardcoding std.heap.page_allocator, pass the allocator parameter that is already provided to the calling functions (writeBmp and writeBmpFromBgra). This allows the caller to control the allocation strategy (e.g., using an arena or a general-purpose allocator).
fn writeBytesViaLibc(allocator: std.mem.Allocator, path: []const u8, data: []const u8) !void {
const path_z = try allocator.dupeZ(u8, path);
defer allocator.free(path_z);
const fp = std.c.fopen(path_z.ptr, "wb") orelse return error.FileWriteFailed;
defer _ = std.c.fclose(fp);
if (std.c.fwrite(data.ptr, 1, data.len, fp) != data.len) return error.FileWriteFailed;
}
| const file = try std.fs.cwd().createFile(path, .{}); | ||
| defer file.close(); | ||
| try file.writeAll(data); | ||
| try writeBytesViaLibc(path, data); |
There was a problem hiding this comment.
Pass the allocator to writeBytesViaLibc.
try writeBytesViaLibc(allocator, path, data);
| const file = try std.fs.cwd().createFile(path, .{}); | ||
| defer file.close(); | ||
| try file.writeAll(data); | ||
| try writeBytesViaLibc(path, data); |
There was a problem hiding this comment.
Pass the allocator to writeBytesViaLibc.
try writeBytesViaLibc(allocator, path, data);
| const gl = struct { | ||
| extern fn glReadPixels(x: i32, y: i32, w: i32, h: i32, format: u32, type_: u32, data: ?*anyopaque) void; | ||
| extern fn glPixelStorei(pname: u32, param: i32) void; | ||
| extern fn glGetError() u32; | ||
| }; |
There was a problem hiding this comment.
When declaring external C functions in Zig (with link_libc = true), use extern "c" fn instead of extern fn without a library name. The "c" is a logical reference to libc, not a literal filename, and is correctly resolved by the compiler across all platforms, including Windows MSVC.
const gl = struct {
extern "c" fn glReadPixels(x: i32, y: i32, w: i32, h: i32, format: u32, type_: u32, data: ?*anyopaque) void;
extern "c" fn glPixelStorei(pname: u32, param: i32) void;
extern "c" fn glGetError() u32;
};
References
- When declaring external C functions in Zig (with
link_libc = true), useextern "c" fninstead ofextern fnwithout a library name. The"c"is a logical reference to libc, not a literal filename, and is correctly resolved by the compiler across all platforms, including Windows MSVC.
| const sapp_metal_get_current_drawable = (struct { | ||
| extern fn sapp_metal_get_current_drawable() ?*const anyopaque; | ||
| }).sapp_metal_get_current_drawable; |
There was a problem hiding this comment.
When declaring external C functions in Zig (with link_libc = true), use extern "c" fn instead of extern fn without a library name. The "c" is a logical reference to libc, not a literal filename, and is correctly resolved by the compiler across all platforms, including Windows MSVC.
const sapp_metal_get_current_drawable = (struct {
extern "c" fn sapp_metal_get_current_drawable() ?*const anyopaque;
}).sapp_metal_get_current_drawable;
References
- When declaring external C functions in Zig (with
link_libc = true), useextern "c" fninstead ofextern fnwithout a library name. The"c"is a logical reference to libc, not a literal filename, and is correctly resolved by the compiler across all platforms, including Windows MSVC.
…ack, address review Expands the v0.34.1 hotfix to make sokol screenshot actually work on macOS Metal end-to-end, not just link cleanly. Three layers stacked on top of the original e9744d3 commit: 1. sokol-zig pin bump (closes #222 issue 2) `backends/sokol/build.zig.zon`: bump the labelle-toolkit/sokol-zig fork pin from 6b8609d → 887b30f. The new commit is the fork's `feat/with-sokol-imgui-no-app` branch HEAD after labelle-toolkit/ sokol-zig#1 merged, which restored the native accessor exports including `_sapp_metal_get_current_drawable` — the symbol the Metal readback path needs. New hash: sokol-0.1.0-pb1HK-OCNwDtCJMfpeiJuF5TsFpITiO13canPXZMSksO `gui/sokol-imgui/build.zig.zon` and `gui/simple-sokol/build.zig.zon` bump in lockstep — the pin comment explicitly requires all three to match (assembler#207) or Zig caches two `sokol_clib` artifacts and two `_sg` states. 2. Flip the Metal readback gate to live `backends/sokol/src/screenshot/metal.zig`: with the symbol now exported, flip `fork_exports_drawable: bool = false → true`. The stub branch is retained for compile-time rollback safety; the live branch's inline `extern fn sapp_metal_get_current_drawable` declaration now resolves cleanly at link time. 3. Address gemini medium-priority review findings on PR #223 - `bmp.zig` ×3: thread `allocator: std.mem.Allocator` through `writeBytesViaLibc` instead of hardcoding `std.heap.page_allocator`; forward the existing allocator from `writeBmp` and `writeBmpFromBgra` call sites. - `gl.zig` ×1: change the inner `extern fn` block (glReadPixels, glPixelStorei, glGetError) to `extern "c" fn`. This is the canonical libc-linkage form — see std.c.fopen and the 630+ other usages in stdlib. Per PR #218's verification, this is the correct direction (gemini sometimes inverts; not this time). - `metal.zig` ×1: same `extern fn` → `extern "c" fn` change on `sapp_metal_get_current_drawable` for symmetry with gl.zig and the stdlib pattern. Verification: - `zig build test` from assembler root: green. - `zig build` from backends/sokol/example: green. - Locally-built 0.34.1 assembler binary + staged backend in the labelle cache: assembler templates regenerate the project build cleanly. The end-to-end `labelle run --screenshot` smoke is blocked on a parallel labelle-imgui pin bump (see follow-up note below) — when only the assembler-side pin moves, the labelle-imgui bridge still pulls in sokol@6b8609d via its own `bridges/sokol/build.zig.zon`, producing two `sokol_clib` artifacts in the final binary and a `cimgui.h file not found` error at the sokol_imgui.c compile. Non-imgui sokol consumers (the example) link and run. Follow-up: labelle-imgui needs a parallel pin bump to 887b30f in both `build.zig.zon` (top-level) and `bridges/sokol/build.zig.zon` before flying-platform-labelle (or any imgui-using sokol consumer) can take 0.34.1 end-to-end.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5033c42. Configure here.
…ost) cursor[bot] caught this on PR #223: the Darwin early-return in backends/sokol/src/screenshot/gl.zig was gated on builtin.os.tag — which is the *host* OS during cross-compilation. Building the window module for aarch64-macos / x86_64-macos from a non-Apple host would skip the bail-out and reintroduce the undefined-symbol link failures this change was meant to fix. backends/sokol/src/screenshot/metal.zig:105 already used builtin.target.os.tag correctly; gl.zig is now consistent.
1 participant
Summary
v0.34.1 now fully closes #222 — all three issues, not just 1 + 3 — and ships the complete sokol screenshot stack for macOS Metal. Stacked on top of the original hotfix (e9744d3):
1. sokol-zig pin bump — closes #222 issue 2
backends/sokol/build.zig.zon: bump from6b8609d→887b30f(labelle-toolkit/sokol-zigfeat/with-sokol-imgui-no-appbranch HEAD after labelle-toolkit/sokol-zig#1 merged). That fork PR restored the native accessor exports including_sapp_metal_get_current_drawable— the symbol the Metal readback needs.sokol-0.1.0-pb1HK-OCNwDtCJMfpeiJuF5TsFpITiO13canPXZMSksObackends/sokol/,gui/sokol-imgui/,gui/simple-sokol/(the pin comment requires they match exactly — see assembler#207).2. Metal readback gate → live
backends/sokol/src/screenshot/metal.zig: flipfork_exports_drawablefromfalsetotrue. With the symbol exported by the bumped fork, the live readback branch now links cleanly. Thefalsebranch is kept as compile-time rollback safety + documentation of the link edge.The inline
extern fn sapp_metal_get_current_drawableis also retagged toextern "c" fn(see review #3 below).3. Address gemini-code-assist medium findings (5/5)
bmp.zigwriteBytesViaLibchardcodesstd.heap.page_allocatorallocator: std.mem.Allocatorparam, forward fromwriteBmpbmp.zigwriteBmpcall site doesn't forwardallocatorallocatorthroughbmp.zigwriteBmpFromBgracall site sameallocatorthroughgl.zigextern fn(glReadPixels/glPixelStorei/glGetError)extern "c" fn(stdlib pattern; seestd.c.fopen)metal.zigextern fn sapp_metal_get_current_drawableextern "c" fnPer PR #218's verification cycle, gemini is correct on the
extern "c"direction this time (feedback_gemini_inverted_correctionsadvises checking; verified by following the stdlib pattern).Issue 1 (bmp.zig) and Issue 3 (gl.zig extern leak)
Original e9744d3 commit unchanged on these — see the original PR body for rationale.
Test plan
zig build testfrom assembler root — greenzig buildfrombackends/sokol/example/— greenlabelle run --screenshot=/tmp/fp.bmp) — blocked on parallel labelle-imgui pin bump (see below)Follow-up required: labelle-imgui pin coordination
labelle-imgui needs a parallel pin bump to
887b30fin both its top-levelbuild.zig.zonandbridges/sokol/build.zig.zon. Without it, the bridge resolvesb.dependency("sokol", .{...})against the OLD fork commit while the assembler resolves against the NEW one — Zig caches them as two separatesokol_clibartifacts, the cimgui include-path injection lands on only one of them, andsokol_imgui.cfails to findcimgui.hat build time.Symptom (without the imgui-side bump):
Non-imgui sokol consumers (
backends/sokol/example/) link and run fine — the issue is strictly the imgui+sokol pin diamond.Closes #222.