Use CollectCentralPackageVersions NuGet.target Target when resolving GlobalPackageReference version

[Nigusu-Allehu]

Bug

Fixes: NuGet/Home#13632

Description

• If we have GlobalPackageRefernnce outside of Directory.Package.Props
• To assert there is no discrepancy between project and lock file, we compare packages between the two, however specifically for GLobalPackageRefernce packages, we do it by manually looking at the Directory.Packages.Props file. When we could not find the package there, we conclude there is a discrepancy between asset files and the project and as a result throw an exception.

This can be fixed by not manually checking the files but by reading from the project properties instead.

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.

[zivkan]

NuGet should not be assuming which file anything is defined in. See our own nuget.client repo as an example, we have a ton of .targets and .props file in the build/ directory, with our repo's build customization. I fear that this fix might work for customers who put their GlobalPackageReference in their Directory.Packages.props, but it will fail for customers who already have it in Directory.Build.props, or in a custom myrepo.props.

[nkolev92]

We should look it up the smae way we lookup PACKAGE_REFERENCE_TYPE_TAG.

now that I think about it, why are we treating GlobalPackageReference differently from any other packages?

[Nigusu-Allehu]

These are two great points. We should be aiming for a more seamless integration for MSBuild files. I have updated the code to be not dependent on specific files but rather get the information from the projects

[zivkan]

You wrote that you updated the code to not dependend on specific files, but this line of code still says project.Imports.FirstOrDefault(i => i.ImportedProject.FullPath.Equals(directoryPackagesPropsPath, PathUtility.GetStringComparisonBasedOnOS())).ImportedProject, so it's getting a specific file.

If this PR was about fixing a CPM bug in dotnet add package, I could understand this method change. But this PR is about dotnet list package, which is read only, so it's not intuitive to me why this ProjectRootElement GetDirectoryPackagePropsRootElement(Project project) method should exist at all.

Can you please explain its usage, to help me understand why we need to look up any ProjectRootElement?

edit: the only usage appears to be in a method named AddPackageVersionIntoItemGroupCPM, which sounds related to dotnet add package, not dotnet list package. In that case, I'm surprised no tests failed, which suggests we don't have any tests to make sure dotnet add package works with CPM. Additionally, we should add a second linked issue that describes the dotnet add package bug. Having said that, I really thought dotnet add package does already support CPM (writing PackageVersion to Directory.Packages.props)

[Nigusu-Allehu]

Thanks for pointing that out. The project.Imports.FirstOrDefault(i => i.ImportedProject.FullPath.Equals(directoryPackagesPropsPath, PathUtility.GetStringComparisonBasedOnOS())).ImportedProject is a little out of scope for this PR. It is found in a method called GetDirectoryBuildPropsRootElement and what it does basically is read the Directory.Package.Props file. However the name of the method says otherwise. I was just cleaning up the code so that the name of the method and its actions are in sync. I wil revert these updates since they are out of scope and make the PR confusing.

[nkolev92]

Can you please help me understand why we're getting a failure that indicates the assets file can't be found in the bug?

I feel like that's a bad error message, and while this may aim to fix the specific scenario bugs, it seems like there could be other cases where we'd get a failure and get a misleading message as well.
Can we try to fix that too?

[Nigusu-Allehu]

The error is as follows: Unable to read a package reference from the project 'PATH_TO_CSPROJ_FILE'. Please make sure that your project file and project.assets.json file are in sync by running restore.

The first sentence could be a little confusing. But I think the second sentence is clear and correct. This error occurs when a library exists in the asset file but could not be found in the project.

However, we ended up logging this error unexpectedly for the following reasons

• The person had their GlobalPackageRefernnce in Directory.Build.Props
• To assert there is no discrepancy between project and lock file, we compare packages between the two, however specifically for GLobalPackageRefernce packages, we do it by manually looking at the Directory.Packages.Props file. When we could not find the package there, we conclude there is a discrepancy between asset files and the project.

This can be fixed by not manually checking the files but by reading from the project properties instead.

