Make PackageSpecDependencyProvider consider the version being requested #5452

nkolev92 · 2023-10-02T22:00:16Z

Bug

Regression? Last working version:

Description

Replacement for #5397.
Projects are expected to be preferred over packages, but only when the version matches. Note that project version does that

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/SourceRepositoryDependencyProvider.cs

Lines 289 to 299 in 83912bc

    
           var packageVersion = packageVersions?.FindBestMatch(libraryRange.VersionRange, version => version); 
        
           if (packageVersion != null) 
        
           { 
        
               return new LibraryIdentity 
        
               { 
        
                   Name = libraryRange.Name, 
        
                   Version = packageVersion, 
        
                   Type = LibraryType.Package 
        
               }; 
        
           }

.

If this is not done, then projects may be selected in frameworks in which the project was never referenced.

More details in #5397 (comment).

Note that the test in question validates the scenario the runtime team is experiencing.

PR Checklist

nkolev92 · 2023-10-03T18:05:08Z

cc @ViktorHofer

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

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

donnie-msft

Not sure if the null version range could be a breaking change?

donnie-msft · 2023-10-06T14:59:06Z

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

@@ -86,7 +86,15 @@ public Library GetLibrary(LibraryRange libraryRange, NuGetFramework targetFramew
            // This must exist in the external references
            if (_externalProjectsByUniqueName.TryGetValue(name, out ExternalProjectReference externalReference))
            {
-                packageSpec = externalReference.PackageSpec;
+                if (externalReference.PackageSpec == null ||
+                    libraryRange.VersionRange.FindBestMatch(new NuGetVersion[] { externalReference.PackageSpec.Version }) != null)


VersionRange can be null to represent "all versions".

Suggested change

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

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

I'm not sure how this should work yet.
It's pretty surprising to me that we've gotten no null refs in the tests.

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

zivkan · 2023-10-13T09:14:05Z

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

Projects are expected to be preferred over packages, but only when the version matches

Why's that? It's not intuitive to me.

I really liked the project.json feature, from .NET Core 1.0 pre-release days, where you could add a package's project to the build, via global.json, and then it would use the project rather than the package. I thought it was regardless of version. This makes it much easier to debug and develop a package which has a bug in another project in a different repo.

Although perhaps this is straying too far from the linked bug and is a different feature request instead.

The version check allows you to be able to explicitly depend on a package if you choose to.
Now it'd replace package with project every single time.

There's obviously a lot of feature options here, like for example, being able to control whether project o package replacement is allowed.

ghost · 2023-10-20T22:00:27Z

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 7 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.

ViktorHofer · 2023-11-03T08:27:35Z

@nkolev92 please re-open this PR

nkolev92 · 2023-11-03T23:29:16Z

Thanks for reopening @jeffkl.

@ViktorHofer I'll get to this soon.

ghost · 2023-11-14T19:00:22Z

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 7 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.

ghost · 2023-11-28T22:00:27Z

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 7 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.

ViktorHofer · 2023-12-06T16:09:12Z

@nkolev92 we would really like to consume this bugfix in runtime as part of upgrading to a nightly 9.0 SDK / Preview 1. I don't think there's much left here, right?

ViktorHofer · 2024-01-08T11:50:57Z

ping @nkolev92, we really need to get this into the .NET 9 Preview 1 for runtime to be able to use ProjectReferences correctly with the new SDK bump. We have been waiting for this for months without a clear indication what was missing / no progress. Please re-open the PR. cc @ericstj

ghost · 2024-01-15T19:00:27Z

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 7 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.

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.

nkolev92 marked this pull request as ready for review October 3, 2023 00:04

nkolev92 requested a review from a team as a code owner October 3, 2023 00:04

jeffkl previously approved these changes Oct 3, 2023

View reviewed changes

jeffkl requested a review from a team October 3, 2023 17:38

ViktorHofer approved these changes Oct 3, 2023

View reviewed changes

kartheekp-ms reviewed Oct 4, 2023

View reviewed changes

test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/RestoreCommandTests.cs Show resolved Hide resolved

kartheekp-ms reviewed Oct 4, 2023

View reviewed changes

src/NuGet.Core/NuGet.ProjectModel/PackageSpecReferenceDependencyProvider.cs Show resolved Hide resolved

donnie-msft requested changes Oct 6, 2023

View reviewed changes

nkolev92 added 6 commits October 9, 2023 14:37

Make the project dep provider consider the version

17c8bf0

fix test

0885442

maybe?:D

8e230a3

cleanup

e73f43a

fix the test

37ec8c7

add a better test name and remove unnnecessary space

41cefd7

nkolev92 dismissed jeffkl’s stale review via 41cefd7 October 9, 2023 22:02

nkolev92 force-pushed the dev-nkolev92-10368 branch from fb5bf29 to 41cefd7 Compare October 9, 2023 22:02

zivkan reviewed Oct 13, 2023

View reviewed changes

ghost 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 Oct 20, 2023

ViktorHofer mentioned this pull request Oct 23, 2023

Use ProjectReferences for inbox libraries source projects dotnet/runtime#93877

Open

ghost closed this Oct 28, 2023

ghost 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 Nov 3, 2023

jeffkl reopened this Nov 3, 2023

jeffkl self-requested a review November 3, 2023 17:02

jeffkl marked this pull request as draft November 7, 2023 18:18

ghost 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 Nov 14, 2023

ghost closed this Nov 21, 2023

nkolev92 deleted the dev-nkolev92-10368 branch November 21, 2023 21:00

nkolev92 restored the dev-nkolev92-10368 branch November 21, 2023 21:00

nkolev92 reopened this Nov 21, 2023

ghost 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 Nov 21, 2023

ghost 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 Nov 28, 2023

ghost closed this Dec 6, 2023

ghost 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 Dec 6, 2023

nkolev92 reopened this Jan 8, 2024

ghost 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 Jan 15, 2024

ghost closed this Jan 22, 2024

nkolev92 deleted the dev-nkolev92-10368 branch July 12, 2024 18:59

ViktorHofer mentioned this pull request Aug 21, 2024

Make PackageSpecDependencyProvider consider the version being requested #5985

Open

3 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make PackageSpecDependencyProvider consider the version being requested #5452

Make PackageSpecDependencyProvider consider the version being requested #5452

nkolev92 commented Oct 2, 2023 •

edited

Loading

nkolev92 commented Oct 3, 2023

donnie-msft left a comment

donnie-msft Oct 6, 2023

nkolev92 Oct 10, 2023

zivkan Oct 13, 2023

nkolev92 Oct 13, 2023

ghost commented Oct 20, 2023

ViktorHofer commented Nov 3, 2023

nkolev92 commented Nov 3, 2023

ghost commented Nov 14, 2023

ghost commented Nov 28, 2023

ViktorHofer commented Dec 6, 2023

ViktorHofer commented Jan 8, 2024

ghost commented Jan 15, 2024

	var packageVersion = packageVersions?.FindBestMatch(libraryRange.VersionRange, version => version);

	if (packageVersion != null)
	{
	return new LibraryIdentity
	{
	Name = libraryRange.Name,
	Version = packageVersion,
	Type = LibraryType.Package
	};
	}

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

Make PackageSpecDependencyProvider consider the version being requested #5452

Make PackageSpecDependencyProvider consider the version being requested #5452

Conversation

nkolev92 commented Oct 2, 2023 • edited Loading

Bug

Description

PR Checklist

nkolev92 commented Oct 3, 2023

donnie-msft left a comment

Choose a reason for hiding this comment

donnie-msft Oct 6, 2023

Choose a reason for hiding this comment

nkolev92 Oct 10, 2023

Choose a reason for hiding this comment

zivkan Oct 13, 2023

Choose a reason for hiding this comment

nkolev92 Oct 13, 2023

Choose a reason for hiding this comment

ghost commented Oct 20, 2023

ViktorHofer commented Nov 3, 2023

nkolev92 commented Nov 3, 2023

ghost commented Nov 14, 2023

ghost commented Nov 28, 2023

ViktorHofer commented Dec 6, 2023

ViktorHofer commented Jan 8, 2024

ghost commented Jan 15, 2024

nkolev92 commented Oct 2, 2023 •

edited

Loading