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.

Sign up for GitHub

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

Jump to bottom

Unix.open_process_args_*: look up program in PATH #10084

Merged
merged 9 commits into from
Dec 21, 2020
Merged

Unix.open_process_args_*: look up program in PATH #10084

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2244deb
Unix.open_process_args_*: look for program in PATH
nojb Dec 13, 2020
396b178
Cleanup
nojb Dec 13, 2020
46845e4
Doc
nojb Dec 14, 2020
228a0c2
Changes
nojb Dec 13, 2020
40ada47
Doc
nojb Dec 20, 2020
9cbbdc6
check-typo
nojb Dec 20, 2020
42e155e
Typo
nojb Dec 20, 2020
e145762
Typo in doc string
xavierleroy Dec 21, 2020
ed47664
Typo in doc string
xavierleroy Dec 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions Changes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ Working version

### Other libraries:

* #10084: Unix.open_process_args* functions now look up the program in the PATH.
This was already the case under Windows, but this is now also done under
Unix. Note that previously the program was interpreted relative to the current
directory.
(Nicolás Ojeda Bär, review by Gabriel Scherer and Xavier Leroy)

### Tools:

### Manual and documentation:
Expand Down
10 changes: 5 additions & 5 deletions otherlibs/unix/unix.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,7 +890,7 @@ let system cmd =
let pid = spawn shell [| shell; "-c"; cmd |] None false [| 0; 1; 2 |] in
snd(waitpid_non_intr pid)

let create_process_gen usepath cmd args optenv
let create_process_gen cmd args optenv
new_stdin new_stdout new_stderr =
let toclose = ref [] in
let close_after () =
Expand All @@ -917,13 +917,13 @@ let create_process_gen usepath cmd args optenv
(if new_stderr = 2 then 2 else file_descr_not_standard new_stderr)
|] in
Fun.protect ~finally:close_after
(fun () -> spawn cmd args optenv usepath redirections)
(fun () -> spawn cmd args optenv true (* usepath *) redirections)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if spawn is not exported, we should make usepath a labelled argument instead of using comments to alleviate the problems of non-labelled boolean arguments.

(Looking at the code, I see that spawn is an external. Can external have labels too?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Externals can have labeled and even optional arguments. There are examples elsewhere in this unix.ml file.

This said, spawn is not exported, and is called only twice in unix.ml, so it doesn't need to have a bulletproof interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'm not adding a label as it seems a bit overkill in this case...


let create_process cmd args new_stdin new_stdout new_stderr =
create_process_gen true cmd args None new_stdin new_stdout new_stderr
create_process_gen cmd args None new_stdin new_stdout new_stderr

let create_process_env cmd args env new_stdin new_stdout new_stderr =
create_process_gen true cmd args (Some env) new_stdin new_stdout new_stderr
create_process_gen cmd args (Some env) new_stdin new_stdout new_stderr

type popen_process =
Process of in_channel * out_channel
Expand All @@ -935,7 +935,7 @@ let popen_processes = (Hashtbl.create 7 : (popen_process, int) Hashtbl.t)

let open_proc prog args envopt proc input output error =
let pid =
create_process_gen false prog args envopt input output error in
create_process_gen prog args envopt input output error in
Hashtbl.add popen_processes proc pid

let open_process_args_in prog args =
Expand Down
3 changes: 3 additions & 0 deletions otherlibs/unix/unix.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,9 @@ val open_process_args_in : string -> string array -> in_channel
The standard output of the command is redirected to a pipe, which can be read
via the returned input channel.

The program is looked up in the [PATH] (this behaviour changed in 4.12;
previously the program was interpreted relative to the current directory).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere in unix.mli we write "The executable file is searched in the path". (4 occurrences.)

Also, you first talk about "the command" and then "the program". For me, "the command" is a shell command line, as used in open_process_in for instance. "The program" is better.

Also, "the program was interpreted relative to the current directory" is still true: the path is not searched for programs like ./foo or foo/bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I pushed revised docs for the open_process_args* functions to make them more uniform with the rest of the docstrings in this file (also added a line about the environment passed to the new process, which was previously missing). Let me know what you think!


@since 4.08.0 *)

val open_process_args_out : string -> string array -> out_channel
Expand Down
3 changes: 3 additions & 0 deletions otherlibs/unix/unixLabels.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,9 @@ val open_process_args_in : string -> string array -> in_channel
The standard output of the command is redirected to a pipe, which can be read
via the returned input channel.

The program is looked up in the [PATH] (this behaviour changed in 4.12;
previously the program was interpreted relative to the current directory).

@since 4.08.0 *)

val open_process_args_out : string -> string array -> out_channel
Expand Down