diff --git a/src/rust/engine/graph/src/lib.rs b/src/rust/engine/graph/src/lib.rs index 4bfb39ed6dc..a6ed4f0cc4d 100644 --- a/src/rust/engine/graph/src/lib.rs +++ b/src/rust/engine/graph/src/lib.rs @@ -144,13 +144,19 @@ impl InnerGraph { let (running_candidate, should_terminate) = if let Some(dirty_candidate) = running_scc .iter() .filter(|&id| self.pg[running_graph[*id]].is_cleaning()) - .max() + .max_by_key(|&id| running_graph[*id]) { // Nodes are being cleaned: clear the highest id entry. (dirty_candidate, false) } else { // There are no nodes being cleaned: terminate the Running node with the highest id. - (running_scc.iter().max().unwrap(), true) + ( + running_scc + .iter() + .max_by_key(|&id| running_graph[*id]) + .unwrap(), + true, + ) }; test_trace_log!( @@ -168,7 +174,7 @@ impl InnerGraph { // predecessor (which as a fellow member of the SCC, must also be reachable). let running_predecessor = running_graph .neighbors_directed(*running_candidate, Direction::Incoming) - .next() + .find(|id| running_scc.contains(id)) .unwrap(); let running_path: Vec<_> = petgraph::algo::all_simple_paths( &running_graph, @@ -184,11 +190,12 @@ impl InnerGraph { let candidate = running_graph[*running_candidate]; if should_terminate { // Render the error, and terminate the Node with it. - let path_strs = running_path + let path = running_path .into_iter() - .map(|rni| self.pg[rni].node().to_string()) - .collect(); - self.pg[candidate].terminate(N::Error::cyclic(path_strs)); + .map(|rni| self.pg[running_graph[rni]].node()) + .collect::>(); + let error = N::cyclic_error(&path); + self.pg[candidate].terminate(error); } else { // Else, clear. let node = self.pg[candidate].node().clone(); diff --git a/src/rust/engine/graph/src/node.rs b/src/rust/engine/graph/src/node.rs index 211b03b8b4b..a597ebf26f1 100644 --- a/src/rust/engine/graph/src/node.rs +++ b/src/rust/engine/graph/src/node.rs @@ -53,6 +53,12 @@ pub trait Node: Clone + Debug + Display + Eq + Hash + Send + 'static { fn cacheable_item(&self, _item: &Self::Item) -> bool { self.cacheable() } + + /// + /// Creates an error instance that represents that a Node dependency was cyclic along the given + /// path. + /// + fn cyclic_error(path: &[&Self]) -> Self::Error; } pub trait NodeError: Clone + Debug + Eq + Send + Sync { @@ -61,11 +67,6 @@ pub trait NodeError: Clone + Debug + Eq + Send + Sync { /// Graph (generally while running). /// fn invalidated() -> Self; - - /// - /// Creates an instance that represents that a Node dependency was cyclic along the given path. - /// - fn cyclic(path: Vec) -> Self; } /// diff --git a/src/rust/engine/graph/src/tests.rs b/src/rust/engine/graph/src/tests.rs index ca7b4930816..b034c40019c 100644 --- a/src/rust/engine/graph/src/tests.rs +++ b/src/rust/engine/graph/src/tests.rs @@ -624,7 +624,7 @@ async fn cyclic_failure() { assert_eq!( graph.create(TNode::new(2), &context).await, - Err(TError::Cyclic) + Err(TError::Cyclic(vec![0, 2, 1])) ); } @@ -740,6 +740,10 @@ impl Node for TNode { fn cacheable(&self) -> bool { self.cacheable } + + fn cyclic_error(path: &[&Self]) -> Self::Error { + TError::Cyclic(path.iter().map(|n| n.id).collect()) + } } impl std::fmt::Display for TNode { @@ -991,15 +995,11 @@ impl Drop for AbortGuard { #[derive(Clone, Debug, Eq, PartialEq)] enum TError { - Cyclic, + Cyclic(Vec), Invalidated, } impl NodeError for TError { fn invalidated() -> Self { TError::Invalidated } - - fn cyclic(_path: Vec) -> Self { - TError::Cyclic - } } diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 4b52864941c..7f2c5ba193b 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -1569,6 +1569,32 @@ impl Node for NodeKey { _ => true, } } + + fn cyclic_error(path: &[&NodeKey]) -> Failure { + let mut path = path.iter().map(|n| n.to_string()).collect::>(); + if !path.is_empty() { + path[0] += " <-"; + path.push(path[0].clone()); + } + let gil = Python::acquire_gil(); + let url = externs::doc_url( + gil.python(), + "targets#dependencies-and-dependency-inference", + ); + throw(format!( + "The dependency graph contained a cycle:\ + \n\n \ + {}\ + \n\n\ + If the dependencies in the above path are for your BUILD targets, you may need to use more \ + granular targets or replace BUILD target dependencies with file dependencies. If they are \ + not for your BUILD targets, then please file a Github issue!\ + \n\n\ + See {} for more information.", + path.join("\n "), + url + )) + } } impl Display for NodeKey { @@ -1613,32 +1639,6 @@ impl NodeError for Failure { fn invalidated() -> Failure { Failure::Invalidated } - - fn cyclic(mut path: Vec) -> Failure { - let path_len = path.len(); - if path_len > 1 { - path[0] += " <-"; - path[path_len - 1] += " <-" - } - let gil = Python::acquire_gil(); - let url = externs::doc_url( - gil.python(), - "targets#dependencies-and-dependency-inference", - ); - throw(format!( - "The dependency graph contained a cycle:\ - \n\n \ - {}\ - \n\n\ - If the dependencies in the above path are for your BUILD targets, you may need to use more \ - granular targets or replace BUILD target dependencies with file dependencies. If they are \ - not for your BUILD targets, then please file a Github issue!\ - \n\n\ - See {} for more information.", - path.join("\n "), - url - )) - } } #[derive(Clone, Debug, Eq, PartialEq)]