SAFE-Stack / SAFE-template

dotnet CLI template for SAFE project
MIT License
280 stars 87 forks source link

Use `Command.fromRawCommand` rather than `Command.fromRawCommandLine` #551

Closed mattgallagher92 closed 1 year ago

mattgallagher92 commented 1 year ago

In Helpers.fs, SAFE Template uses CreateProcess.fromRawCommandLine: https://github.com/SAFE-Stack/SAFE-template/blob/91c167293cddc63f474caf911907fefa245f3bba/Content/default/Helpers.fs#L69-L72

When interacting with the created process, a developer passes a string of arguments. For example: https://github.com/SAFE-Stack/SAFE-template/blob/91c167293cddc63f474caf911907fefa245f3bba/Content/default/Build.fs#L48-L53

This gives the impression that the command is being invoked like it would be from a terminal. However, the way arguments are parsed can be different from how a developer's shell does it. This behaviour can be confusing for developers, who might expect the commands to behave like they do in their shell.

If CreateProcess.fromRawCommand is used instead of CreateProcess.fromRawCommandLine, there's less scope for confusion. In this case, the process accepts a sequence of string arguments.

As a motivating example, consider a developer using bash (the default shell on Ubuntu). Text surrounded by double quotes is not treated as a string literal, whereas text surrounded by single quotes is, so it's common (and sometimes required) to use single quotes and avoid double quotes when delimiting arguments with spaces. However, when sending what appears to be the same argument string to a process created by CreateProcess.fromRawCommandLine, single quotes will be interpreted as part of the arguments to be sent to the process (per Microsot's rules for parsing C++ command line arguments, which the dotnet executable also uses). For example, 'an argument' would be considered a single argument in bash, but as two arguments ('an and argument') when included in a string sent to the process via FAKE. See https://github.com/fsprojects/FAKE/issues/2735 for a more detailed case where this happened.

isaacabraham commented 1 year ago

Sounds good to me. Is this a big thing to do, do you think @mattgallagher92 ?

mattgallagher92 commented 1 year ago

No - would probably take less time than it took me to write this issue :sweat_smile: half an hour ish?

isaacabraham commented 1 year ago

Happy for you to submit a PR for this then.

theprash commented 1 year ago

This is what the shell calls will look like after the change:

dotnet [ "fable"; "-o"; "output"; "-s"; "--run"; "npm"; "run"; "build" ] clientPath