Add a test case for custom preprocessor

[hhugo]

This use case was found in camlp4 and is likely broken on windows. Let's try to fix.

This PR adds a special handling for A '-pp'; Quote q. The command q will be executed by cmd.exe in ocamlc/ocamldep/.. and needs to be escaped as such.

Based on #338 and #339

Should fix #89

[gasche]

CI results are in: this works on Windows under OCaml 4.x, and fails on Windows under OCaml 5.x. So this may be an upstream-OCaml issue, but I haven't tried to investigate and will happily leave this to @hhugo.

[hhugo]

Yet I can't install camlp4 on my machine on ocaml 4.14.1. I need to investigate, The CI failure on OCaml 5 could be due to missing parches in the sunset repo..

[hhugo]

For the record, here is the error I get locally trying to install camlp4.

/config/Camlp4_config.ml
+ '''C:\Users\hugoh\AppData\Local\opam\default\bin/ocamlc.opt' -I +dynlink dynlink.cma -g -I camlp4/config -I camlp4/boot camlp4/config/Camlp4_import.cmo camlp4/config/Camlp4_config.cmo camlp4/boot/Camlp4.cmo camlp4/boot/camlp4boot.cmo -o camlp4/boot/camlp4boot.byte
+ '''C:\Users\hugoh\AppData\Local\opam\default\bin/ocamldep.opt' -pp ''\'''\''camlp4\\\\boot\\\\camlp4boot.byte -D OPT' -modules camlp4/Camlp4/Debug.mli > camlp4/Camlp4/Debug.mli.depends
+ bash --norc -c "'''C:\Users\hugoh\AppData\Local\opam\default\bin/ocamldep.opt' -pp ''\'''\''camlp4\\\\boot\\\\camlp4boot.byte -D OPT' -modules camlp4/Camlp4/Debug.mli > camlp4/Camlp4/Debug.mli.depends"
The system cannot find the path specified.
File "camlp4/Camlp4/Debug.mli", line 1:
Error: Error while running external preprocessor
Command line: ''camlp4\\boot\\camlp4boot.byte -D OPT "camlp4/Camlp4/Debug.mli" > C:\cygwin64\tmp\ocamlpp68ba5a

I found the following piece of code in camlp4 myocamlbuil that I don't really understand but I'd be nice to address it. Maybe @dra27 can help ?

    let hot_camlp4boot_dep, hot_camlp4boot_cmd =
      if use_external_camlp4boot then
        (None, "camlp4boot")
      else
        let exe =
          "camlp4boot" ^
          if C.ocamlnat then
            (* If we are using a native plugin, we might as well use a native
               preprocessor. *)
            ".native"
          else
            ".byte"
        in
        let dep = "camlp4"/"boot"/exe in
        let cmd =
          let ( / ) = Filename.concat in
          (*
           * Workaround ocamlbuild problems on Windows by double-escaping.
           * On systems using forward-slash, the calls to String.escaped will be
           * no-ops anyway and the code will continue to work even once ocamlbuild
           * correctly escapes output (the issue is trying to escape output for both cmd
           * and bash)
           *)
          String.escaped (String.escaped ("camlp4"/"boot"/exe))
        in
        (Some dep, cmd)
    in
    let hot_camlp4boot_cmd =
      if not windows then
        hot_camlp4boot_cmd
      else
        let buf = Buffer.create (String.length hot_camlp4boot_cmd + 5) in
        for i = 0 to String.length hot_camlp4boot_cmd - 1 do
          match hot_camlp4boot_cmd.[i] with
          | '/' ->
            Buffer.add_string buf "\\";
          | x -> Buffer.add_char buf x;
        done;
        Buffer.contents buf          
    in

[hhugo]

The patch against the compiler gives some information about what's going on.
https://gist.githubusercontent.com/fdopen/2def708d5eb7c8764c7b0804587a57dd/raw/2897d6a477dce97b88cdf8633355a99400d0e083/ocaml-4.14.0.patch

