-
Notifications
You must be signed in to change notification settings - Fork 5.2k
update System.CommandLine and adopt to breaking changes #78577
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
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-console Issue Detailsrequired to unblock dependencies flow: dotnet/installer#14991
|
| <!-- Not auto-updated. --> | ||
| <MicrosoftDiaSymReaderNativeVersion>16.9.0-beta1.21055.5</MicrosoftDiaSymReaderNativeVersion> | ||
| <SystemCommandLineVersion>2.0.0-beta4.22355.1</SystemCommandLineVersion> | ||
| <SystemCommandLineVersion>2.0.0-beta4.22564.1</SystemCommandLineVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.CommandLine is also listed in https://github.com/dotnet/runtime/blob/main/eng/Version.Details.xml#L83-L86 . Do we need to update it there as well?
Also, it does not sound right that this is in the "Not auto-updated." block. I would think that everything in Version.Details.xml is auto-updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a subscription set up to auto-update System.CommandLine, and as a result the entry in Version.Details.xml doesn't actually do anything. We can add a subscription though and make this auto-update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a subscription though and make this auto-update.
@jkoritzinsky Could you please add it?
I see that dotnet/sdk has a System.CommandLine subscription (dotnet/sdk#29131). We need to be on a plan where all repos contributing to a product have a subscription for a thing, or none have a subscription. Otherwise, we will end up with build breaks in source-build leg when the bits meet in the installer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR is merged, I'll create and enable a subscription for System.CommandLine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subscription for System.CommandLine.
Btw, it's pulled from dotnet-libraries feed in NuGet.config at root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add it?
We are definitely going to do that, but when the time comes. Currently we are merging too many small breaking changes into S.CL. We would need to update SDK/runtime once a day to account for that.
My current plan is to gather a big batch of breaking changes (there will be a blocked PR in the SDK repo with dependencies update), get the first part of S.CL API approved by the API Review Board and then merge the changes into SDK and runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we need to disable the System.CommandLine subscription in dotnet/sdk then. Otherwise, there are going to be regular build breaks in dotnet/sdk and dotnet/installer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one time S.CL was also flowing into Templating. Please make sure any other subscriptions are also disabled.
|
More places need fixing - build breaks in src/coreclr/tools/dotnet-pgo/PgoRootCommand.cs |
|
Yes, for all these: src/coreclr/tools/ILVerify/Program.cs: private T Get<T>(Option<T> option) => _command.Result.GetValueForOption(option);
src/coreclr/tools/ILVerify/Program.cs: private T Get<T>(Argument<T> argument) => _command.Result.GetValueForArgument(argument);
src/coreclr/tools/aot/ILCompiler/Program.cs: private T Get<T>(Option<T> option) => _command.Result.GetValueForOption(option);
src/coreclr/tools/aot/crossgen2/Program.cs: private T Get<T>(Option<T> option) => _command.Result.GetValueForOption(option);
src/coreclr/tools/dotnet-pgo/Program.cs: private T Get<T>(Option<T> option) => _command.Result.GetValueForOption(option);
src/coreclr/tools/dotnet-pgo/Program.cs: private T Get<T>(Argument<T> argument) => _command.Result.GetValueForArgument(argument);
src/coreclr/tools/r2rdump/Program.cs: private T Get<T>(Option<T> option) => _command.Result.GetValueForOption(option);
|
I've been running |
jkotas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
@jkotas @adamsitnik I've created a darc subscription for System.CommandLine and disabled it. The id for the subscription is |
required to unblock dependencies flow: dotnet/installer#14991