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

Detect cycle between root project and transitive dependency in new dependency resolver #6247

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Jan 31, 2025

Bug

Fixes: NuGet/Home#14052

Description

The new dependency resolver does not check if a transitive dependency happens to have the same name as the root project and instead replaces the root project in the graph with a dependency with a higher version. When the algorithm then tries to create a flattened graph, it throws a KeyNotFoundException trying to find the root project in the resolved graph.

This was unintentionally fixed in #6231 which made the root project always have its own ID but during resolution the algorithm now adds the package with the cycle and its detected during the flattening.

This change makes it so the name of the root project is indexed as 0 and then subsequent calls to index it will return the ID of the project. Then during resolution, there's a check to see a transitive dependency has a cyclic relationship with the root project and if so, leaves it out of the resolved graph. During the flattening of the graph, the cycle is then detected and an error message is emitted.

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.

…dependency with the same name as the root project.
@jeffkl jeffkl self-assigned this Jan 31, 2025
@jeffkl jeffkl requested a review from a team as a code owner January 31, 2025 17:41
nkolev92
nkolev92 previously approved these changes Jan 31, 2025
jebriede
jebriede previously approved these changes Jan 31, 2025
Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

Approve with minor and optional suggestion on test comment.

Nigusu-Allehu
Nigusu-Allehu previously approved these changes Feb 3, 2025
@jeffkl jeffkl enabled auto-merge (squash) February 6, 2025 22:52
@jeffkl jeffkl merged commit 6c434d1 into dev Feb 6, 2025
20 of 23 checks passed
@jeffkl jeffkl deleted the dev-jeffkl-dep-resolver-cycle-project branch February 6, 2025 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants