test/standalone: reinstate std.ChildProcess tests #13042
Conversation
|
oh, didn't realize #11736 exists. |
|
failing error on freebsd from https://builds.hut.lavatech.top/~andrewrk/job/29111 is: which looks identical to #12258 because the second try, without a |
|
Does this do something that #11736 doesn't or is there some other reason you left this open? |
yes:
but either way, close this if there's no use for it here. |
There was a problem hiding this comment.
Thanks alot for your PR. This should unblock me in #12754 and revealed the fix for the other CI standalone tests:
For example execution the binary in test_stage3_debug can break, if test_stage3_release tries to overwrite it (after install step to zig-out), because both use the same repo instance with only different cache directories.
Consequently, a smoll hint provided that zig-out breaks on multiple (different) compiler invocations and what should be used instead for that use case sounds like a good idea.
Some feedback by me from a quick glimpse. Mostly nits except for the leak check.
| const main = b.addExecutable("main", "main.zig"); | ||
| main.setBuildMode(mode); | ||
| const run = main.run(); | ||
| run.addArtifactArg(child); |
| const mode = b.standardReleaseOptions(); | ||
|
|
||
| const child = b.addExecutable("child", "child.zig"); | ||
| child.setBuildMode(mode); |
There was a problem hiding this comment.
Do you expect miscompilations or why do you test here all compilation modes (debug, releasesafe, releasefast, releasesmall) ?
There was a problem hiding this comment.
i don't really expect any difference, but that's why we have tests, right? to potentially catch the unexpected :)
There was a problem hiding this comment.
I do see your point. From personal experience on working with https://github.com/matu3ba/reduze the CI costs are around 1-2 seconds per binary with LLVM, which means in total (build binary, debug, releasefast, releasesafe, releasesmall).
Unless we add a pile of these it utilizing LLVM it sounds fine to me, but I am not the one to decide this with the background that build.zig is rebuild each time in stage3 due to yet unresolved caching problems.
|
|
||
| pub fn main() !void { | ||
| var arena_state = std.heap.ArenaAllocator.init(std.heap.page_allocator); | ||
| const arena = arena_state.allocator(); |
There was a problem hiding this comment.
more ziggish is to use/copy-paste
var arena_state = std.heap.ArenaAllocator.init(std.heap.page_allocator);
defer arena_state.deinit();
const arena = arena_state.allocator();
...There was a problem hiding this comment.
there's a std.process.exit(exit_code) in the end. i don't believe defer will have a chance to run before process.exit?
There was a problem hiding this comment.
All defers run before the exit point of the current and all sub scopes. Thereafter the exit is run.
There was a problem hiding this comment.
$ cat > defer.zig
const std = @import("std");
pub fn main() !void {
std.debug.print("hello world", .{});
defer std.debug.print("hello from defer", .{});
std.process.exit(0);
}
$ zig run defer.zig
hello world
you can see above the defer'ed expression wasn't run due to process.exit terminating the program early.
There was a problem hiding this comment.
You are correct. My bad. defer will execute an expression at the end of the current scope. Mark as resolved, if you want.
| const std = @import("std"); | ||
|
|
||
| // 42 is expected by parent; other values result in test failure | ||
| var exit_code: u8 = 42; |
There was a problem hiding this comment.
s/exit_code/expected_exit_code
Then trim the comment abit.
There was a problem hiding this comment.
the "expected" part is actually in the parent. this is a child - it doesn't check against an expected value but simply exits with an exit code. feels like it would be more confusing with "expected_" prefix.
| pub fn main() !void { | ||
| var gpa_state = std.heap.GeneralPurposeAllocator(.{}){}; | ||
| defer if (gpa_state.deinit()) { | ||
| std.debug.print("found mem leaks", .{}); |
There was a problem hiding this comment.
This print does not set the exit code on memory leakage. You can either be lazy with std.debug.assert (only in debug/releaseSafe) or use @panic to enforce unsuccessful termination.
| return if (test_errors) error.ParentTestError else {}; | ||
| } | ||
|
|
||
| var test_errors = false; |
There was a problem hiding this comment.
This does not need to be in global scope and feels duplicated to ok_code.
There was a problem hiding this comment.
ok_code is for checking child exit code. test_errors indicates check failures in the parent. made some variable renaming to hopefully make it more readable.
|
|
||
| // test stdout pipe; parent verifies | ||
| const stdout = std.io.getStdOut().writer(); | ||
| try stdout.print("hello from stdout", .{}); |
There was a problem hiding this comment.
you can also use writeAll here.
|
|
||
| const hello_stdout = "hello from stdout"; | ||
| var buf: [hello_stdout.len]u8 = undefined; | ||
| try proc.stdout.?.reader().readNoEof(&buf); |
There was a problem hiding this comment.
This is a race condition, because you dont wait for the child process to finish up writing to its stdout. Unfortunately the FreeBSD error trace looks broken, so its hard to decipher.
There was a problem hiding this comment.
true. replaced with readAll.
|
Mhm, this still fails with I suspect that something is broken on hashing and thus You can reproduce this problem by building Zig for release and debug yourself and run this test with a certain time difference such that one build is executing before the other replaces the binary: mkdir -p build/ && cd build/ && cmake .. -DCMAKE_PREFIX_PATH="$HOME/dev/git/bootstrap/zig-bootstrap/musl/out/host/" -GNinja && /usr/bin/time -v ${HOME}/dev/git/cpp/mold/build/mold -run ninja install && cd ..
mkdir -p buildrel/ && cd buildrel/ && cmake .. -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_PREFIX_PATH="$HOME/dev/git/bootstrap/zig-bootstrap/musl/out/host/" -GNinja && /usr/bin/time -v ${HOME}/dev/git/cpp/mold/build/mold -run ninja install && cd ..
|
interesting! i'll try to debug this later. thanks. |
There was a problem hiding this comment.
I have no idea what is wrong with FreeBSD on this. The pipe looks unidirectional (bidirectional ones are not supported) and the call is blocking (non-blocking mode is not supported like on Linux).
You can check, if the FIFO is opened with the correct permissions (O_RDONLY set, O_WRONLY and O_NONBLOCK not set) by adding the assertions for the OS.
The arena allocator does not leak checks (testing that all subfunctions free the memory), so please change that to the gpa which does leak checks.
| const mode = b.standardReleaseOptions(); | ||
|
|
||
| const child = b.addExecutable("child", "child.zig"); | ||
| child.setBuildMode(mode); |
There was a problem hiding this comment.
I do see your point. From personal experience on working with https://github.com/matu3ba/reduze the CI costs are around 1-2 seconds per binary with LLVM, which means in total (build binary, debug, releasefast, releasesafe, releasesmall).
Unless we add a pile of these it utilizing LLVM it sounds fine to me, but I am not the one to decide this with the background that build.zig is rebuild each time in stage3 due to yet unresolved caching problems.
|
|
||
| pub fn main() !void { | ||
| var arena_state = std.heap.ArenaAllocator.init(std.heap.page_allocator); | ||
| const arena = arena_state.allocator(); |
There was a problem hiding this comment.
You are correct. My bad. defer will execute an expression at the end of the current scope. Mark as resolved, if you want.
| var exit_code: u8 = 42; | ||
|
|
||
| pub fn main() !void { | ||
| var arena_state = std.heap.ArenaAllocator.init(std.heap.page_allocator); |
There was a problem hiding this comment.
I missed this one, but the arena allocator does not leak checks in subfunctions. Can you change it to the gpa like in my PR ?
var general_purpose_allocator = std.heap.GeneralPurposeAllocator(.{}){};
defer std.debug.assert(!general_purpose_allocator.deinit());
const gpa = general_purpose_allocator.allocator();
67d5bfe removed std.ChildProcess tests, suggesting to make them standalone instead. this commit does exactly that.
andrewrk
force-pushed
the
test-childproc
branch
from
285724b
to
9b2d3c4
Compare
|
CI checks failing and the PR is stale. Please open a fresh PR if you want to continue these efforts. |
|
CI failure is reproducible with building debug and release and then execute both in parallel Single execution of either succeeds. Strace only the file actions with showed:
Debug has Release has Comparing the strace diff to release without the problem (started alone) shows there is 1 more file descriptor, if the problem occurs, but I am not entirely sure where to look for. |
|
If I use #!/bin/sh
# ZIGCACHE=$(fd -uu zig-cache) && rm -fr $ZIGCACHE && fd -uu zig-cache # uncomment to reproduce error
strace -ff -yy ./build/stage3/bin/zig build -Dtest-filter=test/standalone/child_process/build.zig test-standalone &> debug.strace & strace -ff -yy ./buildrel/stage3/bin/zig build -Dtest-filter=test/standalone/child_process/build.zig test-standalone &> release.straceI get a SIGPIPE instead?. See also my question in the discord forum: https://discord.com/channels/605571803288698900/1019652020308824145/threads/1037196394420768768 Not sure, if EBADF was shadowing SIGPIPE, if the behavior deviates (between compiler versions) or if I missed it somehow. |
|
Looks like the bug only happens on ReleaseFast: #!/bin/sh
set -e
/usr/bin/time -v "${HOME}/dev/git/bootstrap/zig-bootstrap/musl/out/zig-x86_64-linux-musl-native/bin/zig" build -p build1 --search-prefix "${HOME}/dev/git/bootstrap/zig-bootstrap/musl/out/x86_64-linux-musl-native" --zig-lib-dir lib -Dstatic-llvm &
/usr/bin/time -v "${HOME}/dev/git/bootstrap/zig-bootstrap/musl/out/zig-x86_64-linux-musl-native/bin/zig" build -p build2 -Drelease --search-prefix "${HOME}/dev/git/bootstrap/zig-bootstrap/musl/out/x86_64-linux-musl-native" --zig-lib-dir lib -Dstatic-llvm
echo "build releasesafe"
mkdir -p build3/ && cd build3/ && cmake .. -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_PREFIX_PATH="$HOME/dev/git/bootstrap/zig-bootstrap/musl/out/host/" -GNinja && /usr/bin/time -v ${HOME}/dev/git/cpp/mold/build/mold -run ninja install && cd ..
echo "build minsize"
mkdir -p build4/ && cd build4/ && cmake .. -DCMAKE_BUILD_TYPE=MinSizeRel -DCMAKE_PREFIX_PATH="$HOME/dev/git/bootstrap/zig-bootstrap/musl/out/host/" -GNinja && /usr/bin/time -v ${HOME}/dev/git/cpp/mold/build/mold -run ninja install && cd ..
echo "building done"Now uncommenting the first fails and the other succeed: # ./build2/bin/zig build -Dtest-filter=test/standalone/child_process/build.zig test-standalone
# ./build1/bin/zig build -Dtest-filter=test/standalone/child_process/build.zig test-standalone
# ./build3/stage3/bin/zig build -Dtest-filter=test/standalone/child_process/build.zig test-standalone
# ./build4/stage3/bin/zig build -Dtest-filter=test/standalone/child_process/build.zig test-standalone |
67d5bfe removed std.ChildProcess tests, suggesting to make them standalone instead. this commit does exactly that.