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

Markliniubility · 2024-02-25T09:12:45Z

#10130 #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.

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

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 · 2024-02-25T13:50:06Z

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 · 2024-02-25T16:17:25Z

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 · 2024-02-26T04:15:01Z

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 · 2024-02-26T10:18:34Z

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 · 2024-02-28T07:29:53Z

@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 · 2024-03-02T12:37:38Z

@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 · 2024-03-03T09:42:06Z

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 · 2024-03-03T09:48:53Z

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 · 2024-03-03T10:14:36Z

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 · 2024-03-03T10:34:04Z

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 · 2024-04-11T07:16:48Z

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 · 2024-04-11T20:07:22Z

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.

JL03-Yue

Thank you very much for the contribution! The work looks great and it passes manual tests with some small adjustments. Overall it looks amazing and I'm very grateful for this new feature.

src/Tests/dotnet.Tests/CommandTests/ToolUpdateCommandTests.cs

JL03-Yue · 2024-04-11T21:07:16Z

src/Tests/dotnet.Tests/CommandTests/ToolUpdateGlobalOrToolPathCommandTests.cs

+        [Fact]
+        public void GivenAnExistedLowerVersionInstallationItCanUpdateAllThePackageVersion()
+        {
+            CreateInstallCommand($"-g {_packageId} --version {LowerPackageVersion}").Execute();
+            CreateInstallCommand($"-g {_packageId2} --version {LowerPackageVersion}", _packageId2.ToString()).Execute();
+
+            CreateUpdateCommand($"--all -g -v:d").Execute();
+
+            _store.EnumeratePackageVersions(_packageId).Single().Version.ToFullString().Should()
+                .Be(HigherPackageVersion);
+            _store.EnumeratePackageVersions(_packageId2).Single().Version.ToFullString().Should()
+                .Be(HigherPackageVersion);
+        }
+


Thanks for the clear and comprehensive tests in global update @Markliniubility.

JL03-Yue · 2024-04-11T21:07:57Z

src/Tests/dotnet.Tests/CommandTests/ToolUpdateLocalCommandTests.cs

+        public void WhenRunWithUpdateAllItShouldUpdateFromManifestFile()
+        {
+            _toolRestoreCommand.Execute();
+            new ToolRestoreCommand(
+                Parser.Instance.Parse($"dotnet tool update {_packageIdB.ToString()} --local"),
+                _toolPackageDownloaderMock,
+                _toolManifestFinder,
+                _localToolsResolverCache,
+                _fileSystem,
+                _reporter
+            ).Execute();
+
+            _mockFeed.Packages[0].Version = _packageNewVersionA.ToNormalizedString();
+            _mockFeed.Packages[1].Version = _packageNewVersionA.ToNormalizedString();
+
+            _toolUpdateAllLocalCommand.Execute().Should().Be(0);
+
+            AssertUpdateSuccess(packageIdExpected: _packageIdA.ToString());
+            AssertUpdateSuccess(packageIdExpected: _packageIdB.ToString());
+        }


Thanks for the clear and comprehensive tests in local install 😃

JL03-Yue · 2024-04-11T21:10:06Z

src/Cli/dotnet/commands/dotnet-tool/update/ToolUpdateCommandParser.cs


-        public static readonly CliOption<VerbosityOptions> VerbosityOption = ToolInstallCommandParser.VerbosityOption;
+        public static readonly CliOption<bool> UpdateAllOption = ToolAppliedOption.UpdateAllOption;


Thanks for the consolidation of UpdateAllOption in ToolAppliedOption. I personally think it's a great idea since the -all option might applies to other command as well.

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

…mmand.cs Co-authored-by: JL03-Yue <[email protected]>

dsplaisted

Thanks a bunch for this! I have a bit of feedback but this looks close to ready.

dsplaisted · 2024-04-18T19:55:51Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

@@ -47,18 +50,22 @@ internal class ToolInstallGlobalOrToolPathCommand : CommandBase
        private IEnumerable<string> _forwardRestoreArguments;
        private readonly bool _allowRollForward;
        private readonly bool _allowPackageDowngrade;
+        private readonly bool _all;


Naming nit: I think this would be cliearer as _updateAll.

Naming nit: I think this would be cliearer as _updateAll.

Done

dsplaisted · 2024-04-18T19:58:51Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs


        private readonly string _explicitManifestFile;
        private readonly bool _createManifestIfNeeded;
        private readonly bool _allowRollForward;
+        private readonly bool _all;


Same nit as earlier, I think this would be clearer named _updateAll.

dsplaisted · 2024-04-18T20:01:22Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs

+            }
+            else
+            {
+                return ExecuteInstallCommand((PackageId) _packageId);


What happens if you don't pass in either a package ID or the --all option? We should generate an error, but I didn't notice where we're checking for it, so in that case I think we might get a NullReferenceException or similar here with this cast.

Thanks for the note!! Added two tests cases on this in ToolUpdateCommandTests.cs

WhenRunWithoutAllOrPackageIdShowErrorMessage
WhenRunWithBothAllAndPackageIdShowErrorMessage

dsplaisted · 2024-04-18T21:47:10Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs

-            var toolDownloadedPackage = _toolLocalPackageInstaller.Install(manifestFile);
+            if (!packageId.Equals(_packageId))
+            {
+                _toolLocalPackageInstaller = new ToolInstallLocalInstaller(_parseResult, packageId, _toolPackageDownloader);


Rather than recreating the ToolInstallLocalInstaller if the package ID doesn't match, could we instead have the ToolInstallLocalInstaller.Install command take the package ID as an argument?

Unfortunately, this may be a bug for ToolInstallLocalInstaller imo, without newing a new ToolInstallLocalInstaller, the old ToolInstallLocalInstaller will always install the old package even though you passed in a new package ID in ToolInstallLocalInstaller.Install

I'm suggesting we could change ToolInstallLocallInstaller.Install so that it uses a parameter passed in instead of the _packageId field, similar to the way ToolInstallGlobalOrToolPathCommand.ExecuteInstallCommand now works.

I'm suggesting we could change ToolInstallLocallInstaller.Install so that it uses a parameter passed in instead of the _packageId field, similar to the way ToolInstallGlobalOrToolPathCommand.ExecuteInstallCommand now works.

Sorry for the misread. Done!

dsplaisted · 2024-04-18T22:11:29Z

src/Cli/dotnet/commands/dotnet-tool/update/ToolUpdateGlobalOrToolPathCommand.cs

@@ -22,12 +22,13 @@ internal class ToolUpdateGlobalOrToolPathCommand : CommandBase
    {
        private readonly CreateShellShimRepository _createShellShimRepository;
        private readonly CreateToolPackageStoresAndDownloaderAndUninstaller _createToolPackageStoreDownloaderUninstaller;
-        private readonly ToolInstallGlobalOrToolPathCommand _toolInstallGlobalOrToolPathCommand;
+        private readonly Lazy<ToolInstallGlobalOrToolPathCommand> _toolInstallGlobalOrToolPathCommand;


What's the reason for changing this to be lazily initialized?

I see that ToolUpdateLocalCommand uses a similar pattern, but I'm not sure the reason for that. It doesn't seem to be necessary, but maybe I'm missing something, or maybe there used to be a reason but the code changed since then.

Thanks for noticing this! I checked the git blame for the Local counterpart. The commit was in 2019 and there was no signficiant reasons for writing it into Lazy as the code was initially written into Lazy rather than changed to Lazy.

Therefore, I removed the Lazy initialization. Thanks again!

src/Tests/dotnet.Tests/CommandTests/ToolUpdateLocalCommandTests.cs

…s.cs Co-authored-by: Daniel Plaisted <[email protected]>

Markliniubility · 2024-04-19T03:19:05Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

@@ -50,7 +50,7 @@ internal class ToolInstallGlobalOrToolPathCommand : CommandBase
        private IEnumerable<string> _forwardRestoreArguments;
        private readonly bool _allowRollForward;
        private readonly bool _allowPackageDowngrade;
-        private readonly bool _all;
+        private readonly bool _updateAll;


Markliniubility · 2024-04-24T01:39:38Z

@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?

dsplaisted

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 · 2024-04-24T20:47:46Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalInstaller.cs

@@ -19,18 +19,20 @@ internal class ToolInstallLocalInstaller

        private readonly IToolPackageStore _toolPackageStore;
        private readonly IToolPackageDownloader _toolPackageDownloader;
-        private readonly PackageId _packageId;
+        private readonly PackageId? _packageId;


I think we can remove the _packageId field from this class entirely, now that we are passing it as a parameter to the Install method instead.

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalInstaller.cs

…staller.cs Co-authored-by: Daniel Plaisted <[email protected]>

Markliniubility · 2024-04-25T01:53:37Z

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 · 2024-04-25T10:31:13Z

@Markliniubility Thanks a lot for your work on this!

JL03-Yue and others added 7 commits January 31, 2024 13:28

add --all option to parser

1b9cb84

add option to toolupdate; create new file

1b654a3

skeleton 1

674d99a

PoC for list

440f1fd

separating global and local

e8e112f

Framework for subcommand --all

e302c95

initial commit

7516b38

dotnet-issue-labeler bot added Area-Tools untriaged Request triage from a team member labels Feb 25, 2024

Markliniubility changed the title ~~[Issue #37237 #19650] Prototype for dotnet tool update --all~~ [Issue #37237 #19650] Discussion: Prototype for dotnet tool update --all Feb 25, 2024

remove scratch

4b61bd7

tbd for subcommand parser

49c1602

Markliniubility added 5 commits March 2, 2024 02:01

subcommand: update all

28bc88b

order change

4c1b34e

update command options

a4d1dff

Code clean up and refactors

7e9c94c

Update ToolUpdateAllCommand.cs

818ed64

Markliniubility added 2 commits March 3, 2024 01:34

Unit test: ensure input integrity

a6624d1

Update ToolUpdateAllCommandTests.cs

2d942b0

Markliniubility changed the title ~~[Issue #37237 #19650] Discussion: Prototype for dotnet tool update --all~~ [Issue #37237 #19650] Dotnet Cli Feature: dotnet tool update --all Updating all tools with one command Mar 3, 2024

JL03-Yue and others added 4 commits April 10, 2024 15:09

change global install --all logic

72e6d57

Global Update all unit test

461dcad

Local Update all unit testing

c2c8b67

Clean up

74f18ac

JL03-Yue removed the untriaged Request triage from a team member label Apr 11, 2024

JL03-Yue changed the base branch from release/8.0.3xx to release/8.0.4xx April 11, 2024 21:21

JL03-Yue reviewed Apr 11, 2024

View reviewed changes

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs Show resolved Hide resolved

Markliniubility and others added 5 commits April 11, 2024 15:21

Update src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCo…

1f6a2cd

…mmand.cs Co-authored-by: JL03-Yue <[email protected]>

Update src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCo…

f8b977a

…mmand.cs Co-authored-by: JL03-Yue <[email protected]>

Unify Install API

ce60b24

Clean up unused functions

274f83b

clean up

22ce11e

dsplaisted approved these changes Apr 18, 2024

View reviewed changes

Markliniubility and others added 2 commits April 18, 2024 18:02

Update src/Tests/dotnet.Tests/CommandTests/ToolUpdateLocalCommandTest…

df59c00

…s.cs Co-authored-by: Daniel Plaisted <[email protected]>

Nit Fixes

84a9fc2

Markliniubility commented Apr 19, 2024

View reviewed changes

Update ToolInstallLocalCommand.cs

779ad38

dsplaisted approved these changes Apr 24, 2024

View reviewed changes

Markliniubility and others added 2 commits April 24, 2024 15:25

Update src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalIn…

daabd3f

…staller.cs Co-authored-by: Daniel Plaisted <[email protected]>

Nit: remove unnecessary package id in ToolInstallLocalInstaller

c2d36f8

dsplaisted approved these changes Apr 25, 2024

View reviewed changes

dsplaisted merged commit e5165e2 into dotnet:release/8.0.4xx Apr 25, 2024
16 checks passed

tmds mentioned this pull request Jun 19, 2024

[ppc64le][mono] dotnet tool update -g is failing #41672

Closed

ralish mentioned this pull request Aug 14, 2024

Getting System.NullReferenceException when running dotnet tool update -g --all #42598

Closed


		public static readonly CliOption<VerbosityOptions> VerbosityOption = ToolInstallCommandParser.VerbosityOption;
		public static readonly CliOption<bool> UpdateAllOption = ToolAppliedOption.UpdateAllOption;

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

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

Conversation

Markliniubility commented Feb 25, 2024 • edited Loading

KalleOlaviNiemitalo commented Feb 25, 2024

baronfel commented Feb 25, 2024 • edited Loading

Markliniubility commented Feb 26, 2024 • edited Loading

KalleOlaviNiemitalo commented Feb 26, 2024

JL03-Yue commented Feb 28, 2024

Markliniubility commented Mar 2, 2024 • edited Loading

Markliniubility commented Mar 3, 2024 • edited Loading

KalleOlaviNiemitalo commented Mar 3, 2024

Markliniubility commented Mar 3, 2024 • edited Loading

KalleOlaviNiemitalo commented Mar 3, 2024

Markliniubility commented Apr 11, 2024

JL03-Yue commented Apr 11, 2024

JL03-Yue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Markliniubility commented Apr 24, 2024

dsplaisted left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Markliniubility commented Apr 25, 2024

dsplaisted commented Apr 25, 2024

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

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

Markliniubility commented Feb 25, 2024 •

edited

Loading

baronfel commented Feb 25, 2024 •

edited

Loading

Markliniubility commented Feb 26, 2024 •

edited

Loading

Markliniubility commented Mar 2, 2024 •

edited

Loading

Markliniubility commented Mar 3, 2024 •

edited

Loading

Markliniubility commented Mar 3, 2024 •

edited

Loading

dsplaisted left a comment •

edited

Loading