Fix hack for bash when computing commands.

[hhugo]

We have the following hack in place when computing commands on win32

let string_of_command_spec_with_calls call_with_tags call_with_target resolve_virtuals spec =
...
...
  (* The best way to prevent bash from switching to its windows-style
   * quote-handling is to prepend an empty string before the command name. *)
  if Sys.win32 then
    Buffer.add_string b "''";

The current implementation also emit the empty string whenever it processes a Quote spec, this PR fixes this and only emit the empty string at the very beginning of the command.

The diff is clearer if you ignore white-spaces

[gasche]

The previous behavior was intentional (this was the only use of the self recursive function), how did you determine that we should get rid of it now? It looks like Quote str is used to represent option arguments that are themselves commands, in particular for the --ocamlc parameter of Menhir.

[hhugo]

Commands inside Quote, such as --ocamlc for menhir or --pp for ocamlc/ocamldep, (usually) run inside the cmd.exe shell on windows (not bash) using Sys.command or Unix.open_process{,_in,_out,_full}. cmd.exe doesn't like the leading empty string.

[gasche]

Now that this quoting is never done recursively, we would have the option to remove it from string_of_command_spec entirely and push it to its caller. Looking at the code, it looks like this function is fed to sys_command (which would be a reasonable place to put the quoting given that it is the one that performs the bash call on Windows). Do you have a sense of whether the callers of sys_command that do not come from string_of_command_spec would benefit from this same quoting, or if keeping it only in string_of_command_spec is the right move?

[hhugo]

thinking about, it probably makes more sense to move the logic next to the bash invocation but I would rather do this change in #339.

[hhugo]

Also, I search a bit but could not find information about the behavior we're trying to avoid with this hack. @dra27 do you know anything about it ?

[hhugo]

thinking about, it probably makes more sense to move the logic next to the bash invocation but I would rather do this change in #339.

The best place to put it would be https://github.com/ocaml/ocamlbuild/pull/339/files#diff-35ab9d070d2ae7c0fc3cf5506afdbcf99b86a4f8b2ef2b50c94903d11297c0f0R330

let prepare_command_for_windows cmd =
  Array.append (Lazy.force windows_shell) [|"-c"; "''" ^ cmd|]

[dra27]

IIRC this is all to do with how Cygwin applies quoting based on whether the caller is a Cygwin executable or a native Windows executable (as ocamlbuild is here). We have the same thing in opam, but solved very differently. However, it'd be far from trivial I expect to bring that all over from opam (the function cygwin_create_process_env implements the considerable logic to pass an arbitrary array of arguments from a native Windows process to a Cygwin process but then create_process_env itself has to use a lot of other logic to be determine if the executable being called is a Cygwin executable).

When calling bash, I think this is definitely needed, therefore. On the basis that ocamlbuild has only been being used with these patches, it's probably better not to be shifting the logic too much without being able to "mass test" the existing packages which are using it?

[gasche]

Is the present patch a subset of what's in the fdopen repo? I wasn't aware, and if so that does alleviate fears of regression.

[hhugo]

I've moved the ''trick in https://github.com/ocaml/ocamlbuild/pull/339/commits/daf5ff4ea2d4ed584d4e4f83d0d69247281b9de6

[hhugo]

Is the present patch a subset of what's in the fdopen repo? I wasn't aware, and if so that does alleviate fears of regression.

I'm guessing that David was talking about the '' trick already present in the codebase.

The current patch was not part of fdopen repo as far as I know.

[gasche]

You haven't rebased the present PR with your new patch, I think. But I find the result simpler and I would be in favor of merging if the CI agrees.

(I don't mind if we take some reasonable Windows-only risks in the upstream version, given the fact that upstream is already known-broken in many respects on Windows, it's a matter of improving and testing and being reactive in integrating your proposed improvements.)

[hhugo]

You haven't rebased the present PR with your new patch, I think. But I find the result simpler and I would be in favor of merging if the CI agrees.

The patch is in #339 which sits on top of the current PR

[gasche]

Okay. I understand what is going here in the present PR, and propose to go ahead and merge, but I haven't looked at #339 yet.