From a92427bf275bc85d4a0c5f086bbb8156f00f2b6c Mon Sep 17 00:00:00 2001 From: mlugg Date: Tue, 17 Jun 2025 10:03:30 +0100 Subject: [PATCH 1/2] Build.Cache.Path: fix `resolvePosix` empty `sub_path` This function is sometimes used to assume a canonical representation of a path. However, when the `Path` referred to `root_dir` itself, this function previously resolved `sub_path` to ".", which is incorrect; per doc comments, it should set `sub_path` to "". This fix ultimately didn't solve what I was trying to solve, though I'm still PRing it, because it's still *correct*. The background to this commit is quite interesting and worth briefly discussing. I originally worked on this to try and fix a bug in the build system, where if the root package (i.e. the one you `zig build`) depends on package X which itself depends back on the root package (through a `.path` dependency), invalid dependency modules are generated. I hit this case working on ziglang/translate-c, which wants to depend on "examples" (similar to the Zig compiler's "standalone" test cases) which themselves depend back on the translate-c package. However, after this patch just turned that error into another, I realised that this case simply cannot work, because `std.Build` needs to eagerly execute build scripts at `dependency` calls to learn which artifacts, modules, etc, exist. ...at least, that's how the build system is currently designed. One can imagine a world where `dependency` sort of "queues" the call, `artifact` and `module` etc just pretend that the thing exists, and all configure functions are called non-recursively by the runner. The downside is that it becomes impossible to query state set by a dependency's configure script. For instance, if a dependency exposes an artifact, it would become impossible to get that artifact's resolved target in the configure phase. However, as well as allowing recursive package imports (which are certainly kinda nifty), it would also make lazy dependencies far more useful! Right now, lazy dependencies only really work if you use options (`std.Build.option`) to block their usage, since any call to `lazyDependency` causes the dependency to be fetched. However, if we made this change, lazy dependencies could be made far more versatile by only fetching them *if the final step plan requires them*. I'm not 100% sure if this is a good idea or not, but I might open an issue for it soon. --- lib/std/Build/Cache/Path.zig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/std/Build/Cache/Path.zig b/lib/std/Build/Cache/Path.zig index a14c6cd7a4..8822fb64be 100644 --- a/lib/std/Build/Cache/Path.zig +++ b/lib/std/Build/Cache/Path.zig @@ -30,9 +30,11 @@ pub fn join(p: Path, arena: Allocator, sub_path: []const u8) Allocator.Error!Pat pub fn resolvePosix(p: Path, arena: Allocator, sub_path: []const u8) Allocator.Error!Path { if (sub_path.len == 0) return p; + const new_sub_path = try fs.path.resolvePosix(arena, &.{ p.sub_path, sub_path }); return .{ .root_dir = p.root_dir, - .sub_path = try fs.path.resolvePosix(arena, &.{ p.sub_path, sub_path }), + // Use "" instead of "." to represent `root_dir` itself. + .sub_path = if (std.mem.eql(u8, new_sub_path, ".")) "" else new_sub_path, }; } From f3c0555975053a6d3ffd9bc239b732658aebe5d6 Mon Sep 17 00:00:00 2001 From: mlugg Date: Tue, 17 Jun 2025 11:24:58 +0100 Subject: [PATCH 2/2] std.Build: introduce `ConfigHeader.getOutputDir`, small refactor `std.Build.Step.ConfigHeader` emits a *directory* containing a config header under a given sub path, but there's no good way to actually access that directory as a `LazyPath` in the configure phase. This is silly; it's perfectly valid to refer to that directory, perhaps to explicitly pass as a "-I" flag to a different toolchain invoked via a `Step.Run`. So now, instead of the `GeneratedFile` being the actual *file*, it should be that *directory*, i.e. `cache/o/`. We can then easily get the *file* if needed just by using `LazyPath.path` to go "deeper", which there is a helper function for. The legacy `getOutput` function is now a deprecated alias for `getOutputFile`, and `getOutputDir` is introduced. `std.Build.Module.IncludeDir.appendZigProcessFlags` needed a fix after this change, so I took the opportunity to refactor it a little. I was looking at this function while working on ziglang/translate-c yesterday and realised it could be expressed much more simply -- particularly after the `ConfigHeader` change here. I had to update the test `standalone/cmakedefine/` -- it turns out this test was well and truly reaching into build system internals, and doing horrible not-really-allowed stuff like overriding the `makeFn` of a `TopLevelStep`. To top it all off, the test forgot to set `b.default_step` to its "test" step, so the test never even ran. I've refactored it to follow accepted practices and to actually, like, work. --- lib/std/Build/Module.zig | 50 +++++++++-------------- lib/std/Build/Step/ConfigHeader.zig | 21 ++++++---- test/standalone/cmakedefine/build.zig | 57 ++++++++++++--------------- test/standalone/cmakedefine/check.zig | 26 ++++++++++++ 4 files changed, 83 insertions(+), 71 deletions(-) create mode 100644 test/standalone/cmakedefine/check.zig diff --git a/lib/std/Build/Module.zig b/lib/std/Build/Module.zig index 757f51f933..12b55a8c29 100644 --- a/lib/std/Build/Module.zig +++ b/lib/std/Build/Module.zig @@ -173,39 +173,25 @@ pub const IncludeDir = union(enum) { zig_args: *std.ArrayList([]const u8), asking_step: ?*Step, ) !void { - switch (include_dir) { - .path => |include_path| { - try zig_args.appendSlice(&.{ "-I", include_path.getPath2(b, asking_step) }); + const flag: []const u8, const lazy_path: LazyPath = switch (include_dir) { + // zig fmt: off + .path => |lp| .{ "-I", lp }, + .path_system => |lp| .{ "-isystem", lp }, + .path_after => |lp| .{ "-idirafter", lp }, + .framework_path => |lp| .{ "-F", lp }, + .framework_path_system => |lp| .{ "-iframework", lp }, + .config_header_step => |ch| .{ "-I", ch.getOutputDir() }, + .other_step => |comp| .{ "-I", comp.installed_headers_include_tree.?.getDirectory() }, + // zig fmt: on + .embed_path => |lazy_path| { + // Special case: this is a single arg. + const resolved = lazy_path.getPath3(b, asking_step); + const arg = b.fmt("--embed-dir={}", .{resolved}); + return zig_args.append(arg); }, - .path_system => |include_path| { - try zig_args.appendSlice(&.{ "-isystem", include_path.getPath2(b, asking_step) }); - }, - .path_after => |include_path| { - try zig_args.appendSlice(&.{ "-idirafter", include_path.getPath2(b, asking_step) }); - }, - .framework_path => |include_path| { - try zig_args.appendSlice(&.{ "-F", include_path.getPath2(b, asking_step) }); - }, - .framework_path_system => |include_path| { - try zig_args.appendSlice(&.{ "-iframework", include_path.getPath2(b, asking_step) }); - }, - .other_step => |other| { - if (other.generated_h) |header| { - try zig_args.appendSlice(&.{ "-isystem", std.fs.path.dirname(header.getPath()).? }); - } - if (other.installed_headers_include_tree) |include_tree| { - try zig_args.appendSlice(&.{ "-I", include_tree.generated_directory.getPath() }); - } - }, - .config_header_step => |config_header| { - const full_file_path = config_header.output_file.getPath(); - const header_dir_path = full_file_path[0 .. full_file_path.len - config_header.include_path.len]; - try zig_args.appendSlice(&.{ "-I", header_dir_path }); - }, - .embed_path => |embed_path| { - try zig_args.append(try std.mem.concat(b.allocator, u8, &.{ "--embed-dir=", embed_path.getPath2(b, asking_step) })); - }, - } + }; + const resolved_str = try lazy_path.getPath3(b, asking_step).toString(b.graph.arena); + return zig_args.appendSlice(&.{ flag, resolved_str }); } }; diff --git a/lib/std/Build/Step/ConfigHeader.zig b/lib/std/Build/Step/ConfigHeader.zig index 6bfda7667b..967e1edd05 100644 --- a/lib/std/Build/Step/ConfigHeader.zig +++ b/lib/std/Build/Step/ConfigHeader.zig @@ -39,7 +39,8 @@ pub const Value = union(enum) { step: Step, values: std.StringArrayHashMap(Value), -output_file: std.Build.GeneratedFile, +/// This directory contains the generated file under the name `include_path`. +generated_dir: std.Build.GeneratedFile, style: Style, max_bytes: usize, @@ -99,7 +100,7 @@ pub fn create(owner: *std.Build, options: Options) *ConfigHeader { .max_bytes = options.max_bytes, .include_path = include_path, .include_guard_override = options.include_guard_override, - .output_file = .{ .step = &config_header.step }, + .generated_dir = .{ .step = &config_header.step }, }; return config_header; @@ -115,9 +116,15 @@ pub fn addValues(config_header: *ConfigHeader, values: anytype) void { } } -pub fn getOutput(config_header: *ConfigHeader) std.Build.LazyPath { - return .{ .generated = .{ .file = &config_header.output_file } }; +pub fn getOutputDir(ch: *ConfigHeader) std.Build.LazyPath { + return .{ .generated = .{ .file = &ch.generated_dir } }; } +pub fn getOutputFile(ch: *ConfigHeader) std.Build.LazyPath { + return ch.getOutputDir().path(ch.step.owner, ch.include_path); +} + +/// Deprecated; use `getOutputFile`. +pub const getOutput = getOutputFile; fn addValueInner(config_header: *ConfigHeader, name: []const u8, comptime T: type, value: T) !void { switch (@typeInfo(T)) { @@ -234,9 +241,7 @@ fn make(step: *Step, options: Step.MakeOptions) !void { if (try step.cacheHit(&man)) { const digest = man.final(); - config_header.output_file.path = try b.cache_root.join(arena, &.{ - "o", &digest, config_header.include_path, - }); + config_header.generated_dir.path = try b.cache_root.join(arena, &.{ "o", &digest }); return; } @@ -262,7 +267,7 @@ fn make(step: *Step, options: Step.MakeOptions) !void { }); }; - config_header.output_file.path = try b.cache_root.join(arena, &.{sub_path}); + config_header.generated_dir.path = try b.cache_root.join(arena, &.{ "o", &digest }); try man.writeManifest(); } diff --git a/test/standalone/cmakedefine/build.zig b/test/standalone/cmakedefine/build.zig index 50df9d0b7a..3dc0ec3046 100644 --- a/test/standalone/cmakedefine/build.zig +++ b/test/standalone/cmakedefine/build.zig @@ -71,40 +71,35 @@ pub fn build(b: *std.Build) void { }, ); + const check_exe = b.addExecutable(.{ + .name = "check", + .root_module = b.createModule(.{ + .target = b.graph.host, + .root_source_file = b.path("check.zig"), + }), + }); + const test_step = b.step("test", "Test it"); - test_step.makeFn = compare_headers; - test_step.dependOn(&config_header.step); - test_step.dependOn(&pwd_sh.step); - test_step.dependOn(&sigil_header.step); - test_step.dependOn(&stack_header.step); - test_step.dependOn(&wrapper_header.step); + b.default_step = test_step; + test_step.dependOn(addCheck(b, check_exe, config_header)); + test_step.dependOn(addCheck(b, check_exe, pwd_sh)); + test_step.dependOn(addCheck(b, check_exe, sigil_header)); + test_step.dependOn(addCheck(b, check_exe, stack_header)); + test_step.dependOn(addCheck(b, check_exe, wrapper_header)); } -fn compare_headers(step: *std.Build.Step, options: std.Build.Step.MakeOptions) !void { - _ = options; - const allocator = step.owner.allocator; - const expected_fmt = "expected_{s}"; +fn addCheck( + b: *std.Build, + check_exe: *std.Build.Step.Compile, + ch: *ConfigHeader, +) *std.Build.Step { + // We expect `ch.include_path` to only be a basename to infer where the expected output is. + std.debug.assert(std.fs.path.dirname(ch.include_path) == null); + const expected_path = b.fmt("expected_{s}", .{ch.include_path}); - for (step.dependencies.items) |config_header_step| { - const config_header: *ConfigHeader = @fieldParentPtr("step", config_header_step); + const run_check = b.addRunArtifact(check_exe); + run_check.addFileArg(ch.getOutputFile()); + run_check.addFileArg(b.path(expected_path)); - const zig_header_path = config_header.output_file.path orelse @panic("Could not locate header file"); - - const cwd = std.fs.cwd(); - - const cmake_header_path = try std.fmt.allocPrint(allocator, expected_fmt, .{std.fs.path.basename(zig_header_path)}); - defer allocator.free(cmake_header_path); - - const cmake_header = try cwd.readFileAlloc(allocator, cmake_header_path, config_header.max_bytes); - defer allocator.free(cmake_header); - - const zig_header = try cwd.readFileAlloc(allocator, zig_header_path, config_header.max_bytes); - defer allocator.free(zig_header); - - const header_text_index = std.mem.indexOf(u8, zig_header, "\n") orelse @panic("Could not find comment in header filer"); - - if (!std.mem.eql(u8, zig_header[header_text_index + 1 ..], cmake_header)) { - @panic("processed cmakedefine header does not match expected output"); - } - } + return &run_check.step; } diff --git a/test/standalone/cmakedefine/check.zig b/test/standalone/cmakedefine/check.zig new file mode 100644 index 0000000000..f7c60946c8 --- /dev/null +++ b/test/standalone/cmakedefine/check.zig @@ -0,0 +1,26 @@ +pub fn main() !void { + var arena_state: std.heap.ArenaAllocator = .init(std.heap.page_allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + const args = try std.process.argsAlloc(arena); + + if (args.len != 3) return error.BadUsage; + const actual_path = args[1]; + const expected_path = args[2]; + + const actual = try std.fs.cwd().readFileAlloc(arena, actual_path, 1024 * 1024); + const expected = try std.fs.cwd().readFileAlloc(arena, expected_path, 1024 * 1024); + + // The actual output starts with a comment which we should strip out before comparing. + const comment_str = "/* This file was generated by ConfigHeader using the Zig Build System. */\n"; + if (!std.mem.startsWith(u8, actual, comment_str)) { + return error.MissingOrMalformedComment; + } + const actual_without_comment = actual[comment_str.len..]; + + if (!std.mem.eql(u8, actual_without_comment, expected)) { + return error.DoesNotMatch; + } +} +const std = @import("std");