diff --git a/lib/std/Build.zig b/lib/std/Build.zig index 636b5bbe4f..a9a5d18f6e 100644 --- a/lib/std/Build.zig +++ b/lib/std/Build.zig @@ -979,7 +979,7 @@ pub const TestOptions = struct { /// Deprecated; use `.filters = &.{filter}` instead of `.filter = filter`. filter: ?[]const u8 = null, filters: []const []const u8 = &.{}, - test_runner: ?LazyPath = null, + test_runner: ?Step.Compile.TestRunner = null, use_llvm: ?bool = null, use_lld: ?bool = null, zig_lib_dir: ?LazyPath = null, @@ -1136,9 +1136,8 @@ pub fn addRunArtifact(b: *Build, exe: *Step.Compile) *Step.Run { run_step.addArtifactArg(exe); } - if (exe.test_server_mode) { - run_step.enableTestRunnerMode(); - } + const test_server_mode = if (exe.test_runner) |r| r.mode == .server else true; + if (test_server_mode) run_step.enableTestRunnerMode(); } else { run_step.addArtifactArg(exe); } diff --git a/lib/std/Build/Step/Compile.zig b/lib/std/Build/Step/Compile.zig index 511d37d458..7daf10c68c 100644 --- a/lib/std/Build/Step/Compile.zig +++ b/lib/std/Build/Step/Compile.zig @@ -56,8 +56,7 @@ global_base: ?u64 = null, zig_lib_dir: ?LazyPath, exec_cmd_args: ?[]const ?[]const u8, filters: []const []const u8, -test_runner: ?LazyPath, -test_server_mode: bool, +test_runner: ?TestRunner, wasi_exec_model: ?std.builtin.WasiExecModel = null, installed_headers: ArrayList(HeaderInstallation), @@ -268,7 +267,7 @@ pub const Options = struct { version: ?std.SemanticVersion = null, max_rss: usize = 0, filters: []const []const u8 = &.{}, - test_runner: ?LazyPath = null, + test_runner: ?TestRunner = null, use_llvm: ?bool = null, use_lld: ?bool = null, zig_lib_dir: ?LazyPath = null, @@ -347,6 +346,14 @@ pub const HeaderInstallation = union(enum) { } }; +pub const TestRunner = struct { + path: LazyPath, + /// Test runners can either be "simple", running tests when spawned and terminating when the + /// tests are complete, or they can use `std.zig.Server` over stdio to interact more closely + /// with the build system. + mode: enum { simple, server }, +}; + 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) { @@ -411,8 +418,7 @@ pub fn create(owner: *std.Build, options: Options) *Compile { .zig_lib_dir = null, .exec_cmd_args = null, .filters = options.filters, - .test_runner = null, - .test_server_mode = options.test_runner == null, + .test_runner = null, // set below .rdynamic = false, .installed_path = null, .force_undefined_symbols = StringHashMap(void).init(owner.allocator), @@ -438,9 +444,12 @@ pub fn create(owner: *std.Build, options: Options) *Compile { lp.addStepDependencies(&compile.step); } - if (options.test_runner) |lp| { - compile.test_runner = lp.dupe(compile.step.owner); - lp.addStepDependencies(&compile.step); + if (options.test_runner) |runner| { + compile.test_runner = .{ + .path = runner.path.dupe(compile.step.owner), + .mode = runner.mode, + }; + runner.path.addStepDependencies(&compile.step); } // Only the PE/COFF format has a Resource Table which is where the manifest @@ -1399,7 +1408,7 @@ fn getZigArgs(compile: *Compile, fuzz: bool) ![][]const u8 { if (compile.test_runner) |test_runner| { try zig_args.append("--test-runner"); - try zig_args.append(test_runner.getPath2(b, step)); + try zig_args.append(test_runner.path.getPath2(b, step)); } for (b.debug_log_scopes) |log_scope| { diff --git a/lib/std/Build/Step/Run.zig b/lib/std/Build/Step/Run.zig index 37db10916f..a9d4808bc0 100644 --- a/lib/std/Build/Step/Run.zig +++ b/lib/std/Build/Step/Run.zig @@ -1354,6 +1354,9 @@ fn spawnChildAndCollect( _ = child.kill() catch {}; } + // We need to report `error.InvalidExe` *now* if applicable. + try child.waitForSpawn(); + var timer = try std.time.Timer.start(); const result = if (run.stdio == .zig_test) diff --git a/lib/std/process/Child.zig b/lib/std/process/Child.zig index 72277c2e62..fa824e0aec 100644 --- a/lib/std/process/Child.zig +++ b/lib/std/process/Child.zig @@ -73,7 +73,7 @@ cwd: ?[]const u8, /// Once that is done, `cwd` will be deprecated in favor of this field. cwd_dir: ?fs.Dir = null, -err_pipe: ?if (native_os == .windows) void else [2]posix.fd_t, +err_pipe: if (native_os == .windows) void else ?posix.fd_t, expand_arg0: Arg0Expand, @@ -211,7 +211,7 @@ pub fn init(argv: []const []const u8, allocator: mem.Allocator) ChildProcess { .argv = argv, .id = undefined, .thread_handle = undefined, - .err_pipe = null, + .err_pipe = if (native_os == .windows) {} else null, .term = null, .env_map = null, .cwd = null, @@ -293,17 +293,49 @@ pub fn killPosix(self: *ChildProcess) !Term { error.ProcessNotFound => return error.AlreadyTerminated, else => return err, }; - self.waitUnwrapped(); + self.waitUnwrappedPosix(); return self.term.?; } pub const WaitError = SpawnError || std.os.windows.GetProcessMemoryInfoError; +/// On some targets, `spawn` may not report all spawn errors, such as `error.InvalidExe`. +/// This function will block until any spawn errors can be reported, and return them. +pub fn waitForSpawn(self: *ChildProcess) SpawnError!void { + if (native_os == .windows) return; // `spawn` reports everything + if (self.term) |term| { + _ = term catch |spawn_err| return spawn_err; + return; + } + + const err_pipe = self.err_pipe orelse return; + self.err_pipe = null; + + // Wait for the child to report any errors in or before `execvpe`. + if (readIntFd(err_pipe)) |child_err_int| { + posix.close(err_pipe); + const child_err: SpawnError = @errorCast(@errorFromInt(child_err_int)); + self.term = child_err; + return child_err; + } else |_| { + // Write end closed by CLOEXEC at the time of the `execvpe` call, indicating success! + posix.close(err_pipe); + } +} + /// Blocks until child process terminates and then cleans up all resources. pub fn wait(self: *ChildProcess) WaitError!Term { - const term = if (native_os == .windows) try self.waitWindows() else self.waitPosix(); + try self.waitForSpawn(); // report spawn errors + if (self.term) |term| { + self.cleanupStreams(); + return term; + } + switch (native_os) { + .windows => try self.waitUnwrappedWindows(), + else => self.waitUnwrappedPosix(), + } self.id = undefined; - return term; + return self.term.?; } pub const RunResult = struct { @@ -405,26 +437,6 @@ pub fn run(args: struct { }; } -fn waitWindows(self: *ChildProcess) WaitError!Term { - if (self.term) |term| { - self.cleanupStreams(); - return term; - } - - try self.waitUnwrappedWindows(); - return self.term.?; -} - -fn waitPosix(self: *ChildProcess) SpawnError!Term { - if (self.term) |term| { - self.cleanupStreams(); - return term; - } - - self.waitUnwrapped(); - return self.term.?; -} - fn waitUnwrappedWindows(self: *ChildProcess) WaitError!void { const result = windows.WaitForSingleObjectEx(self.id, windows.INFINITE, false); @@ -447,7 +459,7 @@ fn waitUnwrappedWindows(self: *ChildProcess) WaitError!void { return result; } -fn waitUnwrapped(self: *ChildProcess) void { +fn waitUnwrappedPosix(self: *ChildProcess) void { const res: posix.WaitPidResult = res: { if (self.request_resource_usage_statistics) { switch (native_os) { @@ -469,7 +481,7 @@ fn waitUnwrapped(self: *ChildProcess) void { } fn handleWaitResult(self: *ChildProcess, status: u32) void { - self.term = self.cleanupAfterWait(status); + self.term = statusToTerm(status); } fn cleanupStreams(self: *ChildProcess) void { @@ -487,46 +499,6 @@ fn cleanupStreams(self: *ChildProcess) void { } } -fn cleanupAfterWait(self: *ChildProcess, status: u32) !Term { - if (self.err_pipe) |err_pipe| { - defer destroyPipe(err_pipe); - - if (native_os == .linux) { - var fd = [1]posix.pollfd{posix.pollfd{ - .fd = err_pipe[0], - .events = posix.POLL.IN, - .revents = undefined, - }}; - - // Check if the eventfd buffer stores a non-zero value by polling - // it, that's the error code returned by the child process. - _ = posix.poll(&fd, 0) catch unreachable; - - // According to eventfd(2) the descriptor is readable if the counter - // has a value greater than 0 - if ((fd[0].revents & posix.POLL.IN) != 0) { - const err_int = try readIntFd(err_pipe[0]); - return @as(SpawnError, @errorCast(@errorFromInt(err_int))); - } - } else { - // Write maxInt(ErrInt) to the write end of the err_pipe. This is after - // waitpid, so this write is guaranteed to be after the child - // pid potentially wrote an error. This way we can do a blocking - // read on the error pipe and either get maxInt(ErrInt) (no error) or - // an error code. - try writeIntFd(err_pipe[1], maxInt(ErrInt)); - const err_int = try readIntFd(err_pipe[0]); - // Here we potentially return the fork child's error from the parent - // pid. - if (err_int != maxInt(ErrInt)) { - return @as(SpawnError, @errorCast(@errorFromInt(err_int))); - } - } - } - - return statusToTerm(status); -} - fn statusToTerm(status: u32) Term { return if (posix.W.IFEXITED(status)) Term{ .Exited = posix.W.EXITSTATUS(status) } @@ -636,18 +608,9 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void { } }; - // This pipe is used to communicate errors between the time of fork - // and execve from the child process to the parent process. - const err_pipe = blk: { - if (native_os == .linux) { - const fd = try posix.eventfd(0, linux.EFD.CLOEXEC); - // There's no distinction between the readable and the writeable - // end with eventfd - break :blk [2]posix.fd_t{ fd, fd }; - } else { - break :blk try posix.pipe2(.{ .CLOEXEC = true }); - } - }; + // This pipe communicates to the parent errors in the child between `fork` and `execvpe`. + // It is closed by the child (via CLOEXEC) without writing if `execvpe` succeeds. + const err_pipe: [2]posix.fd_t = try posix.pipe2(.{ .CLOEXEC = true }); errdefer destroyPipe(err_pipe); const pid_result = try posix.fork(); @@ -687,6 +650,11 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void { } // we are the parent + errdefer comptime unreachable; // The child is forked; we must not error from now on + + posix.close(err_pipe[1]); // make sure only the child holds the write end open + self.err_pipe = err_pipe[0]; + const pid: i32 = @intCast(pid_result); if (self.stdin_behavior == .Pipe) { self.stdin = .{ .handle = stdin_pipe[1] }; @@ -705,7 +673,6 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void { } self.id = pid; - self.err_pipe = err_pipe; self.term = null; if (self.stdin_behavior == .Pipe) { diff --git a/test/standalone/test_runner_module_imports/build.zig b/test/standalone/test_runner_module_imports/build.zig index 6b24ef7124..ae65308331 100644 --- a/test/standalone/test_runner_module_imports/build.zig +++ b/test/standalone/test_runner_module_imports/build.zig @@ -13,7 +13,10 @@ pub fn build(b: *std.Build) void { const t = b.addTest(.{ .root_module = test_mod, - .test_runner = b.path("test_runner/main.zig"), + .test_runner = .{ + .path = b.path("test_runner/main.zig"), + .mode = .simple, + }, }); const test_step = b.step("test", "Run unit tests"); diff --git a/test/standalone/test_runner_path/build.zig b/test/standalone/test_runner_path/build.zig index 1666031ba3..f2fdbfb0e0 100644 --- a/test/standalone/test_runner_path/build.zig +++ b/test/standalone/test_runner_path/build.zig @@ -8,7 +8,10 @@ pub fn build(b: *std.Build) void { .target = b.graph.host, .root_source_file = b.path("test.zig"), }) }); - test_exe.test_runner = b.path("test_runner.zig"); + test_exe.test_runner = .{ + .path = b.path("test_runner.zig"), + .mode = .simple, + }; const test_run = b.addRunArtifact(test_exe); test_step.dependOn(&test_run.step);