Support '--' in a command line argument as a correct switch indicator #5786

BartoszKlonowski · 2020-10-08T11:16:44Z

This pull request fixes #5714:

It adds the support for double dash ('--') indicator used in command line arguments.
With these changes it is now possible to invoke MSBuild commands using the same '--' switch indicators as it is currently possible with dotnet call.

Changes done in this delivery are:

Add '--' as another correct sequence when checking for valid argument's beginning
Even if '-' beginning would also be recognized when using '--', adding '--' keeps the implementation consistent
Move the validation of switch beginnings into separate method
Get the indicator's length, based on found starting string, so it can be used to correctly extract the name of switch
Without it '--' would still be unrecognized, as hardcoded 1 in
switchName = unquotedCommandLineArg.Substring(1, switchParameterIndicator - 1);
would still result in having the extracted name as "-name"
Cover the GetLengthOfSwitchIndicator() method with unit test
This is to check whether each switch variation has correct length of it's indicator returned.

For more details about the implementation, please check the commit messages done for this pull request.

The example of how this implementation work (presented on version switch for short output) is presented below.

P:\Projekty\MSBuild>dotnet artifacts\bin\MSBuild\Debug\netcoreapp2.1\MSBuild.dll --version
Microsoft (R) Build Engine version 16.9.0-ci-20508-01+13c455688 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.


16.9.0.50801
P:\Projekty\MSBuild>artifacts\bin\MSBuild\Debug\net472\MSBuild.exe --version
Microsoft (R) Build Engine version 16.9.0-ci-20508-01+13c455688 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

16.9.0.50801

Unit test results are attached to this pull request, presenting only the Microsoft.Build.CommandLine, as only this layer should be affected.
Microsoft.Build.CommandLine.UnitTests_net472_x86_implementation - showing the results with PR changes
Microsoft.Build.CommandLine.UnitTests_net472_x86_original - showing the results on main branch
CommandLine_UnitTest_Results.zip

