Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 24, 2024
Merged

Linux: Installing/Updating packages with uppercase versions #43988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d882b5c
[FIX] Save package files with lowercase versions
edvilme Oct 10, 2024
2cd6f8f
[REF] Use NuGet VersionFolderPathResolver
edvilme Oct 10, 2024
84bd0c6
Fix pr comments
edvilme Oct 17, 2024
095af59
Fix file permissions
edvilme Oct 17, 2024
eae1491
Fix additional pr comments
edvilme Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,10 @@ public async Task<string> DownloadPackageAsync(PackageId packageId,
string.Format(LocalizableStrings.IsNotFoundInNuGetFeeds, packageId, source.Source));
}

string nupkgPath = downloadFolder == null || !downloadFolder.HasValue
? Path.Combine(_packageInstallDir.Value, packageId.ToString(),
resolvedPackageVersion.ToNormalizedString(),
$"{packageId}.{resolvedPackageVersion.ToNormalizedString()}.nupkg")
: Path.Combine(downloadFolder.Value.Value,
$"{packageId}.{resolvedPackageVersion.ToNormalizedString()}.nupkg");

var pathResolver = new VersionFolderPathResolver(downloadFolder == null || !downloadFolder.HasValue ? _packageInstallDir.Value : downloadFolder.Value.Value);
nagilson marked this conversation as resolved.
Show resolved Hide resolved
string nupkgPath = pathResolver.GetPackageFilePath(packageId.ToString(), resolvedPackageVersion);
Directory.CreateDirectory(Path.GetDirectoryName(nupkgPath));

