NuGet.Client icon indicating copy to clipboard operation
NuGet.Client copied to clipboard

Make PackageSpecDependencyProvider consider the version being requested

Open ViktorHofer opened this issue 1 year ago • 4 comments

Bug

Fixes: https://github.com/NuGet/Home/issues/10368 Re-submission of https://github.com/NuGet/NuGet.Client/pull/5452

Description

https://github.com/dotnet/runtime/pull/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

  • [x] Meaningful title, helpful description and a linked NuGet/Home issue
  • [x] Added tests
  • [x] 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.

ViktorHofer avatar Aug 21 '24 13:08 ViktorHofer

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

image

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 avatar Sep 03 '24 14:09 ViktorHofer

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 avatar Sep 03 '24 14:09 ViktorHofer

The test failure in CI is unrelated to this change.

ViktorHofer avatar Sep 03 '24 14:09 ViktorHofer

@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 🙇‍♂️

ericstj avatar Sep 04 '24 19:09 ericstj

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.