Commit 0b7123f41d66bdda4da29d59623299d47b29aefb | zig

author	Carl Åstholm <carl@astholm.se> 2024-03-02 22:59:00+01:00
committer	Carl Åstholm <carl@astholm.se> 2024-04-07 15:32:44+02:00
log	0b7123f41d66bdda4da29d59623299d47b29aefb
tree	e909110b2a031944f20a41a0e44c17ccd37bb1c9
parent	129de47a71b954b1118ce188f7032ad726491f53

std.Build: correct behavior of `Step.Compile.installHeader`

Previously, `Step.Compile.installHeader` and friends would incorrectly
modify the default `install` top-level step, when the intent was for
headers to get bundled with and installed alongside an artifact. This
change set implements the intended behavior.

This carries with it some breaking changes; `installHeader` and
`installConfigHeader` both have new signatures, and
`installHeadersDirectory` and `installHeadersDirectoryOptions` have been
merged into `installHeaders`.

3 files changed, 182 insertions(+), 98 deletions(-)

lib
- std
  - Build
    Module.zig
    Step
    Compile.zig
    InstallArtifact.zig

lib/std/Build/Module.zig+14-16

 @@ -265,8 +265,8 @@ fn addShallowDependencies(m: *Module, dependee: *Module) void {
     for (dependee.link_objects.items) |link_object| switch (link_object) {
         .other_step => |compile| {
             addStepDependencies(m, dependee, &compile.step);
-            for (compile.installed_headers.items) |install_step|
+            for (compile.installed_headers.items) |header|
-                addStepDependenciesOnly(m, install_step);
+                addLazyPathDependenciesOnly(m, header.source.path());
         },
         .static_path,
 @@ -691,20 +691,19 @@ pub fn appendZigProcessFlags(
             },
             .other_step => |other| {
                 if (other.generated_h) |header| {
-                    try zig_args.append("-isystem");
+                    try zig_args.appendSlice(&.{ "-isystem", std.fs.path.dirname(header.getPath()).? });
-                    try zig_args.append(std.fs.path.dirname(header.path.?).?);
-                }
-                if (other.installed_headers.items.len > 0) {
-                    try zig_args.append("-I");
-                    try zig_args.append(b.pathJoin(&.{
-                        other.step.owner.install_prefix, "include",
-                    }));
                 }
+                for (other.installed_headers.items) |header| switch (header.source) {
+                    .file => |lp| {
+                        try zig_args.appendSlice(&.{ "-I", std.fs.path.dirname(lp.getPath2(b, asking_step)).? });
+                    },
+                    .directory => |dir| {
+                        try zig_args.appendSlice(&.{ "-I", dir.path.getPath2(b, asking_step) });
+                    },
+                };
             },
             .config_header_step => |config_header| {
-                const full_file_path = config_header.output_file.path.?;
+                try zig_args.appendSlice(&.{ "-I", std.fs.path.dirname(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 });
             },
         }
     }
 @@ -752,9 +751,8 @@ fn linkLibraryOrObject(m: *Module, other: *Step.Compile) void {
     m.link_objects.append(allocator, .{ .other_step = other }) catch @panic("OOM");
     m.include_dirs.append(allocator, .{ .other_step = other }) catch @panic("OOM");
-    for (other.installed_headers.items) |install_step| {
+    for (other.installed_headers.items) |header|
-        addStepDependenciesOnly(m, install_step);
+        addLazyPathDependenciesOnly(m, header.source.path());
-    }
 }
 fn requireKnownTarget(m: *Module) std.Target {

lib/std/Build/Step/Compile.zig+94-61

 @@ -59,7 +59,7 @@ test_runner: ?[]const u8,
 test_server_mode: bool,
 wasi_exec_model: ?std.builtin.WasiExecModel = null,
-installed_headers: ArrayList(*Step),
+installed_headers: ArrayList(InstalledHeader),
 // keep in sync with src/Compilation.zig:RcIncludes
 /// Behavior of automatic detection of include directories when compiling .rc files.
 @@ -249,6 +249,70 @@ pub const Kind = enum {
     @"test",
 };
+pub const InstalledHeader = struct {
+    source: Source,
+    dest_rel_path: []const u8,
+    pub const Source = union(enum) {
+        file: LazyPath,
+        directory: Directory,
+        pub const Directory = struct {
+            path: LazyPath,
+            options: Directory.Options,
+            pub const Options = struct {
+                /// File paths which end in any of these suffixes will be excluded
+                /// from installation.
+                exclude_extensions: []const []const u8 = &.{},
+                /// Only file paths which end in any of these suffixes will be included
+                /// in installation.
+                /// `null` means all suffixes will be included.
+                /// `exclude_extensions` takes precedence over `include_extensions`
+                include_extensions: ?[]const []const u8 = &.{".h"},
+                pub fn dupe(self: Directory.Options, b: *std.Build) Directory.Options {
+                    return .{
+                        .exclude_extensions = b.dupeStrings(self.exclude_extensions),
+                        .include_extensions = if (self.include_extensions) |incs|
+                            b.dupeStrings(incs)
+                        else
+                            null,
+                    };
+                }
+            };
+            pub fn dupe(self: Directory, b: *std.Build) Directory {
+                return .{
+                    .path = self.path.dupe(b),
+                    .options = self.options.dupe(b),
+                };
+            }
+        };
+        pub fn path(self: Source) LazyPath {
+            return switch (self) {
+                .file => |lp| lp,
+                .directory => |dir| dir.path,
+            };
+        }
+        pub fn dupe(self: Source, b: *std.Build) Source {
+            return switch (self) {
+                .file => |lp| .{ .file = lp.dupe(b) },
+                .directory => |dir| .{ .directory = dir.dupe(b) },
+            };
+        }
+    };
+    pub fn dupe(self: InstalledHeader, b: *std.Build) InstalledHeader {
+        return .{
+            .source = self.source.dupe(b),
+            .dest_rel_path = b.dupePath(self.dest_rel_path),
+        };
+    }
+};
 pub fn create(owner: *std.Build, options: Options) *Compile {
     const name = owner.dupe(options.name);
     if (mem.indexOf(u8, name, "/") != null or mem.indexOf(u8, name, "\\") != null) {
 @@ -308,7 +372,7 @@ pub fn create(owner: *std.Build, options: Options) *Compile {
         .out_lib_filename = undefined,
         .major_only_filename = null,
         .name_only_filename = null,
-        .installed_headers = ArrayList(*Step).init(owner.allocator),
+        .installed_headers = ArrayList(InstalledHeader).init(owner.allocator),
         .zig_lib_dir = null,
         .exec_cmd_args = null,
         .filters = options.filters,
 @@ -380,78 +444,47 @@ pub fn create(owner: *std.Build, options: Options) *Compile {
     return self;
 }
-pub fn installHeader(cs: *Compile, src_path: []const u8, dest_rel_path: []const u8) void {
+pub fn installHeader(
-    const b = cs.step.owner;
-    const install_file = b.addInstallHeaderFile(src_path, dest_rel_path);
-    b.getInstallStep().dependOn(&install_file.step);
-    cs.installed_headers.append(&install_file.step) catch @panic("OOM");
-}
-pub const InstallConfigHeaderOptions = struct {
-    install_dir: InstallDir = .header,
-    dest_rel_path: ?[]const u8 = null,
-};
-pub fn installConfigHeader(
     cs: *Compile,
-    config_header: *Step.ConfigHeader,
+    source: LazyPath,
-    options: InstallConfigHeaderOptions,
+    dest_rel_path: []const u8,
 ) void {
-    const dest_rel_path = options.dest_rel_path orelse config_header.include_path;
     const b = cs.step.owner;
-    const install_file = b.addInstallFileWithDir(
+    cs.installed_headers.append(.{
-        .{ .generated = &config_header.output_file },
+        .source = .{ .file = source.dupe(b) },
-        options.install_dir,
+        .dest_rel_path = b.dupePath(dest_rel_path),
-        dest_rel_path,
+    }) catch @panic("OOM");
-    );
+    source.addStepDependencies(&cs.step);
-    install_file.step.dependOn(&config_header.step);
-    b.getInstallStep().dependOn(&install_file.step);
-    cs.installed_headers.append(&install_file.step) catch @panic("OOM");
 }
-pub fn installHeadersDirectory(
+pub fn installHeaders(
-    a: *Compile,
+    cs: *Compile,
-    src_dir_path: []const u8,
+    source: LazyPath,
     dest_rel_path: []const u8,
+    options: InstalledHeader.Source.Directory.Options,
 ) void {
-    return installHeadersDirectoryOptions(a, .{
+    const b = cs.step.owner;
-        .source_dir = .{ .path = src_dir_path },
+    cs.installed_headers.append(.{
-        .install_dir = .header,
+        .source = .{ .directory = .{
-        .install_subdir = dest_rel_path,
+            .path = source.dupe(b),
-    });
+            .options = options.dupe(b),
+        } },
+        .dest_rel_path = b.dupePath(dest_rel_path),
+    }) catch @panic("OOM");
+    source.addStepDependencies(&cs.step);
 }
-pub fn installHeadersDirectoryOptions(
+pub fn installConfigHeader(cs: *Compile, config_header: *Step.ConfigHeader) void {
-    cs: *Compile,
+    cs.installHeader(.{ .generated = &config_header.output_file }, config_header.include_path);
-    options: std.Build.Step.InstallDir.Options,
-) void {
-    const b = cs.step.owner;
-    const install_dir = b.addInstallDirectory(options);
-    b.getInstallStep().dependOn(&install_dir.step);
-    cs.installed_headers.append(&install_dir.step) catch @panic("OOM");
 }
-pub fn installLibraryHeaders(cs: *Compile, l: *Compile) void {
+pub fn installLibraryHeaders(cs: *Compile, lib: *Compile) void {
-    assert(l.kind == .lib);
+    assert(lib.kind == .lib);
     const b = cs.step.owner;
-    const install_step = b.getInstallStep();
+    for (lib.installed_headers.items) |header| {
-    // Copy each element from installed_headers, modifying the builder
+        cs.installed_headers.append(header.dupe(b)) catch @panic("OOM");
-    // to be the new parent's builder.
+        header.source.path().addStepDependencies(&cs.step);
-    for (l.installed_headers.items) |step| {
-        const step_copy = switch (step.id) {
-            inline .install_file, .install_dir => |id| blk: {
-                const T = id.Type();
-                const ptr = b.allocator.create(T) catch @panic("OOM");
-                ptr.* = step.cast(T).?.*;
-                ptr.dest_builder = b;
-                break :blk &ptr.step;
-            },
-            else => unreachable,
-        };
-        cs.installed_headers.append(step_copy) catch @panic("OOM");
-        install_step.dependOn(step_copy);
     }
-    cs.installed_headers.appendSlice(l.installed_headers.items) catch @panic("OOM");
 }
 pub fn addObjCopy(cs: *Compile, options: Step.ObjCopy.Options) *Step.ObjCopy {

lib/std/Build/Step/InstallArtifact.zig+74-21

 @@ -77,12 +77,10 @@ pub fn create(owner: *std.Build, artifact: *Step.Compile, options: Options) *Ins
         },
         .h_dir = switch (options.h_dir) {
             .disabled => null,
-            // https://github.com/ziglang/zig/issues/9698
+            .default => switch (artifact.kind) {
-            .default => null,
+                .lib => .header,
-            //.default => switch (artifact.kind) {
+                else => null,
-            //    .lib => .header,
+            },
-            //    else => null,
-            //},
             .override => |o| o,
         },
         .implib_dir = switch (options.implib_dir) {
 @@ -113,7 +111,8 @@ pub fn create(owner: *std.Build, artifact: *Step.Compile, options: Options) *Ins
     if (self.dest_dir != null) self.emitted_bin = artifact.getEmittedBin();
     if (self.pdb_dir != null) self.emitted_pdb = artifact.getEmittedPdb();
-    if (self.h_dir != null) self.emitted_h = artifact.getEmittedH();
+    // https://github.com/ziglang/zig/issues/9698
+    //if (self.h_dir != null) self.emitted_h = artifact.getEmittedH();
     if (self.implib_dir != null) self.emitted_implib = artifact.getEmittedImplib();
     return self;
 @@ -122,14 +121,14 @@ pub fn create(owner: *std.Build, artifact: *Step.Compile, options: Options) *Ins
 fn make(step: *Step, prog_node: *std.Progress.Node) !void {
     _ = prog_node;
     const self: *InstallArtifact = @fieldParentPtr("step", step);
-    const dest_builder = step.owner;
+    const b = step.owner;
     const cwd = fs.cwd();
     var all_cached = true;
     if (self.dest_dir) |dest_dir| {
-        const full_dest_path = dest_builder.getInstallPath(dest_dir, self.dest_sub_path);
+        const full_dest_path = b.getInstallPath(dest_dir, self.dest_sub_path);
-        const full_src_path = self.emitted_bin.?.getPath2(step.owner, step);
+        const full_src_path = self.emitted_bin.?.getPath2(b, step);
         const p = fs.Dir.updateFile(cwd, full_src_path, cwd, full_dest_path, .{}) catch |err| {
             return step.fail("unable to update file from '{s}' to '{s}': {s}", .{
                 full_src_path, full_dest_path, @errorName(err),
 @@ -145,8 +144,8 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
     }
     if (self.implib_dir) |implib_dir| {
-        const full_src_path = self.emitted_implib.?.getPath2(step.owner, step);
+        const full_src_path = self.emitted_implib.?.getPath2(b, step);
-        const full_implib_path = dest_builder.getInstallPath(implib_dir, fs.path.basename(full_src_path));
+        const full_implib_path = b.getInstallPath(implib_dir, fs.path.basename(full_src_path));
         const p = fs.Dir.updateFile(cwd, full_src_path, cwd, full_implib_path, .{}) catch |err| {
             return step.fail("unable to update file from '{s}' to '{s}': {s}", .{
                 full_src_path, full_implib_path, @errorName(err),
 @@ -156,8 +155,8 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
     }
     if (self.pdb_dir) |pdb_dir| {
-        const full_src_path = self.emitted_pdb.?.getPath2(step.owner, step);
+        const full_src_path = self.emitted_pdb.?.getPath2(b, step);
-        const full_pdb_path = dest_builder.getInstallPath(pdb_dir, fs.path.basename(full_src_path));
+        const full_pdb_path = b.getInstallPath(pdb_dir, fs.path.basename(full_src_path));
         const p = fs.Dir.updateFile(cwd, full_src_path, cwd, full_pdb_path, .{}) catch |err| {
             return step.fail("unable to update file from '{s}' to '{s}': {s}", .{
                 full_src_path, full_pdb_path, @errorName(err),
 @@ -167,14 +166,68 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
     }
     if (self.h_dir) |h_dir| {
-        const full_src_path = self.emitted_h.?.getPath2(step.owner, step);
+        if (self.emitted_h) |emitted_h| {
-        const full_h_path = dest_builder.getInstallPath(h_dir, fs.path.basename(full_src_path));
+            const full_src_path = emitted_h.getPath2(b, step);
-        const p = fs.Dir.updateFile(cwd, full_src_path, cwd, full_h_path, .{}) catch |err| {
+            const full_h_path = b.getInstallPath(h_dir, fs.path.basename(full_src_path));
-            return step.fail("unable to update file from '{s}' to '{s}': {s}", .{
+            const p = fs.Dir.updateFile(cwd, full_src_path, cwd, full_h_path, .{}) catch |err| {
-                full_src_path, full_h_path, @errorName(err),
+                return step.fail("unable to update file from '{s}' to '{s}': {s}", .{
-            });
+                    full_src_path, full_h_path, @errorName(err),
+                });
+            };
+            all_cached = all_cached and p == .fresh;
+        }
+        for (self.artifact.installed_headers.items) |header| switch (header.source) {
+            .file => |lp| {
+                const full_src_path = lp.getPath2(b, step);
+                const full_h_path = b.getInstallPath(h_dir, header.dest_rel_path);
+                const p = fs.Dir.updateFile(cwd, full_src_path, cwd, full_h_path, .{}) catch |err| {
+                    return step.fail("unable to update file from '{s}' to '{s}': {s}", .{
+                        full_src_path, full_h_path, @errorName(err),
+                    });
+                };
+                all_cached = all_cached and p == .fresh;
+            },
+            .directory => |dir| {
+                const full_src_dir_path = dir.path.getPath2(b, step);
+                const full_h_prefix = b.getInstallPath(h_dir, header.dest_rel_path);
+                var src_dir = b.build_root.handle.openDir(full_src_dir_path, .{ .iterate = true }) catch |err| {
+                    return step.fail("unable to open source directory '{s}': {s}", .{
+                        full_src_dir_path, @errorName(err),
+                    });
+                };
+                defer src_dir.close();
+                var it = try src_dir.walk(b.allocator);
+                next_entry: while (try it.next()) |entry| {
+                    for (dir.options.exclude_extensions) |ext| {
+                        if (std.mem.endsWith(u8, entry.path, ext)) continue :next_entry;
+                    }
+                    if (dir.options.include_extensions) |incs| {
+                        for (incs) |inc| {
+                            if (std.mem.endsWith(u8, entry.path, inc)) break;
+                        } else {
+                            continue :next_entry;
+                        }
+                    }
+                    const full_src_entry_path = b.pathJoin(&.{ full_src_dir_path, entry.path });
+                    const full_dest_path = b.pathJoin(&.{ full_h_prefix, entry.path });
+                    switch (entry.kind) {
+                        .directory => try cwd.makePath(full_dest_path),
+                        .file => {
+                            const p = fs.Dir.updateFile(cwd, full_src_entry_path, cwd, full_dest_path, .{}) catch |err| {
+                                return step.fail("unable to update file from '{s}' to '{s}': {s}", .{
+                                    full_src_entry_path, full_dest_path, @errorName(err),
+                                });
+                            };
+                            all_cached = all_cached and p == .fresh;
+                        },
+                        else => continue,
+                    }
+                }
+            },
         };
-        all_cached = all_cached and p == .fresh;
     }
     step.result_cached = all_cached;