Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from CommandLineParser and McMaster.Extensions.CommandLineUtils to System.CommandLine #1016

Open
adamsitnik opened this issue Jan 13, 2019 · 21 comments

Comments

@adamsitnik
Copy link
Member

As of today, we are using:

  • CommandLineParser to parse arguments for BenchmarkSwitcher
  • McMaster.Extensions.CommandLineUtils to parse the arguments for our first global tool (BenchmarkDotNet as global tool #1006)

Having two dependencies to parse command line arguments is not good.

I think that we should switch to System.CommandLine if possible (project)

System.CommandLine is the future of parsing command line arguments in .NET. It's not a MS-only project, it has been built together by the community and MS based on experiences from many existing command line parsing projects.

I have seen a 1h internal demo at MS and to tell the long story short it has everything that CommandLineParser and McMaster.Extensions.CommandLineUtils have and a lot of more cool features. You can read more about the motivations for it here

What this task requires:

  1. Make sure that System.CommandLine supports everything that CommandLineParser gives us as of today:
    • parsing simple arguments (1 value)
    • parsing more complex arguments (IEnumerable<string>)
    • parsing arguments with aliases (example: -i --inprocess)
    • help with samples
  2. Making sure that System.CommandLine supports everything that McMaster.Extensions.CommandLineUtils gives us as of today for the global tool

The global tool was merged to https://github.com/dotnet/BenchmarkDotNet/tree/tools branch. So a person working on this task would have to create branch out of tools branch.

If System.CommandLine meets all our needs, then I can talk with @KathleenDollard about publishing a signed package to Nuget.org and we can switch (dotnet/command-line-api#356)

@wojtpl2 perhaps you would be interested in this issue?

@WojciechNagorski
Copy link
Contributor

@adamsitnik I would like to do this but I think System.CommandLine is still not ready yet. There is more information dotnet/command-line-api#345 (comment).

@KathleenDollard made great PR dotnet/command-line-api#363 but it is not finished yet. This approache for me it will be the best option. Something similar to CommandLineParser.

Currently we can use only core APIs directly from System.CommandLine like here: https://github.com/dotnet/command-line-api/wiki/Your-first-app-with-System.CommandLine
Of course I can use it, but keep in mind that this code will change in the future.

@WojciechNagorski
Copy link
Contributor

@adamsitnik If we want to use System.CommandLine we need published package to Nuget.org or at least a preview version.

@WojciechNagorski
Copy link
Contributor

System.CommandLine supports:

  • parsing simple arguments (1 value)
  • parsing more complex arguments (IEnumerable<string>)
  • parsing arguments with aliases (example: -i --inprocess)
  • command with arguments.

I don't know yet:

  • help with samples

I'll create PR next week.

some preview:

C:\Users\wnagorsx\source\repos\ConsoleApp7\ConsoleApp7
λ dotnet run -- --job Dry -m 1
Unrecognized command or argument '1'

ConsoleApp7:
  BDN :)

Usage:
  ConsoleApp7 [options]

Options:
  -j, --job          Dry/Short/Medium/Long or Default
  -r, --runtimes     Full target framework moniker for .NET Core and .NET. For Mono just 'Mono', for CoreRT just 'CoreRT'. First one will be marked as baseline!
  -e, --exporters    GitHub/StackOverflow/RPlot/CSV/JSON/HTML/XML
  -m, --memory       Prints memory statistics
  --version          Display version information

@WojciechNagorski
Copy link
Contributor

WojciechNagorski commented Jan 22, 2019

I found one problem in System.CommandLine. It supports multiple value, but in different way. You always have to repeat alias of parameter for each value like this:
--runtime net472 --runtime netcoreapp2.1

I created issue in System.CommandLine dotnet/command-line-api#383

@WojciechNagorski
Copy link
Contributor

My PR dotnet/command-line-api#384 to System.CommandLine is marged. Now we can switch to System.CommandLine.

@WojciechNagorski
Copy link
Contributor

I created second PR dotnet/command-line-api#385

@adamsitnik
Copy link
Member Author

@wojtpl2 awesome! great job!

@WojciechNagorski
Copy link
Contributor

I push changes to https://github.com/wojtpl2/BenchmarkDotNet/tree/system_commandLine All test are passed:
image
Example output:

BenchmarkDotNet.Samples:
  BenchmarkDotNet

Usage:
  BenchmarkDotNet.Samples [options]

Options:
  -j, --job <J>                                  Dry/Short/Medium/Long or Default
  -r, --runtimes <R>                             Full target framework moniker for .NET Core and .NET. For Mono just 'Mono', for CoreRT just 'CoreRT'. First one will be marked as baseline!
  -e, --exporters <E>                            GitHub/StackOverflow/RPlot/CSV/JSON/HTML/XML
  -m, --memory <M>                               Prints memory statistics
  -d, --disasm <D>                               Gets disassembly of benchmarked code
  -p, --profiler <P>                             Profiles benchmarked code using selected profiler. Currently the only available is "ETW" for Windows.
  -f, --filter <F>                               Glob patterns
  -i, --inProcess <I>                            Run benchmarks in Process
  -a, --artifacts <A>                            Valid path to accessible directory
  --outliers <OUTLIERS>                          None/OnlyUpper/OnlyLower/All
  --affinity <AFFINITY>                          Affinity mask to set for the benchmark process
  --allStats <ALLSTATS>                          Displays all statistics (min, max & more)
  --allCategories <ALLCATEGORIES>                Categories to run. If few are provided, only the benchmarks which belong to all of them are going to be executed
  --anyCategories <ANYCATEGORIES>                Any Categories to run
  --attribute <ATTRIBUTE>                        Run all methods with given attribute (applied to class or method)
  --join <JOIN>                                  Prints single table with results for all benchmarks
  --keepFiles <KEEPFILES>                        Determines if all auto-generated files should be kept or removed after running the benchmarks.
  --counters <COUNTERS>                          Hardware Counters
  --cli <CLI>                                    Path to dotnet cli (optional).
  --packages <PACKAGES>                          The directory to restore packages to (optional).
  --coreRun <CORERUN>                            Path(s) to CoreRun (optional).
  --monoPath <MONOPATH>                          Optional path to Mono which should be used for running benchmarks.
  --clrVersion <CLRVERSION>                      Optional version of private CLR build used as the value of COMPLUS_Version env var.
  --coreRtVersion <CORERTVERSION>                Optional version of Microsoft.DotNet.ILCompiler which should be used to run with CoreRT. Example: "1.0.0-alpha-26414-01"
  --ilcPath <ILCPATH>                            Optional IlcPath which should be used to run with private CoreRT build.
  --launchCount <LAUNCHCOUNT>                    Affinity mask to set for the benchmark process
  --warmupCount <WARMUPCOUNT>                    How many warmup iterations should be performed. If you set it, the minWarmupCount and maxWarmupCount are ignored. By default calculated by the heuristic.
  --minWarmupCount <MINWARMUPCOUNT>              Minimum count of warmup iterations that should be performed. The default is 6.
  --maxWarmupCount <MAXWARMUPCOUNT>              Maximum count of warmup iterations that should be performed. The default is 50.
  --iterationTime <ITERATIONTIME>                Desired time of execution of an iteration in milliseconds. Used by Pilot stage to estimate the number of invocations per iteration. 500ms by default
  --iterationCount <ITERATIONCOUNT>              How many target iterations should be performed. By default calculated by the heuristic.
  --minIterationCount <MINITERATIONCOUNT>        Minimum number of iterations to run. The default is 15.
  --maxIterationCount <MAXITERATIONCOUNT>        Maximum number of iterations to run. The default is 100.
  --invocationCount <INVOCATIONCOUNT>            Invocation count in a single iteration. By default calculated by the heuristic.
  --unrollFactor <UNROLLFACTOR>                  How many times the benchmark method will be invoked per one iteration of a generated loop. 16 by default
  --runOncePerIteration <RUNONCEPERITERATION>    Run the benchmark exactly once per iteration.
  --info <INFO>                                  Run the benchmark exactly once per iteration.
  --list <LIST>                                  Prints all of the available benchmark names. Flat/Tree
  --disasmDepth <DISASMDEPTH>                    Sets the recursive depth for the disassembler.
  --disasmDiff <DISASMDIFF>                      Generates diff reports for the disassembler.
  --buildTimeout <BUILDTIMEOUT>                  Build timeout in seconds.
  --stopOnFirstError <STOPONFIRSTERROR>          Stop on first error.
  --statisticalTest <STATISTICALTEST>            Threshold for Mann-Whitney U Test. Examples: 5%, 10ms, 100ns, 1s
  --version                                      Display version information

But there is still some work to do.
I'm waiting for third PR dotnet/command-line-api#389 and for published version of System.CommandLine.

@adamsitnik Should I wait for published version of System.CommandLine or should I use version from MyGet?

dotnet add package --source https://dotnet.myget.org/F/system-commandline/api/v3/index.json System.CommandLine.Experimental -v 0.1.0-alpha-63724-02

@adamsitnik
Copy link
Member Author

I have left a suggestion for dotnet/command-line-api#389 (review)

Should I wait for published version of System.CommandLine

You can open a PR. As soon as @KathleenDollard let's me know that System.CommandLine will be published to nuget.org I am going to merge it

Once again big thanks for doing this!

@WojciechNagorski
Copy link
Contributor

There is still some missing function in System.CommandLine:

@adamsitnik
Copy link
Member Author

Parameter value with + separator

it's nice to have, but not mandatory

Case sensitive - BDN supports case sensitive

it depends on what is the golden standard of command line tools - should every tool be case sensitive or insensitive?

Examples of usage

this is a must have, without it the --help option is not very usefull

@jonsequitur
Copy link

Windows command line tools are more often case insensitive and *nix/macOS tools are usually case sensitive. (Think git branch -d vs git branch -D.)

System.CommandLine can support different casing like the example above via aliases but it would be awkward for example if you want to support all mixed case variants, e.g. --eXpoRterS.

@adamsitnik
Copy link
Member Author

@jonsequitur thanks for the explanation! in such case, I think that it's fine to be case-sensitive (we might add some aliases, but rather very few)

@jzabroski
Copy link
Contributor

Interesting. I prefer FluentCommandLineParser, personally. I dislike Attributes on my classes.

Related to this, I know some of you work for Microsoft, and I had previously requested the .NET team consider modifying the .dll format so that System.CommandLine parameters were discoverable via PowerShell so that we could have auto-complete, but the issue was closed. See: https://github.com/dotnet/coreclr/issues/19821

@jzabroski
Copy link
Contributor

It might be an interesting Trojan Horse to get my idea adopted if I build a PowerShell script that enables auto-completion for benchmarkDotNet. Thoughts? (I felt the response in the CoreCLR thread was one of "I don't see a demo so its not worth doing" circular logic).

@jonsequitur
Copy link

Why limit it to Powershell? :)

