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

Quote program path given to exec #3325

Closed
wants to merge 2 commits into from
Closed

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Apr 1, 2020

Hi!
When running ocaml bootstrap.ml, there are calls to ocamllex, ocamldep, and ocamlc. The calls are not quoted: if the path contains a space, the shell won’t be able to find the executable, and Dune cannot be compiled.
The bug is not present in 2.0.1 and recent versions.
Thanks!

@MisterDA MisterDA requested a review from a user April 1, 2020 22:07
@rgrinberg rgrinberg requested a review from dra27 April 1, 2020 22:25
@MisterDA
Copy link
Contributor Author

MisterDA commented Apr 2, 2020

I suppose that Filename.quote_command would be a better choice, but it would require to somewhat capture parameters given to the formatter, one after the other, to quote them, in the exec function:

let exec fmt =
  ksprintf (fun cmd ->
    print_endline cmd;
    Sys.command cmd)
    fmt

bootstrap.ml Outdated
@@ -193,7 +193,7 @@ let ocamldep = get_prog bin_dir "ocamldep"
let run_ocamllex src =
let dst = String.sub src ~pos:0 ~len:(String.length src - 1) in
let x = Sys.file_exists dst in
let n = exec "%s -q %s" ocamllex src in
let n = exec "\"%s\" -q %s" ocamllex src in
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use Filename.quote here? /cc @dra27 for the windows side of things

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m on Windows, that’s how I found the bug. I think the best patch would be to use Filename.quote_command since the command has arguments.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. The Windows CI is failing however :/

@MisterDA
Copy link
Contributor Author

MisterDA commented Apr 2, 2020

Well, that’s weird. Is it C:/PROGRA~1/OCaml\bin\ocamldep.opt.exe that cannot be found?

@rgrinberg
Copy link
Member

@diml would it be too much effort to embed spawn (from stdune) in the bootstrap? Would be great to just avoid all of these quoting issues altogether.

@ghost
Copy link

ghost commented Apr 7, 2020

@rgrinberg note that this is in the 1.x branch. IIRC, we don't have Unix in the bootstrap of 1.x. If we are serious about fixing this once and for all in 1.x, I'd suggest to backport Dune 2.x bootstrap instead.

@MisterDA
Copy link
Contributor Author

MisterDA commented Apr 9, 2020

The 1.x branch is still used by Opam when bootstrapping.

@ghost
Copy link

ghost commented Apr 9, 2020

I see. @dra27 have you tried switching to dune 2 for the opam bootstrap? Or I guess it's not possible because opam still builds with 4.02?

@MisterDA
Copy link
Contributor Author

MisterDA commented Apr 9, 2020

No, IIRC some of the bundled dependencies still use jbuilder files which are rejected by recent Dune.

@MisterDA MisterDA marked this pull request as draft April 9, 2020 11:54
@rgrinberg
Copy link
Member

We aren't maintaining 1.x anymore.

@rgrinberg rgrinberg closed this Nov 4, 2021
@MisterDA MisterDA deleted the fix-quotes branch May 10, 2023 09:18
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

Successfully merging this pull request may close these issues.

2 participants