-
Notifications
You must be signed in to change notification settings - Fork 689
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
Fix packages.config to project mapping in NuGet.exe #5818
Conversation
da3905f
to
1c21d27
Compare
Dictionary<PackageReference, List<string>> packageReferenceToProjects = new(PackageReferenceComparer.Instance); | ||
|
||
foreach (string configFile in packageRestoreInputs.PackagesConfigFiles) | ||
foreach (string configFile in packageRestoreInputs.PackagesConfigFiles.Distinct()) |
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.
Ensure we don't process the same config twice.
There's more performant ways of doing but:
- It's NuGet.exe, so this cost is not everywhere and very minor.
- More code = more risk, potentially more challenges getting approval.
834f1b8
to
0b08ee6
Compare
@@ -276,24 +276,24 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac | |||
|
|||
if (packageRestoreInputs.RestoringWithSolutionFile) | |||
{ | |||
Dictionary<string, string> configToProjectPath = GetPackagesConfigToProjectPath(packageRestoreInputs); | |||
Dictionary<string, HashSet<string>> configToProjectPath = GetPackagesConfigToProjectsPath(packageRestoreInputs); |
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.
While multiple projects to 1 packages.config is not recommended, it is obviously something that does happen, and I don't want to rock the boat in a patch release, so the fix is to just handle that scenario.
https://github.com/NuGet/Client.Engineering/issues/2851 covers more.
test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetRestoreCommandTest.cs
Show resolved
Hide resolved
test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs
Show resolved
Hide resolved
395167e
to
0ffdb5a
Compare
Blocked by some restore issues. |
e35086b
to
547267d
Compare
Bug
Fixes: NuGet/Home#13456
Regression? Last working version:
Description
The implementation for PC vulnerability checking did not account for the fact that multiple projects might be in the same folder and lead to duplicate packages.config to project mapping.
This fixes that and adds test cases in both nuget.exe restore and msbuild -t:restore.
I've add some in line comments to aid with reviewing.
Please note that this will be patched in 6.10, so take that into consideration when reviewing (ie. focus on correctness, extra clean-up can be done as follow-up and review the tests for correctness).
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation