[Issue #37237 #19650] Dotnet Cli Feature: `dotnet tool update --all` Updating all tools with one command

[Markliniubility]

#37237 #19650 Cr. @JL03-Yue who wrote the design and lots of proof-of-concept code

This is a prototype for dotnet tool update. The feature is usable however there are several concerns to be resolved.

1. Required Argument for subcommand update The update command has a required argument packageId (i.e. dotnet tool update --all is syntaxically not allow by the Cli Parser). There can be ways to hack out the requirement (which is used in this PR) but the hack makes exception handling very ugly, and should not be a long term solution.

Possible solution: we can have a new subcommand like dotnet tool update-all which is seperate from update

2. Performance This prototype is calling ToolUpdateCommand multiple times by manually constructing a new ParseResult for each tool in dotnet tool list. Constructing new ParseResult involves with some minor string parsing and can be potentially slow; however, update --all is not a latency sensitive feature. I think we can have this simple version first as a slow feature is better than no feature.

Moreover, the bottleneck of performance is download/internet speed rather than the parsing of some really short strings. Performance improvement requires a lot of refactoring and can take a very long time (e.g. extract some static install and uninstall functions from all these non-static install and uninstall functions).

Since this is a prototype open to discussion, I just manually test it without formal written tests

Demo

PS > dotnet tool install -g Cake.tool --version 1.0.0 -v m
Skipping NuGet package signature verification.
Tool 'cake.tool' was reinstalled with the stable version (version '1.0.0').
PS > dotnet tool update --all --global
Skipping NuGet package signature verification.
PS > dotnet tool list -g
Package Id      Version      Commands
----------------------------------------
cake.tool       4.0.0        dotnet-cake

> dotnet tool update --all -g -v:d
// omitted too many logs
[NuGet Manager] [Info]   OK https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/registrations2-semver2/cake.tool/index.json 83ms
[NuGet Manager] [Info]   GET https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/flat2/cake.tool/index.json 
[NuGet Manager] [Info]   OK https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/flat2/cake.tool/index.json 69ms
[NuGet Manager] [Info]   GET https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/flat2/cake.tool/4.0.0/cake.tool.4.0.0.nupkg
[NuGet Manager] [Info]   OK https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/flat2/cake.tool/4.0.0/cake.tool.4.0.0.nupkg 270ms
Skipping NuGet package signature verification.
Tool 'cake.tool' was reinstalled with the stable version (version '4.0.0').

[KalleOlaviNiemitalo]

the hack makes exception handling very ugly

Would it be less ugly if you changed the arity of PackageIdArgument to ArgumentArity.ZeroOrOne and added a validator (CliArgument.Validators or CliCommand.Validators)? I imagine the validator could then flag an error if neither PackageIdArgument nor --all was specified, and System.CommandLine would report it nicely.

[baronfel]

I was talking about this with @JL03-Yue on Friday I think - the way we thought about approaching it was to model dotnet tool update --all as a completely separate System.CommandLine CliCommand that is a subcommand of dotnet tool update - in this way you could completely control the set of CliOptions and CliArguments that apply to -all without making the invocation/execution/modeling of the existing dotnet tool update command more cumbersome.

[Markliniubility]

I was talking about this with @JL03-Yue on Friday I think - the way we thought about approaching it was to model dotnet tool update --all as a completely separate System.CommandLine CliCommand that is a subcommand of dotnet tool update - in this way you could completely control the set of CliOptions and CliArguments that apply to -all without making the invocation/execution/modeling of the existing dotnet tool update command more cumbersome.

Thanks! That's certainly a very doable way: let me try this and update this PR ~~in 1-2 days~~ on Friday March 1st

[KalleOlaviNiemitalo]

model dotnet tool update --all as a completely separate System.CommandLine CliCommand

Huh… this seems like it could affect CompletionItem.Kind and syntax highlighting if a Language Server Protocol for System.CommandLine is ever implemented.

[JL03-Yue]

@Markliniubility Thank you so much for your contribution. For the subcommand for --all, you can refer to the Subcommands of a given command in System.CommandLine.CliCommand. More details can be referred to the updated design details for update --all feature.

[Markliniubility]

@JL03-Yue Thank you for your help all the way through! I couldn't do this without your help and you have been so knowledgable!

I have made the changes into subcommands and now it follows the intended behaviors. I have also added come shared options into this new command such that users can specify things for update --all like verbosity or prerelease, etc.

I will finish unit-test writing by the end of this weekend. Is there any other testings we need besides unit testing?

[Markliniubility]

I will finish unit-test writing by the end of this weekend

Done.

One caveat is that user must use dotnet update --all --global instead of dotnet update -g --all as --all is a subcommand. I wonder if changing the subcommand name to all instead of --all would be better. Either way, that will be a one line tiny change.

[KalleOlaviNiemitalo]

Would it allow --global --all if you set CliOption.Recursive = true for --global instead of adding --global to the subcommand separately? https://github.com/dotnet/command-line-api/blob/5ea97af07263ea3ef68a18557c8aa3f7e3200bda/src/System.CommandLine/CliOption.cs#L60-L63

[Markliniubility]

Would it allow --global --all if you set CliOption.Recursive = true for --global instead of adding --global to the subcommand separately? https://github.com/dotnet/command-line-api/blob/5ea97af07263ea3ef68a18557c8aa3f7e3200bda/src/System.CommandLine/CliOption.cs#L60-L63

Thank you for your input! You have been an dotnet and System.Commandline experts and I really appreciate this piece of advise. However, I don't think this is a good method as this overcomplicates a thing that can be solved easily by renaming. Many options here are used across many commands in dotnet cli. Massively marking them as Recursive can make this senario work, but it may bring bugs and some unforeseen development difficulty to somewhere else in the dotnet/sdk subcommand development.

[KalleOlaviNiemitalo]

all is already the name of a NuGet package; apparently not a .NET tool package, though. (It has no dependencies either, unlike the NPM "everything" package.)

[Markliniubility]

Would it allow --global --all if you set CliOption.Recursive = true for --global instead of adding --global to the subcommand separately? https://github.com/dotnet/command-line-api/blob/5ea97af07263ea3ef68a18557c8aa3f7e3200bda/src/System.CommandLine/CliOption.cs#L60-L63

Thank you for the suggestion again! This indeed works. I will leave to @JL03-Yue to decide whether we shall change the all the options to recursive and which options to keep.

[KalleOlaviNiemitalo]

According to https://github.com/dotnet/command-line-api/issues/2343#issue-2173762441 and https://github.com/dotnet/command-line-api/issues/2355, they are "planning to remove global options", which I think means CliOption.Recursive. But that branch of dotnet/command-line-api isn't publishing NuGet packages yet, so it'll be a while before .NET SDK is ported to it. If the plan is implemented, I guess .NET SDK can switch to using an --all option (rather than a subcommand) and a validator.

[Markliniubility]

@JL03-Yue Thank you for helping me on the testing! I have updated the unit tests to include all senario with mocked file system

[JL03-Yue]

@JL03-Yue Thank you for helping me on the testing! I have updated the unit tests to include all scenario with mocked file system

Thanks for the update. I added some more details the updated design details for update --all feature. The conflicted logic for -all is added.

[Markliniubility]

@JL03-Yue Thank you for helping me on the testing! I have updated the unit tests to include all scenario with mocked file system

Thanks for the update. I added some more details the updated design details for update --all feature. The conflicted logic for -all is added.

@JL03-Yue Thanks. I will finished up in the next 60 hours with adding more unit testing on

1. Unit test on when conflicted options like packageId are passed along with --all
2. Create two test packages and update all packages.

[Markliniubility]

Thanks for the update. I added some more details the updated design details for update --all feature

@JL03-Yue Thanks for updated change. I have added the unit testings for the local and global install with two packages.

[JL03-Yue]

Thanks for the update. I added some more details the updated design details for update --all feature

@JL03-Yue Thanks for updated change. I have added the unit testings for the local and global install with two packages.

Thank you very much for your contribution. This is very impressive and we will take actions on it shortly.

[Markliniubility]

@JL03-Yue @dsplaisted Hi! Thank you all for reviewing and helping on this PR. Is there anything else needed for this PR prior to LGTM & approval for merging?

[Markliniubility]

Looking good, a few more comments, plus:

What happens if you run dotnet tool install --all? The update and install command share code and basically work the same, but this feels like maybe it should be an error. We should decide what we want the behavior to be and add a test to cover it.

@dsplaisted. There is no change for running dotnet tool install --all. We didn't add --all into the install parser, such that the parser will not parse --all as an option. Instead, it will work like originally and treat --all as package id.

demo:

[dsplaisted]

@Markliniubility Thanks a lot for your work on this!