From 5b04fcdac39a24d6a0c2e76fef30e35bcefadbdf Mon Sep 17 00:00:00 2001 From: Michael Weiss Date: Fri, 3 Jan 2025 15:31:23 +0100 Subject: [PATCH] Replace the `daggy` crate with `petgraph` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `daggy` crate doesn't seem maintained anymore as the last commit was over three years ago [0] and there isn't really any activity since then. Daggy is already based on top of `petgraph` and described as follows: > The most prominent type is Dag - a wrapper around petgraph’s Graph > data structure, exposing a refined API targeted towards directed > acyclic graph related functionality. This means that we can switch directly to `petgraph` without too much effort. We'll loose some of the refined API, especially walking graphs via iterators, but it doesn't really matter too much as the `Acyclic` type, "a wrapper around graph types that enforces an acyclicity invariant", works well enough (just a bit less "refined"). We also already used `petgraph` directly for the `tree-of` command. This direct dependency on `petgraph` became the trigger for dropping the dependency on `daggy` since `petgraph` yanked the release of version `0.6.6` with the new release of version `0.7.0` [1] and the resulting CI failure for the `petgraph` update [2] made me take a closer look at the situation (we don't necessarily have to drop `daggy` just yet but it seems for the best given it's unclear future and the duplicated `petgraph` dependency that causes incompatibilities / build failures). [0]: https://github.com/mitchmindtree/daggy [1]: https://github.com/petgraph/petgraph/issues/712 [2]: https://github.com/science-computing/butido/pull/446 Signed-off-by: Michael Weiss --- Cargo.lock | 11 -------- Cargo.toml | 1 - src/commands/tree_of.rs | 12 +++------ src/job/dag.rs | 18 ++++++------- src/package/dag.rs | 58 ++++++++++++++++++++++++----------------- 5 files changed, 46 insertions(+), 54 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 206886c4..ce155460 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -221,7 +221,6 @@ dependencies = [ "colored", "config", "csv", - "daggy", "dialoguer", "diesel", "diesel_migrations", @@ -527,16 +526,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "daggy" -version = "0.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "91a9304e55e9d601a39ae4deaba85406d5c0980e106f65afcf0460e9af1e7602" -dependencies = [ - "petgraph", - "serde", -] - [[package]] name = "darling" version = "0.20.10" diff --git a/Cargo.toml b/Cargo.toml index 6b862373..64c10bcd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,7 +34,6 @@ clap_complete = "4" colored = "2" config = { version = "0.15", default-features = false, features = [ "toml" ] } csv = "1" -daggy = { version = "0.8", features = [ "serde" ] } dialoguer = "0.11" diesel = { version = "2", features = ["postgres", "chrono", "uuid", "serde_json", "r2d2"] } diesel_migrations = "2" diff --git a/src/commands/tree_of.rs b/src/commands/tree_of.rs index 0ca2648e..86224558 100644 --- a/src/commands/tree_of.rs +++ b/src/commands/tree_of.rs @@ -14,14 +14,12 @@ use anyhow::Error; use anyhow::Result; use clap::ArgMatches; use petgraph::dot::Dot; -use petgraph::graph::DiGraph; use resiter::AndThen; use crate::config::Configuration; use crate::package::condition::ConditionData; use crate::package::Dag; use crate::package::DependencyType; -use crate::package::Package; use crate::package::PackageName; use crate::package::PackageVersionConstraint; use crate::repository::Repository; @@ -73,10 +71,8 @@ pub async fn tree_of(matches: &ArgMatches, repo: Repository, config: &Configurat .map(|package| Dag::for_root_package(package.clone(), &repo, None, &condition_data)) .and_then_ok(|dag| { if dot { - let petgraph: DiGraph = (*dag.dag()).clone().into(); - let dot = Dot::with_attr_getters( - &petgraph, + dag.dag(), &[ petgraph::dot::Config::EdgeNoLabel, petgraph::dot::Config::NodeNoLabel, @@ -96,13 +92,11 @@ pub async fn tree_of(matches: &ArgMatches, repo: Repository, config: &Configurat println!("{:?}", dot); Ok(()) } else if serial_buildorder { - let petgraph: DiGraph = (*dag.dag()).clone().into(); - - let topo_sorted = petgraph::algo::toposort(&petgraph, None) + let topo_sorted = petgraph::algo::toposort(dag.dag(), None) .map_err(|_| Error::msg("Cyclic dependency found!"))?; for node in topo_sorted.iter().rev() { - let package = petgraph.node_weight(*node).unwrap(); + let package = dag.dag().node_weight(*node).unwrap(); println!("{}", package.display_name_version()); } println!(); diff --git a/src/job/dag.rs b/src/job/dag.rs index 077c9e4e..90977381 100644 --- a/src/job/dag.rs +++ b/src/job/dag.rs @@ -8,9 +8,9 @@ // SPDX-License-Identifier: EPL-2.0 // -use daggy::Dag as DaggyDag; -use daggy::Walker; use getset::Getters; +use petgraph::acyclic::Acyclic; +use petgraph::graph::DiGraph; use uuid::Uuid; use crate::job::Job; @@ -24,7 +24,7 @@ use crate::util::docker::ImageName; #[derive(Debug, Getters)] pub struct Dag { #[getset(get = "pub")] - dag: DaggyDag, + dag: Acyclic>, } impl Dag { @@ -46,17 +46,17 @@ impl Dag { }; Dag { - dag: dag.dag().map(build_job, |_, e| (*e).clone()), + dag: Acyclic::<_>::try_from_graph(dag.dag().map(build_job, |_, e| (*e).clone())) + .unwrap(), // The dag.dag() is already acyclic so this cannot fail } } pub fn iter(&'_ self) -> impl Iterator + '_ { - self.dag.graph().node_indices().map(move |idx| { - let job = self.dag.graph().node_weight(idx).unwrap(); // TODO - let children = self.dag.children(idx); + self.dag.node_indices().map(move |idx| { + let job = self.dag.node_weight(idx).unwrap(); // TODO + let children = self.dag.neighbors_directed(idx, petgraph::Outgoing); let children_uuids = children - .iter(&self.dag) - .filter_map(|(_, node_idx)| self.dag.graph().node_weight(node_idx)) + .filter_map(|node_idx| self.dag.node_weight(node_idx)) .map(Job::uuid) .cloned() .collect(); diff --git a/src/package/dag.rs b/src/package/dag.rs index dea1ed94..0d69be29 100644 --- a/src/package/dag.rs +++ b/src/package/dag.rs @@ -15,12 +15,15 @@ use std::io::Write; use anyhow::anyhow; use anyhow::Context; -use anyhow::Error; use anyhow::Result; -use daggy::Walker; use getset::Getters; use indicatif::ProgressBar; use itertools::Itertools; +use petgraph::acyclic::Acyclic; +use petgraph::data::Build; +use petgraph::graph::DiGraph; +use petgraph::graph::EdgeIndex; +use petgraph::graph::NodeIndex; use ptree::Style; use ptree::TreeItem; use resiter::AndThen; @@ -37,10 +40,10 @@ use crate::repository::Repository; #[derive(Debug, Getters)] pub struct Dag { #[getset(get = "pub")] - dag: daggy::Dag, + dag: Acyclic>, #[getset(get = "pub")] - root_idx: daggy::NodeIndex, + root_idx: NodeIndex, } #[derive(Clone, PartialEq, Eq, Hash, Debug)] @@ -114,8 +117,8 @@ impl Dag { /// and adds corresponding nodes to the DAG. The edges are added later in `add_edges()`. fn add_sub_packages<'a>( repo: &'a Repository, - mappings: &mut HashMap<&'a Package, daggy::NodeIndex>, - dag: &mut daggy::Dag<&'a Package, DependencyType>, + mappings: &mut HashMap<&'a Package, NodeIndex>, + dag: &mut Acyclic>, p: &'a Package, progress: Option<&ProgressBar>, conditional_data: &ConditionData<'_>, @@ -180,8 +183,8 @@ impl Dag { // TODO: It seems easier and more efficient to do this in `add_sub_packages` as well (it // makes that function more complex but doing it separately is weird). fn add_edges( - mappings: &HashMap<&Package, daggy::NodeIndex>, - dag: &mut daggy::Dag<&Package, DependencyType>, + mappings: &HashMap<&Package, NodeIndex>, + dag: &mut Acyclic>, conditional_data: &ConditionData<'_>, ) -> Result<()> { for (package, idx) in mappings { @@ -193,9 +196,14 @@ impl Dag { *pkg.name() == dep_name && dep_constr.matches(pkg.version()) }) .try_for_each(|(dep, dep_idx)| { - dag.add_edge(*idx, *dep_idx, dep_kind.clone()) + dag.try_add_edge(*idx, *dep_idx, dep_kind.clone()) .map(|_| ()) - .map_err(Error::from) + // Only debug formatting is available for the error and for + // cycles it is quite useless (the context below is much more + // helpful than, e.g., "Cycle(Cycle(NodeIndex(0)))") but we'll + // include it for completeness and in case of the other errors + // that could theoretically occur: + .map_err(|e| anyhow!(format!("{e:?}"))) .with_context(|| { anyhow!( "Failed to add package dependency DAG edge \ @@ -215,7 +223,7 @@ impl Dag { } // Create an empty DAG and use the above helper functions to compute the dependency graph: - let mut dag: daggy::Dag<&Package, DependencyType> = daggy::Dag::new(); + let mut dag = Acyclic::>::new(); let mut mappings = HashMap::new(); trace!("Building the package dependency DAG for package {:?}", p); @@ -234,10 +242,11 @@ impl Dag { trace!("Finished building the package DAG"); Ok(Dag { - dag: dag.map( + dag: Acyclic::<_>::try_from_graph(dag.map( |_, p: &&Package| -> Package { (*p).clone() }, |_, e| (*e).clone(), - ), + )) + .unwrap(), // The dag is already acyclic so this cannot fail root_idx, }) } @@ -249,9 +258,8 @@ impl Dag { /// The order of the packages is _NOT_ guaranteed by the implementation pub fn all_packages(&self) -> Vec<&Package> { self.dag - .graph() .node_indices() - .filter_map(|idx| self.dag.graph().node_weight(idx)) + .filter_map(|idx| self.dag.node_weight(idx)) .collect() } @@ -261,7 +269,7 @@ impl Dag { } #[derive(Clone)] -pub struct DagDisplay<'a>(&'a Dag, daggy::NodeIndex, Option); +pub struct DagDisplay<'a>(&'a Dag, NodeIndex, Option); impl TreeItem for DagDisplay<'_> { type Child = Self; @@ -270,7 +278,6 @@ impl TreeItem for DagDisplay<'_> { let p = self .0 .dag - .graph() .node_weight(self.1) .ok_or_else(|| anyhow!("Error finding node: {:?}", self.1)) .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?; @@ -281,7 +288,6 @@ impl TreeItem for DagDisplay<'_> { Some(edge_idx) => self .0 .dag - .graph() .edge_weight(edge_idx) .ok_or_else(|| anyhow!("Error finding edge: {:?}", self.2)) .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?, @@ -295,12 +301,16 @@ impl TreeItem for DagDisplay<'_> { } fn children(&self) -> Cow<[Self::Child]> { - let c = self.0.dag.children(self.1); - Cow::from( - c.iter(&self.0.dag) - .map(|(edge_idx, node_idx)| DagDisplay(self.0, node_idx, Some(edge_idx))) - .collect::>(), - ) + let mut children_walker = self + .0 + .dag + .neighbors_directed(self.1, petgraph::Outgoing) + .detach(); + let mut children = Vec::::new(); + while let Some((edge_idx, node_idx)) = children_walker.next(&self.0.dag) { + children.push(DagDisplay(self.0, node_idx, Some(edge_idx))); + } + Cow::from(children) } }