Skip to content

Commit

Permalink
Redo: Handle execveZ failures in Command.zig tests (#3176)
Browse files Browse the repository at this point in the history
#3130 do over. Github seems
to have lost track of the fork status for the previous work. (perhaps
permissions?). Rebased the PR to main as I can't see why it could
possibly fail.

<details>
  <summary>Previous description</summary>
Bit of a rabbit hole came up while trying to package ghostty for nixos.
zig build test would just hang when run as part of checkPhase in a nix
build. (rather than the nix develop ... command run in CI).

Here's what I understand so far:

/usr/bin/env does not exist in a nix build sandbox. This only works as a
test binary when running in nix develop as it shares its environment
with the user where this compatibility crutch exists.
When executing Command.zig tests that reference /usr/bin/env the
eventual call to fork+execevZ will duplicate the running test process as
execevZ returns rather than dissapearing into the new exec'd process.
Duplicating a test process via fork does unexepected things. zig build
test will hang for <reasons?>. A test binary created via -Demit-test-exe
will run two copies of the test suite producing two different failure
outputs for the same test. (or ~4 copies of the test framework, one
extra for each test that fails this way)

/bin/sh can be used and an alternative test target. It isn't amazing as
it's relying on stdenv creating /bin/sh and it just existing on user
systems. Other alternatives I can think of would require build flags or
some sort of contract with packaging around what binary will exist for
the Command.zig tests.
</details>
  • Loading branch information
mitchellh authored Dec 29, 2024
2 parents 25a4a89 + 600eea0 commit 65a1c0d
Showing 1 changed file with 58 additions and 14 deletions.
72 changes: 58 additions & 14 deletions src/Command.zig
Original file line number Diff line number Diff line change
Expand Up @@ -587,8 +587,8 @@ test "createNullDelimitedEnvMap" {
test "Command: pre exec" {
if (builtin.os.tag == .windows) return error.SkipZigTest;
var cmd: Command = .{
.path = "/usr/bin/env",
.args = &.{ "/usr/bin/env", "-v" },
.path = "/bin/sh",
.args = &.{ "/bin/sh", "-v" },
.pre_exec = (struct {
fn do(_: *Command) void {
// This runs in the child, so we can exit and it won't
Expand All @@ -598,7 +598,7 @@ test "Command: pre exec" {
}).do,
};

try cmd.start(testing.allocator);
try cmd.testingStart();
try testing.expect(cmd.pid != null);
const exit = try cmd.wait(true);
try testing.expect(exit == .Exited);
Expand Down Expand Up @@ -629,12 +629,12 @@ test "Command: redirect stdout to file" {
.args = &.{"C:\\Windows\\System32\\whoami.exe"},
.stdout = stdout,
} else .{
.path = "/usr/bin/env",
.args = &.{ "/usr/bin/env", "-v" },
.path = "/bin/sh",
.args = &.{ "/bin/sh", "-c", "echo hello" },
.stdout = stdout,
};

try cmd.start(testing.allocator);
try cmd.testingStart();
try testing.expect(cmd.pid != null);
const exit = try cmd.wait(true);
try testing.expect(exit == .Exited);
Expand Down Expand Up @@ -663,13 +663,13 @@ test "Command: custom env vars" {
.stdout = stdout,
.env = &env,
} else .{
.path = "/usr/bin/env",
.args = &.{ "/usr/bin/env", "sh", "-c", "echo $VALUE" },
.path = "/bin/sh",
.args = &.{ "/bin/sh", "-c", "echo $VALUE" },
.stdout = stdout,
.env = &env,
};

try cmd.start(testing.allocator);
try cmd.testingStart();
try testing.expect(cmd.pid != null);
const exit = try cmd.wait(true);
try testing.expect(exit == .Exited);
Expand Down Expand Up @@ -699,13 +699,13 @@ test "Command: custom working directory" {
.stdout = stdout,
.cwd = "C:\\Windows\\System32",
} else .{
.path = "/usr/bin/env",
.args = &.{ "/usr/bin/env", "sh", "-c", "pwd" },
.path = "/bin/sh",
.args = &.{ "/bin/sh", "-c", "pwd" },
.stdout = stdout,
.cwd = "/usr/bin",
.cwd = "/tmp",
};

try cmd.start(testing.allocator);
try cmd.testingStart();
try testing.expect(cmd.pid != null);
const exit = try cmd.wait(true);
try testing.expect(exit == .Exited);
Expand All @@ -718,7 +718,51 @@ test "Command: custom working directory" {

if (builtin.os.tag == .windows) {
try testing.expectEqualStrings("C:\\Windows\\System32\r\n", contents);
} else if (builtin.os.tag == .macos) {
try testing.expectEqualStrings("/private/tmp\n", contents);
} else {
try testing.expectEqualStrings("/usr/bin\n", contents);
try testing.expectEqualStrings("/tmp\n", contents);
}
}

// Test validate an execveZ failure correctly terminates when error.ExecFailedInChild is correctly handled
//
// Incorrectly handling an error.ExecFailedInChild results in a second copy of the test process running.
// Duplicating the test process leads to weird behavior
// zig build test will hang
// test binary created via -Demit-test-exe will run 2 copies of the test suite
test "Command: posix fork handles execveZ failure" {
if (builtin.os.tag == .windows) {
return error.SkipZigTest;
}
var td = try TempDir.init();
defer td.deinit();
var stdout = try createTestStdout(td.dir);
defer stdout.close();

var cmd: Command = .{
.path = "/not/a/binary",
.args = &.{ "/not/a/binary", "" },
.stdout = stdout,
.cwd = "/bin",
};

try cmd.testingStart();
try testing.expect(cmd.pid != null);
const exit = try cmd.wait(true);
try testing.expect(exit == .Exited);
try testing.expect(exit.Exited == 1);
}

// If cmd.start fails with error.ExecFailedInChild it's the _child_ process that is running. If it does not
// terminate in response to that error both the parent and child will continue as if they _are_ the test suite
// process.
fn testingStart(self: *Command) !void {
self.start(testing.allocator) catch |err| {
if (err == error.ExecFailedInChild) {
// I am a child process, I must not get confused and continue running the rest of the test suite.
posix.exit(1);
}
return err;
};
}

0 comments on commit 65a1c0d

Please sign in to comment.