|Anonymous | Login | Signup for a new account||2017-09-23 00:24 CEST|
|Main | My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0006903||OCaml||platform support (windows, cross-compilation, etc)||public||2015-06-14 11:55||2017-02-14 13:55|
|Target Version||4.03.1+dev||Fixed in Version||4.05.0 +dev/beta1/beta2/beta3/rc1|
|Summary||0006903: Unix.execvpe doesn't change environment on Cygwin and maybe Windows|
|Description||The following reproduces the issue:|
let env = Array.concat [ [| "LAPIN=lapin" |] ; Unix.environment () ]
let () =
match fork () with
| 0 -> execvpe "env" [| "env" |] env
| x -> ()
Run inside cygwin and pipe the output to 'grep lapin'. You should see nothing (unless you already had that environment variable defined).
The code in otherlibs/unix/execvp.c looks fine but does nothing. However, Unix.execve works. Both bindings are very old: execvpe in 1996 and execve in 1995 (this one, untouched since then).
The code for Unix.execvpe plays with the environ variable rather than calling execvpe(); I don't know why and this dates back to 1996: "Renommage unix.h -> unixsupport.h Petites adaptations pour Win32." ( ac9f8e8094ea19199a24fd5c67053718198ed172 / rev 951 ). One possibility is that is execvpe wasn't available back then on Windows.
As for why this doesn't work, if I remember correctly, Windows has two environments: one for ansi, one for unicode and this would require getters and setters rather than direct access to the variable.
I'm attaching a patch. I've only tested it on Cygwin and it fixes the testcase above.
I've also been a bit surprised by the following in otherslibs/threads/unix.ml (indent mine):
let execve proc args env = do_exec (fun () -> _execve proc args env)
let execvp proc args = do_exec (fun () -> _execvp proc args)
let execvpe proc args = do_exec (fun () -> _execvpe proc args)
I guess that not specifying the last argument for execvpe works because of partial application but it's a bit surprising to see it written that way.
|Tags||No tags attached.|
|Attached Files||ocaml_execvpe.patch [^] (840 bytes) 2015-06-14 11:55 [Show Content]|
Could you also add the failing case to the testsuite?
(A new directory tests/lib-unix would seem fitting)
> Windows has two environments
On a related note, see also 0004499: libc functions don't see changes caused by native Windows calls (SetEnvironmentVariable).
Attached patch might cause build to fail on Mac OS X.
This may be related to http://caml.inria.fr/mantis/view.php?id=5693 [^]
It was actually me asking nbraud over IRC to comment here because I wasn't home and was worried I would forget.
I just checked and, from "man execvpe" on Linux:
The execvpe() function is a GNU extension.
This has just gotten me very very sad because I believe manipulating environ in a threaded environment is racy. Well, forking in a threaded environment is racy but it's a good idea troubles as much as possible.
Right, execvpe() is not POSIX, just a GNU extension. Could you please revise your patch accordingly? At a minimum, it should be
// your new code using execvpe()
// the old implementation that modifies "environ"
Alternatively, you could implement execvpe in terms of execve() (which is POSIX), using caml_search_exe_in_path (from the OCaml runtime system) to find the actual location of the executable.
NB: execvpe() is also available on Windows.
Musl-libc is MIT-licensed and has an implementation of execvpe on top of execve which can be seen at http://git.musl-libc.org/cgit/musl/tree/src/process/execvp.c [^] .
Would it be OK to re-use that for platforms without execvpe() and to call execvpe() directly otherwise?
I don't know if I'll be able to do something myself before the next release and I don't want to "lock" the update of the patch to myself. In any case I'll do my best to review and test any patch on the topic.
|No, it's not OK to reuse the Musl code, because 1- it would probably not work under Windows, and 2- the OCaml runtime already contains functions to search executables in PATH, see "caml/osdeps.h".|
|Realistically I won't have time to push this forward in time for the next release: I'd like to write and test a couple patches to see and compare them but I'm too short on time for this during January and I think this issue can wait a bit more if needed.|
Fixed by commit [trunk 3627221], should be in 4.05.
|2015-06-14 11:55||adrien||New Issue|
|2015-06-14 11:55||adrien||File Added: ocaml_execvpe.patch|
|2015-06-14 12:15||gasche||Note Added: 0014067|
|2015-06-15 10:27||frisch||Relationship added||related to 0004499|
|2015-06-15 10:28||frisch||Note Added: 0014069|
|2015-06-15 17:58||doligez||Status||new => acknowledged|
|2015-06-15 17:58||doligez||Target Version||=> 4.03.0+dev / +beta1|
|2015-08-01 15:43||nbraud||Note Added: 0014312|
|2015-08-03 13:11||adrien||Note Added: 0014314|
|2015-11-28 12:36||xleroy||Note Added: 0014886|
|2015-11-28 12:36||xleroy||Status||acknowledged => feedback|
|2015-12-03 21:30||adrien||Note Added: 0015020|
|2015-12-03 21:30||adrien||Status||feedback => new|
|2015-12-04 16:09||xleroy||Note Added: 0015035|
|2016-01-11 07:39||adrien||Note Added: 0015248|
|2016-01-22 17:20||doligez||Status||new => confirmed|
|2016-01-22 17:20||doligez||Target Version||4.03.0+dev / +beta1 => 4.03.1+dev|
|2017-02-14 13:55||xleroy||Note Added: 0017254|
|2017-02-14 13:55||xleroy||Status||confirmed => resolved|
|2017-02-14 13:55||xleroy||Resolution||open => fixed|
|2017-02-14 13:55||xleroy||Fixed in Version||=> 4.05.0 +dev/beta1/beta2/beta3/rc1|
|2017-02-23 16:46||doligez||Category||OCaml windows => platform support (windows, etc)|
|2017-02-23 17:16||doligez||Category||platform support (windows, etc) => platform support (windows, cross-compilation, etc)|
|Copyright © 2000 - 2011 MantisBT Group|