Update System.CommandLine version #84229

am11 · 2023-04-02T16:19:36Z

Adapting to the (ongoing) breaking changes..

ghost · 2023-04-02T16:19:45Z

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Adopting to the (ongoing) breaking changes..

Author:	am11
Assignees:	-
Labels:	`area-Infrastructure-coreclr`, `community-contribution`
Milestone:	-

am11 · 2023-04-02T16:26:11Z

@adamsitnik, fyi, I was expecting a small win in crossgen2 size, which is published as NaitveAOT app, but it has 203520 bytes size regression on osx-arm64-release after this update (HEAD vs. HEAD~1). 🙁

$ ./build.sh pack -c Release
$ stat -f%z artifacts/bin/coreclr/osx.arm64.Release/crossgen2/osx-arm64/publish/crossgen2
28680536

adamsitnik

@am11 WOW, thanks a lot for updating all the tools to the most recent System.CommandLine version!

The code looks great, I am going to hit the approve button, but I need to mark it as "NO MERGE" because we need to update few other repos that depend on S.CL and merge all the updates at once.

BTW @am11 do you have any feedback regarding the recent changes?

How did you like the switch from having multiple Option ctors to one and using properties to initialize Description, custom parser and default value factory?

How do you feel about the removal of InvocationContext and ParseResult being the only input for SetAction?

Was removing context.ExitCode and returning an int from action intuitive?

adamsitnik · 2023-04-03T08:19:03Z

src/coreclr/tools/InjectResource/Program.cs

+CliOption<FileInfo> binOption = new("--bin") { Description = "Binary data to attach to the image" };
+CliOption<FileInfo> imageOption = new("--image") { Description = "PE image to add the binary resource into" };
+CliOption<string> nameOption = new("--name") { Description = "Resource name" };
+var rootCommand = new CliRootCommand("Inject native resources into a Portable Executable image");


nit: style consistency with other symbols ctor usage

Suggested change

var rootCommand = new CliRootCommand("Inject native resources into a Portable Executable image");

CliRootCommand rootCommand = new("Inject native resources into a Portable Executable image");

adamsitnik · 2023-04-03T09:27:29Z

but it has 203520 bytes size regression on osx-arm64-release after this update

That is very strange. The only thing that could cause size growth are new localizations moved from the SDK to S.CL in dotnet/command-line-api#2041 but I would definitely not expect them to take 20 MB!

am11 · 2023-04-03T10:38:02Z

The only thing that could cause size growth are new localizations moved from the SDK to S.CL in dotnet/command-line-api#2041

@jkotas stripped localization resources from runtime tools in #72478, does moving localization from SDK to S.CL have any effect on how SatelliteResourceLanguages (project property) is handled?

but I would definitely not expect them to take 20 MB!

It's +200 KB difference, not 20 MB. 😉 Not a huge deal, but it was bit of a surprise. I was hoping / expecting that it will go the other way (a few hundred KBs would be shaved off with all the great refactoring, simplification, removal of dependencies / wrappers / indirections)

BTW @am11 do you have any feedback regarding the recent changes?

Overall, APIs name, shape and classification have improved. 👍

Some feedback:

Help does not show value placement anymore:

--- before
+++ after

$ crossgen2 --help
...

-  --inputbubbleref <inputbubbleref>                                                                    Input bubble reference file(s) to be added to bubble (any use of this option is unsupported!)  []
+  --inputbubbleref                                                                                                         Input bubble reference file(s) to be added to bubble (any use of this option is unsupported!)  []
  --composite                                                                                          Emit a composite R2R image comprising a number of input assemblies
-  --compositekeyfile <compositekeyfile>                                                                KeyFile(.snk) for specifying a Public Key for the composite image created
+  --compositekeyfile                                                                                                       KeyFile(.snk) for specifying a Public Key for the composite image created

There is no control over the order of --help and --version option; with main branch crossgen2 -h will show version at the bottom but with PR it's at the top. Perhaps, explicit CliOption.AddHelp() and CliOption.AddVersion() would improve/fix the user-experience for this? i.e. if user calls them, then API should simply disable the default one and respect the order of the one issued by the user.
Extending help has been made complicated compared to last release. See related change in CommandLineHelpers.cs.
- First we need to know RootCommand will add --help automagically. Then we need to find that particular instance by enumerating over .Options (after the object construction) and overwrite the value. This is not a great UX and can be improved.
Similar to help, the API to "create RootCommand with version option named --version and alias -v" is now more complicated. See related change in CommandLineHelpers.cs.
CliArgument has HasDescription, but CliOption doesn't.. We use it to create repro/replay mechanism. Hoping to have it back in CliOption for consistency. (I realize that it is simply: DefaultValueFactory is not null)
There is no way to GetDefaultValue of CliOption (IValueDescriptor interface implementation is removed), and option.DefaultValueFactory(...requires cli argument.. which is internal...), so I had to compromise richness of the functionality. See CommandLineHelpers.cs where I've skipped it for now to avoid reflection (top obtain option.Argument). This compromise makes the resulting repro package (--make-repro) a bit bigger than before.

