Fix null reference in PM UI for project references without framework - #6030
Conversation
| if (frameworks.Length == 0) | ||
| if (frameworks is null) | ||
| { | ||
| frameworks = Array.Empty<NuGetFramework>(); |
There was a problem hiding this comment.
When is the frameworks element empty in a real project?
There was a problem hiding this comment.
Can you explain these changes?
| if (frameworks.Length == 0) | ||
| if (frameworks is null) | ||
| { | ||
| frameworks = Array.Empty<NuGetFramework>(); |
There was a problem hiding this comment.
Can you explain these changes?
| .TargetFrameworks | ||
| .SelectMany(f => GetProjectPackageReferences(f.FrameworkName, targetsList)) | ||
| .GroupBy(p => p.PackageIdentity) | ||
| .Select(g => g.OrderBy(p => p.TargetFramework, FrameworkSorter).First()) |
There was a problem hiding this comment.
In here, you're basically deduplicating and choosing the first one.
Can this cause issues when different frameworks of the same project have different references?
Would all of them be shown?
netstandard2.0
P1 -> P2 -> A, B
net472
P1 -> P2 -> A, C
This can be done in a follow up since it seems like this is not a new behavior, but how the old works as well.
There was a problem hiding this comment.
This is definitely going to cause issues with multiple frameworks, currently the PM UI uses this FrameworkSorter to decide which packages to show, this behavior is also present in top-level 😭 we need to design a better solution for this problem
There was a problem hiding this comment.
Here's my understanding.
- The PM UI will show all transitive packages.
- If a package is available with different versions in a different framework, the PM UI does not guarantee which one of the versions is shown.
My fear here was whether assumption #1 is correct.
It feels like a bigger issue.
There was a problem hiding this comment.
👀I'm not following the sort & choose first TFM logic. Can a test case be added that demonstrates why the first TFM works? In Nikolche's example I think this means only net472 will be added to projectReferences for group p.PackageIdentity ?
There was a problem hiding this comment.
The PM UI will show all transitive packages.
You are right, it will show all top level and transitive's, but just as you mentioned in second point we do not guarantee which one is shown. There cannot be any duplicated package items in the same level (Top and transitive).
I thought the PM UI was choosing a framework before creating the list, at least we are displaying more information that I believed.
If a package is available with different versions in a different framework, the PM UI does not guarantee which one of the versions is shown.
With the above configuration we see that the transitive origin data is using the other package version.
|
@donnie-msft @nkolev92 I merged the PR since the open comment wasn't related to this PR changes. But definitely something to have in mind. |
3 participants
Bug
Fixes: NuGet/Home#13737
Description
Project References are now considered when getting the projects transitive packages, in some scenarios there is no framework in the project reference on the assets file, this caused a null reference in the PM UI when trying to get the framework.
This PR will ignore those project references (in the PM UI for showing transitive's) that don't have a null reference.
PR Checklist