Skip to content

Commit 87a5d2f

Browse files
committed
Show example cycle when ls-apis errors about them
`cargo xtask ls-apis check` will report an error if the software component graph contains a cycle, as well as if the deployment units form a cycle, but prints only the name of a node in the graph that was in the cycle. If you've added a cycle to either of these graphs, the error and node can tell you about the what that is wrong. Going a bit further and printing an example cycle including the troublesome node can help with the how you got there. With this change, the first approach at #10231 (which introduced a cycle in, but all in one deployment unit: sled-agent -> propolis-server -> sled-agent) errors with a more clear message about propolis-server and sled-agent being the problems: ``` Error: graph of server-managed API dependencies between components has a cycle (includes node: "propolis-server", example cycle: propolis-server, omicron-sled-agent) ```
1 parent b845b31 commit 87a5d2f

1 file changed

Lines changed: 45 additions & 4 deletions

File tree

dev-tools/ls-apis/src/system_apis.rs

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,39 @@ impl<'a> IdOrdItem for FilteredApiConsumer<'a> {
119119
id_upcast!();
120120
}
121121

122+
/// Find a cycle in `graph` and render that cycle as a String. Returns None if
123+
/// there is no cycle containing `node`.
124+
///
125+
/// This is primarily useful for debugging, especially when considering graphs
126+
/// that are typically acyclic. Any cycle containing `node` is, hopefully,
127+
/// informative. There is no particular order promised about how the cycle is
128+
/// printed.
129+
fn print_example_cycle<Name: fmt::Display>(
130+
graph: &Graph<&Name, &ClientPackageName>,
131+
reverse_nodes: &BTreeMap<&NodeIndex, &&Name>,
132+
node: &NodeIndex,
133+
) -> Option<String> {
134+
let mut sccs = petgraph::algo::scc::tarjan_scc(&graph);
135+
sccs.retain(|scc| scc.contains(node));
136+
137+
let Some(first_cycle) = sccs.get(0) else {
138+
return None;
139+
};
140+
141+
let mut cycle_rendered = String::new();
142+
for component in first_cycle.iter() {
143+
if !cycle_rendered.is_empty() {
144+
cycle_rendered.push_str(", ");
145+
}
146+
147+
use fmt::Write;
148+
write!(cycle_rendered, "{}", reverse_nodes.get(&component).unwrap())
149+
.unwrap()
150+
}
151+
152+
Some(cycle_rendered)
153+
}
154+
122155
impl SystemApis {
123156
/// Load information about APIs in the system based on both developer-
124157
/// maintained metadata and Cargo-provided metadata
@@ -861,10 +894,14 @@ impl SystemApis {
861894
let reverse_nodes: BTreeMap<_, _> =
862895
nodes.iter().map(|(s_c, node)| (node, s_c)).collect();
863896
if let Err(error) = petgraph::algo::toposort(&graph, None) {
897+
let example_cycle =
898+
print_example_cycle(&graph, &reverse_nodes, &error.node_id())
899+
.expect("graph has a cycle containing the node");
864900
bail!(
865901
"graph of server-managed API dependencies between components \
866-
has a cycle (includes node: {:?})",
867-
reverse_nodes.get(&error.node_id()).unwrap()
902+
has a cycle (includes node: {:?}, example cycle: {})",
903+
reverse_nodes.get(&error.node_id()).unwrap(),
904+
example_cycle,
868905
);
869906
}
870907

@@ -876,10 +913,14 @@ impl SystemApis {
876913
let reverse_nodes: BTreeMap<_, _> =
877914
nodes.iter().map(|(d_u, node)| (node, d_u)).collect();
878915
if let Err(error) = petgraph::algo::toposort(&graph, None) {
916+
let example_cycle =
917+
print_example_cycle(&graph, &reverse_nodes, &error.node_id())
918+
.expect("graph has a cycle containing the node");
879919
bail!(
880920
"graph of server-managed API dependencies between deployment \
881-
units has a cycle (includes node: {:?})",
882-
reverse_nodes.get(&error.node_id()).unwrap()
921+
units has a cycle (includes node: {:?}, example cycle: {})",
922+
reverse_nodes.get(&error.node_id()).unwrap(),
923+
example_cycle,
883924
);
884925
}
885926

0 commit comments

Comments
 (0)