Skip to content
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

rename std.ChildProcess.exec to avoid ambiguity with posix exec which replaces current image with new process #5853

Closed
tysonclugg opened this issue Jul 12, 2020 · 7 comments · Fixed by #17623
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@tysonclugg
Copy link

The std.ChildProcess.exec function doesn't follow the expected behaviour of replacing the executable with the new program being exec'ed. See the SO question "Differences between fork and exec" for details on how fork and exec should behave.

Currently, std.ChildProcess.exec creates a std.ChildProcess instance and then calls its spawn method - returning an ExecResult struct:

pub fn exec(args: struct {
allocator: *mem.Allocator,
argv: []const []const u8,
cwd: ?[]const u8 = null,
cwd_dir: ?fs.Dir = null,
env_map: ?*const BufMap = null,
max_output_bytes: usize = 50 * 1024,
expand_arg0: Arg0Expand = .no_expand,
}) !ExecResult {
const child = try ChildProcess.init(args.argv, args.allocator);
defer child.deinit();
child.stdin_behavior = .Ignore;
child.stdout_behavior = .Pipe;
child.stderr_behavior = .Pipe;
child.cwd = args.cwd;
child.cwd_dir = args.cwd_dir;
child.env_map = args.env_map;
child.expand_arg0 = args.expand_arg0;
try child.spawn();

I expect that std.ChildProcess.exec should not fork or spawn, and it should have a !noreturn result (perhaps ExecError!noreturn with ExecError being much the same as a SpawnError).

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Jul 12, 2020
@Vexu Vexu added this to the 0.7.0 milestone Jul 12, 2020
@tgschultz
Copy link
Contributor

However, coming from a Windows environment, this is not the behavior I expect. Should std.ChildProcess.exec be expected to follow POSIX definitions?

Perhaps instead of making it noreturn, it can simply be renamed to ChildProcess.spawn or ChildProcess.create to avoid confusion.

@daurnimator
Copy link
Collaborator

Yeah it might be best if we avoid exec at all.

Give that the module is ChildProcess, I don't think any "replaceCurrentProcess" doesn't make sense and belongs elsewhere (process.zig?).

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Oct 17, 2020
@andrewrk andrewrk changed the title std.ChildProcess.exec calls spawn (fork/exec) instead of exec rename std.ChildProcess.exec to avoid ambiguity with posix exec which replaces current image with new process Oct 17, 2020
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 17, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@andrewrk andrewrk added the accepted This proposal is planned. label Nov 23, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.11.0 Nov 23, 2021
@nektro
Copy link
Contributor

nektro commented Dec 30, 2021

any thoughts on run? doing some reworking in the above PR

@tysonclugg
Copy link
Author

Why not follow POSIX conventions?

Even in Windows, some form of POSIX has been available for the last 29 years (since Windows NT 3.1).

I didn't ask for exec to be renamed.

@tysonclugg
Copy link
Author

Regarding the POSIX question, I see a number of possible outcomes:

  1. Zig closely follows the C/C++ precedent and provides a clean mapping of zig methods to various syscalls, including those defined by POSIX. Zig provides an exec method that closely follows the POSIX spec (as per the intent of the issue as it was raised).
  2. Zig specifically opts out from trying to provide wrappers for any syscalls such as execve. You would need to use libc methods such as exec to get access to the low-level syscalls.
  3. Something between 1 & 2, which means every time a question like this pops up an ad-hoc decision will need to be made. Zig continues to provide methods like std.ChildProcess.exec that don't follow established conventions, and people coming from C/C++ backgrounds (ie: the primary target audience) keep getting tripped up because of the differences.
  4. Something else, yet to be proposed/decided.

To be clear, I'm absolutely fine with 1 and 2. I'm very much against 3, and obviously can't have an opinion on 4 until a proposal is made.

@tysonclugg
Copy link
Author

If std.ChildProcess is considered a "helper" library of abstract methods (option 4) and not a clean mapping of syscalls (option 1), then renaming exec to something else to avoid the issues of option 3 makes more sense. Perhaps run might be a good name in that case, but it should be very clear from both the method names, the documentation, and the website (where any C/C++ comparisons are made) that these abstract methods are not 1-1 replacements of the C/C++ equivalents.

@matu3ba
Copy link
Contributor

matu3ba commented Jan 3, 2023

then renaming exec to something else to avoid the issues of option 3 makes more sense

True, run is also used by Python and its nice and short. Rust uses Command::new, which I find not nice.
I'll create a PR.

matu3ba added a commit to matu3ba/zig that referenced this issue Jan 4, 2023
matu3ba added a commit to matu3ba/zig that referenced this issue Jan 4, 2023
Do no touch non-Zig code. Adjust error names as well, if associated.
Justification: exec is a unix concept, the portable version should be
called differently.

Closes ziglang#5853.
matu3ba added a commit to matu3ba/zig that referenced this issue Jan 4, 2023
Justification: exec, execv etc are unix concepts and portable version
should be called differently.

Do no touch non-Zig code. Adjust error names as well, if associated.
Closes ziglang#5853.
matu3ba added a commit to matu3ba/zig that referenced this issue Oct 19, 2023
Justification: exec, execv etc are unix concepts and portable version
should be called differently.

Do no touch non-Zig code. Adjust error names as well, if associated.
Closes ziglang#5853.
matu3ba added a commit to matu3ba/zig that referenced this issue Oct 19, 2023
Justification: exec, execv etc are unix concepts and portable version
should be called differently.

Do no touch non-Zig code. Adjust error names as well, if associated.
Closes ziglang#5853.
matu3ba added a commit to matu3ba/zig that referenced this issue Oct 20, 2023
Justification: exec, execv etc are unix concepts and portable version
should be called differently.

Do no touch non-Zig code. Adjust error names as well, if associated.
Closes ziglang#5853.
matu3ba added a commit to matu3ba/zig that referenced this issue Oct 20, 2023
Justification: exec, execv etc are unix concepts and portable version
should be called differently.

Do no touch non-Zig code. Adjust error names as well, if associated.
Closes ziglang#5853.
matu3ba added a commit to matu3ba/zig that referenced this issue Oct 20, 2023
Justification: exec, execv etc are unix concepts and portable version
should be called differently.

Do no touch non-Zig code. Adjust error names as well, if associated.
Closes ziglang#5853.
@andrewrk andrewrk modified the milestones: 0.15.0, 0.12.0 Oct 22, 2023
@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Oct 22, 2023
andrewrk pushed a commit that referenced this issue Oct 22, 2023
Justification: exec, execv etc are unix concepts and portable version
should be called differently.

Do no touch non-Zig code. Adjust error names as well, if associated.
Closes #5853.
krichprollsch added a commit to lightpanda-io/zig-v8-fork that referenced this issue Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
7 participants