Launching the 'dotnet build' command is possible with both '-' and '--' switches, while launching the 'dotnet msbuild' command is only possible with single-dash swith (for example: '-help' or '-h'). To keep both products consistent it is necessary to add the double-dash to the list of supported switch indicators. Changes provided in this commit implements the '--' switch indicator. With these changes it is now possible to call 'dotnet msbuild --<switch>' as well as 'dotnet msbuild -<switch>' and 'dotnet msbuild -<switch>', where <switch> is any of supported command line option name. Implementation is done by: - adding "--" as considered sequence when checking for correct command line argument's beginning, - considering the switch indicator's length (2 for '--', 1 for '-' and '\', 0 for none) when extracting the name of switch from unquoted command line argument

The minor corrections done to the GetLengthOfSwitchIndicator() method are: - Use .Length property to return the length of each indicator variant This way will avoid the hardcoded magic values - Improve the comment formatting - Remove additional spaces inside the parameters braces

Unit test covering the GetLengthOfSwitchIndicator checks whether for each available indicator (- or / or --) it's correct length is returned. Hardcoded values are used (instead of using .Length property) to check if returned value matches exactly the known length.

Forgind

This should work (and thank you!) but it affects a lot more switches than I'd expected from the bug report, meaning that in addition to permitting --help to work, it also lets users use -- with any other switch. That doesn't bother me, but I know there was a relevant difference between - and -- with early command shells (- when followed by several characters meant treating each character as a separate flag, whereas -- treated it as a single flag), which means someone might want this change to only apply to help and not more broadly.

Forgind · 2020-10-08T15:41:53Z

src/MSBuild.UnitTests/XMake_Tests.cs

+        [Fact]
+        public void GetLengthOfSwitchIndicatorTest()
+        {
+            var commandLineSwitchWithSlash = "/Switch";


It might be good to have a test validating that building with --help works.

+1

This might be overkill but: Consider a case for -help, /help, and --help and verifying part of the output is expected. Potentially in the help test just below this test.

@benvillalobos Can you elaborate on:

verifying part of the output is expected

Thing is, that MSBuildApp.Execute() method only returns the ExitType.
How to get the string returned from command line execution?

Or perhaps checking whether each call (command with '--help', '-h, '/h') returns ExitType.Success will be enough?

Verifying that it succeeded would be enough for me. I think it's probably more complicated than it's worth to look at the output, but you might be able to capture it with a logger you can pass in alongside the --help switch.

Forgind · 2020-10-08T15:42:34Z

src/MSBuild/XMake.cs

+        {
+            if (unquotedSwitch.StartsWith("--", StringComparison.Ordinal))
+            {
+                return "--".Length;


I'd probably just put 2, 1, and 0 here to avoid allocating strings unnecessarily.

These strings are constants and so aren't allocated. This compiles to

ldstr "--" call instance int32 [System.Private.CoreLib]System.String::get_Length()

However, it's probably worth avoiding that call/making the JIT constant-fold it, so I would also prefer the constants.

rainersigwald

Thanks! A couple of minor suggestions but this looks great. I definitely expected this behavior, not a restriction to just the help switch.

rainersigwald · 2020-10-08T15:49:56Z

src/MSBuild/XMake.cs

+        {
+            if (unquotedSwitch.StartsWith("--", StringComparison.Ordinal))
+            {
+                return "--".Length;


These strings are constants and so aren't allocated. This compiles to

ldstr "--" call instance int32 [System.Private.CoreLib]System.String::get_Length()

However, it's probably worth avoiding that call/making the JIT constant-fold it, so I would also prefer the constants.

src/MSBuild/XMake.cs

benvillalobos

Great PR! I'd suggest having a test case to verify the output of -help, --help, and /help are all the same, but I won't block on it.

BartoszKlonowski · 2020-10-09T11:09:14Z

Thanks for the review everyone! I will provide this PR with all the corrections today.

The if statement in the switch indication's validation has been reduced from separate checking '--' and '-' beginnings, to only one '-' check. This is due to '-' beginning being a superset of '--' indicator, so additional check for separate '--' can be considered redundant and can be removed for optimize reasons. Co-authored-by: Rainer Sigwald <raines@microsoft.com>

To avoid using ldstr call and cover the review remark, each constant string.Length call has been replaced with constant value. Please see: dotnet#5786 (comment)

Unit test checking successfull call of Help command has been extended with the additional '--' switch indicator's case: "msbuild.exe --help" Also other cases has been added, so it's verified that additional indicator doesn't affect the already existing ones.

Forgind

Thanks for the test! Looks good to me, although it would be nice to refactor it into a Theory (see comments and delete the rest).

Forgind · 2020-10-12T23:35:01Z

src/MSBuild.UnitTests/XMake_Tests.cs

        [Fact]
        public void Help()
        {


Suggested change

[Fact]

public void Help()

{

[Theory]

[InlineData("-?")]

[InlineData("-h")]

[InlineData("--help")]

[InlineData(@"/h")]

public void Help(string switch)

{

Forgind · 2020-10-12T23:35:56Z

src/MSBuild.UnitTests/XMake_Tests.cs

+            MSBuildApp.Execute(
+#if FEATURE_GET_COMMANDLINE
+                @"c:\bin\msbuild.exe -? "
+#else
+                new [] {@"c:\bin\msbuild.exe", "-?"}
+#endif
+            ).ShouldBe(MSBuildApp.ExitType.Success);


Suggested change

MSBuildApp.Execute(

#if FEATURE_GET_COMMANDLINE

@"c:\bin\msbuild.exe -? "

#else

new [] {@"c:\bin\msbuild.exe", "-?"}

#endif

).ShouldBe(MSBuildApp.ExitType.Success);

MSBuildApp.Execute(

#if FEATURE_GET_COMMANDLINE

@$"c:\bin\msbuild.exe {switch} "

#else

new [] {@"c:\bin\msbuild.exe", switch}

#endif

).ShouldBe(MSBuildApp.ExitType.Success);

Forgind

Looks great, thanks!

rainersigwald

Looks awesome, thanks!

rainersigwald · 2020-10-13T15:40:26Z

src/MSBuild.UnitTests/XMake_Tests.cs

 #else
-                    new [] {@"c:\bin\msbuild.exe", "-?"}
+                new [] {@"c:\bin\msbuild.exe", $"{indicator}"}


Slight redundancy here:

Suggested change

new [] {@"c:\bin\msbuild.exe", $"{indicator}"}

new [] {@"c:\bin\msbuild.exe", indicator}

rainersigwald · 2020-10-13T16:45:47Z

Thanks @BartoszKlonowski!

BartoszKlonowski added 3 commits October 7, 2020 12:51

Forgind approved these changes Oct 8, 2020

View reviewed changes

rainersigwald reviewed Oct 8, 2020

View reviewed changes

benvillalobos approved these changes Oct 8, 2020

View reviewed changes

BartoszKlonowski and others added 3 commits October 9, 2020 16:25

Use constants when returning length of indicator

990c225

To avoid using ldstr call and cover the review remark, each constant string.Length call has been replaced with constant value. Please see: dotnet#5786 (comment)

Forgind approved these changes Oct 12, 2020

View reviewed changes

Forgind approved these changes Oct 13, 2020

View reviewed changes

rainersigwald approved these changes Oct 13, 2020

View reviewed changes

Use Theory for multiple Help cases tests

3d1f45d

BartoszKlonowski force-pushed the command-line-double-dash-support branch from dfbf090 to 3d1f45d Compare October 13, 2020 15:53

rainersigwald merged commit f0af25b into dotnet:master Oct 13, 2020

BartoszKlonowski mentioned this pull request Mar 1, 2021

General "switch" description in help message - update for "--" indicator #6205

Merged

rainersigwald mentioned this pull request Jul 26, 2021

Internal MSBuild error when --maxcpucount:8 supplied in cmdline (dotnet build) #6689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support '--' in a command line argument as a correct switch indicator #5786

Support '--' in a command line argument as a correct switch indicator #5786

BartoszKlonowski commented Oct 8, 2020

Forgind left a comment

Forgind Oct 8, 2020

benvillalobos Oct 8, 2020

BartoszKlonowski Oct 9, 2020

Forgind Oct 12, 2020

Forgind Oct 8, 2020

rainersigwald Oct 8, 2020

rainersigwald left a comment

rainersigwald Oct 8, 2020

benvillalobos left a comment

BartoszKlonowski commented Oct 9, 2020

Forgind left a comment

Forgind Oct 12, 2020

Forgind Oct 12, 2020

Forgind left a comment

rainersigwald left a comment

rainersigwald Oct 13, 2020

rainersigwald commented Oct 13, 2020

-        [Fact]
-        public void Help()
-        {
+        [Theory]
+        [InlineData("-?")]
+        [InlineData("-h")]
+        [InlineData("--help")]
+        [InlineData(@"/h")]
+        public void Help(string switch)
+        {

	new [] {@"c:\bin\msbuild.exe", $"{indicator}"}
	new [] {@"c:\bin\msbuild.exe", indicator}

Support '--' in a command line argument as a correct switch indicator #5786

Support '--' in a command line argument as a correct switch indicator #5786

Conversation

BartoszKlonowski commented Oct 8, 2020

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos left a comment

Choose a reason for hiding this comment

BartoszKlonowski commented Oct 9, 2020

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald commented Oct 13, 2020