dotnet local tool fails when run via FAKE, but succeeds when run via command line

mattgallagher92 commented 1 year ago

Description

I'm seeing different behaviour when running a dotnet local tool (grate) via FAKE than when running it directly. In particular, it fails when run via FAKE, but works without error when run directly from the command line.

Repro steps

Details in https://github.com/mattgallagher92/fake-dotnet-local-tool-bug, but the gist is that the following command doesn't work when run via FAKE:

dotnet grate
    --noninteractive
    --databasetype postgresql
    --connectionstring 'Host=localhost;Database=my_database;Username=postgres;Password=aVeryStrongPassword!'
    --adminconnectionstring 'Host=localhost;Database=postgres;Username=postgres;Password=aVeryStrongPassword!'

Expected behavior

The command to work when executed via FAKE.

Actual behavior

The command fails when run via FAKE, but succeeds when run directly from the command line.

Known workarounds

Do not use FAKE for this command.

Related information

Ubuntu 22.04
dotnet 6.0.406
Severity: annoying
FAKE 5.23.1

github-actions[bot] commented 1 year ago

Welcome to the FAKE community! Thank you so much for creating your first issue and therefore improving the project!

yazeedobaid commented 1 year ago

Thanks for reporting, can you please changing the command to be like follows:

Target.create "ApplyLocalDatabaseMigrations" (fun _ ->
    let grateCommand =
        "grate"
        + " --noninteractive"
        + " --databasetype postgresql"
-       + $" --connectionstring '%s{localDbConnectionString}'"
-       + $" --adminconnectionstring '%s{localDbAdminConnectionString}'"
+       + $" --connectionstring {localDbConnectionString}"
+       + $" --adminconnectionstring {localDbAdminConnectionString}"
    // This works locally but not via FAKE
    run Tools.dotnet grateCommand dbSrcPath
)

Removing the quotations around the connection string.

I have executed the command directly on the terminal with quotations and it gave me the same error you reported. So seems quotations are not acceptable around connection strings for grate tool.

Also, on the re-produce repository, you mentioned in the prerequisites section that you need .NET 6 SDK to run FAKE. However, you are running the build script using dotnet run as a project, not an interactive script through FAKE CLI dotnet fake build. In that case, you need .NET 6 since the runner uses SDK assemblies to compile the script.

mattgallagher92 commented 1 year ago

Thanks for your response Yazeed 🙂 I've clarified why .NET 6 is needed in the repo.

That's interesting. As mentioned, it works fine for me directly from the terminal when using single quotes.

Connection strings sometimes have spaces in them—see for example https://www.connectionstrings.com/sql-server/, example quoted below—so it would seem to me that quotes are necessary for the command line parser to be able to treat the connection string as a single argument.

Standard Security Server=myServerAddress;Database=myDataBase;User Id=myUsername;Password=myPassword;

In fact, you can see an official grate example where double quotes are used in grate: https://github.com/erikbra/grate/blob/main/examples/local/run-migration.ps1

In addition, in bash, text in double quotes is not treated as a string literal, whereas text in single quotes is treated as a string literal. Hence my use of single quotes.

Does FAKE run the commands in a particular shell (not bash)? Is it the same shell that you used? Maybe that's the source of the problem.

sWW26 commented 1 year ago

I can reproduce this issue on my machine, running in pwsh on Windows 11.

Running the command "raw" outside of FAKE works with single quotes however I can only get it to work inside FAKE if I use double quotes.

matthid commented 1 year ago

Note command line parsing on windows is a complicated thing. When you use the CreateProcess.fromRawCommandLine function (which you do in your sample). The string is forwarded without modification to ProcessStartInfo and the program itself. The program itself is responsible for parsing the string into arguments. We don't use any shell and directly start the program. When you use bash, pwsh or the command line the shell has its own logic for parsing and creating the command line string. This might include replacing single quote string with double quoted strings before starting the process...

