Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix new resolver when using transitive pinning to resolve subgraphs correctly #6149

Merged
merged 11 commits into from
Dec 16, 2024

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Nov 16, 2024

Bug

Fixes: NuGet/Home#13938

Description

This fixes the new dependency resolver when transitive pinning is enabled and a package subgraph happens to contain versions that should or should not be eclipsed.

The way eclipsing works is for each subgraph, the first one wins:

Project1
└── A 1.0.0
    ├── B 1.0.0  
    └── C 1.0.0
        └── B 2.0.0

In this case, B 2.0.0 is part of the graph of A which already depends on B 1.0.0 so B 1.0.0 is chosen.

However, if a higher version comes in through a different subgraph, a different version of B could win:

Project1
├── A 1.0.0
│   ├── B 1.0.0  
│   └── C 1.0.0
│       └── B 2.0.0
└── F 1.0.0
    └── B 3.0.0

In this case, B 3.0.0 is not in the subgraph of A and its version eclipses B 1.0.0.

Transitive pinning is supposed to be syntactic sugar for a direct dependency and so if you pinned C 1.0.0 in the first example, the resolver treats the graph as:

Project1
├── A 1.0.0
│   └── B 1.0.0  
└── C 1.0.0
    └── B 2.0.0

Now B 2.0.0 is not part of the subgraph of A and the resolver should pick B 2.0.0. However, the current implementation of the new resolver only looks to see if the proposed B 2.0.0 and the already chosen B 1.0.0 have the common ancestors. This leads to the new resolver resolving the wrong graph and logging downgrade warnings as described in NuGet/Home#13938

The fix is to modify the paths for each item if it is pinned to correctly assess if a package should eclipse another. But when the paths are modified, the resolver must also keep track of the original parent in order to correctly create downgrade warnings if one exists.

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.

@jeffkl jeffkl force-pushed the dev-nkolev92-additionalAlgoTests branch from 4ceba0a to bac3158 Compare November 19, 2024 21:39
@microsoft-github-policy-service microsoft-github-policy-service bot 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 27, 2024
@jeffkl jeffkl force-pushed the dev-nkolev92-additionalAlgoTests branch from bac3158 to bfb9b58 Compare December 2, 2024 20:04
@microsoft-github-policy-service microsoft-github-policy-service bot 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 2, 2024
@jeffkl jeffkl force-pushed the dev-nkolev92-additionalAlgoTests branch 3 times, most recently from b9bc2bd to 34d7faf Compare December 4, 2024 21:32
@jeffkl jeffkl force-pushed the dev-nkolev92-additionalAlgoTests branch from 34d7faf to e0a77bc Compare December 5, 2024 17:58
@jeffkl jeffkl self-assigned this Dec 9, 2024
@jeffkl jeffkl changed the title Addtional algo tests Fix new resolver when using transitive pinning to resolve subgraphs correctly Dec 9, 2024
@jeffkl jeffkl marked this pull request as ready for review December 9, 2024 18:45
@jeffkl jeffkl requested a review from a team as a code owner December 9, 2024 18:45
jeffkl
jeffkl previously approved these changes Dec 10, 2024
jgonz120
jgonz120 previously approved these changes Dec 10, 2024
Copy link
Contributor

@jgonz120 jgonz120 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine! I definitely don't know much about this area, that method is massive though. A refactor would definitely help make it easier to understand.

@jeffkl jeffkl dismissed stale reviews from jgonz120 and themself via 2057add December 10, 2024 19:32
Copy link
Member Author

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in DependencyGraphResolver look correct.

Still not 100% convinced RestoreCommand_WithPackageDrivenDowngradeAndTransitivePinning_ElevatesTransitiveToDirect_AndDoesNotRaiseWarning is the best solution, despite what the old algorithm did.

I did want to do add some IncludeAssets/ExcludeAssets tests to make sure they're not broken with this change.

@nkolev92 nkolev92 enabled auto-merge (squash) December 16, 2024 18:27
@nkolev92 nkolev92 requested a review from jgonz120 December 16, 2024 18:27
@nkolev92 nkolev92 merged commit 7e6aa2e into dev Dec 16, 2024
23 checks passed
@nkolev92 nkolev92 deleted the dev-nkolev92-additionalAlgoTests branch December 16, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants