Avoid depending on child process execution when not supported by host OS #10782

topolarity · 2022-02-03T23:28:05Z

In accordance with the requesting issue (#10750):

zig test skips any tests that it cannot spawn, returning success
zig run and zig build exit with failure, reporting the command the cannot be run
zig clang, zig ar, etc. already punt directly to the appropriate clang/lld main(), even before this change
Native libc Detection is not supported

Additionally, exec() and related Builder functions error at run-time, reporting the command that cannot be run

matu3ba

just a nit on can_spawn else looking good.

Did you check what is still missing?
Any of translate-c, ar, dlltool, lib, ranlib ?

matu3ba · 2022-02-03T23:54:06Z

lib/std/process.zig

@@ -936,6 +936,12 @@ pub const can_execv = switch (builtin.os.tag) {
    else => true,
 };

+/// Tells whether spawning child processes is supported (e.g. via ChildProcess)
+pub const can_spawn = switch (builtin.os.tag) {


This will likely need to be extended to encompass other stuff, so add a TODO for it.

Do you mean that can_spawn will still need to be updated to be correct for other targets? If so, more than happy to add the TODO

Prefer to make the TODO in a github issue rather than a comment, please :-)

andrewrk

Thanks! Left you some review comments.

andrewrk · 2022-02-04T01:04:31Z

lib/std/child_process.zig

            return self.spawnPosix();
+        } else {
+            unreachable;


I think this spawn function should be untouched, or if you want to make the compile error clearer, put at the top of it:

if (!std.process.can_spawn) { @compileError("the target operating system cannot spawn processes"); }

andrewrk · 2022-02-04T01:05:28Z

lib/std/process.zig

@@ -936,6 +936,12 @@ pub const can_execv = switch (builtin.os.tag) {
    else => true,
 };

+/// Tells whether spawning child processes is supported (e.g. via ChildProcess)
+pub const can_spawn = switch (builtin.os.tag) {


Prefer to make the TODO in a github issue rather than a comment, please :-)

andrewrk · 2022-02-04T01:07:31Z

build.zig

@@ -229,6 +229,11 @@ pub fn build(b: *Builder) !void {
    const version = if (opt_version_string) |version| version else v: {
        const version_string = b.fmt("{d}.{d}.{d}", .{ zig_version.major, zig_version.minor, zig_version.patch });

+        // If we can't spawn a process for git, just trust the version string


if we can't spawn a process for git, emit a fatal error here saying that it must be provided with -Dversion-string.

src/Compilation.zig

src/main.zig

andrewrk · 2022-02-04T01:12:37Z

src/main.zig

@@ -4081,7 +4093,7 @@ extern "c" fn ZigClang_main(argc: c_int, argv: [*:null]?[*:0]u8) c_int;
 extern "c" fn ZigLlvmAr_main(argc: c_int, argv: [*:null]?[*:0]u8) c_int;

 /// TODO https://github.com/ziglang/zig/issues/3257
-fn punt_to_clang(arena: Allocator, args: []const []const u8) error{OutOfMemory} {
+pub fn punt_to_clang(arena: Allocator, args: []const []const u8) error{OutOfMemory} {


I don't think these should be made public or used outside this file

topolarity · 2022-02-04T03:00:59Z

Thanks for the review, folks!

Did you check what is still missing? Any of translate-c, ar, dlltool, lib, ranlib ?

These are already covered without this change 👍 Zig calls directly into ZigLlvmAr_main

As for what's left: I thought I had covered all of the usage of ChildProcess with my most recent commit, but @andrewrk's request to make referencing spawn() a compile-time error (probably a good idea, I agree) is still triggering on an unknown reference I haven't been able to suss out yet.

I'll dive in deeper tomorrow and see if I can eliminate the lingering references to ChildProcess.spawn, and address the rest of your comments.

andrewrk · 2022-02-04T03:29:11Z

I'll dive in deeper tomorrow and see if I can eliminate the lingering references to ChildProcess.spawn, and address the rest of your comments.

This is a deficiency of zig compile errors; would be nice if it could explain how this dependency is being dragged in. Someday, we'll have it. For now, a workaround is to temporarily delete the spawn function from the std lib and then it tends to generate more helpful errors.

topolarity · 2022-02-05T02:34:00Z

This is a deficiency of zig compile errors; would be nice if it could explain how this dependency is being dragged in. Someday, we'll have it. For now, a workaround is to temporarily delete the spawn function from the std lib and then it tends to generate more helpful errors.

This tip was a huge help; ended up being short work to track things down.

I believe I've addressed all the feedback in the latest changes now. Let me know if anything still needs work 👍

zigazeljko · 2022-02-05T11:18:10Z

Why is argvCmd needed? Doesn't std.mem.join do exactly the same?

topolarity · 2022-02-05T16:10:52Z

Why is argvCmd needed? Doesn't std.mem.join do exactly the same?

Nice catch. I updated the PR to use std.mem.join

In accordance with the requesting issue (ziglang#10750): - `zig test` skips any tests that it cannot spawn, returning success - `zig run` and `zig build` exit with failure, reporting the command the cannot be run - `zig clang`, `zig ar`, etc. already punt directly to the appropriate clang/lld main(), even before this change - Native `libc` Detection is not supported Additionally, `exec()` and related Builder functions error at run-time, reporting the command that cannot be run

and adjust the warning message for invoking LLD twice in the same process.

andrewrk · 2022-02-07T05:31:20Z

Excellent work. Congrats on landing your first Zig PR 🎉

matu3ba reviewed Feb 4, 2022

View reviewed changes

andrewrk requested changes Feb 4, 2022

View reviewed changes

topolarity force-pushed the gate-child-processes branch from 665a9c5 to b4056b7 Compare February 5, 2022 02:22

topolarity requested a review from andrewrk February 5, 2022 02:23

topolarity force-pushed the gate-child-processes branch 2 times, most recently from 3909724 to 3773f5e Compare February 5, 2022 02:30

topolarity and others added 3 commits February 6, 2022 22:21

Replace argvCmd with std.mem.join

c1cf158

stage2: pass proper can_exit_early value to LLD

33fa296

and adjust the warning message for invoking LLD twice in the same process.

andrewrk force-pushed the gate-child-processes branch from d174dee to 33fa296 Compare February 7, 2022 05:31

andrewrk merged commit 2113538 into ziglang:master Feb 7, 2022

topolarity deleted the gate-child-processes branch February 7, 2022 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid depending on child process execution when not supported by host OS #10782

Avoid depending on child process execution when not supported by host OS #10782

topolarity commented Feb 3, 2022

matu3ba left a comment

matu3ba Feb 3, 2022

topolarity Feb 4, 2022

andrewrk Feb 4, 2022

andrewrk left a comment

andrewrk Feb 4, 2022

andrewrk Feb 4, 2022

andrewrk Feb 4, 2022

andrewrk Feb 4, 2022

topolarity commented Feb 4, 2022

andrewrk commented Feb 4, 2022

topolarity commented Feb 5, 2022

zigazeljko commented Feb 5, 2022

topolarity commented Feb 5, 2022

andrewrk commented Feb 7, 2022

Avoid depending on child process execution when not supported by host OS #10782

Avoid depending on child process execution when not supported by host OS #10782

Conversation

topolarity commented Feb 3, 2022

matu3ba left a comment

Choose a reason for hiding this comment

matu3ba Feb 3, 2022

Choose a reason for hiding this comment

topolarity Feb 4, 2022

Choose a reason for hiding this comment

andrewrk Feb 4, 2022

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

andrewrk Feb 4, 2022

Choose a reason for hiding this comment

andrewrk Feb 4, 2022

Choose a reason for hiding this comment

andrewrk Feb 4, 2022

Choose a reason for hiding this comment

andrewrk Feb 4, 2022

Choose a reason for hiding this comment

topolarity commented Feb 4, 2022

andrewrk commented Feb 4, 2022

topolarity commented Feb 5, 2022

zigazeljko commented Feb 5, 2022

topolarity commented Feb 5, 2022

andrewrk commented Feb 7, 2022