dotnet nuget why reports requested as well as resolved versions #7017
Conversation
|
Using the same example that @rainersigwald used in the original feature request, it looks like this now: 
Big thanks to @Frulfump for the formatting idea that I really like and implemented. |
| "net9.0/linux-x64": { | ||
| }, |
There was a problem hiding this comment.
I deleted this file and the test that uses it, because RID specific targets still use the same top level package as the RIDless target, so it doesn't make sense for the rid specific target to not have the direct packages that the project defines under $.project.frameworks
| "dependencies": { | ||
| "Microsoft.NETCore.Platforms": "1.1.1", | ||
| "Microsoft.NETCore.Targets": "1.1.3" |
There was a problem hiding this comment.
I deleted this file and the test that uses it, because I believe it's an error for these packages to be listed as a dependency of another package, but then not be a library itself in the targets or libraries sections of the assets file. It was causing my refactor of DependerGraphFinder to crash.
However, I did find a real world project that had an issue like this: NuGet/Home#14698
So, perhaps we could restore this file and its test. But I don't understand the scenario that led to this. I assume this assets file was hand-edited, rather than copied unmodified after a restore. I've been unable to create a simple repro that I can use in a test.
dotnet-policy-service
bot
added
the
Status:No recent activity
zivkan
removed
the
Status:No recent activity
Frulfump
left a comment
Frulfump
left a comment
There was a problem hiding this comment.
Great to see this being implemented, can't wait to use it!
| } | ||
| } | ||
| } | ||
| throw new Exception(Strings.WhyCommand_Error_InconsistentAssetsFile); |
There was a problem hiding this comment.
Not sure if applicable here but this would trigger CA2201 Don't throw reserved exception types CA2201
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2201 if that analyzer is enabled
|
|
||
| // if we have already traversed this node's children, continue | ||
| if (visited.Contains(currentPackageId)) | ||
| if (userInputFrameworks.Count > 0 && |
There was a problem hiding this comment.
nit: I prefer the conditions being on the newline. It may be what most of our repo does as well.
martinrrm
left a comment
martinrrm
left a comment
There was a problem hiding this comment.
Left some comments not blocking this PR. Great refactor!
| { | ||
| string resolved = pkgNode.ResolvedVersion.OriginalVersion ?? pkgNode.ResolvedVersion.ToString(); | ||
| string requested = pkgNode.RequestedVersion.ToString("p", VersionRangeFormatter.Instance); | ||
| text = $"{node.Id}@{resolved} ({requested})"; |
There was a problem hiding this comment.
I like how we are displaying the requested version, but is inconsistent with the format in other commands like dotnet package list
For example, a floating version would look like this in list:
And different in why
I'm not saying that one is better than the other, especially since the commands have a very different output, but we should be aiming to have the same display format for requested versions.
| return false; | ||
|
|
||
| return string.Equals(x.Id, y.Id, StringComparison.CurrentCultureIgnoreCase); | ||
| return string.Equals(Id, other.Id, StringComparison.CurrentCultureIgnoreCase) |
There was a problem hiding this comment.
Do we need CurrentCulture for package Id's?
There was a problem hiding this comment.
Good catch! We should be using OrdinalIgnoreCase
Bug
Fixes: NuGet/Home#13752
FIxes: NuGet/Home#14695
Description
Shows the requested and resolved versions for each package. Removes the version for project references, making it more obvious that the graph node is indeed a project.
The above feature modified the same lines needed to fix running why on a legacy csproj, so both are being fixed in the same pull request, rather than two PRs that would need to be opened sequentially.
I used copilot to implement showing the requested version, but when reviewing its changes to DependencyGraphFinder.cs, I found the file's algorithm confusing. So, I commit copilot's changes unreviewed, then rewrote the class from scratch, with a few small changes in other files. Overall, it reduces product code by about 100 lines. If my rewrite isn't liked, we can revert the refactor commit from the branch, but then if someone has questions about the changes to DependencyGraphFinder, I may not be able to answer it.
The algorithm of the new class is, for each target in the assets file, create an in-memory
LockFileTargetLibrarywith the project's direct package and project references as dependencies, then recursively convert aLockFileTargetLibraryinto aDependencyNode. It does this with no intermediate state, other than the "fake"LockFileTargetLibraryfor the project's root. The tree is filtered for the package requested on thedotnet package whycommand line, but the way it's written, it will be trivial (change one if statement) to either show the complete unfiltered tree, or filter on multiple packages, or a partial package id match.The implementation has a performance optimization, so it doesn't create the tree/graph and then filter as a second step. Instead, it returns nulls when the package is filtered out and there are no children. Dependencies that return null are removed, so when no dependency has the package, the node looks like it has no dependencies and so also gets removed if the package name also doesn't match. So, entire subtrees that don't contain the searched requested package avoid being created, with minimal (hopefully zero) memory allocations.
In order to have stronger guarantees on nullability, I modified
DependencyNodeto be an abstract class soPackageNodes must have both a requested version and resolved version.The refactor found some tests that use an invalid assets file. I deleted these tests as the multi-rid nature of the tests are very difficult to replicate, but mostly test scenarios that theoretically possible, but not really practical. I'll comment on the two assets files explaining which each is infeasible, as justification why to delete the test rather than spending a lot of effort to fix it.
PR Checklist