-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Linux: Installing/Updating packages with uppercase versions #43988
Conversation
31cbe77
to
b738f3c
Compare
235af9c
to
fbebbe1
Compare
src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
Outdated
Show resolved
Hide resolved
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.
I think having an E2E or at least some unit tests for the installation path generated would help long term.
src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
Outdated
Show resolved
Hide resolved
6cf45af
to
a6afa5f
Compare
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.
There's still some pending feedback I'd like to resolve / hear from others on, but great job on your first PR in the SDK! 😊 This was a lot more challenging than expected which is our fault so I do apologize for that. I think you did a great job independently investigating and contacting the nuget team so great work 👏
@@ -6,6 +6,7 @@ | |||
using Microsoft.DotNet.Tools; | |||
using Microsoft.Extensions.EnvironmentAbstractions; | |||
using NuGet.Frameworks; | |||
using NuGet.Packaging; |
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.
On ToolRestoreCommand
line 157, I would lowercase the package.Version going into the Tool Resolver cache. I would also do this in the localToolsResolverCache line 26, 149. I think its possible that data could come to bite us later if we access it somewhere without lowercasing the version. But part of this is just because Im not knowledgeable about this codebase so I am being pretty precautious; doing this may cause other problems too.
Also going to ping @dsplaisted to see his thoughts.
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.
ToolRestoreCommand.PackageHasBeenRestored
Loads the Cache of tool restore commands previously executed and then the locations of those tools that were previously restored -> TryGetMatchingRestoredCommand
-> LocalTolsResolverCache
Save
uses RestoredCommand.Executable
to cache the location of the package, it includes Version
as well which is not lowercased but that Version
is not used to generate a path so it's ok, also I think we want to store the true version of the package and not the lowercased one for nuget's directory purposes
-> ToolRestoreCommand.InstallPackages
has IToolPackage
with Command
with Executable
-> ToolPackageDownloader.InstallPackage
-> Uses Nuget Version in the Version
but toolReturnPackageDirectory
for the directory which happens at ToolPackageStoreAndQuery
from GetPackageDirectory
which already lowercased the version.................
TLDR: This is the correct approach and we dont need to lowercase this version I think 😵
af0c692
to
256a496
Compare
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.
LGTM, thanks!
256a496
to
2cd6f8f
Compare
src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageDependencies.cs
Outdated
Show resolved
Hide resolved
Hello @nagilson @dsplaisted, |
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.
One thing I thought was a side effect of this change at first is
that dotnet tool list will only show a lowercase version. This doesnt matter in terms of a breaking change on Linux because even in 7.0 it didnt quite work properly.
However on Windows it did, but it looks like even on Windows it was lowercased in the past, so that's not a concern.
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.
Fantastic work, this was much harder than anticipated. You did a great job digging through the code, thank you. I would not expect a typical new grad to be able to do that. Let's get this merged!
Thanks! It was very interesting and useful to get to know the tooling a bit better :) |
Addresses #39105 #24419 #41844, NuGet/Home#13844, etc
When attempting to install or update a tool on Linux, the system looks for the file with the version as is, instead of lowercasing it. Packages are ideally stored with lowercase versions on them according to the NuGet team, so the system fails to find them.
Instead, versions are lowercased and the
VersionFolderPathResolver
API is used for path operations to be compliant with changes from the NuGet codebase.