Download tool packages from NuGet feeds directly, instead of implicitly restoring #33835

JL03-Yue · 2023-07-06T21:23:17Z

PR for issue #31134

Currently, when installing a dotnet tool package, the code restores a temporary project using toolPackageInstaller. This PR updated the methods handling tools to use NuGet API when installing, updating, restoring, and uninstalling global and local tools.

This PR also changes the tests that test the installing, updating, restoring, and uninstalling global and local tools. It also controls the verbosity output from NuGet API when downloading tools.

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

dsplaisted · 2023-07-28T23:02:34Z

/azp run

azure-pipelines · 2023-07-28T23:02:39Z

Pull request contains merge conflicts.

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

dsplaisted

Great job getting this working!

Some of this is pretty complicated to review, I took a first pass and added my comments, but I will probably have to look at it again later.

Thanks!

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

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

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

src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs

nkolev92 · 2023-08-17T17:06:46Z

This implementation must consider PackageSourceMapping. We're moving away from an implementation that considers it, we must not lose that.

I'll review in more detail later.

dsplaisted · 2023-08-17T22:55:16Z

This implementation must consider PackageSourceMapping. We're moving away from an implementation that considers it, we must not lose that.

I'll review in more detail later.

@nkolev92 @zivkan We're using NuGet APIs to download a package. The code for that is here. Is there a way to respect package source mapping when calling those APIs?

nkolev92 · 2023-08-18T00:00:25Z

Yeah, I'll link examples later when I review it.

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

dsplaisted

Looking pretty good, great job!

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

nkolev92 · 2023-09-12T16:33:02Z

NuGet warns for http source since NuGet 6.3 (https://devblogs.microsoft.com/nuget/https-everywhere/) Before this change, dotnet tool install will run implicit restore so NuGet is able to warn customer about http source when installing dotnet tool. However, after this change, there will be no warning for http source when installing dotnet tool. Hi @JonDouglas @aortiz-msft @nkolev92 @zivkan do you have any concerns about this?

That's a great catch! I think that should be added as well.

JonDouglas · 2023-09-12T17:04:18Z

We want to have a consistent experience everywhere we can. Since most of that is implemented at restore, could we somehow provide a central API or similar piping for NuGet APIs as well? That way it is rather easy for people to contribute these types of changes without having to know about all the NuGet best security practices and years of work going into restore security?

nkolev92 · 2023-09-12T18:25:21Z

We want to have a consistent experience everywhere we can. Since most of that is implemented at restore, could we somehow provide a central API or similar piping for NuGet APIs as well? That way it is rather easy for people to contribute these types of changes without having to know about all the NuGet best security practices and years of work going into restore security?

http warnings, PSM, signature verification are concerns at different parts of restore, so they're not really implemented at the same level.

Adding a central API end up being something that's not used anywhere within the NuGet code itself, so we wouldn't know how to best author it.
After some discussions (maybe @zivkan was a part of that?), downloading was implemented on the SDK side (plus time constraints).

@zivkan mentioned this in a side-chat, but I think this is showing that we need to add some tests testing NuGet scenarios within in this repo, regardless of how stuff gets implemented :)

dsplaisted

I still want to take a look at the tests, but I think I've reviewed everything else in detail now.

Let's take a look at the feedback together.

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

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

dsplaisted

I've finished reviewing the tests now too. Some of the feedback here we've already discussed live.

I also think we should add tests for some scenarios where the package has already been downloaded or the tool has already been installed in another local tools folder.

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

src/Tests/Microsoft.DotNet.PackageInstall.Tests/NuGetPackageInstallerTests.cs

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

dsplaisted · 2023-09-15T21:28:36Z

src/Tests/Microsoft.DotNet.PackageInstall.Tests/NuGetPackageInstallerTests.cs

+                packageSourceMapping: mockPackageSourceMapping).GetAwaiter().GetResult();
+            a.Should().Throw<NuGetPackageInstallerException>().And.Message.Should().Contain(string.Format(Cli.NuGetPackageDownloader.LocalizableStrings.FailedToGetPackageUnderPackageSourceMapping, TestPackageId));
+        }
+


I think we should have a test where package source mapping is configured correctly, and verify that it can download successfully. I'm not sure yet what the best way to write such a test would be.

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

…ID>\<Package Version>

dsplaisted

Great job! We're almost there, just a few remaining comments.

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

dsplaisted · 2023-09-18T14:57:33Z

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

+                    NuGetv3LocalRepository localRepository = new(toolDownloadDir.Value);
+                    var package = localRepository.FindPackage(packageId.ToString(), packageVersion);


This only applies to local tools. So I tihnk it should probably be in a block that only applies to local tools (such as an else statement from the previous suggestion).

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

src/Cli/dotnet/NugetPackageDownloader/LocalizableStrings.resx

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

src/Tests/Microsoft.DotNet.PackageInstall.Tests/ToolPackageDownloaderTests.cs

…fine tests in ToolPackageDownloaderTests

dotnet-issue-labeler bot added Area-Tools untriaged Request triage from a team member labels Jul 6, 2023

JL03-Yue requested a review from dsplaisted July 6, 2023 21:34

JL03-Yue changed the title ~~Download tool packages from nuget when installing global tools~~ Tool install global command: Download tool packages from nuget when installing global tools Jul 7, 2023

JL03-Yue force-pushed the 31134-tool-nuget branch from 81bacd3 to 08c6a31 Compare July 10, 2023 19:40

dsplaisted reviewed Jul 17, 2023

View reviewed changes

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs Outdated Show resolved Hide resolved

zivkan reviewed Aug 3, 2023

View reviewed changes

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs Outdated Show resolved Hide resolved

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs Outdated Show resolved Hide resolved

JL03-Yue changed the title ~~Tool install global command: Download tool packages from nuget when installing global tools~~ Download tool packages from NuGet feeds directly, instead of implicitly restoring Aug 11, 2023

dsplaisted reviewed Aug 14, 2023

View reviewed changes

nkolev92 reviewed Aug 18, 2023

View reviewed changes

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs Outdated Show resolved Hide resolved

This was referenced Aug 22, 2023

Adding test for ToolPackageDownloader in sdk #34847

Closed

Tool install local command: Download tool packages from nuget when installing local tools #34004

Closed

zivkan reviewed Aug 28, 2023

View reviewed changes

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs Outdated Show resolved Hide resolved

dsplaisted reviewed Aug 28, 2023

View reviewed changes

JL03-Yue changed the base branch from main to release/8.0.1xx August 30, 2023 20:59

JL03-Yue requested review from a team, andriipatsula, ericstj, joperezr, tmat, arkalyanms and AntonLapounov as code owners August 30, 2023 20:59

dsplaisted reviewed Sep 13, 2023

View reviewed changes

JL03-Yue added 6 commits September 13, 2023 14:38

Resolve naming conventions, variable scopes, class range

4dcc5aa

Resolve test failures 1

893d794

Pass in parameters and rename conventions

b6a7c2d

rollback directory unchanged

ae2934b

Change verbosityOptions

6b5da76

Resolve versionRange

160244a

dsplaisted reviewed Sep 15, 2023

View reviewed changes

JL03-Yue added 6 commits September 15, 2023 15:43

Pass targetFramework and add GetBestPackageVersionAsync method

6b9c7c8

Update PSM test, update duplicate download error, remove unused code

4245c83

fix check duplicate global packages

ea5eaa6

Change verbosity type and add packageSourceMapping tests

c1e3177

Add test for when local tool rollback, it should delete the <Package …

797ebb4

…ID>\<Package Version>

update rollback directory test

3b9dddc

dsplaisted approved these changes Sep 18, 2023

View reviewed changes

JL03-Yue added 2 commits September 18, 2023 11:21

Adding scope to global tool duplicate download check; clean up te; re…

e7568cb

…fine tests in ToolPackageDownloaderTests

Include package version in cannot match version error

38ee802

JL03-Yue force-pushed the 31134-tool-nuget branch from 78dd94c to 38ee802 Compare September 18, 2023 21:42

MiYanni mentioned this pull request Sep 18, 2023

dotnet tool restore --interactive hangs forever #24220

Closed

JL03-Yue mentioned this pull request Sep 19, 2023

figure out why toolPackageStore return a path with quote #35508

Open

JL03-Yue merged commit 28fa9be into dotnet:release/8.0.1xx Sep 19, 2023
16 checks passed

KalleOlaviNiemitalo mentioned this pull request Sep 28, 2023

[8.0.100-rc.2.23469.4] some deprecated global tools get NuGetPackageNotFoundException when installing #35566

Closed

IanOfOz mentioned this pull request Nov 6, 2023

dotnet tool install have trouble installing when an private feed is in nuget.config #9555

Closed

JL03-Yue mentioned this pull request Apr 4, 2024

Installing a dotnet global tool should ignore the global.json in the current folder #14721

Open

filzrev mentioned this pull request May 1, 2024

Duplicate nupkg files are placed when install .NET Global Tool with .NET 8 SDK #40528

Open

This was referenced May 28, 2024

ToolPathResolver broken due to .NET 8.0 SDK Team nuke-build/nuke#1386

Closed

Handle duplicate packages in nuget cache (#1386) nuke-build/nuke#1387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download tool packages from NuGet feeds directly, instead of implicitly restoring #33835

Download tool packages from NuGet feeds directly, instead of implicitly restoring #33835

JL03-Yue commented Jul 6, 2023 •

edited

Loading

dsplaisted commented Jul 28, 2023

azure-pipelines bot commented Jul 28, 2023

dsplaisted left a comment

nkolev92 commented Aug 17, 2023

dsplaisted commented Aug 17, 2023

nkolev92 commented Aug 18, 2023

dsplaisted left a comment

nkolev92 commented Sep 12, 2023

JonDouglas commented Sep 12, 2023

nkolev92 commented Sep 12, 2023

dsplaisted left a comment

dsplaisted left a comment

dsplaisted Sep 15, 2023

dsplaisted left a comment

dsplaisted Sep 18, 2023

		NuGetv3LocalRepository localRepository = new(toolDownloadDir.Value);
		var package = localRepository.FindPackage(packageId.ToString(), packageVersion);

Download tool packages from NuGet feeds directly, instead of implicitly restoring #33835

Download tool packages from NuGet feeds directly, instead of implicitly restoring #33835

Conversation

JL03-Yue commented Jul 6, 2023 • edited Loading

dsplaisted commented Jul 28, 2023

azure-pipelines bot commented Jul 28, 2023

dsplaisted left a comment

Choose a reason for hiding this comment

nkolev92 commented Aug 17, 2023

dsplaisted commented Aug 17, 2023

nkolev92 commented Aug 18, 2023

dsplaisted left a comment

Choose a reason for hiding this comment

nkolev92 commented Sep 12, 2023

JonDouglas commented Sep 12, 2023

nkolev92 commented Sep 12, 2023

dsplaisted left a comment

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

dsplaisted Sep 15, 2023

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

dsplaisted Sep 18, 2023

Choose a reason for hiding this comment

JL03-Yue commented Jul 6, 2023 •

edited

Loading