No longer call into cmd.exe to execute a posix shell on windows #339
Conversation
There was a problem hiding this comment.
Couple of suggestions, but this looks very sensible. I'd definitely recommend the cygcheck trick - if ocamlbuild is run from cmd/powershell then by default bash is often going to be WSL, so it may be a good fallback ocamlbuild just to fail at that point (rather than run WSL's bash and the results be very surprising).
When ocamlbuild runs within an opam package (which is where it should be running 99% of the time), everything'll work as expected.
| let rec iter = function | ||
| | [] -> [| "bash.exe" ; "--norc" ; "--noprofile" |] | ||
| | hd::tl -> | ||
| let dash = Filename.concat hd "dash.exe" in |
There was a problem hiding this comment.
A possible stronger trick for this, to avoid calling either WSL or the bash which gets exposed by Git-for-Windows (e.g. in Scoop, or when selecting the not-recommended "make all the utilities available" option).
First of all attempt to resolve cygcheck.exe. If that resolves, look for the shells in that directory (note that both MSYS2 and Cygwin have a cygcheck binary).
There was a problem hiding this comment.
Are you suggesting
- to only do the cygcheck trick and fail if not found or
- to fallback to the current logic ?
There was a problem hiding this comment.
I implemented 2. for now
hhugo
requested review from
dra27,
gasche and
kit-ty-kate
and removed request for
dra27
Co-authored-by: David Allsopp <[email protected]>
src/my_std.ml
Outdated
| @@ -275,13 +275,80 @@ let sys_file_exists x = | |||
| try Array.iter (fun x -> if x = basename then raise Exit) a; false | |||
| with Exit -> true | |||
|
|
|||
| (* https://github.com/ocaml/opam/blame/master/src/core/opamStd.ml *) | |||
There was a problem hiding this comment.
(Luckily opam uses the same license so reusing code as-is is just fine.)
@hhugo could you maybe tweak your URL to contain a hash of a recent commit, instead of master? This would be helpful if someone in the future wants to now whether our local copy should be updated.
(I would have considered moving the copied-almost-exactly-as-is code to a separate file, to make such future tracking easier, but oh well.)
There was a problem hiding this comment.
I've updated the comment
|
There was some logic elsewhere to read the PATH (that was not handling quoted path). |
src/ocamlbuild_executor.ml
Outdated
| if Sys.win32 | ||
| then | ||
| let args = My_std.prepare_command_for_windows cmd in | ||
| open_process_args_full args.(0) args (Unix.environment ()) |
There was a problem hiding this comment.
Why are you calling Unix.environment () again here, what is the expected difference with just env?
There was a problem hiding this comment.
no reason, I've changed it back.
based on top of #338
Inspired by #330.
Sys.command,Unix.open_process_in,Unix.open_process_fullall go throughcmd.exeto execute the given command.In our case, it means we call cmd.exe to call bash.
This is unnecessary, and complicate things. We can directly call a posix shell and not have to worry about another layer of escaping.
In this PR, we use
open_process_args*funtions to avoid going throughcmd.exe