Make PackageSpecDependencyProvider consider the version being requested #5985

ViktorHofer · 2024-08-21T13:29:10Z

Bug

Fixes: NuGet/Home#10368
Re-submission of #5452

Description

dotnet/runtime#106329 is currently blocked because of this NuGet tooling bug.

NuGet gets confused with transitive project and package dependencies with the same identity (name) in a multi-targeting project. It incorrectly selects the wrong type (project instead of package).

Projects are expected to be preferred over packages, but only when the version matches. If the version doesn't match, projects shouldn't be selected in frameworks which don't declare a ProjectReference to those projects.

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

cc @nkolev92

I'm happy to do as much validation as necessary. Please understand that this defect currently blocks one of our planned infrastructure modernization items. We can't merge the runtime PR until this fix flows into both the .NET SDK and VS and we can depend on the newer toolchain as otherwise restore would fail.

Fixes: NuGet/Home#10368 Re-submit of NuGet#5452 NuGet gets confused with transitive project and package dependencies with the same identity (name) in a multi-targeting project and selects the wrong type (project instead of package). Projects are expected to be preferred over packages, but only when the version matches. If the version doesn't match, projects shouldn't be selected in frameworks which don't declare a ProjectReference to those projects.

ViktorHofer · 2024-09-03T14:27:09Z

The following entry gets added to a project's project.assets.json file with this change:

NuGet apparently records that the project node can't be selected and therefore prefers the package one. I don't think that's a big deal but it would be better if that project node wouldn't exist at all.

Meaning, this is a good change overall but we should file a follow-up issue to make NuGet not even consider the project node (because the project doesn't have a P2P, even transitively, to it).

ViktorHofer · 2024-09-03T14:30:06Z

I tested this change on top of my PR in runtime with the following steps on Windows:

mkdir temp-git
cd temp-git

# Prep nuget-client
git clone https://github.com/ViktorHofer/NuGet.Client
cd NuGet.Client
git checkout 10368fix
.\configure.cmd
.\build.cmd
# The build eventually fails to invoke some tests because of a missing 8.0.100 SDK (at least on my machine) but that isn't interesting for this validation.

# Prep runtime
cd ..
git clone https://github.com/ViktorHofer/runtime
cd runtime
git checkout UseP2PsEverywhere

# Restore runtime and the SDK and notice the NU1201 errors
.\build.cmd -restore

# Patch the local SDK
cp ..\NuGet.Client\artifacts\NuGet.Versioning\bin\Debug\netstandard2.0\NuGet.Versioning.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Packaging\bin\Debug\netcoreapp5.0\NuGet.Packaging.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.ProjectModel\bin\Debug\netcoreapp5.0\NuGet.ProjectModel.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Protocol\bin\Debug\netcoreapp5.0\NuGet.Protocol.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Credentials\bin\Debug\netcoreapp5.0\NuGet.Credentials.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.DependencyResolver.Core\bin\Debug\netcoreapp5.0\NuGet.DependencyResolver.Core.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Frameworks\bin\Debug\netstandard2.0\NuGet.Frameworks.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.LibraryModel\bin\Debug\netstandard2.0\NuGet.LibraryModel.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Commands\bin\Debug\netcoreapp5.0\NuGet.Commands.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Common\bin\Debug\netstandard2.0\NuGet.Common.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Configuration\bin\Debug\netstandard2.0\NuGet.Configuration.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Build.Tasks.Console\bin\Debug\net8.0\NuGet.Build.Tasks.Console.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.Build.Tasks\bin\Debug\net8.0\NuGet.Build.Tasks.dll .dotnet\sdk\9.0.100-preview.7.24407.12\
cp ..\NuGet.Client\artifacts\NuGet.CommandLine.XPlat\bin\Debug\net8.0\NuGet.CommandLine.XPlat.dll .dotnet\sdk\9.0.100-preview.7.24407.12\

# Restore runtime without any errors
.\build.cmd -restore

ViktorHofer · 2024-09-03T14:50:50Z

The test failure in CI is unrelated to this change.

ericstj · 2024-09-04T19:33:32Z

@nkolev92 is this something you can help review? Please let us know if you'd like to see additional changes or validation here. Thank you 🙇‍♂️

Nigusu-Allehu · 2024-09-04T20:43:26Z

test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/RestoreCommandTests.cs

+            var request = ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, mainPackageSpec, systemNumericsVectorPackageSpec);
+
+            var restoreCommand = new RestoreCommand(request);
+            RestoreResult result = await restoreCommand.ExecuteAsync();
+
+            result.Success.Should().BeTrue(because: logger.ShowMessages());
+            result.LockFile.LogMessages.Should().BeEmpty();
+            var net80Target = result.LockFile.Targets.Single(e => e.TargetFramework.Equals(NuGetFramework.Parse("net8.0")));
+            var netstandard20 = result.LockFile.Targets.Single(e => e.TargetFramework.Equals(NuGetFramework.Parse("netstandard2.0")));
+            net80Target.Libraries.Should().HaveCount(1);
+            var net80Vectors = net80Target.Libraries.Single(e => e.Name.Equals(systemNumericsVectorName));
+            net80Vectors.Version.Should().Be(new NuGetVersion(1, 0, 0));
+            netstandard20.Libraries.Should().HaveCount(3);
+            var ns20Target = netstandard20.Libraries.Single(e => e.Name.Equals(systemNumericsVectorName));
+            ns20Target.Version.Should().Be(new NuGetVersion(4, 4, 0));


Suggested change

var request = ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, mainPackageSpec, systemNumericsVectorPackageSpec);

var restoreCommand = new RestoreCommand(request);

RestoreResult result = await restoreCommand.ExecuteAsync();

result.Success.Should().BeTrue(because: logger.ShowMessages());

result.LockFile.LogMessages.Should().BeEmpty();

var net80Target = result.LockFile.Targets.Single(e => e.TargetFramework.Equals(NuGetFramework.Parse("net8.0")));

var netstandard20 = result.LockFile.Targets.Single(e => e.TargetFramework.Equals(NuGetFramework.Parse("netstandard2.0")));

net80Target.Libraries.Should().HaveCount(1);

var net80Vectors = net80Target.Libraries.Single(e => e.Name.Equals(systemNumericsVectorName));

net80Vectors.Version.Should().Be(new NuGetVersion(1, 0, 0));

netstandard20.Libraries.Should().HaveCount(3);

var ns20Target = netstandard20.Libraries.Single(e => e.Name.Equals(systemNumericsVectorName));

ns20Target.Version.Should().Be(new NuGetVersion(4, 4, 0));

var request = ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, mainPackageSpec, systemNumericsVectorPackageSpec);

var restoreCommand = new RestoreCommand(request);

// Act

RestoreResult result = await restoreCommand.ExecuteAsync();

// Assert

result.Success.Should().BeTrue(because: logger.ShowMessages());

result.LockFile.LogMessages.Should().BeEmpty();

var net80Target = result.LockFile.Targets.Single(e => e.TargetFramework.Equals(NuGetFramework.Parse("net8.0")));

var netstandard20 = result.LockFile.Targets.Single(e => e.TargetFramework.Equals(NuGetFramework.Parse("netstandard2.0")));

net80Target.Libraries.Should().HaveCount(1);

var net80Vectors = net80Target.Libraries.Single(e => e.Name.Equals(systemNumericsVectorName));

net80Vectors.Version.Should().Be(new NuGetVersion(1, 0, 0));

netstandard20.Libraries.Should().HaveCount(3);

var ns20Target = netstandard20.Libraries.Single(e => e.Name.Equals(systemNumericsVectorName));

ns20Target.Version.Should().Be(new NuGetVersion(4, 4, 0));

Nigusu-Allehu · 2024-09-04T21:00:35Z

src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs

@@ -101,7 +101,17 @@ public Library GetLibrary(LibraryRange libraryRange, NuGetFramework targetFramew
            // This must exist in the external references
            if (_externalProjectsByUniqueName.TryGetValue(name, out ExternalProjectReference externalReference))
            {
-                packageSpec = externalReference.PackageSpec;
+                // A library with a null version range means that all versions should match.
+                if (externalReference.PackageSpec == null ||


Do we need this check: externalReference.PackageSpec == null?

@nkolev92 had it in the original PR. This check guards against null before the PackageSpec is dereferenced in the later check externalReference.PackageSpec.Version.

Nigusu-Allehu · 2024-09-04T22:24:58Z

src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs

            if (_externalProjectsByUniqueName.TryGetValue(name, out ExternalProjectReference externalReference))
            {
-                packageSpec = externalReference.PackageSpec;
+                // A library with a null version range means that all versions should match.
+                if (externalReference.PackageSpec == null ||
+                    libraryRange.VersionRange == null ||
+                    libraryRange.VersionRange.FindBestMatch(new NuGetVersion[] { externalReference.PackageSpec.Version }) != null)
+                {
+                    packageSpec = externalReference.PackageSpec;
+                }
+                else
+                {
+                    externalReference = null;
+                }
            }

            if (externalReference == null && packageSpec == null)


Suggested change

if (_externalProjectsByUniqueName.TryGetValue(name, out ExternalProjectReference externalReference))

{

packageSpec = externalReference.PackageSpec;

// A library with a null version range means that all versions should match.

if (externalReference.PackageSpec == null ||

libraryRange.VersionRange == null ||

libraryRange.VersionRange.FindBestMatch(new NuGetVersion[] { externalReference.PackageSpec.Version }) != null)

{

packageSpec = externalReference.PackageSpec;

}

else

{

externalReference = null;

}

}

if (externalReference == null && packageSpec == null)

if (!_externalProjectsByUniqueName.TryGetValue(name, out ExternalProjectReference externalReference))

{

// No project found

return null;

}

if (externalReference.PackageSpec != null &&

libraryRange.VersionRange != null &&

libraryRange.VersionRange.FindBestMatch(new NuGetVersion[] { externalReference.PackageSpec.Version }) == null))

{

// The package does not have a project with a version that best matches the range

return null;

}

packageSpec = externalReference.PackageSpec;

ViktorHofer requested a review from a team as a code owner August 21, 2024 13:29

dotnet-policy-service bot added the Community PRs created by someone not in the NuGet team label Aug 21, 2024

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 28, 2024

Update PackageSpecReferenceDependencyProvider.cs

59e82dd

dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 2, 2024

Nigusu-Allehu reviewed Sep 4, 2024

View reviewed changes

aortiz-msft added the Merge next release PRs that should not be merged until the dev branch targets the next release label Sep 5, 2024

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 13, 2024

nkolev92 self-assigned this Sep 16, 2024

dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 16, 2024

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 27, 2024

nkolev92 removed the Merge next release PRs that should not be merged until the dev branch targets the next release label Sep 27, 2024

dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make PackageSpecDependencyProvider consider the version being requested #5985

Make PackageSpecDependencyProvider consider the version being requested #5985

ViktorHofer commented Aug 21, 2024 •

edited

Loading

ViktorHofer commented Sep 3, 2024 •

edited

Loading

ViktorHofer commented Sep 3, 2024 •

edited

Loading

ViktorHofer commented Sep 3, 2024 •

edited

Loading

ericstj commented Sep 4, 2024

Nigusu-Allehu Sep 4, 2024

Nigusu-Allehu Sep 4, 2024 •

edited

Loading

ericstj Sep 16, 2024

Nigusu-Allehu Sep 4, 2024

Make PackageSpecDependencyProvider consider the version being requested #5985

Are you sure you want to change the base?

Make PackageSpecDependencyProvider consider the version being requested #5985

Conversation

ViktorHofer commented Aug 21, 2024 • edited Loading

Bug

Description

PR Checklist

ViktorHofer commented Sep 3, 2024 • edited Loading

ViktorHofer commented Sep 3, 2024 • edited Loading

ViktorHofer commented Sep 3, 2024 • edited Loading

ericstj commented Sep 4, 2024

Nigusu-Allehu Sep 4, 2024

Choose a reason for hiding this comment

Nigusu-Allehu Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

ericstj Sep 16, 2024

Choose a reason for hiding this comment

Nigusu-Allehu Sep 4, 2024

Choose a reason for hiding this comment

ViktorHofer commented Aug 21, 2024 •

edited

Loading

ViktorHofer commented Sep 3, 2024 •

edited

Loading

ViktorHofer commented Sep 3, 2024 •

edited

Loading

ViktorHofer commented Sep 3, 2024 •

edited

Loading

Nigusu-Allehu Sep 4, 2024 •

edited

Loading