From 90754c6786ac272be5301ec01688220596d12e6d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 1 Mar 2024 05:19:11 -0800 Subject: [PATCH] std.Build.Step.Compile: installHeader and friends depend on Compile step Currently, `installHeader`, `installConfigHeader`, and friends all add the installation step as a dependency of the root install step. This means that if a library is installed with a custom top-level step, then the headers are not installed. To fix this, this commit changes these functions to instead have the compile step depend on the header installation stpe. This way, as long as the compile step for an artifact is consumed, the headers will be installed. Two sets of tests are included to verify this behavior: - A unit test in std.Build.Step.Compile that verifies that dependencies are correctly connected. - A standalone test that reproduces the issue reported in #17204, and verifies that the headers are installed even without the install step. Resolves #17204 --- lib/std/Build/Step/Compile.zig | 111 +++++++++++++++++- test/standalone/install_header_dep/build.zig | 35 ++++++ .../install_header_dep/exists_in.zig | 40 +++++++ test/standalone/install_header_dep/foo.h | 1 + test/standalone/install_header_dep/foo.zig | 1 + 5 files changed, 183 insertions(+), 5 deletions(-) create mode 100644 test/standalone/install_header_dep/build.zig create mode 100644 test/standalone/install_header_dep/exists_in.zig create mode 100644 test/standalone/install_header_dep/foo.h create mode 100644 test/standalone/install_header_dep/foo.zig diff --git a/lib/std/Build/Step/Compile.zig b/lib/std/Build/Step/Compile.zig index 5ee92ffc2282..ae400a9bc2d0 100644 --- a/lib/std/Build/Step/Compile.zig +++ b/lib/std/Build/Step/Compile.zig @@ -382,7 +382,7 @@ pub fn create(owner: *std.Build, options: Options) *Compile { pub fn installHeader(cs: *Compile, src_path: []const u8, dest_rel_path: []const u8) void { const b = cs.step.owner; const install_file = b.addInstallHeaderFile(src_path, dest_rel_path); - b.getInstallStep().dependOn(&install_file.step); + cs.step.dependOn(&install_file.step); cs.installed_headers.append(&install_file.step) catch @panic("OOM"); } @@ -404,7 +404,7 @@ pub fn installConfigHeader( dest_rel_path, ); install_file.step.dependOn(&config_header.step); - b.getInstallStep().dependOn(&install_file.step); + cs.step.dependOn(&install_file.step); cs.installed_headers.append(&install_file.step) catch @panic("OOM"); } @@ -426,14 +426,13 @@ pub fn installHeadersDirectoryOptions( ) void { const b = cs.step.owner; const install_dir = b.addInstallDirectory(options); - b.getInstallStep().dependOn(&install_dir.step); + cs.step.dependOn(&install_dir.step); cs.installed_headers.append(&install_dir.step) catch @panic("OOM"); } pub fn installLibraryHeaders(cs: *Compile, l: *Compile) void { assert(l.kind == .lib); const b = cs.step.owner; - const install_step = b.getInstallStep(); // Copy each element from installed_headers, modifying the builder // to be the new parent's builder. for (l.installed_headers.items) |step| { @@ -448,7 +447,7 @@ pub fn installLibraryHeaders(cs: *Compile, l: *Compile) void { else => unreachable, }; cs.installed_headers.append(step_copy) catch @panic("OOM"); - install_step.dependOn(step_copy); + cs.step.dependOn(step_copy); } cs.installed_headers.appendSlice(l.installed_headers.items) catch @panic("OOM"); } @@ -1887,3 +1886,105 @@ fn moduleNeedsCliArg(mod: *const Module) bool { else => continue, } else false; } + +// https://github.com/ziglang/zig/issues/17204 +test "installed header dependencies" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + + var graph: std.Build.Graph = .{ + .arena = arena.allocator(), + .cache = .{ + .gpa = arena.allocator(), + .manifest_dir = std.fs.cwd(), + }, + .zig_exe = "test", + .env_map = std.process.EnvMap.init(arena.allocator()), + .global_cache_root = .{ .path = "test", .handle = std.fs.cwd() }, + }; + + var builder = try std.Build.create( + &graph, + .{ .path = "test", .handle = std.fs.cwd() }, + .{ .path = "test", .handle = std.fs.cwd() }, + &.{}, + ); + + const config_header = builder.addConfigHeader(.{}, .{}); + + const foo = builder.addStaticLibrary(.{ + .name = "foo", + .root_source_file = .{ .path = "foo.zig" }, + .optimize = .Debug, + .target = builder.resolveTargetQuery(.{ + .cpu_arch = .aarch64, + .os_tag = .linux, + .abi = .gnu, + }), + }); + + // Install some headers as part of the foo library and verify that + // it declares a dependency on each header installation step. + { + foo.installHeader("foo.h", "foo.h"); + foo.installConfigHeader(config_header, .{ + .dest_rel_path = "config.h", + }); + foo.installHeadersDirectoryOptions(.{ + .source_dir = .{ .path = "test/include" }, + .install_dir = .header, + .install_subdir = "test", + }); + + const foo_step = foo.step; + for (foo.installed_headers.items) |header_step| { + const found = for (foo_step.dependencies.items) |dep| { + if (dep == header_step) { + break true; + } + } else false; + + errdefer std.debug.print("direct: missing dependency: {s}\n", .{header_step.name}); + try std.testing.expect(found); + } + } + + // Create another library, install foo's headers into it, and verify that + // it declares a dependency on each header installation step. + const bar = builder.addStaticLibrary(.{ + .name = "bar", + .root_source_file = .{ .path = "bar.zig" }, + .optimize = .Debug, + .target = builder.resolveTargetQuery(.{ + .cpu_arch = .aarch64, + .os_tag = .linux, + .abi = .gnu, + }), + }); + + { + bar.installLibraryHeaders(foo); + + const foo_step = foo.step; + const bar_step = bar.step; + + // installLibraryHeaders adds new steps for the headers, + // but also a copy of those from the original library. + // + // So each header could be depended on by either foo or bar. + var all_dependencies = std.ArrayList(*std.Build.Step).init(arena.allocator()); + try all_dependencies.appendSlice(foo_step.dependencies.items); + try all_dependencies.appendSlice(bar_step.dependencies.items); + + for (bar.installed_headers.items) |header_step| { + const found = for (all_dependencies.items) |dep| { + if (dep == header_step) { + break true; + } + } else false; + + errdefer std.debug.print("transitive: missing dependency: {s}\n", .{header_step.name}); + try std.testing.expect(found); + } + } +} diff --git a/test/standalone/install_header_dep/build.zig b/test/standalone/install_header_dep/build.zig new file mode 100644 index 000000000000..d1ab82bb0049 --- /dev/null +++ b/test/standalone/install_header_dep/build.zig @@ -0,0 +1,35 @@ +//! Verify that Step.Compile.installHeader correctly declare a dependency on +//! the Step itself. +//! +//! Test for https://github.com/ziglang/zig/issues/17204. + +const std = @import("std"); + +pub fn build(b: *std.Build) void { + const target = b.standardTargetOptions(.{}); + + const foo = b.addStaticLibrary(.{ + .name = "foo", + .root_source_file = .{ .path = "foo.zig" }, + .optimize = .Debug, + .target = target, + }); + foo.installHeader("foo.h", "foo.h"); + + const exists_in = b.addExecutable(.{ + .name = "exists_in", + .root_source_file = .{ .path = "exists_in.zig" }, + .optimize = .Debug, + .target = target, + }); + + const run = b.addRunArtifact(exists_in); + run.addDirectoryArg(.{ .path = b.getInstallPath(.header, ".") }); + run.addArgs(&.{"foo.h"}); + run.expectExitCode(0); + run.step.dependOn(&foo.step); + + const test_step = b.step("test", "Test it"); + b.default_step = test_step; + test_step.dependOn(&run.step); +} diff --git a/test/standalone/install_header_dep/exists_in.zig b/test/standalone/install_header_dep/exists_in.zig new file mode 100644 index 000000000000..25b3536fc381 --- /dev/null +++ b/test/standalone/install_header_dep/exists_in.zig @@ -0,0 +1,40 @@ +//! Verifies that a file exists in a directory. +//! +//! Usage: +//! +//! ``` +//! exists_in +//! ``` +//! +//! Where `/` is the full path to the file. + +const std = @import("std"); + +pub fn main() !void { + var arena_state = std.heap.ArenaAllocator.init(std.heap.page_allocator); + const arena = arena_state.allocator(); + defer arena_state.deinit(); + + try run(arena); +} + +fn run(allocator: std.mem.Allocator) !void { + var args = try std.process.argsWithAllocator(allocator); + defer args.deinit(); + _ = args.next() orelse unreachable; // skip binary name + + const dir_path = args.next() orelse { + std.log.err("missing argument", .{}); + return error.BadUsage; + }; + + const relpath = args.next() orelse { + std.log.err("missing argument", .{}); + return error.BadUsage; + }; + + var dir = try std.fs.cwd().openDir(dir_path, .{}); + defer dir.close(); + + _ = try dir.statFile(relpath); +} diff --git a/test/standalone/install_header_dep/foo.h b/test/standalone/install_header_dep/foo.h new file mode 100644 index 000000000000..97f529e00008 --- /dev/null +++ b/test/standalone/install_header_dep/foo.h @@ -0,0 +1 @@ +// Header file diff --git a/test/standalone/install_header_dep/foo.zig b/test/standalone/install_header_dep/foo.zig new file mode 100644 index 000000000000..5d05076dc3a3 --- /dev/null +++ b/test/standalone/install_header_dep/foo.zig @@ -0,0 +1 @@ +// Source file