Skip to content

Build: unable to run zig build test inside a test #15104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
perillo opened this issue Mar 28, 2023 · 9 comments
Closed

Build: unable to run zig build test inside a test #15104

perillo opened this issue Mar 28, 2023 · 9 comments
Labels
bug Observed behavior contradicts documented or intended behavior zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@perillo
Copy link
Contributor

perillo commented Mar 28, 2023

Zig Version

0.11.0-dev.2297+28d6dd75a

Steps to Reproduce and Observed Behavior

build.zig

const std = @import("std");

pub fn build(b: *std.Build) void {
    const step = b.step("test-bug", "Test bug");
    const tmp_path = b.makeTempPath();

    const init_lib = b.addSystemCommand(&.{ b.zig_exe, "init-lib" });
    init_lib.cwd = tmp_path;
    init_lib.setName("zig init-lib");

    const sed = b.addSystemCommand(&.{ "sh", "-c", "sed -i 's/main_tests.step/main_tests.run().step/' build.zig" });
    sed.cwd = tmp_path;
    sed.step.dependOn(&init_lib.step);

    const run_test = b.addSystemCommand(&.{ b.zig_exe, "build", "test" });
    run_test.cwd = tmp_path;
    run_test.setName("zig build test");
    run_test.expectStdOutEqual("");
    run_test.step.dependOn(&sed.step);

    const cleanup = b.addRemoveDirTree(tmp_path);
    cleanup.step.dependOn(&run_test.step);

    step.dependOn(&cleanup.step);
}

Run test

$ zig build test-bug
info: Created build.zig
info: Created src/main.zig
info: Next, try `zig build --help` or `zig build test`
zig build test: error: run test: error: unable to spawn /home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache/tmp/a443d6d1b38a60b5/zig-cache/o/eb475aa61ecabe0e827b11a7a86daaf7/test: NotDir
Build Summary: 1/3 steps succeeded; 1 failed (disable with -fno-summary)
test transitive failure
└─ run test failure
   └─ zig test Debug native success 1s MaxRSS:149M
error: the following build command failed with exit code 1:
/home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache/tmp/a443d6d1b38a60b5/zig-cache/o/95242fcff26bb153af8b3e87a6c401fc/build /home/manlio/.local/share/sdk/zig/master/zig /home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache/tmp/a443d6d1b38a60b5 /home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache/tmp/a443d6d1b38a60b5/zig-cache /home/manlio/.cache/zig test

zig build test: error: the following command exited with code 1 (expected exited with code 0):
cd /home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache/tmp/a443d6d1b38a60b5 && /home/manlio/.local/share/sdk/zig/master/zig build test 
Build Summary: 2/5 steps succeeded; 1 failed (disable with -fno-summary)
test-bug transitive failure
└─ RemoveDir /home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache/tmp/a443d6d1b38a60b5 transitive failure
   └─ zig build test failure
      └─ run sh success 4ms MaxRSS:3M
         └─ zig init-lib success 3ms MaxRSS:31M
error: the following build command failed with exit code 1:
/home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache/o/667f53d524dfc3f8a2d1e7289dc1d9c3/build /home/manlio/.local/share/sdk/zig/master/zig /home/manlio/src/zig/src/test.localhost/zig-issues/15009 /home/manlio/src/zig/src/test.localhost/zig-issues/15009/zig-cache /home/manlio/.cache/zig test-bug

Expected Behavior

No error.

Note that the cache directory containing the test executable exists, after the zig build test-bug command terminates.
Also note that running zig build test-bug again will result in a success. The bug only happen when the cache is empty.

See also #15064.

@chrboesch
Copy link
Contributor

Error found and PR created.

@Vexu Vexu added this to the 0.11.0 milestone Mar 29, 2023
@Vexu Vexu added the zig build system std.Build, the build runner, `zig build` subcommand, package management label Mar 29, 2023
@perillo
Copy link
Contributor Author

perillo commented Mar 30, 2023

I found the actual error.
The inner build_runner has stdin close, so when opening a directory for the build_root, the OS assigns the fd 0.

Here is an example reproducing the problem:

const std = @import("std");
const debug = std.debug;
const fs = std.fs;
const os = std.os;

pub fn main() !void {
    os.close(0);

    var handle = try std.fs.cwd().makeOpenPath("tmp", .{});
    debug.print("handle: {}\n", .{handle});
}

@perillo
Copy link
Contributor Author

perillo commented Mar 30, 2023

