From ad0dc8528fd35b27027e23897c2feddbb7357cb4 Mon Sep 17 00:00:00 2001 From: Jeff Kluge Date: Fri, 13 Sep 2024 13:06:24 -0700 Subject: [PATCH 1/2] Properly handle missing package versions in new dependency resolver --- .../RestoreCommand/DependencyGraphResolver.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs b/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs index 7a3e83e5a60..62fbb23ccce 100644 --- a/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs +++ b/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs @@ -205,6 +205,12 @@ async static (state) => IReadOnlyDictionary currentOverrides = importRefItem.VersionOverrides!; bool directPackageReferenceFromRootProject = importRefItem.IsDirectPackageReferenceFromRootProject; + // Packages with missing versions should not be added to the graph + if (currentRef.LibraryRange.VersionRange == null) + { + continue; + } + if (!findLibraryEntryCache.TryGetValue(currentRefRangeIndex, out Task? refItemTask)) { Debug.Fail("This should not happen"); From 8f9e46701a0d857e76a240cc5c023c592e6e907d Mon Sep 17 00:00:00 2001 From: Jeff Kluge Date: Fri, 13 Sep 2024 14:38:48 -0700 Subject: [PATCH 2/2] Update and add test --- .../RestoreCommand/DependencyGraphResolver.cs | 22 ++++--- ...estoreCommand_AlgorithmEquivalencyTests.cs | 63 +++++++++++++++++++ 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs b/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs index 62fbb23ccce..55832c49083 100644 --- a/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs +++ b/src/NuGet.Core/NuGet.Commands/RestoreCommand/DependencyGraphResolver.cs @@ -205,12 +205,6 @@ async static (state) => IReadOnlyDictionary currentOverrides = importRefItem.VersionOverrides!; bool directPackageReferenceFromRootProject = importRefItem.IsDirectPackageReferenceFromRootProject; - // Packages with missing versions should not be added to the graph - if (currentRef.LibraryRange.VersionRange == null) - { - continue; - } - if (!findLibraryEntryCache.TryGetValue(currentRefRangeIndex, out Task? refItemTask)) { Debug.Fail("This should not happen"); @@ -656,6 +650,12 @@ async static (state) => for (int i = 0; i < refItemResult.Item.Data.Dependencies.Count; i++) { var dep = refItemResult.Item.Data.Dependencies[i]; + // Packages with missing versions should not be added to the graph + if (dep.LibraryRange.VersionRange == null) + { + continue; + } + LibraryDependencyIndex depIndex = refItemResult.GetDependencyIndexForDependency(i); if ((dep.SuppressParent == LibraryIncludeFlags.All) && (importRefItem.LibraryDependencyIndex != rootProjectRefItem.LibraryDependencyIndex)) { @@ -724,8 +724,8 @@ async static (state) => LibraryDependency dep = refItemResult.Item.Data.Dependencies[i]; LibraryDependencyIndex depIndex = refItemResult.GetDependencyIndexForDependency(i); - //Suppress this node - if (!importRefItem.IsCentrallyPinnedTransitivePackage && suppressions!.Contains(depIndex)) + // Skip this node if the VersionRange is null or if its not transitively pinned and PrivateAssets=All + if (dep.LibraryRange.VersionRange == null || (!importRefItem.IsCentrallyPinnedTransitivePackage && suppressions!.Contains(depIndex))) { continue; } @@ -918,6 +918,12 @@ async static (state) => for (int i = 0; i < node.Item.Data.Dependencies.Count; i++) { var dep = node.Item.Data.Dependencies[i]; + + if (dep.LibraryRange.VersionRange == null) + { + continue; + } + if (StringComparer.OrdinalIgnoreCase.Equals(dep.Name, node.Item.Key.Name) || StringComparer.OrdinalIgnoreCase.Equals(dep.Name, rootGraphNode.Key.Name)) { // Cycle diff --git a/test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommand_AlgorithmEquivalencyTests.cs b/test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommand_AlgorithmEquivalencyTests.cs index 7828e8a7f7c..9b1bd7de36a 100644 --- a/test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommand_AlgorithmEquivalencyTests.cs +++ b/test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommand_AlgorithmEquivalencyTests.cs @@ -884,6 +884,69 @@ public async Task RestoreCommand_HigherLevelSuppressionsWin_VerifiesEquivalency( result.LockFile.Targets[0].Libraries[2].Name.Should().Be("X"); } + [Theory] + [InlineData(true)] + [InlineData(false)] + // Project 1 -> Project 2 -> a (null) + public async Task RestoreCommand_WithNullPackageVersion_AndRaisesErrorForOneProjectAndNotTheOther(bool isCPMEnabled) + { + // Arrange + using var pathContext = new SimpleTestPathContext(); + var packageA = new SimpleTestPackageContext("a", "1.0.0"); + + await SimpleTestPackageUtility.CreateFolderFeedV3Async( + pathContext.PackageSource, + PackageSaveMode.Defaultv3, + packageA); + + var project2spec = ProjectTestHelpers.GetPackageSpec("Project2", + pathContext.SolutionRoot, + framework: "net472"); + + project2spec.TargetFrameworks[0].Dependencies.Add(new LibraryDependency( + new LibraryRange( + "a", + versionRange: isCPMEnabled ? null : VersionRange.All, + LibraryDependencyTarget.PackageProjectExternal))); + + var project1spec = ProjectTestHelpers.GetPackageSpec("Project1", + pathContext.SolutionRoot, + framework: "net472") + .WithTestProjectReference(project2spec); + + project1spec.RestoreMetadata.CentralPackageVersionsEnabled = isCPMEnabled; + project2spec.RestoreMetadata.CentralPackageVersionsEnabled = isCPMEnabled; + + // Act & Assert + (var result, _) = await ValidateRestoreAlgorithmEquivalency(pathContext, project1spec, project2spec); + + if (isCPMEnabled) + { + // Additional assert + result.Success.Should().BeTrue(); + result.LogMessages.Should().BeEmpty(); + + result.LockFile.Targets.Should().HaveCount(1); + result.LockFile.Targets[0].Libraries.Should().HaveCount(1); + result.LockFile.Targets[0].Libraries[0].Name.Should().Be("Project2"); + result.LockFile.Targets[0].Libraries[0].Version.Should().Be(new NuGetVersion("1.0.0")); + } + else + { + + result.Success.Should().BeTrue(); + result.LogMessages.Select(e => e.Code).Should().BeEquivalentTo([NuGetLogCode.NU1602]); + + result.LockFile.Targets.Should().HaveCount(1); + result.LockFile.Targets[0].Libraries.Should().HaveCount(2); + result.LockFile.Targets[0].Libraries[0].Name.Should().Be("a"); + result.LockFile.Targets[0].Libraries[0].Version.Should().Be(new NuGetVersion("1.0.0")); + + result.LockFile.Targets[0].Libraries[1].Name.Should().Be("Project2"); + result.LockFile.Targets[0].Libraries[1].Version.Should().Be(new NuGetVersion("1.0.0")); + } + } + // Here's why package driven dependencies should flow. // Say we have P1 -> P2 -> P3 -> A 1.0.0 -> B 2.0.0 // -> B 1.5.0