Skip to content

Some uses of Filename.quote_command for better quoting#9476

Merged
nojb merged 1 commit intoocaml:trunkfrom
nojb:fix_sys_command_quoting
Apr 20, 2020
Merged

Some uses of Filename.quote_command for better quoting#9476
nojb merged 1 commit intoocaml:trunkfrom
nojb:fix_sys_command_quoting

Conversation

@nojb
Copy link
Copy Markdown
Contributor

@nojb nojb commented Apr 20, 2020

The change in filecompare.ml fixes an actual bug. The rest look potentially problematic.

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Apr 20, 2020

For info, this function can be used only in places where the arguments are individually available, so it it not possible to use it when the command line is made available as a single string (see eg the definition of use_output in Toploop).

@bobot
Copy link
Copy Markdown
Contributor

bobot commented Apr 20, 2020

What is the advantage of Sys.command with Filename.quote_command if it works only when arguments are individually available compare to a create_process?

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Apr 20, 2020

What is the advantage of Sys.command with Filename.quote_command if it works only when arguments are individually available compare to a create_process?

I guess they are roughly equivalent, but using Unix.create_process requires Unix, setting up file descriptors for redirection, etc.

Sys.command is widely used to quickly execute shell commands, but getting the quoting right across platforms is notoriously complicated, hence the motivation for Filename.quote_command.

@bobot
Copy link
Copy Markdown
Contributor

bobot commented Apr 20, 2020

Thanks, it could be interesting to add (another MR) a simple Unix.command_quote_command that would do all the boiler plate and avoid the complication of the shell.

@xavierleroy
Copy link
Copy Markdown
Contributor

It is a design decision that ocamltest does not depend on the Unix library. This could be questioned later, but for the moment Sys.command + Filename.quote_command is the way to go.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Very nice! Thank you. This should be good to merge if CI agrees. (@nojb: given that this affects ocamltest, you may want to also run a precheck if you haven't already)

@gasche
Copy link
Copy Markdown
Member

gasche commented Apr 20, 2020

(I had already looked at those Sys.command usages during our discussion of #9465 )

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Apr 20, 2020

Very nice! Thank you. This should be good to merge if CI agrees. (@nojb: given that this affects ocamltest, you may want to also run a precheck if you haven't already)

Good idea: https://ci.inria.fr/ocaml/job/precheck/361/

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Apr 20, 2020

precheck OK, merging.

@nojb nojb merged commit 15b08d2 into ocaml:trunk Apr 20, 2020
@nojb nojb deleted the fix_sys_command_quoting branch April 20, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants