Fix staged pps on Windows

[nojb]

Fixes #10867

Dune passes a command line to the compiler (via the -ppx argument) which is then executed by the system shell, cmd.exe on Windows. However, it appears cmd.exe does not accept forward slashes as directory separator in the command part.

Here we fix this by replacing forward slashes by backward slashes in the command path.

[hhugo]

An alternative approach is to quote the path of the executable.

Also, you might want to further escape commands that are meant to run from "cmd.exe" (see the second code snippet below)

Here are two functions extracted from the ocamlbuild codebase

    let quote_filename_for_cmd_if_needed x =
      (* external commands are unfortunately called with cmd.exe (via Sys.command).
         Cmd.exe has strange quoting rules. The most notorious quirk is, that
         you can't use forward slashes as path separators at the first position,
         unless you quote the expression explicitly.
         cmd.exe will interpret the slash and everything thereafter as first
         parameter. Eg. 'bin/foo -x' is treated like 'bin /foo -x'. *)
      if Shell.is_simple_filename x && not (String.contains x '/')
      then x
      else My_std.quote_cmd ("\"" ^ x ^ "\"")
    in

(* See https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way *)
let quote_cmd s =
  let b = Buffer.create (String.length s + 20) in
  String.iter
    (fun c ->
       match c with
       | '(' | ')' | '!' | '^' | '%' | '\"' | '<' | '>' | '&' | '|' ->
         Buffer.add_char b '^'; Buffer.add_char b c
       | _ ->
         Buffer.add_char b c)
    s;
  Buffer.contents b

[hhugo]

It would be nice to introduce a test demonstrating the issue and its resolution.

[nojb]

An alternative approach is to quote the path of the executable.

Indeed. I tried it but the approach seemed to be more complicated ultimately. One difficulty is that there are several layers of quoting (Dune quotes, spawn, the library used by Dune to lunch external processes also adds its own quoting, etc). So any quoting done by Dune to the argument of -ppx needs to "survive" the later quoting, which is tricky.

It would be nice to introduce a test demonstrating the issue and its resolution.

Any test using staged_pps would demonstrate the issue; unfortunately, tests are not ran on Windows...

[nojb]

It would be nice to introduce a test demonstrating the issue and its resolution.

Any test using staged_pps would demonstrate the issue; unfortunately, tests are not ran on Windows...

Here is the output of one of the tests in the testsuite (slightly adapted) with the current patch (the failure at the end is expected):

$ ~/dune/_build/install/default/bin/dune build --verbose
Shared cache: enabled
Shared cache location:
C:\Users\nojebar\AppData\Local\Microsoft\Windows\INetCache\dune\db
Workspace root: "C:\\Users\\nojebar\\tmp\\test_ppx"
Auto-detected concurrency: 12
Dune context:
 { name = "default"
 ; kind = "default"
 ; profile = Dev
 ; merlin = true
 ; fdo_target_exe = None
 ; build_dir = In_build_dir "default"
 ; instrument_with = []
 }
Actual targets:
- alias @@default
Running[1]: (cd _build/default && C:\Users\nojebar\mlfi\mlfi-ins\bin\ocamlc.opt.exe -g -w -24 -I .ppx/a100acfd78285438214250364d95255a -I .driver1.objs/byte -I .dummy_rewriter.objs/byte -no-alias-deps -o .ppx/a100acfd78285438214250364d95255a/dune__exe___ppx.cmo -c -impl .ppx/a100acfd78285438214250364d95255a/_ppx.ml-gen)
Running[2]: (cd _build/default && C:\Users\nojebar\mlfi\mlfi-ins\bin\ocamlopt.opt.exe -g -w -24 -I .ppx/a100acfd78285438214250364d95255a -I .driver1.objs/byte -I .driver1.objs/native -I .dummy_rewriter.objs/byte -I .dummy_rewriter.objs/native -intf-suffix .ml-gen -no-alias-deps -o .ppx/a100acfd78285438214250364d95255a/dune__exe___ppx.cmx -c -impl .ppx/a100acfd78285438214250364d95255a/_ppx.ml-gen)
Running[3]: (cd _build/default && C:\Users\nojebar\mlfi\mlfi-ins\bin\ocamlopt.opt.exe -g -w -24 -o .ppx/a100acfd78285438214250364d95255a/ppx.exe driver1.cmxa dummy_rewriter.cmxa .ppx/a100acfd78285438214250364d95255a/dune__exe___ppx.cmx)
Running[4]: (cd _build/default && C:\Users\nojebar\mlfi\mlfi-ins\bin\ocamlc.opt.exe -w @1..3@5..28@30..39@43@46..47@49..57@61..62@67@69-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -ppx ".ppx\a100acfd78285438214250364d95255a\ppx.exe --as-ppx --cookie \"library-name=\\\"foo\\\"\"" -bin-annot -I .foo.objs/byte -no-alias-deps -opaque -o .foo.objs/byte/foo.cmo -c -impl foo.ml)
File ".foo.objs/byte/_unknown_", line 1, characters 0-0:
Command [4] exited with code 2:
$ (cd _build/default && C:\Users\nojebar\mlfi\mlfi-ins\bin\ocamlc.opt.exe -w @1..3@5..28@30..39@43@46..47@49..57@61..62@67@69-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -ppx ".ppx\a100acfd78285438214250364d95255a\ppx.exe --as-ppx --cookie \"library-name=\\\"foo\\\"\"" -bin-annot -I .foo.objs/byte -no-alias-deps -opaque -o .foo.objs/byte/foo.cmo -c -impl foo.ml)
Hello, staged ppx!
File "foo.ml", line 1:
Error: External preprocessor does not produce a valid file
Command line: .ppx\a100acfd78285438214250364d95255a\ppx.exe --as-ppx --cookie "library-name=\"foo\"" "C:\cygwin64\tmp\build_804ad9_dune\camlppx3e5640" "C:\cygwin64\tmp\build_804ad9_dune\camlppx1cb334"

[hhugo]

Any test using staged_pps would demonstrate the issue; unfortunately, tests are not ran on Windows...

https://github.com/ocaml/dune/actions/runs/10656796066/job/29535855125?pr=10869 seems to perform some testing on windows

[nojb]

Any test using staged_pps would demonstrate the issue; unfortunately, tests are not ran on Windows...

https://github.com/ocaml/dune/actions/runs/10656796066/job/29535855125?pr=10869 seems to perform some testing on windows

Well spotted; I added a staged_pps test to the set of tests run under Windows (only one before, now two :))

[hhugo]

So any quoting done by Dune to the argument of -ppx needs to "survive" the later quoting, which is tricky.

I would assume that dune does the correct escaping on Unix already. Maybe you only need to escape for "cmd.exe" ?

[nojb]

So any quoting done by Dune to the argument of -ppx needs to "survive" the later quoting, which is tricky.

I would assume that dune does the correct escaping on Unix already. Maybe you only need to escape for "cmd.exe" ?

Thanks for prodding me to investigate this further. It turns out that a similar issue affects the compiler: ocaml/ocaml#13415. Once that issue is fixed, one can fix the issue here simply by using Filename.quote_command. I will adapt the PR accordingly (using a workaround for the compiler issue).

[nojb]

I have pushed a revised version using quoting instead of replacing forward slashes by backslashes, which seems a more general solution.

Since the compiler version of Filename.quote_command is buggy (and does not exist for some of the versions of OCaml that Dune supports), I included a working version directly in stdune. It could be removed in the future (when the minimum supported OCaml version is >= 5.3).

The patch itself ends up being bigger, but it makes more sense.

[ejgallego]

Thanks so much folks, patch tested using opam pin dune with a default Windows Opam 2.2 switch and a full coq-lsp build, which IMVHO is a good stress test.