How did you like the switch from having multiple Option ctors to one and using properties to initialize Description, custom parser and default value factory?

It's good that we don't need to allocate an array for aliases new CliOption(new [] {"--configuration", "-c"}) became new CliOption("--configuration", "-c"). I had mixed feeling about moving Description from ctor to init prop, consistent with other props, but given its frequent usage it's a bit lengthy name to type.

How do you feel about the removal of InvocationContext and ParseResult being the only input for SetAction?

Love it! We don't have any usage beyond ParseResult and ExitCode. :)

am11 · 2023-04-03T10:40:07Z

src/coreclr/tools/Common/CommandLineHelpers.cs

                    object val = res.CommandResult.GetValue(option);
-                    if (val is not null && !(descriptor.HasDefaultValue && descriptor.GetDefaultValue().Equals(val)))
+                    if (val is not null)


@adamsitnik, this was the functionality compromise. I couldn't find a way to obtain the default value of CliOption.

@am11 thank you for your feedback! I've opened dotnet/command-line-api#2218 to address that.

Do we need a new release with that change? From latest build: https://dev.azure.com/dnceng-public/public/_build/results?buildId=303826&view=results

davidwrighton · 2023-05-18T18:24:57Z

I spoke to @adamsitnik offline and he tells me that this is still wanted, but it will be a month or so before we're ready to review for real.

adamsitnik

@am11 could you please update to the provided version? We are very close to merging dotnet/sdk#29131 and I would love to merge this PR as soon as the SDK gets updated.

If you don't have the time please let me know, I can take it from here.

Thanks!

eng/Version.Details.xml

eng/Versions.props

adamsitnik

@am11 could you please update to the provided version? We are very close to merging dotnet/sdk#29131 and I would love to merge this PR as soon as the SDK gets updated.

If you don't have the time please let me know, I can take it from here.

Thanks!

am11 · 2023-06-09T18:25:29Z

Updated to 2.0.0-beta4.23307.1.

adamsitnik

@am11 big thanks for working on this! I've found two things that we should address before merging. PTAL at my comments and let me know if you have the time to fix them.

adamsitnik · 2023-06-12T11:17:53Z

src/coreclr/tools/Common/CommandLineHelpers.cs

                    object val = res.CommandResult.GetValue(option);
-                    if (val is not null && !(descriptor.HasDefaultValue && descriptor.GetDefaultValue().Equals(val)))
+                    if (val is not null)


@am11 thank you for your feedback! I've opened dotnet/command-line-api#2218 to address that.

src/coreclr/tools/Common/CommandLineHelpers.cs

src/coreclr/tools/dotnet-pgo/PgoRootCommand.cs

adamsitnik · 2023-06-12T11:49:05Z

src/coreclr/tools/r2rtest/R2RTest.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.Build" Version="$(MicrosoftBuildVersion)" />


thank you for replacing the hardcoded version 👍

adamsitnik · 2023-06-12T19:47:21Z

The failure is unrelated: #87414

Please don't merge yet as we need to figure out the right order in dotnet/sdk#33157

…and-line-api-usage # Conflicts: # eng/Versions.props # src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs # src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs

adamsitnik · 2023-06-29T12:36:25Z

