Clean up error experience when downloading non-tools

[Forgind]

Fixes #37010

This is the new error experience:

This roughly works, but the current version feels pretty hacky to me, and I'm not a fan of that. In particular, where did magic 139 come from? Apparently it's the "failed to find a tool" message length, but I didn't see how I could access the real message, so that was the best I could do for now.

[baronfel]

Looking at the image in the description (thank you!) the part that stands out to me is that we know what the failure is now. Can we prevent rendering everything after "Package {id} is not a dotnet tool"? That would make the output much clearer.

[KalleOlaviNiemitalo]

Does this search work if the package is unlisted and the version is explicitly specified?

Or worse: publish a .NET tool package, publish a non-tool package with the same name and a higher version, unlist the .NET tool version of the package, and try to install the tool with an explicit version.

[Forgind]

Does this search work if the package is unlisted and the version is explicitly specified?

I have no idea whether unlisting the package affects this API. I think I have an idea for testing and will try to do that today if possible.

Or worse: publish a .NET tool package, publish a non-tool package with the same name and a higher version, unlist the .NET tool version of the package, and try to install the tool with an explicit version.

I don't think having a higher-versioned non-tool package of the same name would be a problem in this case. It should find the latest version that matches the criteria, which in this case means having the dotnet tool package type.

[Forgind]

Does this search work if the package is unlisted and the version is explicitly specified?

Yes! I tested with the elemental.sysinfotool package, as that's what we've used for testing unlisted packages previously, and it can still successfully install it.

PS C:\bugs\temp> dotnet tool install -g elemental.sysinfotool --version 0.5.0
You can invoke the tool using the following command: sysinfo
Tool 'elemental.sysinfotool' (version '0.5.0') was successfully installed.

[MiYanni]

nit: Holy moly. The formatting here confused me so bad. Took me a while to figure out the curly braces were an initializer list for the new SearchFilter. Might be a bit easer to read as:

Suggested change

	var results = await searchResource.SearchAsync(packageId.ToString(), new SearchFilter(includePrerelease: includePreview, filter: SearchFilterType.IsLatestVersion)
	{
	PackageTypes = [NuGet.Packaging.Core.PackageType.DotnetTool.Name]
	}, skip: 0, take: 10, log: _verboseLogger, cancellationToken: cancellationToken).ConfigureAwait(false);
	var searchFilter = new SearchFilter(includePrerelease: includePreview, filter: SearchFilterType.IsLatestVersion) { PackageTypes = [NuGet.Packaging.Core.PackageType.DotnetTool.Name] };
	var results = await searchResource.SearchAsync(packageId.ToString(), searchFilter, skip: 0, take: 10, log: _verboseLogger, cancellationToken: cancellationToken).ConfigureAwait(false);

[joeloff]

I tried running this PR in a dogfood environment and see a completely different error:

C:\git\sdk>dotnet tool install System.ValueTuple
Unhandled exception: System.IO.DirectoryNotFoundException: Could not find a part of the path 'C:\Users\**********\.nuget\packages\system.valuetuple\4.5.0\tools'.

[Forgind]

I tried running this PR in a dogfood environment and see a completely different error:

C:\git\sdk>dotnet tool install System.ValueTuple
Unhandled exception: System.IO.DirectoryNotFoundException: Could not find a part of the path 'C:\Users\**********\.nuget\packages\system.valuetuple\4.5.0\tools'.

When I first made the PR, I got the results I specified in the description. @baronfel suggested using a strongly-typed version of the same idea, and since I didn't like how hacky my original version was, I committed it, but as I continued testing, I found:
#43045 (comment)

That means this PR currently does nothing. The error you found was what happens when this PR fails to have the desired effect.

I'm still waiting to find out whether there's a good reason that API is consistently returning null.

[joeloff]

Thanks, that makes sense and I understand better now where @marcpopMSFT wanted me to pick this up

[joeloff]

Think I figured out the rest. If the package already exists, we assume it is a tool package and then we can fail. Updating the PR with an additional check that should take care of the DirectoryNotFound exception.

[Forgind]

@joeloff, can you make sure to re-test:

Does this search work if the package is unlisted and the version is explicitly specified?

before merging? I only tested with the version prior to this, which was different enough I could easily imagine it no longer being true.

[joeloff]

@joeloff, can you make sure to re-test:

Does this search work if the package is unlisted and the version is explicitly specified?

before merging? I only tested with the version prior to this, which was different enough I could easily imagine it no longer being true.

Yeah, I'll retest that

[joeloff]

Looks like unlisted still works from my dogfood

[Forgind]

I can't approve, but I think this looks good to me.

Might consider adding a couple tests, though

[joeloff]

@baronfel here's a screenshot of the new error experience

[baronfel]

Ok you're going to hate me but if we know the package isn't a tool we shouldn't show the rest of the exception (everything from line 2 onwards).

[joeloff]

Ok you're going to hate me but if we know the package isn't a tool we shouldn't show the rest of the exception (everything from line 2 onwards).

I was thinking the same thing after I pasted the screenshot, so we're on the same page

[Forgind]

I seem to remember you made the same comment to me, and after looking around, I found that I could just change throw new ToolPackageException into throw new GracefulException, and then everything worked. Might want to start with that and see if it works 🙂

[joeloff]

I seem to remember you made the same comment to me, and after looking around, I found that I could just change throw new ToolPackageException into throw new GracefulException, and then everything worked. Might want to start with that and see if it works 🙂

Yup, the extra text is coming from InstallToolCommandLowLevelErrorConverter that's adding yielding additional strings.

[joeloff]

And here's the screenshot for switching to GracefulException

[baronfel]

[marcpopMSFT]

Remaining errors were the known container errors

[martincostello]

This change appears to be the source of #43651 and has broken tool installation for non-NuGet.org package feeds that are used as proxies, such as JFrog Artifactory.