using FileStream destinationStream = File.Create(nupkgPath);
bool success = await ExponentialRetry.ExecuteWithRetryOnFailure(async () => await resource.CopyNupkgToStreamAsync(
packageId.ToString(),
Expand Down Expand Up @@ -218,7 +214,10 @@ public async Task<string> GetPackageUrl(PackageId packageId,
SourceRepository repository = GetSourceRepository(source);
if (repository.PackageSource.IsLocal)
{
return Path.Combine(repository.PackageSource.Source, $"{packageId}.{resolvedPackageVersion}.nupkg");
return Path.Combine(
edvilme marked this conversation as resolved.
Show resolved Hide resolved
repository.PackageSource.Source,
new VersionFolderPathResolver(repository.PackageSource.Source).GetPackageFileName(packageId.ToString(), resolvedPackageVersion)
);
}

ServiceIndexResourceV3 serviceIndexResource = repository.GetResourceAsync<ServiceIndexResourceV3>().Result;
Expand Down Expand Up @@ -307,9 +306,8 @@ await GetPackageMetadataAsync(packageId.ToString(), packageVersion, packagesSour
return (source, packageVersion);
}

private string GetNupkgUrl(string baseUri, PackageId id, NuGetVersion version) =>
baseUri + id.ToString() + "/" + version.ToNormalizedString() + "/" + id.ToString() +
"." + version.ToNormalizedString() + ".nupkg";
private string GetNupkgUrl(string baseUri, PackageId id, NuGetVersion version) => baseUri + id.ToString() + "/" + version.ToNormalizedString() + "/" + id.ToString() +
"." + version.ToNormalizedString().ToLowerInvariant() + ".nupkg";

internal IEnumerable<FilePath> FindAllFilesNeedExecutablePermission(IEnumerable<string> files,
string targetPath)
Expand Down
6 changes: 3 additions & 3 deletions src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,11 @@ public IToolPackage InstallPackage(PackageLocation packageLocation, PackageId pa
}
NuGetVersion packageVersion = nugetPackageDownloader.GetBestPackageVersionAsync(packageId, versionRange, packageSourceLocation).GetAwaiter().GetResult();

rollbackDirectory = isGlobalTool ? toolDownloadDir.Value: Path.Combine(toolDownloadDir.Value, packageId.ToString(), packageVersion.ToString());
rollbackDirectory = isGlobalTool ? toolDownloadDir.Value: new VersionFolderPathResolver(toolDownloadDir.Value).GetInstallPath(packageId.ToString(), packageVersion);

if (isGlobalTool)
{
NuGetv3LocalRepository nugetPackageRootDirectory = new(Path.Combine(_toolPackageStore.GetRootPackageDirectory(packageId).ToString().Trim('"'), packageVersion.ToString()));
NuGetv3LocalRepository nugetPackageRootDirectory = new(new VersionFolderPathResolver(_toolPackageStore.Root.Value).GetInstallPath(packageId.ToString(), packageVersion));
var globalPackage = nugetPackageRootDirectory.FindPackage(packageId.ToString(), packageVersion);

if (globalPackage != null)
Expand Down Expand Up @@ -303,7 +303,7 @@ private static async Task<NuGetVersion> DownloadAndExtractPackage(
}

// Extract the package
var nupkgDir = Path.Combine(packagesRootPath, packageId.ToString(), version.ToString());
var nupkgDir = new VersionFolderPathResolver(packagesRootPath).GetInstallPath(packageId.ToString(), version);
await nugetPackageDownloader.ExtractPackageAsync(packagePath, new DirectoryPath(nupkgDir));

return version;
Expand Down
8 changes: 5 additions & 3 deletions src/Cli/dotnet/ToolPackage/ToolPackageInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.DotNet.Tools;
using Microsoft.Extensions.EnvironmentAbstractions;
using NuGet.Frameworks;
using NuGet.Packaging;
Copy link
Member

@nagilson nagilson Oct 14, 2024 •
edited
Loading

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.

Copy link
Member

@nagilson nagilson Oct 24, 2024 •
edited
Loading

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 😵

using NuGet.ProjectModel;
using NuGet.Versioning;
nagilson marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -85,7 +86,8 @@ public ToolPackageInstance(PackageId id,
_lockFile =
new Lazy<LockFile>(
() => new LockFileFormat().Read(assetsJsonParentDirectory.WithFile(AssetsFileName).Value));
var toolsPackagePath = Path.Combine(PackageDirectory.Value, Id.ToString(), Version.ToNormalizedString(), "tools");
var installPath = new VersionFolderPathResolver(PackageDirectory.Value).GetInstallPath(Id.ToString(), Version);
var toolsPackagePath = Path.Combine(installPath, "tools");
Frameworks = Directory.GetDirectories(toolsPackagePath)
.Select(path => NuGetFramework.ParseFolder(Path.GetFileName(path)));
}
Expand Down Expand Up @@ -127,7 +129,7 @@ private FilePath LockFileRelativePathToFullFilePath(string lockFileRelativePath,
return PackageDirectory
.WithSubDirectories(
Id.ToString(),
library.Version.ToNormalizedString())
library.Version.ToNormalizedString().ToLowerInvariant())
.WithFile(lockFileRelativePath);
}

Expand Down Expand Up @@ -209,7 +211,7 @@ private ToolConfiguration DeserializeToolConfiguration(LockFileTargetLibrary lib
PackageDirectory
.WithSubDirectories(
Id.ToString(),
library.Version.ToNormalizedString())
library.Version.ToNormalizedString().ToLowerInvariant())
.WithFile(dotnetToolSettings.Path);

var configuration = ToolConfigurationDeserializer.Deserialize(toolConfigurationPath.Value);
Expand Down
2 changes: 1 addition & 1 deletion src/Cli/dotnet/commands/dotnet-add/dotnet-add-package/AddPackageParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ internal static class AddPackageParser
var allowPrerelease = context.ParseResult.GetValue(PrereleaseOption);
return QueryVersionsForPackage(packageId, context.WordToComplete, allowPrerelease, CancellationToken.None)
.Result
.Select(version => new CompletionItem(version.ToNormalizedString()));
.Select(version => new CompletionItem(version.ToNormalizedString().ToLowerInvariant()));
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/Mocks/MockPackageResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public string GetPackageDirectory(string packageId, NuGetVersion version)
public string GetPackageDirectory(string packageId, NuGetVersion version, out string packageRoot)
{
packageRoot = _root;
return Path.Combine(_root, packageId, version.ToNormalizedString(), "path");
return Path.Combine(_root, packageId, version.ToNormalizedString().ToLowerInvariant(), "path");
}

public string ResolvePackageAssetPath(LockFileTargetLibrary package, string relativePath) => Path.Combine(GetPackageDirectory(package.Name, package.Version), relativePath);
Expand Down
Loading