diff --git a/CHANGELOG.md b/CHANGELOG.md index 128e54699..bd36dba6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ - Extra `-debug` packages will not be taken into account when determining packages that need upgrades. - `-Auk`: don't display a diff (or even ask to) if the hash didn't change. Useful with `--git`. +- `-A`: a bug involving incorrect build order which would occasionally lead to + top-level packages being marked as dependencies and subsequently being removed + via the effects of `-a`. ## 4.0.2 (2024-08-10) diff --git a/rust/aura-core/src/aur/dependencies.rs b/rust/aura-core/src/aur/dependencies.rs index de6d3d081..9d8ed0810 100644 --- a/rust/aura-core/src/aur/dependencies.rs +++ b/rust/aura-core/src/aur/dependencies.rs @@ -494,12 +494,29 @@ where /// let res = aura_core::aur::dependencies::build_order::<()>(&[]).unwrap(); /// assert!(res.is_empty()); /// ``` -pub fn build_order(to_build: &[Buildable]) -> Result>, Error> { +pub fn build_order(mut to_build: Vec) -> Result>, Error> { info!("Determining build order."); - let graph = dep_graph(to_build); - - petgraph::algo::toposort(&graph, None) + // NOTE 2024-08-15 This re-sorting is a workaround around an issue with the + // toposort results that occurs due to a combination of: + // + // 1. Packages having AUR dependencies. + // 2. Package names having a certain alphabetical ordering. + // 3. The insertion order in the input Vec relative to that alphabetical ordering. + // + // Experimentally, it seems that by sorting by dep-count we can avoid the + // issue. Note also that this is almost certainly the same issue I was + // seeing previously of "Aura uninstalling itself" due to the effects of + // `-a`. + to_build.sort_by_key(|b| b.deps.len()); + to_build.reverse(); + let graph = dep_graph(&to_build); + + // NOTE Testing only. + // let dot = Dot::new(&graph); + // std::fs::write("dep-graph.dot", format!("{:?}", dot)).unwrap(); + + let topo = petgraph::algo::toposort(&graph, None) .map_err(|cycle| { shortest_cycle(cycle.node_id(), &graph) .into_iter() @@ -513,8 +530,9 @@ pub fn build_order(to_build: &[Buildable]) -> Result>, Error // order again at the very end. .map(|ix| graph.node_weight(ix)) .collect::>>() - .ok_or(Error::MalformedGraph)? - .into_iter() + .ok_or(Error::MalformedGraph)?; + + topo.into_iter() // --- Split the packages by "layer" --- // .fold( (Vec::new(), Vec::new(), HashSet::new()), @@ -531,14 +549,14 @@ pub fn build_order(to_build: &[Buildable]) -> Result>, Error // that thanks to the topological sort that they've already // processed into early layers. Also see comment (1) below. deps.extend(&buildable.deps); - (layers, vec![buildable.name.as_str()], deps) + (layers, vec![buildable.name.to_string()], deps) } else { // (1) While all of the Buildable's official (non-AUR) deps // are also included in its `deps` field, only its own name // is ever added to the build order "group", and thus we // never try to mistakenly build an official package as an // AUR one. - group.push(buildable.name.as_str()); + group.push(buildable.name.to_string()); deps.extend(&buildable.deps); (layers, group, deps) } @@ -706,7 +724,7 @@ mod test { assert_eq!(v.len(), g.node_count()); assert_eq!(1, g.edge_count()); - let o = build_order::<()>(&v).unwrap(); + let o = build_order::<()>(v).unwrap(); assert_eq!(vec![vec!["b"], vec!["a"],], o); } @@ -735,7 +753,7 @@ mod test { assert_eq!(v.len(), g.node_count()); assert_eq!(4, g.edge_count()); - let mut o = build_order::<()>(&v).unwrap(); + let mut o = build_order::<()>(v).unwrap(); for v in o.iter_mut() { v.sort(); } @@ -776,10 +794,48 @@ mod test { assert_eq!(v.len(), g.node_count()); assert_eq!(5, g.edge_count()); - let mut o = build_order::<()>(&v).unwrap(); + let mut o = build_order::<()>(v).unwrap(); for v in o.iter_mut() { v.sort(); } assert_eq!(vec![vec!["d"], vec!["b", "c"], vec!["a", "e", "f"]], o); } + + #[test] + fn real_failing_graph() { + let v = vec![ + Buildable { + name: "mgba-git".to_string(), + deps: HashSet::new(), + }, + Buildable { + name: "sway-git".to_string(), + deps: vec!["wlroots-git".to_string()].into_iter().collect(), + }, + Buildable { + name: "wlroots-git".to_string(), + deps: HashSet::new(), + }, + Buildable { + name: "timelineproject-hg".to_string(), + deps: HashSet::new(), + }, + ]; + + let g = dep_graph(&v); + assert_eq!(v.len(), g.node_count()); + assert_eq!(1, g.edge_count()); + + let mut o = build_order::<()>(v).unwrap(); + for v in o.iter_mut() { + v.sort(); + } + assert_eq!( + vec![ + vec!["wlroots-git"], + vec!["mgba-git", "sway-git", "timelineproject-hg"] + ], + o + ); + } } diff --git a/rust/aura-pm/src/command/aur.rs b/rust/aura-pm/src/command/aur.rs index e89edef78..32fffc340 100644 --- a/rust/aura-pm/src/command/aur.rs +++ b/rust/aura-pm/src/command/aur.rs @@ -513,8 +513,9 @@ fn install_work( } // --- Determine the best build order --- // - let order: Vec> = - aura_core::aur::dependencies::build_order(&to_build).map_err(Error::Deps)?; + let is_single = to_build.len() == 1; + let order: Vec> = + aura_core::aur::dependencies::build_order(to_build).map_err(Error::Deps)?; debug!("Build order: {:?}", order); // --- Install repo dependencies --- // @@ -528,7 +529,6 @@ fn install_work( } // --- Build and install each layer of AUR packages --- // - let is_single = to_build.len() == 1; let caches = env.caches(); let alpm = env.alpm().map_err(Error::Env)?; for raw_layer in order.into_iter().apply(Finished::new) {