ProcessTasks.StartShell should not add quotes on windows.

[Danielku15]

Usage Information

8.1.2, net8.0, Windows

Description

As you might know, correct quoting of arguments can be a nightmare. Nuke always adds double quotes when launching processes via ProcessTasks.StartShell which can lead to wrong arguments passed when launching a batch file on windows.

While I'm not sure about the overall impact of such a change, I think Nuke should not add double quotes on Windows. cmd.exe seem to handle things fine if you do not add any quotes around the overall command while it breaks actual quotes when you need pass arguments with spaces to your script.

Reproduction Steps

Nuke:

var bat = (RootDirectory / "test.bat");
bat.WriteAllText(
    """
    @echo off
    SET Arg1=%~1
    echo Arg1=%Arg1%
    """);
var cmdArgs = new Arguments();
cmdArgs.Add("{value}", RootDirectory / "with spaces");
ProcessTasks.StartShell($"{bat} {cmdArgs.RenderForExecution()}").AssertWaitForExit();

This launches a command like

C:\Windows\System32\cmd.exe /c "C:\myproject\test.bat \"C:\myproject\with spaces\""

While a command line like this would deliver the expected result:

C:\Windows\System32\cmd.exe /c C:\myproject\test.bat "C:\myproject\with spaces"

Expected Behavior

Arg1=C:\myproject\with spaces

Actual Behavior

Arg1=\"C:\myproject\with spaces\"

Regression?

No response

Known Workarounds

 protected virtual IProcess RunBatch(
        string command,
        string? workingDirectory = null,
        IReadOnlyDictionary<string, string>? environmentVariables = null,
        int? timeout = null,
        bool? logOutput = null,
        bool? logInvocation = null,
        Action<OutputType, string>? logger = null,
        Func<string, string>? outputFilter = null)
    {
        return ProcessTasks.StartProcess(
            toolPath: ToolPathResolver.GetPathExecutable(IsWin ? "cmd" : "bash"),
            arguments: IsWin ? $"/c {command}" : $"-c {command.DoubleQuote()}",
            workingDirectory,
            environmentVariables,
            timeout,
            logOutput,
            logInvocation,
            logger,
            outputFilter);
    }

Could you help with a pull-request?

Yes

[Danielku15]

Some other things I noticed: Things might be tricky when the launched application also has spaces. What seems to work really as expected is:

1. Add /s to cmd.exe
2. Wrap the argument with double quotes
3. Do not escape any quotes inside the command

C:\Windows\System32\cmd.exe /s /c ""C:\myproject\test.bat" "C:\myproject\with spaces""

Arg1=C:\myproject\with spaces

Something like this should deliver the expected result:

 public static IProcess StartShell(
        string command,
        string? workingDirectory = null,
        IReadOnlyDictionary<string, string>? environmentVariables = null,
        int? timeout = null,
        bool? logOutput = null,
        bool? logInvocation = null,
        Action<OutputType, string>? logger = null,
        Func<string, string>? outputFilter = null)
{
    return ProcessTasks.StartProcess(
        toolPath: ToolPathResolver.GetPathExecutable(IsWin ? "cmd" : "bash"),
        arguments: IsWin ? $"/s /c \"{command}\"" : $"-c {command.DoubleQuote()}",
        workingDirectory,
        environmentVariables,
        timeout,
        logOutput,
        logInvocation,
        logger,
        outputFilter);
}

Related docs for the option and the exact behavior. If we don't want to go for /s nuke would need to respect the special logic described and quote accordingly.

/S      Modifies the treatment of string after /C or /K (see below)
...
If /C or /K is specified, then the remainder of the command line after
the switch is processed as a command line, where the following logic is
used to process quote (") characters:

    1.  If all of the following conditions are met, then quote characters
        on the command line are preserved:

        - no /S switch
        - exactly two quote characters
        - no special characters between the two quote characters,
          where special is one of: &<>()@^|
        - there are one or more whitespace characters between the
          two quote characters
        - the string between the two quote characters is the name
          of an executable file.

    2.  Otherwise, old behavior is to see if the first character is
        a quote character and if so, strip the leading character and
        remove the last quote character on the command line, preserving
        any text after the last quote character.