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/<digest>`. 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

@@ -173,39 +173,25 @@ pub const IncludeDir = union(enum) {
         zig_args: *std.ArrayList([]const u8),
         asking_step: ?*Step,
     ) !void {
-        switch (include_dir) {
+        const flag: []const u8, const lazy_path: LazyPath = switch (include_dir) {
-            .path => |include_path| {
+            // zig fmt: off
-                try zig_args.appendSlice(&.{ "-I", include_path.getPath2(b, asking_step) });
+            .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) });
+        const resolved_str = try lazy_path.getPath3(b, asking_step).toString(b.graph.arena);
-            },
+        return zig_args.appendSlice(&.{ flag, resolved_str });
-            .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) }));
-            },
-        }
     }
 };
 

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 {
+pub fn getOutputDir(ch: *ConfigHeader) std.Build.LazyPath {
-    return .{ .generated = .{ .file = &config_header.output_file } };
+    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, &.{
+        config_header.generated_dir.path = try b.cache_root.join(arena, &.{ "o", &digest });
-            "o", &digest, config_header.include_path,
-        });
         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();
 }
 

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;
+    b.default_step = test_step;
-    test_step.dependOn(&config_header.step);
+    test_step.dependOn(addCheck(b, check_exe, config_header));
-    test_step.dependOn(&pwd_sh.step);
+    test_step.dependOn(addCheck(b, check_exe, pwd_sh));
-    test_step.dependOn(&sigil_header.step);
+    test_step.dependOn(addCheck(b, check_exe, sigil_header));
-    test_step.dependOn(&stack_header.step);
+    test_step.dependOn(addCheck(b, check_exe, stack_header));
-    test_step.dependOn(&wrapper_header.step);
+    test_step.dependOn(addCheck(b, check_exe, wrapper_header));
 }
 
-fn compare_headers(step: *std.Build.Step, options: std.Build.Step.MakeOptions) !void {
+fn addCheck(
-    _ = options;
+    b: *std.Build,
-    const allocator = step.owner.allocator;
+    check_exe: *std.Build.Step.Compile,
-    const expected_fmt = "expected_{s}";
+    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 run_check = b.addRunArtifact(check_exe);
-        const config_header: *ConfigHeader = @fieldParentPtr("step", config_header_step);
+    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");
+    return &run_check.step;
-
-        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");
-        }
-    }
 }

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");