Version as of right now still fails with:

File "<command-line>", line 1, characters 8-9:
Error: String literal not terminated
File "serlib/ser_tactics.ml", line 1:
Error: Error while running external preprocessor
Command line: "".ppx/341932708445810a638020e478ebc4f1/ppx.exe" ^"--as-ppx^" ^"--cookie^" ^"library-name=\^"serlib\^"^"" "C:\Users\User\AppData\Local\Temp\build_a1f119_dune\camlppx2a3e02" "C:\Users\User\AppData\Local\Temp\build_a1f119_dune\camlppxa04776"

from _build/log:

$ (cd _build/default && "C:\Users\User\AppData\Local\opam\4.14.2\bin\ocamldep.opt.exe ^"-modules^" ^"-ppx^" ^"\^"\^".ppx/341932708445810a638020e478ebc4f1/ppx.exe\^" ^^\^"--as-ppx^^\^" ^^\^"--cookie^^\^" ^^\^"library-name=\^^\^"serlib\^^\^"^^\^"\^"^" ^"-impl^" ^"serlib/ser_tactics.ml^"") > _build/default/serlib/.serlib.objs/serlib__Ser_tactics.impl.d
> File "<command-line>", line 1, characters 8-9:
> Error: String literal not terminated
> File "serlib/ser_tactics.ml", line 1:
> Error: Error while running external preprocessor
> Command line: "".ppx/341932708445810a638020e478ebc4f1/ppx.exe" ^"--as-ppx^" ^"--cookie^" ^"library-name=\^"serlib\^"^"" "C:\Users\User\AppData\Local\Temp\build_a1f119_dune\camlppx2a3e02" "C:\Users\User\AppData\Local\Temp\build_a1f119_dune\camlppxa04776"
>

[ejgallego]

Managed to finish compiling coq-lsp using the first version of the patch, tested and it works fine!

So indeed, no other showstopper for native Opam 2.2 support that I could see.

[nojb]

Thanks @ejgallego for the confirmation that the first fix works. I was hoping the second one would work as well, but the number of nested quotations in the log makes my head hurt :) Anyway, I will try to see if I can figure out what is going on. Otherwise, maybe we will have to fall back to the first fix for now.

[nojb]

Discussion upstream ocaml/ocaml#13417 (comment) also points to another advantage of the "backslash" solution: it does not increase the total command length, whose limit is which is pretty tight on Windows. Accordingly, I am planning to revise the patch to go back to the first solution.

[hhugo]

Why not use String.map directly?

[nojb]

Fixed, thanks.

[hhugo]

prog is no longer quoted_for_shell. Is it expected?

[nojb]

No, it is wrong: fixed in 4d27a90

[nojb]

Thanks!

[nojb]

I'm happy with the fix in its current state. @hhugo: what do you think?

@ejgallego: if you could confirm once again that the current state works for the Coq build, that would be ideal.

[hhugo]

I'm happy with the fix in its current state. @hhugo: what do you think?

Looks fine but I'm still surprised that we don't need any escaping for cmd.exe.

[nojb]

I'm happy with the fix in its current state. @hhugo: what do you think?

Looks fine but I'm still surprised that we don't need any escaping for cmd.exe.

You mean for the backslashes? The program path component is still quoted with quote_for_shell (which is essentially Filename.quote) after switching the slashes.

[hhugo]

I m referring to the escaping done by

(* See https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way *)
let quote_cmd s =
  let b = Buffer.create (String.length s + 20) in
  String.iter
    (fun c ->
       match c with
       | '(' | ')' | '!' | '^' | '%' | '\"' | '<' | '>' | '&' | '|' ->
         Buffer.add_char b '^'; Buffer.add_char b c
       | _ ->
         Buffer.add_char b c)
    s;
  Buffer.contents b

[nojb]

I m referring to the escaping done by

This escaping would probably be needed if the command-line in question contained "enough" of these special metacharacters. In practice, they don't, so we don't see the need. We could try to implement this escaping regardless, but there are so many layers of (de)quoting (Dune -> spawn -> cmd.exe) that it becomes hard to know if the quoting works in all cases except in the cases actually used, where the current quoting works (see below). This is why I am not attempting to do this here. But in absolute terms, yes, there are some cases which are not handled correctly right now. I try to explain next why the current quoting works in the usual cases. A staged ppx is called by passing the following string to _wsystem, ie cmd.exe:

C:\> .ppx\a100acfd78285438214250364d95255a\ppx.exe --as-ppx --cookie "library-name=\"foo\"" "C:\cygwin64\tmp\foo" "C:\cygwin64\tmp\bar"

[Here we are using backslashes for the program path, as in this PR.] cmd.exe (as per this page) works as follows: it maps a command-line to a command-line (doing certain substitutions, etc) and then calls CreateProcess with it (there is no argv in native Windows world). Here the only characters that cause any issue are the double quotes.

Upon seeing a double quote, cmd.exe will copy it to its output command-line and and then copy everything that follows verbatim until the next double quote (this is explained in this page). This means that CreateProcess receives the same command-line. This can be verified using Process Monitor, a kind of strace for Windows:

Then CreateProcess splits this command-line into components following the rules explained in this page and calls the ppx binary. These components are:

C:\Users\nojebar\dune>child.exe --as-ppx --cookie "library-name=\"foo\"" "C:\cygwin64\tmp\foo" "C:\cygwin64\tmp\bar"
0: [child.exe]
1: [--as-ppx]
2: [--cookie]
3: [library-name="foo"]
4: [C:\cygwin64\tmp\foo]
5: [C:\cygwin64\tmp\bar]

which is correct (the right-hand side of the "cookie" library-name= should be a valid OCaml expression). [Here, child.exe is a tiny program that simply outputs the arguments received in Sys.argv.]

One case where this would break down would be if either the program path or the cookie value had embedded double quotes, but this is unusual.

[hhugo]

Can't the user pass arbitrary flags ? Can't that break the current logic?

[nojb]

Can't the user pass arbitrary flags ? Can't that break the current logic?

Yes, they will break the current logic if the flags contain things like embedded double quotes, etc. But that is somewhat orthogonal to the issue being treated here.

To be clear, I am not saying that issue shouldn't be fixed. What I am saying is that this PR fixes one issue, which affects every use of staged ppxs in Dune: the presence of forward slashes in the program path. One could consider this as "quoting special characters" issue, but it has a simple solution: replace them by backward slashes. This fix is simple and localized and also, it doesn't increase the command length, which is nice.

The more general issue of correctly quoting command-lines in all circumstances should be addressed separately, in my view, because: 1) it is less critical (it only triggers when flags containing certain unusual characters are used), and 2) more complicated to fix :)

Makes sense?

[hhugo]

Can't the user pass arbitrary flags ? Can't that break the current logic?

Yes, they will break the current logic if the flags contain things like embedded double quotes, etc. But that is somewhat orthogonal to the issue being treated here.

To be clear, I am not saying that issue shouldn't be fixed. What I am saying is that this PR fixes one issue, which affects every use of staged ppxs in Dune: the presence of forward slashes in the program path. One could consider this as "quoting special characters" issue, but it has a simple solution: replace them by backward slashes. This fix is simple and localized and also, it doesn't increase the command length, which is nice.

The more general issue of correctly quoting command-lines in all circumstances should be addressed separately, in my view, because: 1) it is less critical (it only triggers when flags containing certain unusual characters are used), and 2) more complicated to fix :)

Makes sense?

Sounds reasonable

[nojb]

Merged, thanks for the help @hhugo @ejgallego !

[ejgallego]

Thanks to you! Confirmed that dune's main branch is working properly.