Refactor new dependency graph resolution algorithm #6231
Conversation
jeffkl
force-pushed
the
dev-jeffkl-dependencygraphresolver-refactor
branch
from
f75e418 to
b9d32cd
Compare
| { | ||
| // Create a new array, copy the existing path into it, and add the new library range index | ||
| LibraryRangeIndex[] newPath = new LibraryRangeIndex[existingPath.Length + 1]; | ||
|
|
There was a problem hiding this comment.
format: The spacing here is probably unnecessary.
| /// <summary> | ||
| /// Gets or sets a <see cref="Task{TResult}" /> that returns a <see cref="GraphItem{TItem}" /> containing a <see cref="RemoteResolveResult" /> that represents the resolved graph item after looking it up in the configured feeds. | ||
| /// </summary> | ||
| public required Task<GraphItem<RemoteResolveResult>> FindLibraryTask { get; set; } |
There was a problem hiding this comment.
public required Task<GraphItem<RemoteResolveResult>> FindLibraryTask { get; init; }
| /// <summary> | ||
| /// Gets or sets a value indicating whether or not this dependency graph item is a transitively pinned dependency. | ||
| /// </summary> | ||
| public bool IsCentrallyPinnedTransitivePackage { get; set; } |
There was a problem hiding this comment.
Looking at this, all of these need to be init.
Checked locally it compiles if you do that.
| /// <returns><see langword="true" /> if the package should be pruned, otherwise <see langword="false" />.</returns> | ||
| private static bool ShouldPrunePackage( | ||
| RestoreRequest restoreRequest, | ||
| IReadOnlyDictionary<string, PrunePackageReference>? packagesToPrune, |
There was a problem hiding this comment.
Seems like you decide to go back to string lookup for pruning?
Was that intentional?
| LibraryDependency dependency, | ||
| LibraryIdentity parentLibrary, | ||
| bool isRootProject, | ||
| RemoteWalkContext context, |
There was a problem hiding this comment.
This is unused, now that you're using the logger everywhere.
| public GraphItem<RemoteResolveResult> Item { get; } | ||
| if (chosenDependencyGraphItemSuppressions.Count == 1 && chosenDependencyGraphItemSuppressions[0].Count == 0 && HasCommonAncestor(chosenResolvedItem.Path, currentDependencyGraphItemPath)) | ||
| { | ||
| // Skip this item if it has no suppressions and has a common ancestor |
There was a problem hiding this comment.
This is confusing to me there's a suppression with no hashset of suppressions and one with.
What's the difference between them?
Do you have a test suggestion that can help me make sense of this?
|
|
||
| public LibraryRangeIndex RangeIndex { get; } | ||
| bool isCentrallyPinnedTransitivePackage = chosenResolvedItem.IsCentrallyPinnedTransitivePackage; |
There was a problem hiding this comment.
Similar as others, unnecessary obfuscation.
This might also warrant a comment.
The idea is that if a package was transitive pinned before, i remains the same.
| { | ||
| return _dependencyIndices[dependencyIndex]; | ||
| } | ||
| chosenResolvedItem = new ResolvedDependencyGraphItem(currentGraphItem, _indexingTable) |
There was a problem hiding this comment.
nit: I know it's not used later, but it feels like we shouldn't be reassigning the property we spent all the time comparing against in a non-while loop :D
| // 3. This child is not a direct package reference and there is already a direct package reference to it | ||
| if (childLibraryDependency.LibraryRange.VersionRange == null | ||
| || (!currentDependencyGraphItem.IsCentrallyPinnedTransitivePackage | ||
| && suppressions!.Contains(childLibraryDependencyIndex)) |
There was a problem hiding this comment.
Indent this one level further. It's the same condition as 1334.
| hasInstallBeenCalledAlready = true; | ||
| } | ||
|
|
||
| TargetFrameworkInformation? projectTargetFramework = _request.Project.GetTargetFramework(pair.Framework); | ||
| // Get the corresponding TargetFrameworkInformation from the restore request | ||
| TargetFrameworkInformation? projectTargetFramework = _request.Project.GetTargetFramework(frameworkRuntimePair.Framework); |
There was a problem hiding this comment.
This should never be null.
If it is null, the projectFramewokrRuntimePairs generation is wrong.
Alternatively the framework runtime pairs, can include this type.
Bug
Related: https://github.com/NuGet/Client.Engineering/issues/3008
Description
This is a major refactoring of the new dependency resolution algorithm to clean up the code and add more comments. Some of the changes include:
ResolveAsync()method is easier to readDependencyGraphResolverapartialclass and move its child classes to separate filesTo validate these changes, I've run restores against some key repositories and verified that the performance is the same and soare the outputs. All tests are passing and the only test updates I made were to improve reliability. One of the tests depended on a specific order of packages which is no longer deterministic due to parallelization.
Once this is merged, I will be working on a markdown file explaining the design and implementation of the algorithm for future contributors.
PR Checklist