(* external commands are unfortunately called 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'.
 * Because the most used build tools are unix-centric (ocamlbuild, gmake)
 * and are not aware of it, such errors are quite common, especially when
 * calling the preprocessor. ( ocamlc -pp 'subdir/exe' ... )
 *
 * Therefore, I replace every slash inside the first subexpression with a
 * backslash.
 * I think this replacement is safe. No OCaml developer will write 'bin/foo',
 * if he wants to call a executable 'bin' with parameter /foo.
 *)

[gasche]

This makes sense, the failing command in the testsuite does contain a forward slash:

''ocamldep.opt -pp '''./preprocessor.byte 2' -modules main.ml > main.ml.depends

On the other hand, the comment you quote mentions "unless you quote it explicitly", and it looks like an attempt at quoting was made here, there are in fact three quotes right after the -pp. I don't understand the quoting rules at play, it looks like ''foo.exe is acceptable in Windows-land, but I would expect -pp "''foo.exe" rather than `-pp '''foo.exe. Is the latter also correct? If you have a Windows machine, can you find out which quoting would work correctly here?

[hhugo]

This makes sense, the failing command in the testsuite does contain a forward slash:

''ocamldep.opt -pp '''./preprocessor.byte 2' -modules main.ml > main.ml.depends

On the other hand, the comment you quote mentions "unless you quote it explicitly", and it looks like an attempt at quoting was made here, there are in fact three quotes right after the -pp. I don't understand the quoting rules at play, it looks like ''foo.exe is acceptable in Windows-land, but I would expect -pp "''foo.exe" rather than `-pp '''foo.exe. Is the latter also correct? If you have a Windows machine, can you find out which quoting would work correctly here?

After digging a bit, I wish I never did !!!

• We call bash -norc -c "ocamlc -pp 'preprocessor..'" which goes through cmd.exe \c
• bash execute what's left from ocamlc -pp 'preprocessor..' with bash escaping/quoting
• ocamlc execute what's left of preprocessor.. with cmd.exe

going the other way

• we want to execute "./preprocessor.exe" args in cmd
• we have to call Sys.command with ^"./preprocessor.exe^" args
• from bash we can call ocamlc.opt -verbose -dsource -pp '^"./preprocessor.exe^" args' main.ml
• finally, calling bash from cmd, we get bash --norc --verbose -c ^"ocamlc.opt -dsource -pp '^^^\^"./preprocessor.exe^^^\^" args' main.ml"

I still need to figure out some steps..

[hhugo]

https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

[hhugo]

I believe this PR is now ready for review.

We're now checking in the CI that camlp4 can be installed with the current ocamlbuild and we're running the external.ml tests that depend on camlp4 and menhir.

[hhugo]

Rebased

[gasche]

It feels very fishy to me to special-case -pp for special quoting rule. Why not apply this logic to all Quote arguments?

[hhugo]

Should we change the definition of Quote ?
It's currently

| Quote of spec  (** A string that should be quoted like a
                      filename but isn't really one. *)

[gasche]

My understanding is that Quote is in fact used for arguments that are themselves commands or prefixes of command, meant to be passed to the shell at some point. We could indeed change the (fairly unclear) documentation to reflect this.

[hhugo]

done

[gasche]

It is odd that we have to do this here. Should the function that runs post_cmd be calling My_std.sys_command instead?

[hhugo]

The testsuite doesn't currently have access to ocamlbuild the lib. Should I make the change ?

[gasche]

I'm okay with keeping this file separate from the ocamlbuild logic, as an external driver, but I would rather have the Windows-testing logic in ocamlbuild_test.ml rather than in each caller of the test function.

[hhugo]

done

[gasche]

The logic here is fairly complex and also not clearly correct: if q is to be interpreted as a command, then we need different quoting rules for the first argument of q, which is not necessarily an argument on which the encode function will be called. So encode will run on a follow-up argument and the logic does not match the intent.

An alternative proposal would be as follows: on Windows, ensure that is_simple_filename fails on filenames that contain a /. Does that suffice to prevent the issue that you are guarding against here? (I would be happy to keep your nice, descriptive comment that explains why we need to protect certain occurrences of /, which could move to Shell if we do it this way.)

[hhugo]

I don't like moving this to Shell because it should only apply inside cmd.
I've change the logic to apply no only on the first encoded element.

[hhugo]

Remember that windows has more constraint on command line size.