The failure is unrelated (#42518

dotnet-issue-labeler bot added the area-Infrastructure-coreclr label Apr 2, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 2, 2023

am11 force-pushed the feature/update-command-line-api-usage branch from e8997f1 to 5cfc399 Compare April 2, 2023 17:14

am11 mentioned this pull request Apr 2, 2023

move IValueDescriptor to System.CommandLine.NamingConventionBinder dotnet/command-line-api#2124

Merged

am11 marked this pull request as ready for review April 2, 2023 18:26

am11 requested a review from MichalStrehovsky as a code owner April 2, 2023 18:26

am11 force-pushed the feature/update-command-line-api-usage branch 2 times, most recently from 0da30e3 to 99c6f80 Compare April 2, 2023 19:54

build-analysis bot mentioned this pull request Apr 2, 2023

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

adamsitnik approved these changes Apr 3, 2023

View reviewed changes

adamsitnik added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 3, 2023

adamsitnik mentioned this pull request Apr 3, 2023

Port existing dotnet org applications to latest version of System.CommandLine dotnet/command-line-api#2146

Closed

5 tasks

am11 commented Apr 3, 2023

View reviewed changes

jeffschwMSFT requested a review from davidwrighton May 1, 2023 20:17

jeffschwMSFT added area-crossgen2-coreclr and removed area-Infrastructure-coreclr labels May 15, 2023

adamsitnik reviewed Jun 8, 2023

View reviewed changes

eng/Version.Details.xml Outdated Show resolved Hide resolved

eng/Version.Details.xml Outdated Show resolved Hide resolved

eng/Versions.props Outdated Show resolved Hide resolved

adamsitnik reviewed Jun 8, 2023

View reviewed changes

Update System.CommandLine version

70426dd

am11 force-pushed the feature/update-command-line-api-usage branch from 99c6f80 to 70426dd Compare June 9, 2023 16:36

build-analysis bot mentioned this pull request Jun 9, 2023

Tracking issue for CI build timeouts #76454

Closed

Merge dotnet/main into feature/update-command-lin..

34333cd

adamsitnik reviewed Jun 12, 2023

View reviewed changes

address code review feedback

12c0822

Apply suggestions from code review

cd03059

adamsitnik removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 12, 2023

adamsitnik mentioned this pull request Jun 12, 2023

Revert "Revert "[main] Update dependencies from dotnet/command-line-api (#29131)"" dotnet/sdk#33157

Merged

Bump Microsoft.SourceBuild.Intermediate.command-line-api

a68384c

build-analysis bot mentioned this pull request Jun 12, 2023

WasmTemplateTests.ConsolePublishAndRun fails with exit code 137 #87414

Closed

Merge remote-tracking branch 'upstream/main' into feature/update-comm…

0b05310

…and-line-api-usage # Conflicts: # eng/Versions.props # src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs # src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs

adamsitnik force-pushed the feature/update-command-line-api-usage branch from 52a67e4 to 0b05310 Compare June 28, 2023 19:03

build-analysis bot mentioned this pull request Jun 29, 2023

windows-x86 release MSBuild_CMake is failing #88172

Closed

adamsitnik merged commit c5b27d6 into dotnet:main Jun 30, 2023

am11 deleted the feature/update-command-line-api-usage branch June 30, 2023 14:21

marcpopMSFT mentioned this pull request Jun 30, 2023

[main] Update dependencies from dotnet/sdk dotnet/installer#16864

Merged

ghost locked as resolved and limited conversation to collaborators Jul 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update System.CommandLine version #84229

Update System.CommandLine version #84229

am11 commented Apr 2, 2023 •

edited

Loading

ghost commented Apr 2, 2023

am11 commented Apr 2, 2023

adamsitnik left a comment

adamsitnik Apr 3, 2023

adamsitnik commented Apr 3, 2023

am11 commented Apr 3, 2023 •

edited

Loading

am11 Apr 3, 2023

adamsitnik Jun 12, 2023

am11 Jun 12, 2023

davidwrighton commented May 18, 2023 •

edited

Loading

adamsitnik left a comment

adamsitnik left a comment

am11 commented Jun 9, 2023

adamsitnik left a comment

adamsitnik Jun 12, 2023

adamsitnik Jun 12, 2023

adamsitnik commented Jun 12, 2023

adamsitnik commented Jun 29, 2023

	var rootCommand = new CliRootCommand("Inject native resources into a Portable Executable image");
	CliRootCommand rootCommand = new("Inject native resources into a Portable Executable image");

Update System.CommandLine version #84229

Update System.CommandLine version #84229

Conversation

am11 commented Apr 2, 2023 • edited Loading

ghost commented Apr 2, 2023

am11 commented Apr 2, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik Apr 3, 2023

Choose a reason for hiding this comment

adamsitnik commented Apr 3, 2023

am11 commented Apr 3, 2023 • edited Loading

am11 Apr 3, 2023

Choose a reason for hiding this comment

adamsitnik Jun 12, 2023

Choose a reason for hiding this comment

am11 Jun 12, 2023

Choose a reason for hiding this comment

davidwrighton commented May 18, 2023 • edited Loading

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

am11 commented Jun 9, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik Jun 12, 2023

Choose a reason for hiding this comment

adamsitnik Jun 12, 2023

Choose a reason for hiding this comment

adamsitnik commented Jun 12, 2023

adamsitnik commented Jun 29, 2023

am11 commented Apr 2, 2023 •

edited

Loading

am11 commented Apr 3, 2023 •

edited

Loading

davidwrighton commented May 18, 2023 •

edited

Loading