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

nojb · 2020-12-13T11:30:43Z

Fixes #10083, see the discussion in #10057, especially #10057 (comment) and #10057 (comment)

gasche · 2020-12-13T12:07:13Z

otherlibs/unix/unix.mli

@@ -879,7 +879,7 @@ val open_process_args_in : string -> string array -> in_channel
   command to run, and the second argument specifies the argument array passed
   to the command.  This function runs the command in parallel with the program.
   The standard output of the command is redirected to a pipe, which can be read
-   via the returned input channel.
+   via the returned input channel. The program is looked up in the [PATH].


I think we would need a @since marker for the PATH lookup, or at least a clear indication in the documentation that older versions of OCaml may not do it.

I added a clarification about this to the docstring, but without using a @since marker since it wasn't clear to me how to specify more than one such marker.

gasche · 2020-12-13T12:09:20Z

otherlibs/unix/unix.ml

@@ -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)


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?)

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.

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

frou · 2020-12-13T12:31:51Z

Changes

@@ -26,6 +26,11 @@ Working version

 ### Other libraries:

+- #10084: Unix.open_process_args_* functions now look up the program in the PATH


The glob only matches 3 of the 4 functions because of the trailing underscore 😈

This is a breaking change, strictly speaking, so it should be listed with a * instead of a -.

Why is this breaking? Assume the current directory contains an executable named foo.exe, and PATH does not contain ., like it should. Unix.open_process_args "foo.exe" ... succeeds with the current implementation and fails with the new implementation.

Good point, I amended the Changes entry and the docstring for the function.

gasche

I am in favor of the change, which improves compatibility between the different platforms and seems more consistent with what our users actually want.

gasche · 2020-12-14T10:08:45Z

Changes

@@ -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 previusly the program was interpreted relative to the current


Suggested change

Unix. Note that previusly the program was interpreted relative to the current

Unix. Note that previously the program was interpreted relative to the current

Thanks, fixed.

gasche · 2020-12-14T10:09:40Z

otherlibs/unix/unix.mli

@@ -879,7 +879,9 @@ val open_process_args_in : string -> string array -> in_channel
   command to run, and the second argument specifies the argument array passed
   to the command.  This function runs the command in parallel with the program.
   The standard output of the command is redirected to a pipe, which can be read
-   via the returned input channel.
+   via the returned input channel. The program is looked up in the [PATH] (this


Could you break a line before this new sentence, so that the behavior changes catches the eye?

gasche

This is an approval stamp.

Before we merge, do we need to do anything specific to the fact that we are proposing a change to the sdlib that is "a bugfix" or "a breaking change" according to who you ask?

I was wondering if we needed more opinions on this; let me cc the @ocaml/caml-devel group, in case people want to chime in. If there is no negative feedback for about a week, please feel free to remind me to merge.

nojb · 2020-12-14T10:26:57Z

I was wondering if we needed more opinions on this; let me cc the @ocaml/caml-devel group, in case people want to chime in. If there is no negative feedback for about a week, please feel free to remind me to merge.

Sounds good, thanks!

xavierleroy

I have zero problems with the change of semantics. Yes, technically it is a breaking change, so that's why it must be starred in the change log. But the previous behavior was an oversight, and the only way to do something useful with the previous behavior was to use absolute paths /bin/ls, in which case the new behavior is the same. So, I don't think we have an issue here.

Nonetheless I have hairs to split about the documentation strings, see below.

xavierleroy · 2020-12-14T16:14:51Z

otherlibs/unix/unix.mli

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


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.

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!

xavierleroy

The new documentation is just fine (modulo a typo that I fixed by a direct push), thank you very much!

I'm so happy that I'll merge right now.

The `Unix.open_process_args*` used to treat their first arguments as a file name for the executable to be run, like `execv` does. This commit ensures that the first argument is searched in the executable path, like `execvp` does. This is a more useful behavior. Closes: ocaml#10083

nojb added 2 commits December 13, 2020 12:25

Unix.open_process_args_*: look for program in PATH

2244deb

Cleanup

396b178

gasche reviewed Dec 13, 2020

View reviewed changes

frou reviewed Dec 13, 2020

View reviewed changes

nojb force-pushed the unix_args_path branch 3 times, most recently from b1bb24c to 4fdb4ac Compare December 14, 2020 10:03

gasche approved these changes Dec 14, 2020

View reviewed changes

nojb added 2 commits December 14, 2020 11:14

Doc

46845e4

Changes

228a0c2

nojb force-pushed the unix_args_path branch from 4fdb4ac to 228a0c2 Compare December 14, 2020 10:14

gasche approved these changes Dec 14, 2020

View reviewed changes

xavierleroy reviewed Dec 14, 2020

View reviewed changes

Doc

40ada47

nojb force-pushed the unix_args_path branch from f77966f to 40ada47 Compare December 20, 2020 23:50

nojb and others added 4 commits December 21, 2020 00:55

check-typo

9cbbdc6

Typo

42e155e

Typo in doc string

e145762

Typo in doc string

ed47664

xavierleroy approved these changes Dec 21, 2020

View reviewed changes

xavierleroy merged commit b9172d4 into ocaml:trunk Dec 21, 2020

rbjorklin mentioned this pull request Mar 21, 2021

docs: clarification for Unix.open_process_args_in #10057

Closed

lpw25 mentioned this pull request Sep 3, 2021

Port #10084: Unix.open_process_args* functions now look up the program in the PATH ocaml-flambda/flambda-backend#214

Open

lukemaurer mentioned this pull request May 6, 2022

Port #10084: Unix.open_process_args*: Look up program in PATH ocaml-flambda/flambda-backend#645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nojb commented Dec 13, 2020

gasche Dec 13, 2020 •

edited

nojb Dec 13, 2020

gasche Dec 13, 2020

xavierleroy Dec 13, 2020

nojb Dec 13, 2020

frou Dec 13, 2020

xavierleroy Dec 13, 2020

nojb Dec 13, 2020

gasche left a comment

gasche Dec 14, 2020

nojb Dec 14, 2020

gasche Dec 14, 2020

nojb Dec 14, 2020

gasche left a comment

nojb commented Dec 14, 2020

xavierleroy left a comment

xavierleroy Dec 14, 2020

nojb Dec 20, 2020

xavierleroy left a comment

		@@ -26,6 +26,11 @@ Working version

		### Other libraries:

		- #10084: Unix.open_process_args_* functions now look up the program in the PATH

	Unix. Note that previusly the program was interpreted relative to the current
	Unix. Note that previously the program was interpreted relative to the current

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

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

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

Conversation

nojb commented Dec 13, 2020

gasche Dec 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasche left a comment

Choose a reason for hiding this comment

nojb commented Dec 14, 2020

xavierleroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierleroy left a comment

Choose a reason for hiding this comment

gasche Dec 13, 2020 •

edited