find_smallest_cycle: Do not return single node for asap mode #1182
Conversation
For asap mode, it is possible for gather_deps to prematurely select a single node instead of a cycle, as observed in bug 917259. Continue search for a true cycle in this case. When not in asap mode, we sometimes need to select a single node here, as demonstrated by PerlRebuildBugTestCase which selects automake this way. The included AlternativesGzipTestCase demonstrates correct behavior, though it fails to reproduce bug 917259. Even though this patch has not yet been proven to fix bug 917259, it is still useful because it limits the ability of find_smallest_cycle to misbehave. Bug: https://bugs.gentoo.org/917259 Signed-off-by: Zac Medico <zmedico@gentoo.org>
There was a problem hiding this comment.
Thank you! I'm not really that worried about the test because I've not seen any instances of this in the wild apart from that one report, and I feel like with the combination of this and the Perl test, we already have a lot better coverage for this than we did before.
| Test bug 917259, where app-alternatives/gzip was upgraded before | ||
| its pigz RDEPEND was installed. | ||
| """ | ||
| ebuilds = { |
There was a problem hiding this comment.
Yeah, I imagine we need some deep @system deps in-between in order to get the bad behaviour to reproduce.
If I had to guess, maybe it involved pigz depending on zlib, but dunno. Not going to worry about it unless we can reproduce it, as the problem is pretty well-understood.
| @@ -9339,6 +9341,15 @@ def find_smallest_cycle(mergeable_nodes, local_priority_range): | |||
| if gather_deps( | |||
| priority, mergeable_nodes, selected_nodes, node | |||
| ): | |||
| if len(selected_nodes) == 1 and asap_mode: | |||
There was a problem hiding this comment.
Thanks. I should've really thought of this before when we spoke about it. We should still obviously think about proper priorities but this is good enough.
|
oh, I guess we should tag bug 756199 too, even if it's not fully fixed (or is it now?) |
|
I'll just close this one for now, since I reproduced the bug and found that changing the topological sort to remove only one node at a time solved the problem: diff --git a/lib/_emerge/depgraph.py b/lib/_emerge/depgraph.py
index 8d72c3241..1c99ade43 100644
--- a/lib/_emerge/depgraph.py
+++ b/lib/_emerge/depgraph.py
@@ -9464,8 +9464,8 @@ class depgraph:
ignore_priority=ignore_priority
)
if leaves:
- cycle_digraph.difference_update(leaves)
- selected_nodes.extend(leaves)
+ cycle_digraph.remove(leaves[0])
+ selected_nodes.append(leaves[0])
break
else:
selected_nodes.extend(cycle_digraph) |
For asap mode, it is possible for gather_deps to prematurely select a single node instead of a cycle, as observed in bug
917259. Continue search for a true cycle in this case.
When not in asap mode, we sometimes need to select a single node here, as demonstrated by PerlRebuildBugTestCase which selects automake this way.
The included AlternativesGzipTestCase demonstrates correct behavior, though it fails to reproduce bug 917259. Even though this patch has not yet been proven to fix bug 917259, it is still useful because it limits the ability of find_smallest_cycle to misbehave.
Bug: https://bugs.gentoo.org/917259