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

child_process + Build: rename exec to run + all related code #17623

Merged
merged 1 commit into from
Oct 22, 2023

Conversation

matu3ba
Copy link
Contributor

@matu3ba matu3ba commented 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 #5853.

@squeek502
Copy link
Collaborator

Personally, I don't think this should affect --test-no-exec and instances of verbs like 'execute` in comments, etc.

@matu3ba matu3ba marked this pull request as draft October 20, 2023 07:22
@matu3ba
Copy link
Contributor Author

matu3ba commented Oct 20, 2023

Personally, I don't think this should affect --test-no-exec and instances of verbs like 'execute` in comments, etc.

~~I do agree with comments, but disagree insofar as process spawn comments and help page in src/main.zig are not consistent, for example there is update-and-run in the repl:

-        // Here we ignore the CC environment variable and exec `cc` as a child process.
+        // Here we ignore the CC environment variable and run `cc` as a child process.
-    \\  --test-no-exec                 Compiles test binary without running it
+    \\  --test-no-run                  Compiles test binary without running it
-    \\            run  Execute the output file, if it is an executable or test.
+    \\            run  Run the output file, if it is an executable or test.
-        fatal("the following command cannot be executed ({s} does not support spawning a child process):\n{s}", .{ @tagName(builtin.os.tag), cmd });
+        fatal("the following command cannot be run ({s} does not support spawning a child process):\n{s}", .{ @tagName(builtin.os.tag), cmd });
 const repl_help =
     \\Commands:
     \\         update  Detect changes to source files and update output files.
-    \\            run  Execute the output file, if it is an executable or test.
+    \\            run  Run the output file, if it is an executable or test.
     \\ update-and-run  Perform an `update` followed by `run`.
     \\           help  Print this text
     \\           exit  Quit this repl
  1. From my point of view, we should either change the repl commands to update-and-exec or change things complete to run within src/main.zig. What do you think about this?
  2. How is exec in the repl a different thing than doing it in the test runner? If not, then why not unify to make users needing to remember less?

I did ask for clarification in the discord how those two things should be aligned to make things consistent, which is pending an answer.

@squeek502
Copy link
Collaborator

squeek502 commented Oct 20, 2023

IMO confusion only arises when specifically dealing with a function named exec, as it's fairly reasonable to assume a function named exec might work like the libc function exec.

However, the verb execute and by extension the shortened version exec isn't inherently confusing. --test-no-exec makes sense with or without any context about how the libc function exec works, or what the underlying ChildProcess function is named.

@matu3ba matu3ba force-pushed the mv_exec_run2 branch 2 times, most recently from 08e249f to dc16bc4 Compare October 20, 2023 21:28
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 matu3ba marked this pull request as ready for review October 20, 2023 21:29
@rohlem
Copy link
Contributor

rohlem commented Oct 22, 2023

Just my 2c, but I think I prefer the verb execute to the verb run in this context.
In my mind execute is connected with an external actor (here parent process) triggering the execution of task,
whereas run sounds like the activity a subject (here child process) is performing by itself.

I.e., if a child process called a function ChildProcess.run for itself, I would think that might be by design,
whereas If a child process called a function ChildProcess.exec(ute) for itself, I'd read the documentation to make sure it's not spawning another child process (as is the intention behind the current API).

That said, it's probably highly subjective and I'll gladly let the maintainers decide.

@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. labels Oct 22, 2023
@andrewrk andrewrk merged commit fd2239b into ziglang:master Oct 22, 2023
10 checks passed
@matu3ba matu3ba deleted the mv_exec_run2 branch October 22, 2023 18:47
mitchellh added a commit to mitchellh/libxev that referenced this pull request Oct 24, 2023
Small updates for latest Zig updates:

- ziglang/zig#17623
- ziglang/zig#17392

Tested with `zig build install` and `zig build test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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