The new System.CommandLine does the heavy lifting for completions regardless of the shell:

https://github.com/dotnet/command-line-api/wiki/Features-overview#Suggestions

@jzabroski
Copy link
Contributor

@jonsequitur
PERFECT! When did this happen??

@WojciechNagorski
Copy link
Contributor

@adamsitnik I've seen that `System.CommandLine' is on Nuget: https://www.nuget.org/packages/System.CommandLine.Experimental/0.2.0-alpha.19156.3

I'm going to update my PR, this or next week.

@adamsitnik
Copy link
Member Author

@wojtpl2 awesome! thanks for all your contributions to System.CommandLine to make the port possible!

@JobaDiniz
Copy link
Contributor

JobaDiniz commented Jun 13, 2023

Is this still needed? I have experience with System.ComandLine and would like to grab this issue if BenchmarkDotNet team are still interested to make this move.

@AndreyAkinshin
Copy link
Member

@JobaDiniz Unfortunately, System.CommandLine is still in the preview stage (the current version is 2.0.0-beta4.22272.1 published on June 02, 2022). NuGet doesn't allow to create a stable version of a package that references another prerelease package. Therefore, if we switch to System.CommandLine right now, we will not be able to publish stable versions of BenchmarkDotNet.

While switching to System.CommandLine still looks reasonable, we have to wait until a stable version of this package is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants