nuke icon indicating copy to clipboard operation
nuke copied to clipboard

Quoted output of AbsolutePath

Open Roadrunner67 opened this issue 2 years ago • 2 comments

Description

Often when I use AbsolutePath for command line parameters, it works just great on my development rig. But when it is run on a build agent with spaces in the directory structure, it fails.

Until now the cure is usually:

MyTool($"\"{RootDirectory / "DirA" / "SomeFile.xml"}\"); I really hate these escaped quotes and would prefer something like:

MyTool({(RootDirectory / "DirA" / "SomeFile.xml").ToQuotedString());

Maybe someone has a more elegant suggestion?

Usage Example

(RootDirectory / "DirA" / "SomeFile.xml").ToQuotedString()

Alternative

$""{RootDirectory / "DirA" / "SomeFile.xml"}"

Roadrunner67 avatar Aug 26 '22 08:08 Roadrunner67

I will add IFormattable to AbsolutePath. Then you can do .ToString(format) and also use it in interpolated strings:

var x = path.ToString("d");
var y = $"{path:d}";

Specifiers would be:

  • d - Double quote
  • dn - Double quote if needed
  • same with s for single quotes

Even better would be a interpolated string handler for Tool and ProcessTasks.StartProcess that uses dn as default, because I think it's the most intended.

matkoch avatar Aug 28 '22 18:08 matkoch

[PublicAPI]
[InterpolatedStringHandler]
public ref struct AutoQuoteStringHandler
{
    private DefaultInterpolatedStringHandler _builder;

    public AutoQuoteStringHandler(
        int literalLength,
        int formattedCount,
        out bool handlerIsValid)
    {
        _builder = new(literalLength, formattedCount);
        handlerIsValid = true;
    }

    public void AppendLiteral(string value)
    {
        _builder.AppendLiteral(value);
    }

    public void AppendFormatted(object obj, int alignment = 0, string format = null)
    {
        AppendFormatted(obj.ToString(), alignment, format);
    }

    public void AppendFormatted(string value, int alignment = 0, string format = null)
    {
        (value, format) = !(value.IsDoubleQuoted() || value.IsSingleQuoted() || format == "nq")
            ? (value.DoubleQuoteIfNeeded(), null)
            : (value, format);
        _builder.AppendFormatted(value, alignment, format);
    }

    public void AppendFormatted(AbsolutePath path, int alignment = 0, string format = null)
    {
        _builder.AppendFormatted(path, alignment, format ?? "dn");
    }

    public string ToStringAndClear()
    {
        return _builder.ToStringAndClear();
    }
}

matkoch avatar Aug 30 '22 15:08 matkoch

Was struggling with the upgrade from 6.3.0 to 7.0.0

After the upgrade the parameter of the node was double quoted by default.

NodeExe($"{yarnCommand}", workingDirectory: path, logger: LogErrorAsWarning);

I had to add the nq format.

NodeExe($"{yarnCommand:nq}", workingDirectory: path, logger: LogErrorAsWarning);

I guess it should have been the default option.

Am I doing something wrong?

maxcherednik avatar May 16 '23 09:05 maxcherednik

Same here. After looking into the code I assumed that nq or null is the default, but I too get quoted paths resulting in double-double quotes in my commands if I have spaces in the path. I opened #1203 to clarify.

cz-dev-ge avatar Jun 19 '23 07:06 cz-dev-ge

Just stumbled upon this behaviour -- every string part with space will be quoted... E.g.

var moreArguments = "/OPTION1 /OPTION2"
ArgumentStringHandler x = $"{path} {moreArguments}"

We probably will avoid ArgumentStringHandler.

StefanBertels avatar Jul 04 '24 10:07 StefanBertels

@StefanBertels If you don't want it to quote your spaced parameters, use :nq

This has turned into an overcomplicated general implementation of ArgumentStringHandler. IMHO it should really just have been implemented as an AbsoultePath.ToString(), where paths should have been quoted if they included spaces (or \ escaped in linux). Paths were the originator for this quoting that have since caused so many problems.

You cannot entirely avoid ArgumentStringHandler as it is used in Tool to pass parameters to everything deriving from Tool

Roadrunner67 avatar Jul 04 '24 11:07 Roadrunner67

We had to look into nuke's source code to find and understand the bug we had. I now get the idea why it's implemented this way...

But while ArgumentStringHandler solves some(!) quoting problems it produces others. It's not obvious -- especially when partial strings, AbsolutePath etc. get mixed.

If you refactor things using Rider, e.g. (anti-)inlining, you can easily introduce different behaviour. There is a good chance errors get through because behaviour heavily depends on combination of spaces in values.

The tests below shows some quoting problems, I marked lines as "NOT OK" if they IMO are buggy -- while not saying that nuke should solve them.

Suggestion: Force users to use explicit formatters, i.e. throw errors when format is invalid or missing (in ArgumentStringHandler).

This IMHO is buggy in ArgumentStringHandler (compare with test cases below):

 public string ToStringAndClear()
    {
        return _builder.ToStringAndClear().TrimMatchingDoubleQuotes();
    }

TEST CODE

using System;
using System.IO;
using System.Linq;
using Nuke.Common.IO;
using Nuke.Common.Tooling;
using Xunit;
using Xunit.Abstractions;

public class PathTest(ITestOutputHelper output)
{
    [Fact]
    void Test()
    {
        AbsolutePath pathWithSpace = @"c:\my file.ext";
        AbsolutePath pathWithoutSpace = @"c:\pagefile.sys";
        var pathWithSpaceAsString = @"c:\my file.ext";
        AbsolutePath echo = Path.GetFullPath(@"..\..\..\..\Echo\bin\Debug\net8.0\Echo.exe");

        var i = 0;

        {
            // OK
            var p = ProcessTasks.StartProcess(echo, $"inline default {pathWithSpace}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // NOT OK
            var inlineAsIs = "another default";
            var p = ProcessTasks.StartProcess(echo, $"{inlineAsIs} {pathWithSpace}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK (explicit)
            var p = ProcessTasks.StartProcess(echo, $"inline nq {pathWithSpace:nq}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK, but not obvious where quoting happens
            var p = ProcessTasks.StartProcess(echo, $"inline dn {pathWithSpace:dn}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var arg2 = @""" ";
            var p = ProcessTasks.StartProcess(echo, $"inline quoteAndSpace {arg2}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var arg2 = @""""""" ";
            var p = ProcessTasks.StartProcess(echo, $"inline quotesAndSpace {arg2}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var p = ProcessTasks.StartProcess(echo, $"inline nq nospace {pathWithoutSpace:nq}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var p = ProcessTasks.StartProcess(echo, $"inline d nospace {pathWithoutSpace:d}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK (but explicit format silently ignored)
            string pathWithoutSpaceString = pathWithoutSpace;
            var p = ProcessTasks.StartProcess(echo, $"inline d nospace string {pathWithoutSpaceString:d}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var args = $"dn {pathWithSpace:dn}";
            var p = ProcessTasks.StartProcess(echo, args);
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // NOT OK
            var p = ProcessTasks.StartProcess(echo, $"{pathWithSpace}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var p = ProcessTasks.StartProcess(echo, $" {pathWithSpace}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // NOT OK
            var arg2 = "secondArgument";
            var p = ProcessTasks.StartProcess(echo, $"{pathWithSpace} {arg2} {pathWithSpace}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // NOT OK
            var arg1 = "first Argument";
            var p = ProcessTasks.StartProcess(echo, $"{arg1}{pathWithSpace}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var p = ProcessTasks.StartProcess(echo, $"{pathWithSpaceAsString} {pathWithSpace:nq}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var p = ProcessTasks.StartProcess(echo, $"{pathWithSpaceAsString} {pathWithSpaceAsString:nq}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // OK
            var p = ProcessTasks.StartProcess(echo, $"{pathWithSpaceAsString} {pathWithSpace:sn}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }

        {
            // NOT OK (and: explicit format unknown => ignored)
            var p = ProcessTasks.StartProcess(echo, $"{pathWithSpaceAsString} {pathWithSpaceAsString:sn}");
            var o = String.Join("\n", p.AssertWaitForExit().Output.Select(_ => _.Text));
            output.WriteLine($"{++i,2}: {p.Arguments,-50} => {o}");
        }
    }
}

Test output (character ^ is separating command line args)

 1: inline default "c:\my file.ext"                    => inline^default^c:\my file.ext
 2: another default" "c:\my file.ext                   => another^default c:\my^file.ext
 3: inline nq c:\my file.ext                           => inline^nq^c:\my^file.ext
 4: inline dn "c:\my file.ext"                         => inline^dn^c:\my file.ext
 5: inline quoteAndSpace "\" "                         => inline^quoteAndSpace^" 
 6: inline quotesAndSpace "\"\"\" "                    => inline^quotesAndSpace^""" 
 7: inline nq nospace c:\pagefile.sys                  => inline^nq^nospace^c:\pagefile.sys
 8: inline d nospace "c:\pagefile.sys"                 => inline^d^nospace^c:\pagefile.sys
 9: inline d nospace string c:\pagefile.sys            => inline^d^nospace^string^c:\pagefile.sys
10: dn "c:\my file.ext"                                => dn^c:\my file.ext
11: c:\my file.ext                                     => c:\my^file.ext
12:  "c:\my file.ext"                                  => c:\my file.ext
13: c:\my file.ext" secondArgument "c:\my file.ext     => c:\my^file.ext secondArgument c:\my^file.ext
14: first Argument""c:\my file.ext                     => first^Argumentc:\my^file.ext
15: "c:\my file.ext" c:\my file.ext                    => c:\my file.ext^c:\my^file.ext
16: "c:\my file.ext" c:\my file.ext                    => c:\my file.ext^c:\my^file.ext
17: "c:\my file.ext" 'c:\my file.ext'                  => c:\my file.ext^'c:\my^file.ext'
18: c:\my file.ext" "c:\my file.ext                    => c:\my^file.ext c:\my^file.ext

Echo.exe is just this:

Echo.csproj

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
   </PropertyGroup>
</Project>

Program.cs

System.Console.WriteLine(string.Join("^", System.Environment.GetCommandLineArgs()[1..]));

StefanBertels avatar Jul 04 '24 14:07 StefanBertels

I agree that refactoring is very likely to introduce unexpected misbehaviour.

If you have this code:

            var arg1 = "first Argument";
            var arguments = $"{arg1}{pathWithSpace}";
            var p = ProcessTasks.StartProcess(echo, arguments);

..you would expect it to be functionally identical to:

            var arg1 = "first Argument";
            var p = ProcessTasks.StartProcess(echo, $"{arg1}{pathWithSpace}");

But sadly it is not.

Roadrunner67 avatar Jul 05 '24 06:07 Roadrunner67