@andrewrk, do you known why build_root_directory.handle.fd is 0 in build_runner.zig?

@perillo
Copy link
Contributor Author

perillo commented Mar 30, 2023

I updated the build_runner.zig file to open /dev/null/ at the start of the main function, and this fixed the issue. So the problem is having file descriptor 0 for cwd_dir. I confirmed that cwd_dir.?.handle is not invalid, by using os.dup.

Another change that fixes the issue is to set, in std/Build/RunStep.spawnChildAndCollect, child.stdin_behavior to always be .Inherit for .infer_from_args.

@chrboesch
Copy link
Contributor

chrboesch commented Mar 30, 2023

What confuses me is this section in fs.zig

    fn openDirFlagsZ(self: Dir, sub_path_c: [*:0]const u8, flags: u32) OpenError!Dir {
        const result = if (need_async_thread)
            std.event.Loop.instance.?.openatZ(self.fd, sub_path_c, flags, 0)
        else
            os.openatZ(self.fd, sub_path_c, flags, 0);
        const fd = result catch |err| switch (err) {
            error.FileTooBig => unreachable, // can't happen for directories
            error.IsDir => unreachable, // we're providing O.DIRECTORY
            error.NoSpaceLeft => unreachable, // not providing O.CREAT
            error.PathAlreadyExists => unreachable, // not providing O.CREAT
            error.FileLocksNotSupported => unreachable, // locking folders is not supported
            error.WouldBlock => unreachable, // can't happen for directories
            error.FileBusy => unreachable, // can't happen for directories
            else => |e| return e,
        };
        return Dir{ .fd = fd };
    }

The file descriptor is set via the first parameter here: openatZ(self.fd, But returned is the value from the return of the function const fd = result -> return Dir{ .fd = fd };
I would like to understand why it is done that way. Because the return value is 0 (= no error) and this is taken as file descriptor.

@perillo
Copy link
Contributor Author

perillo commented Mar 30, 2023

What confuses me is this section in fs.zig

    fn openDirFlagsZ(self: Dir, sub_path_c: [*:0]const u8, flags: u32) OpenError!Dir {
        const result = if (need_async_thread)
            std.event.Loop.instance.?.openatZ(self.fd, sub_path_c, flags, 0)
        else
            os.openatZ(self.fd, sub_path_c, flags, 0);
        const fd = result catch |err| switch (err) {
            error.FileTooBig => unreachable, // can't happen for directories
            error.IsDir => unreachable, // we're providing O.DIRECTORY
            error.NoSpaceLeft => unreachable, // not providing O.CREAT
            error.PathAlreadyExists => unreachable, // not providing O.CREAT
            error.FileLocksNotSupported => unreachable, // locking folders is not supported
            error.WouldBlock => unreachable, // can't happen for directories
            error.FileBusy => unreachable, // can't happen for directories
            else => |e| return e,
        };
        return Dir{ .fd = fd };
    }

The file descriptor is set via the first parameter here: openatZ(self.fd, But returned is the value from the return of the function const fd = result -> return Dir{ .fd = fd }; I would like to understand why it is done that way. Because the return value is 0 (= no error) and this is taken as file descriptor.

Note that the function called is openat, that requires dirfd.

@chrboesch
Copy link
Contributor

Note that the function called is openat, that requires dirfd.

Ah ok, I was going in the wrong direction.

perillo added a commit to perillo/zig that referenced this issue Mar 31, 2023
When running, as an example, a `zig build test` command in a build file,
what happens is the following:

  1. The builder_runner is executed with stdin closed, resulting in
     cwd_dir.fd to be 0 in std.Build.RunStep.
  2. The test is executed with stdin set to a pipe, so that spawn will
     fail, since cwd_dir.fs is now not a directory.

Add a new is_test_command field to RunStep; it must be set to true when
RunStep runs a zig test command.

Update the spawnChildAndCollect function, so that when running a zig
test command, stdin_behavior is set to .Ignore.

Fixes ziglang#15104
perillo added a commit to perillo/zig that referenced this issue Mar 31, 2023
After commit ede5dcf (make the build runner and test runner talk to each other),
the std.Build.addTest function no longer runs tests, but the build.zig
files in init-exe and init-lib where not updated.

Rename main_tests to lib_tests and "Run library tests" to "Run unit tests",
for consistency with init-exe.

Use the new Build.addRunArtifact function, instead of the deprecated
CompileStep.run method.

Add a RunStep to exe_tests and lib_tests.

In the addCliTests function in tests/test.zig:
  - set the new RunStep.is_test_command field to true, when running the
    `zig build test` command.  See ziglang#15104.

  - Remove an empty line in the addCliTests function in tests/tests.zig.

Closes ziglang#15009
@perillo
Copy link
Contributor Author

perillo commented Mar 31, 2023

Here is a simplified example showing the cause of the problem:

const std = @import("std");
const debug = std.debug;
const fs = std.fs;
const mem = std.mem;
const process = std.process;

const ChildProcess = std.ChildProcess;

pub fn main() !void {
    var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator);
    defer arena.deinit();

    const allocator = arena.allocator();

    const self_exe = try fs.selfExePathAlloc(allocator);
    const argv = try process.argsAlloc(allocator);

    if (argv.len == 1) {
        // parent: spawn child process with stdin closed.
        var child = std.process.Child.init(&.{ self_exe, "child" }, allocator);
        child.stdin_behavior = .Close;

        _ = try child.spawnAndWait();

        return;
    }

    const role: []const u8 = argv[1];
    if (mem.eql(u8, role, "child")) {
        // child: spawn grandchild process with cwd_dir set to a new open
        // directory and stdin set to a pipe.
        var cwd_dir = try fs.cwd().makeOpenPath("tmp", .{});
        debug.assert(cwd_dir.fd == 0);

        var child = std.process.Child.init(&.{ self_exe, "grandchild" }, allocator);
        child.stdin_behavior = .Pipe;
        child.cwd_dir = cwd_dir;

        // spawn will change cwd_dir fd to a pipe, so spawning grandchild will
        // fail with NotDir.
        _ = try child.spawnAndWait();

        return;
    } else if (mem.eql(u8, role, "grandchild")) {
        // grandchild: never executed.

        unreachable;
    }

    unreachable;
}
$ zig run artificial-example.zig
error: NotDir
/home/manlio/.local/share/sdk/zig/master/lib/std/child_process.zig:368:9: 0x214576 in waitPosix (artificial-example)
        return self.term.?;
        ^
/home/manlio/.local/share/sdk/zig/master/lib/std/child_process.zig:247:13: 0x2145f6 in wait (artificial-example)
            try self.waitPosix();
            ^
/home/manlio/.local/share/sdk/zig/master/lib/std/child_process.zig:209:9: 0x20f959 in spawnAndWait (artificial-example)
        return self.wait();
        ^
/home/manlio/src/zig/src/test.localhost/zig-issues/15009/artificial-example.zig:45:13: 0x20f5d7 in main (artificial-example)
        _ = try child.spawnAndWait();
            ^

perillo added a commit to perillo/zig that referenced this issue Mar 31, 2023
After commit ede5dcf (make the build runner and test runner talk to each other),
the std.Build.addTest function no longer runs tests, but the build.zig
files in init-exe and init-lib where not updated.

Rename main_tests to lib_tests and "Run library tests" to "Run unit tests",
for consistency with init-exe.

Use the new Build.addRunArtifact function, instead of the deprecated
CompileStep.run method.

Add a RunStep to exe_tests and lib_tests.

In the addCliTests function in tests/test.zig:
  - set the new RunStep.is_test_command field to true, when running the
    `zig build test` command.  See ziglang#15104.

  - Remove an empty line in the addCliTests function in tests/tests.zig.

Closes ziglang#15009
perillo added a commit to perillo/zig that referenced this issue Apr 3, 2023
After commit ede5dcf (make the build runner and test runner talk to each other),
the std.Build.addTest function no longer runs tests, but the build.zig
files in init-exe and init-lib where not updated.

Rename main_tests to lib_tests and "Run library tests" to "Run unit tests",
for consistency with init-exe.

Use the new Build.addRunArtifact function, instead of the deprecated
CompileStep.run method.

Add a RunStep to exe_tests and lib_tests.

In the addCliTests function in tests/test.zig:
  - set the new RunStep.is_test_command field to true, when running the
    `zig build test` command.  See ziglang#15104.

  - Remove an empty line in the addCliTests function in tests/tests.zig.

Closes ziglang#15009
perillo added a commit to perillo/zig that referenced this issue Apr 6, 2023
Currently, these tests work because of issue ziglang#15059, but ziglang#15059 requires
issue ziglang#15104 to be fixed, and it is taking a lot of time.

Temporarily disable the init-lib and init-exe tests in
`test/tests.addCliTests`.
@andrewrk
Copy link
Member

Fix landed in 1728d92.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants