Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[regression] ppx broken on Windows in 4.9 (was working on 4.8) #1672

Closed
nojb opened this issue Sep 8, 2023 · 3 comments
Closed

[regression] ppx broken on Windows in 4.9 (was working on 4.8) #1672

nojb opened this issue Sep 8, 2023 · 3 comments

Comments

@nojb
Copy link
Contributor

nojb commented Sep 8, 2023

image
Could it be related to #1585?

@nojb
Copy link
Contributor Author

nojb commented Sep 8, 2023

Could it be related to #1585?

I confirm this is the culprit. The root cause is that Filename.quote_command is given a string that it cannot handle and it fails.

Filename.quote_command prog args

The backtrace in my case is:

failed with Failure("Filename.quote_command: bad file name C:\\cygwin64\\home\\nojebar\\mlfi\\_build/default/.ppx/878efc8ece20bcbf2dd8c437a0746ae0/ppx.exe --as-ppx --cookie \"library-name=\\\"js_core\\\"\"")
# 0.03 Typing_aux.raise_error - Failure
Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
Called from Stdlib__Filename.Win32.quote_command in file "filename.ml", line 234, characters 6-28
Called from Merlin_kernel__Pparse.commandline in file "merlin/src/ocaml/driver/pparse.ml" (inlined), line 42, characters 2-34
Called from Merlin_kernel__Pparse.apply_rewriter in file "merlin/src/ocaml/driver/pparse.ml", line 48, characters 13-41
Called from Merlin_kernel__Pparse.rewrite in file "merlin/src/ocaml/driver/pparse.ml", line 118, characters 4-87
Called from Merlin_kernel__Pparse.apply_rewriters_str in file "merlin/src/ocaml/driver/pparse.ml", line 129, characters 16-61
Called from Merlin_kernel__Pparse.apply_rewriters in file "merlin/src/ocaml/driver/pparse.ml", line 144, characters 20-70
Called from Merlin_kernel__Mppx.rewrite.(fun) in file "merlin/src/kernel/mppx.ml", line 27, characters 4-76

However, the call to Filename.quote_command is only used for logging, so it can be easily avoided. The following patch fixes it:

diff --git a/src/ocaml/driver/pparse.ml b/src/ocaml/driver/pparse.ml
index 077e93ff..0bc75ac0 100644
--- a/src/ocaml/driver/pparse.ml
+++ b/src/ocaml/driver/pparse.ml
@@ -39,7 +39,7 @@ let report_error = function
       "External preprocessor does not produce a valid file. Command line: %s" cmd
 
 let commandline prog args =
-  Filename.quote_command prog args
+  Printf.sprintf "%s %s" prog (String.concat ~sep:" " (List.map ~f:Filename.quote args))
 
 let apply_rewriter magic ppx (fn_in, failures) =
   let title = "apply_rewriter" in

This also has the virtue that it matches what the function System.run_in_directory actually uses.

cc @3Rafal @voodoos

@voodoos
Copy link
Collaborator

voodoos commented Sep 11, 2023

Thanks for taking the time to investigate! I remember we took great care to keep existing windows behavior untouched, it a shame we overlooked that debug call.

I will apply your fix and probably do a patch-release soon.

@nojb
Copy link
Contributor Author

nojb commented Sep 11, 2023

I will apply your fix and probably do a patch-release soon.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants