ocaml / ocamlbuild

The legacy OCamlbuild build manager
Other
122 stars 82 forks source link

Add a test case for custom preprocessor #334

Closed hhugo closed 5 months ago

hhugo commented 6 months ago

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 https://github.com/ocaml/ocamlbuild/issues/89

gasche commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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 !!!

going the other way

I still need to figure out some steps..

hhugo commented 5 months ago

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

hhugo commented 5 months ago

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 commented 5 months ago

Rebased