dotnet / jitutils

MIT License
144 stars 59 forks source link

Update System.CommandLine version #375

Closed am11 closed 1 year ago

am11 commented 1 year ago

Other dotnet repos have recently updated command-line-api version following the breaking changes after the API review: https://github.com/dotnet/command-line-api/issues/2146. Lets bring jitutils to the same plan now, as these conversion tweaks are fresh in our muscle memory.

cc @jakobbotsch, @adamsitnik

adamsitnik commented 1 year ago

@jakobbotsch Is there something more that we should do before merging this PR?

The CI is green and the changes LGTM, but I am simply not familiar with how this repo works and how it's used by other repos.

jakobbotsch commented 1 year ago

Thanks for updating this @am11!!

When I run jit-analyze locally with this change I see exceptions:

❯ jit-analyze --base "C:\dev\temp\diffs\getTypeLayout\diff2" --diff "C:\dev\temp\diffs\getTypeLayout\diff"
Unhandled exception. System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')
   at System.SZArrayHelper.get_Item[T](Int32 index)
   at ManagedCodeGen.JitAnalyzeRootCommand.<>c.<.ctor>b__58_4(ArgumentResult result) in C:\dev\dotnet\jitutils\src\jit-analyze\JitAnalyzeRootCommand.cs:line 47
   at System.CommandLine.CliArgument`1.GetDefaultValue(ArgumentResult argumentResult)
   at System.CommandLine.Parsing.ArgumentResult.ValidateAndConvert(Boolean useValidators)
   at System.CommandLine.Parsing.CommandResult.ValidateOptions(Boolean completeValidation)
   at System.CommandLine.Parsing.CommandResult.Validate(Boolean completeValidation)
   at System.CommandLine.Parsing.ParseOperation.Validate()
   at System.CommandLine.Parsing.ParseOperation.Parse()
   at System.CommandLine.Parsing.CliParser.Parse(CliCommand command, IReadOnlyList`1 arguments, String rawInput, CliConfiguration configuration)
   at System.CommandLine.Parsing.CliParser.Parse(CliCommand command, IReadOnlyList`1 args, CliConfiguration configuration)
   at System.CommandLine.CliCommand.Parse(IReadOnlyList`1 args, CliConfiguration configuration)
   at System.CommandLine.CliConfiguration.Invoke(String[] args)
   at ManagedCodeGen.Program.Main(String[] args) in C:\dev\dotnet\jitutils\src\jit-analyze\Program.cs:line 899

Seems like that's from the OverrideTotalBaseMetric "factory". @adamsitnik @am11 is it expected that the factory is invoked even without the CLI argument being passed?

am11 commented 1 year ago

@jakobbotsch, thanks for the test, it is fixed. Could you try with the last commit? :)

The old API had a third argument (bool) to select whether the default value should go down the factory method. The new API has a separate / explicit properties for each.

false from old API maps to CustomParser true maps to DefaultValueFactory and CustomParser

jakobbotsch commented 1 year ago

@jakobbotsch, thanks for the test, it is fixed. Could you try with the last commit? :)

Yep, works now. Thanks!