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: topological sort when cycles appear in leaf nodes #879

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Sep 30, 2024

There were two issues with our topological sort implementation:

  • cycles between two noarch packages or two non-noarch packages were never "broken". This is fixed by this PR (going to add a test next)
  • When the root node (or leaf node) contained a cycle, it was not part of the result. The code is changed to first detect any and all cycles in the graph, and then find the root nodes with these cycles in mind.

The changes came up because a user was installing holoviews which has a circular dependency on panel. It failed later in the un-clobbering (because some packages were just missing after the topological sort because of this faulty "root" detection).

if p1_noarch && !p2_noarch {
cycle_breaks.insert((pi1.clone(), pi2.clone()));
break;
} else if !p1_noarch && p2_noarch {
} else if !p1_noarch && p2_noarch || i == cycle.len() - 1 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the added condition here to also return a cycle break if none of the packages is noarch and we're in the last iteration.

@baszalmstra baszalmstra merged commit 8e86a8a into conda:main Oct 1, 2024
15 checks passed
@baszalmstra baszalmstra mentioned this pull request Oct 1, 2024
@wolfv wolfv deleted the fix-topological-sort branch October 1, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants