command-line-api icon indicating copy to clipboard operation
command-line-api copied to clipboard

Beta 4 Invoke(Environment.CommandLine) fails

Open ptr727 opened this issue 3 years ago • 7 comments

Beta 3 works fine. Updated to Beta 4, and commandline parsing fails.

I suspect the issue in CommandExtension.Invoke() when passing in Environment.CommandLine as the commandline. It appears as if the CommandLineStringSplitter.Instance.Split(commandLine) is treating arg[0] as a command, vs. arg[0] being this executable.

using System.CommandLine;
using TestCmdLine;

RootCommand rootCommand = CommandLineOptions.CreateRootCommand();
int ret = rootCommand.Invoke(Environment.CommandLine);

return ret;

// Beta 3:

// C:\Users\piete\source\repos\TestCmdLine\bin\Debug\net6.0\TestCmdLine.dll
//Required command was not provided.
//Option '--globaloption' is required.

//Description:
//  Testing app 123

//Usage:
//  TestCmdLine[command][options]

//Options:
//--globaloption<globaloption>(REQUIRED)  Required global string
//  --version                                 Show version information
//  -?, -h, --help                            Show help and usage information

//Commands:
//  testing Testing command 123


// C:\Users\piete\source\repos\TestCmdLine\bin\Debug\net6.0\TestCmdLine.dll testing --globaloption foo --testoption
// TestCommand: GlobalOption = foo, TestOption = True


// Beta 4:
// C:\Users\piete\source\repos\TestCmdLine\bin\Debug\net6.0\TestCmdLine.dll
//Unrecognized command or argument 'C:\Users\piete\source\repos\TestCmdLine\bin\Debug\net6.0\TestCmdLine.dll'.

//Description:
//  Testing command 123

//Usage:
//  TestCmdLine testing[options]

//Options:
//  --testoption                              Optional command bool
//  --globaloption <globaloption> (REQUIRED)  Required global string
//  -?, -h, --help                            Show help and usage information
using System;
using System.Collections.Generic;
using System.CommandLine;
using System.CommandLine.NamingConventionBinder;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace TestCmdLine;

internal class CommandLineOptions
{
    public string? GlobalOption { get; set; }
    public bool TestOption { get; set; }

    public static RootCommand CreateRootCommand()
    {
        // Root command and global options
        RootCommand command = new("Testing app 123");
        AddGlobalOptions(command);

        // Test command
        command.AddCommand(CreateTestCommand());

        return command;
    }

    private static void AddGlobalOptions(RootCommand command)
    {
        command.AddGlobalOption(
            new Option<string>("--globaloption")
            {
                Description = "Required global string",
                IsRequired = true
            });
    }

    private static Command CreateTestCommand()
    {
        Command command = new Command("testing")
        {
            Description = "Testing command 123",
            Handler = CommandHandler.Create<CommandLineOptions>(TestCommand)
        };

        command.AddOption(
            new Option<bool>("--testoption")
            {
                Description = "Optional command bool",
                IsRequired = false
            });
        
        return command;
    }

    private static int TestCommand(CommandLineOptions options)
    {
        Console.WriteLine($"TestCommand: GlobalOption = {options.GlobalOption}, TestOption = {options.TestOption}");
        return 0;
    }
}

ptr727 avatar Jun 28 '22 21:06 ptr727

It appears as if the CommandLineStringSplitter.Instance.Split(commandLine) is treating arg[0] as a command, vs. arg[0] being this executable.

Ah, I see what's going on. This is an intentional behavior change introduced in order to fix #1663. If you're parsing Environment.CommandLine, you should strip off arg[0].

jonsequitur avatar Jun 28 '22 21:06 jonsequitur

Stripping of arg[0] is not so simple, it means I have to parse the commandline, and then I run into issues with forward and backslash processing differences between linux and windows when dealing with quoted strings. E.g. https://stackoverflow.com/questions/298830/split-string-containing-command-line-parameters-into-string-in-c-sharp E.g. https://github.com/ptr727/Utilities/blob/main/Utilities/CommandLineEx.cs

This has been and issue in your predecessor library, and the predecessor library before that, I was happy that you allowed Environment.CommandLine to be used as is, and now it is broken again.

How can I pass in rootCommand.Invoke(Environment.CommandLine) and let you do the parsing, I don't want to do tricky parsing that should be in the CLI library?

ptr727 avatar Jun 28 '22 21:06 ptr727

Slashes and quotes are handled by CommandLineStringSplitter, so you should just be able to do this:

string[] args = CommandLineStringSplitter.Instance.Split(Environment.CommandLine).ToArray()[1..];

rootCommand.Invoke(args);

jonsequitur avatar Jun 28 '22 22:06 jonsequitur

Ok, thx.

May I suggest that Invoke(string) restores the previous behavior, and that Invoke(string[]) could use the new behavior, or a version that takes no arguments and uses Environment.CommandLine internally and removes arg[0], or some other deterministic use, else everybody would just have to manually remove arg[0], makes little sense.

And, this was not documented as a breaking change, I understand this is beta, but almost every beta release has some break that causes me headaches when not documented or conversion from the previous version is not obvious. I appreciate your efforts and standardization, but post beta, please please, more complete list of breaking changes.

ptr727 avatar Jun 28 '22 22:06 ptr727

This change was intended to make the behavior clearer. Previously, the first argument would only be stripped off if it was a possible match for the executable name, which was confusing, so restoring the previous behavior would reintroduce that confusion. We decided as well that having a different behavior for Invoke(string) and Invoke(string[]) would be hard to understand. Reducing the complexity here also provided a performance benefit.

We do normally label breaking changes: https://github.com/dotnet/command-line-api/issues?q=label%3A%22Breaking+Change%22+is%3Aclosed, though in the release issues we've only been mentioning the ones that affect the more commonly-used APIs. Apologies that this one slipped through.

On that note, you should also be aware of #1758.

Out of curiosity, what's the motivation for parsing Environment.CommandLine rather than the string[] args passed to Main?

jonsequitur avatar Jun 28 '22 22:06 jonsequitur

Problem with the string[] args is that it breaks when using e.g. "D:\", where the " was treated as an embedded quote, not a terminator quote.

Also, the vanilla args[] does not (did not) support unicode or UTF8 characters (critical when piping between processes), thus one was forced to use the W/unicode API to get the original commandline, then parse by hand.

With the introduction of .NET Core, I asked, and the team did not want to change the behavior from how WIN32 in C++ works, even if broken.

The McMaster, gsscoder, and can't remember the other CLI library I used before did not want to include the "correct" parser as I referenced on stackoverflow, thus myself and others just created the manual parsing code, and ignored the broken args[] behavior. Found one: https://github.com/gsscoder/commandline/issues/473

Now, things may have changed, maybe .NET Core now correctly parses in args[], but that may be something you could verify, I've given up, or more I can't be bothered to try to get a team to fix something I think is/was broken.

And yep, #1758 sorta describes the issues with not correctly parsing. See: https://github.com/ptr727/Utilities/blob/main/UtilitiesTests/CommandLineTests.cs Also the stackoverflow article has grown to be very complete.

ptr727 avatar Jun 28 '22 23:06 ptr727

Thanks, @ptr727. This is useful context for #1758, which is a good place to continue this discussion.

jonsequitur avatar Jun 29 '22 02:06 jonsequitur