Repository navigation
Add CycleAwareDependencyGraphDumper to visualize cycles - #2241
Open
arturobernalg wants to merge 1 commit into
Open
arturobernalg wants to merge 1 commit into
arturobernalg wants to merge 1 commit into
Conversation
DependencyGraphDumper recurses forever on a graph containing a cycle, and TreeDependencyVisitor avoids that crash only by hiding every node already seen anywhere in the graph, cycle or not, so a normal diamond dependency disappears from the output along with the actual cycle. CycleAwareDependencyGraphDumper only tracks the path from the root to the node currently being visited: it prints a node and marks it '(cycle)' once it reaches an ancestor of itself, instead of descending into it, and otherwise leaves diamonds fully visible. Fixing this also exposed a latent bug in formatIndentation(): it picked out the node currently being formatted by comparing it against nodes.peekFirst() with ==, which breaks once the same node instance appears twice in the path, i.e. exactly in a cycle. Comparing by position in the walk instead fixes it for both dumpers.
gnodet-bot
approved these changes
Oct 28, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
LGTM. No issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A dependency graph in FULL verbosity mode can contain a node that depends on an ancestor of itself. DependencyGraphDumper has no cycle protection, so it recurses forever on such a graph. The documented workaround, wrapping it in TreeDependencyVisitor, avoids the crash but tracks every node ever visited anywhere in the graph, not just the current path, so a normal diamond dependency reached a second time is skipped along with the actual cycle.
CycleAwareDependencyGraphDumper instead tracks only the path from the root to the node currently being visited. A node is treated as a cycle only when it's already its own ancestor on that path: it gets printed once with a "(cycle)" marker and its subtree is cut off (visitEnter returns false), while a diamond dependency is still printed in full wherever it occurs.
Getting this right surfaced a latent bug in the existing formatIndentation(): it picks out the node currently being formatted by comparing it to nodes.peekFirst() with ==. That breaks as soon as the same node instance appears twice in the path, which is exactly what happens in a cycle, producing garbled indentation. Comparing by position in the walk instead of identity fixes it for both dumpers.
Addresses #1561. A previous attempt at this (#1758) ran into review feedback on this same class of identity-comparison bug and was closed after going stale; this reuses the parent's own path/consumer instead of keeping a second deque, so the indentation logic isn't duplicated.
Added CycleAwareDependencyGraphDumperTest: one test builds the existing cycles.txt fixture and asserts the exact expected output (fails before the formatIndentation fix with garbled indentation), another asserts a cycle-free graph dumps identically to the plain DependencyGraphDumper.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
check one of the following: