Skip to content

Commit

Permalink
feat: port package graph validation (#5566)
Browse files Browse the repository at this point in the history
### Description

Porting our cycle detection from Go to Rust

### Testing Instructions

Added new unit tests. Compare to
[ValidateGraph](https://github.com/vercel/turborepo/blob/261aea9cb4f30204bef8d3f638326f9b2dd9e85a/cli/internal/util/graph.go#L12)
in Go

---------

Co-authored-by: Chris Olszewski <Chris Olszewski>
  • Loading branch information
chris-olszewski authored Jul 21, 2023
1 parent 95d090a commit c0ee0de
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 2 deletions.
4 changes: 4 additions & 0 deletions crates/turborepo-lib/src/package_graph/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ pub enum Error {
PackageJson(#[from] crate::package_json::Error),
#[error("package.json must have a name field")]
PackageJsonMissingName,
#[error("cyclic dependency detected:\n{0}")]
CyclicDependencies(String),
#[error("{0} depends on itself")]
SelfDependency(WorkspaceNode),
#[error(transparent)]
Lockfile(#[from] turborepo_lockfiles::Error),
}
Expand Down
127 changes: 125 additions & 2 deletions crates/turborepo-lib/src/package_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use std::{
};

use anyhow::Result;
use itertools::Itertools;
use petgraph::visit::EdgeRef;
use turbopath::{AbsoluteSystemPath, AnchoredSystemPathBuf};
use turborepo_lockfiles::Lockfile;

Expand Down Expand Up @@ -63,8 +65,33 @@ impl PackageGraph {
PackageGraphBuilder::new(repo_root, root_package_json)
}

pub fn validate(&self) -> Result<()> {
// TODO
pub fn validate(&self) -> Result<(), builder::Error> {
// This is equivalent to AcyclicGraph.Cycles from Go's dag library
let cycles_lines = petgraph::algo::tarjan_scc(&self.workspace_graph)
.into_iter()
.filter(|cycle| cycle.len() > 1)
.map(|cycle| {
let workspaces = cycle
.into_iter()
.map(|id| self.workspace_graph.node_weight(id).unwrap());
format!("\t{}", workspaces.format(", "))
})
.join("\n");

if !cycles_lines.is_empty() {
return Err(builder::Error::CyclicDependencies(cycles_lines));
}

for edge in self.workspace_graph.edge_references() {
if edge.source() == edge.target() {
let node = self
.workspace_graph
.node_weight(edge.source())
.expect("edge pointed to missing node");
return Err(builder::Error::SelfDependency(node.clone()));
}
}

Ok(())
}

Expand Down Expand Up @@ -143,6 +170,14 @@ impl fmt::Display for WorkspaceName {
}
}

impl fmt::Display for WorkspaceNode {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
WorkspaceNode::Root => f.write_str("ROOT_NODE"),
WorkspaceNode::Workspace(workspace) => workspace.fmt(f),
}
}
}
impl From<String> for WorkspaceName {
fn from(value: String) -> Self {
Self::Other(value)
Expand All @@ -157,6 +192,8 @@ impl<'a> From<&'a str> for WorkspaceName {

#[cfg(test)]
mod test {
use std::assert_matches::assert_matches;

use serde_json::json;
use turbopath::AbsoluteSystemPathBuf;

Expand All @@ -175,6 +212,7 @@ mod test {
let closure =
pkg_graph.transitive_closure(Some(&WorkspaceNode::Workspace(WorkspaceName::Root)));
assert!(closure.contains(&WorkspaceNode::Root));
assert!(pkg_graph.validate().is_ok());
}

#[test]
Expand Down Expand Up @@ -213,6 +251,7 @@ mod test {
.build()
.unwrap();

assert!(pkg_graph.validate().is_ok());
let closure = pkg_graph.transitive_closure(Some(&WorkspaceNode::Workspace("a".into())));
assert_eq!(
closure,
Expand Down Expand Up @@ -327,6 +366,7 @@ mod test {
.build()
.unwrap();

assert!(pkg_graph.validate().is_ok());
let foo = WorkspaceName::from("foo");
let bar = WorkspaceName::from("bar");

Expand Down Expand Up @@ -354,4 +394,87 @@ mod test {
HashSet::from_iter(vec![&a, &b, &c,])
);
}

#[test]
fn test_circular_dependency() {
let root =
AbsoluteSystemPathBuf::new(if cfg!(windows) { r"C:\repo" } else { "/repo" }).unwrap();
let pkg_graph = PackageGraph::builder(
&root,
PackageJson::from_value(json!({ "name": "root" })).unwrap(),
)
.with_package_manger(Some(PackageManager::Npm))
.with_package_jsons(Some({
let mut map = HashMap::new();
map.insert(
root.join_component("package_a"),
PackageJson::from_value(json!({
"name": "foo",
"dependencies": {
"bar": "*"
}
}))
.unwrap(),
);
map.insert(
root.join_component("package_b"),
PackageJson::from_value(json!({
"name": "bar",
"dependencies": {
"baz": "*",
}
}))
.unwrap(),
);
map.insert(
root.join_component("package_c"),
PackageJson::from_value(json!({
"name": "baz",
"dependencies": {
"foo": "*",
}
}))
.unwrap(),
);
map
}))
.with_lockfile(Some(Box::new(MockLockfile {})))
.build()
.unwrap();

assert_matches!(
pkg_graph.validate(),
Err(builder::Error::CyclicDependencies(_))
);
}

#[test]
fn test_self_dependency() {
let root =
AbsoluteSystemPathBuf::new(if cfg!(windows) { r"C:\repo" } else { "/repo" }).unwrap();
let pkg_graph = PackageGraph::builder(
&root,
PackageJson::from_value(json!({ "name": "root" })).unwrap(),
)
.with_package_manger(Some(PackageManager::Npm))
.with_package_jsons(Some({
let mut map = HashMap::new();
map.insert(
root.join_component("package_a"),
PackageJson::from_value(json!({
"name": "foo",
"dependencies": {
"foo": "*"
}
}))
.unwrap(),
);
map
}))
.with_lockfile(Some(Box::new(MockLockfile {})))
.build()
.unwrap();

assert_matches!(pkg_graph.validate(), Err(builder::Error::SelfDependency(_)));
}
}

0 comments on commit c0ee0de

Please sign in to comment.