[zivkan]

This error occurs when a library exists in the asset file but could not be found in the project.

A similar issue came up recently when a customer was trying to investigate vulnerabilities in their package, so I created this issue:

• dotnet list package should warn when restore is not current Home#13703

I think the ideal solution is that dotnet list package shouldn't read any PackageReferences from the project. Only use the MSBuild evaluation to find the assets file, nothing else. Everything else is read from the assets file. This gets rid of any complexity when the assets file and project file are out of sync, so less weird behaviour from the point of view of customers, and less complex code for us. Win-win. Well, except when it's out of sync.

In addition, we should check how we can use the MSBuild evaulation to create a PackageSpec of the project (if we switch MSBuild evaulation to static graph API, we can re-use code from static graph restore. But I hope it's not too hard to use our existing abstraction to generate a PackageSpec from a Project too), so that we can do a restore no-op check (check the PackageSpec hash against the one in project.nuget.cache), and when the hash is different, write a warning telling the customer a restore is needed.

[Nigusu-Allehu]

dotnet list package rendering results from the assets file would definitely be cleaner. However, we defined dotnet list package as a command that would list all package references in a project: https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-list-package. And, removing the project properties check would result in us providing "false information" when the two sources of information are out of sync.

However, if we trigger restore every time a user runs dotnet list package, we would be able to use the assets file only. This makes it less likely for the assets files and the project to be out of sync.

I like the idea of using MSBuild evaluation to create a package spec of the project. However, if we have the package spec, do we need to do a restore no-op check? Since the idea is to list the packageRefrences of a project, we could just list the dependencies form the package spec.

[Nigusu-Allehu]

A similar patter of directly checking Directory.Packages.Props is also used here

NuGet.Client/src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs

Line 369 in 2a15947

ProjectRootElement directoryBuildPropsRootElement = GetDirectoryBuildPropsRootElement(project);

. I did not get the time to look at its effects yet though.

[zivkan]

This error occurs when a library exists in the asset file but could not be found in the project.

A similar issue came up recently when a customer was trying to investigate vulnerabilities in their package, so I created this issue:

• dotnet list package should warn when restore is not current Home#13703

I think the ideal solution is that dotnet list package shouldn't read any PackageReferences from the project. Only use the MSBuild evaluation to find the assets file, nothing else. Everything else is read from the assets file. This gets rid of any complexity when the assets file and project file are out of sync, so less weird behaviour from the point of view of customers, and less complex code for us. Win-win. Well, except when it's out of sync.

In addition, we should check how we can use the MSBuild evaulation to create a PackageSpec of the project (if we switch MSBuild evaulation to static graph API, we can re-use code from static graph restore. But I hope it's not too hard to use our existing abstraction to generate a PackageSpec from a Project too), so that we can do a restore no-op check (check the PackageSpec hash against the one in project.nuget.cache), and when the hash is different, write a warning telling the customer a restore is needed.

[zivkan]

You wrote that you updated the code to not dependend on specific files, but this line of code still says project.Imports.FirstOrDefault(i => i.ImportedProject.FullPath.Equals(directoryPackagesPropsPath, PathUtility.GetStringComparisonBasedOnOS())).ImportedProject, so it's getting a specific file.

If this PR was about fixing a CPM bug in dotnet add package, I could understand this method change. But this PR is about dotnet list package, which is read only, so it's not intuitive to me why this ProjectRootElement GetDirectoryPackagePropsRootElement(Project project) method should exist at all.

Can you please explain its usage, to help me understand why we need to look up any ProjectRootElement?

edit: the only usage appears to be in a method named AddPackageVersionIntoItemGroupCPM, which sounds related to dotnet add package, not dotnet list package. In that case, I'm surprised no tests failed, which suggests we don't have any tests to make sure dotnet add package works with CPM. Additionally, we should add a second linked issue that describes the dotnet add package bug. Having said that, I really thought dotnet add package does already support CPM (writing PackageVersion to Directory.Packages.props)