To repeat it in other words: Windows itself has no knowledge of what an argument-list is, but it will always 'just' use a single argument-string for starting processes. This lead to a design where ProcessStartInfo only has a 'string' as well which complicates things even further as other OSes like Linux know what an argument list is, but we still have to go through this API unfortunately.

Usually programs use the Microsoft C++ Rules for parsing the command line string into arguments (https://learn.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?redirectedfrom=MSDN&view=msvc-170#parsing-c-command-line-arguments).

NOTE: To complicate things even further programs don't have to use these rules when they use a different compiler/stack, and there are programs which parse the 'raw string' differently. However, I'm pretty sure dotnet follows these rules.

As you can see from the rules there is no special handling for single quotes, therefore they should be treated as part of the arguments (I guess that is what you mean with 'not working'). So the correct thing is to use double quotes and properly escape quotes within the connection string.

However, I'd recommend to you to use an argument-list in FAKE and leave those details to the core-process module. As the documentation points out, use CreateProcess.fromRawCommandLine only when you have a "a properly escaped command line"

So the TLDR is to use

let arguments =
    Arguments.Empty
    |> Arguments.appendIf true "-Verbose"
Command.RawCommand("file", arguments)
|> CreateProcess.fromCommand

or at least

[ "arg1"; "arg2"; "arg3" ]
|> CreateProcess.fromRawCommand "./folder/mytool.exe"

As we suggest in the docs.

As an alternative you can use any other custom builder, like BlackFox.CommandLine.

At the very least you should escape the connection-string with Args.toWindowsCommandLine ['connection-string'] which will return the string you want to use.

mattgallagher92 commented 1 year ago

Thanks for the patient and detailed explanation @matthid! The repo I linked to is cut down from a SAFE app. The code that you referred to is in the SAFE Template, so I'll probably raise an issue against the SAFE template repo to suggest that this is handled differently.

To help make sure I pass on the right information, I'll repeat back what I've understood from your explanation. If you could confirm that my understanding is correct (or let me know if I've made any mistakes), I'd be grateful :slightly_smiling_face:

Despite the code looking syntactically similar to calling a command via a terminal, no shell is invoked. Instead, the program is invoked directly, with arguments passed to it programmatically.
CreateProcess.fromRawCommandLine results in a single string being sent directly to the program (dotnet in this case), which interprets how the string should be split into arguments. This behaviour is different from when I use bash (on Ubuntu), because in that case bash determines the arguments sent to the program (using its own semantics for single quotes).
dotnet receives the argument string and parses it according to Microsoft's rules for parsing C++ command-line arguments. In particular, single quotes are not interpreted as argument delimiters, hence the error I'm seeing. (Presumably the connection string argument is parsed as though it actually has single quotes at the start and end.)
Any of the methods you've suggested above are appropriate resolutions (I'll probably go for the argument list passed to the result of CreateProcess.fromRawCommand).

matthid commented 1 year ago

Sure, this is a really painful area. So I hope this helps reducing debugging time... Yes the essence is correct, feel free to just link this issue directly.

Just one small technicality (which you can ignore, and is not important at this point):

This behaviour is different from when I use bash (on Ubuntu), because in that case bash determines the arguments sent to the program (using its own semantics for single quotes).

When comparing with Ubuntu there is an additional complication (as mentioned above as quoted text): Bash can here in fact build a list of arguments and send it to the OS, but yes quoting would be removed at that point. Fake would forward the raw string to ProcessStartInfo - which only has the single string API - and the framework would parse it with the Microsoft logic (again no single quote supported - EXCEPT on some mono versions which have added special support IIRC) and give it to the OS. In contrast on Windows this 'usally' happens on the new process instead...

mattgallagher92 commented 1 year ago

Thanks @matthid! I can confirm that both the solution using Command.fromRawCommand and the one using Command.fromCommand work for me. For my simple use case where the arguments are hardcoded, I prefer Command.fromRawCommand.

As there is no change needed for FAKE, I will close this issue and instead raise an issue in the SAFE Template repo, mentioning this one.

fsprojects / FAKE