Add support for calculating build order (#57)

This is basically oxidecomputer/crucible#1097 , but generalized and with tests

Adds support for topologically sorting packages, so they can be built in dependency-first order.

Fixes #56

Author: smklein

Cargo.toml

@@ -30,4 +30,5 @@ tempfile = "3.4"
 thiserror = "1.0"
 tokio = { version = "1.26", features = [ "full" ] }
 toml = "0.7.3"
+topological-sort = "0.2.2"
 walkdir = "2.3"

src/config.rs

@@ -4,12 +4,97 @@
 
 //! Configuration for a package.
 
-use crate::package::{Package, PackageOutput};
+use crate::package::{Package, PackageOutput, PackageSource};
 use crate::target::Target;
 use serde_derive::Deserialize;
 use std::collections::BTreeMap;
 use std::path::Path;
 use thiserror::Error;
+use topological_sort::TopologicalSort;
+
+/// Describes a set of packages to act upon.
+pub struct PackageMap<'a>(BTreeMap<&'a String, &'a Package>);
+
+// The name of a file which should be created by building a package.
+#[derive(Clone, Eq, Hash, Ord, PartialEq, PartialOrd)]
+struct OutputFile(String);
+
+impl<'a> PackageMap<'a> {
+    pub fn build_order(&self) -> PackageDependencyIter<'a> {
+        let lookup_by_output = self
+            .0
+            .iter()
+            .map(|(name, package)| (OutputFile(package.get_output_file(name)), (*name, *package)))
+            .collect::<BTreeMap<_, _>>();
+
+        // Collect all packages, and sort them in dependency order,
+        // so we know which ones to build first.
+        let mut outputs = TopologicalSort::<OutputFile>::new();
+        for (package_output, (_, package)) in &lookup_by_output {
+            match &package.source {
+                PackageSource::Local { .. }
+                | PackageSource::Prebuilt { .. }
+                | PackageSource::Manual => {
+                    // Skip intermediate leaf packages; if necessary they'll be
+                    // added to the dependency graph by whatever composite package
+                    // actually depends on them.
+                    if !matches!(
+                        package.output,
+                        PackageOutput::Zone {
+                            intermediate_only: true
+                        }
+                    ) {
+                        outputs.insert(package_output.clone());
+                    }
+                }
+                PackageSource::Composite { packages: deps } => {
+                    for dep in deps {
+                        outputs.add_dependency(OutputFile(dep.clone()), package_output.clone());
+                    }
+                }
+            }
+        }
+
+        PackageDependencyIter {
+            lookup_by_output,
+            outputs,
+        }
+    }
+}
+
+/// Returns all packages in the order in which they should be built.
+///
+/// Returns packages in batches that may be built concurrently.
+pub struct PackageDependencyIter<'a> {
+    lookup_by_output: BTreeMap<OutputFile, (&'a String, &'a Package)>,
+    outputs: TopologicalSort<OutputFile>,
+}
+
+impl<'a> Iterator for PackageDependencyIter<'a> {
+    type Item = Vec<(&'a String, &'a Package)>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.outputs.is_empty() {
+            return None;
+        }
+        let batch = self.outputs.pop_all();
+        assert!(
+            !batch.is_empty() || self.outputs.is_empty(),
+            "cyclic dependency in package manifest!"
+        );
+
+        Some(
+            batch
+                .into_iter()
+                .map(|output| {
+                    *self.lookup_by_output.get(&output).unwrap_or_else(|| {
+                        panic!("Could not find a package which creates '{}'", output.0)
+                    })
+                })
+                .collect(),
+        )
+    }
+}
 
 /// Describes the configuration for a set of packages.
 #[derive(Deserialize, Debug)]
@@ -21,24 +106,28 @@ pub struct Config {
 
 impl Config {
     /// Returns target packages to be assembled on the builder machine.
-    pub fn packages_to_build(&self, target: &Target) -> BTreeMap<&String, &Package> {
-        self.packages
-            .iter()
-            .filter(|(_, pkg)| target.includes_package(pkg))
-            .map(|(name, pkg)| (name, pkg))
-            .collect()
+    pub fn packages_to_build(&self, target: &Target) -> PackageMap<'_> {
+        PackageMap(
+            self.packages
+                .iter()
+                .filter(|(_, pkg)| target.includes_package(pkg))
+                .map(|(name, pkg)| (name, pkg))
+                .collect(),
+        )
     }
 
     /// Returns target packages which should execute on the deployment machine.
-    pub fn packages_to_deploy(&self, target: &Target) -> BTreeMap<&String, &Package> {
-        let all_packages = self.packages_to_build(target);
-        all_packages
-            .into_iter()
-            .filter(|(_, pkg)| match pkg.output {
-                PackageOutput::Zone { intermediate_only } => !intermediate_only,
-                PackageOutput::Tarball => true,
-            })
-            .collect()
+    pub fn packages_to_deploy(&self, target: &Target) -> PackageMap<'_> {
+        let all_packages = self.packages_to_build(target).0;
+        PackageMap(
+            all_packages
+                .into_iter()
+                .filter(|(_, pkg)| match pkg.output {
+                    PackageOutput::Zone { intermediate_only } => !intermediate_only,
+                    PackageOutput::Tarball => true,
+                })
+                .collect(),
+        )
     }
 }
 
@@ -61,3 +150,107 @@ pub fn parse<P: AsRef<Path>>(path: P) -> Result<Config, ParseError> {
     let contents = std::fs::read_to_string(path.as_ref())?;
     parse_manifest(&contents)
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_order() {
+        let pkg_a_name = String::from("pkg-a");
+        let pkg_a = Package {
+            service_name: String::from("a"),
+            source: PackageSource::Manual,
+            output: PackageOutput::Tarball,
+            only_for_targets: None,
+            setup_hint: None,
+        };
+
+        let pkg_b_name = String::from("pkg-b");
+        let pkg_b = Package {
+            service_name: String::from("b"),
+            source: PackageSource::Composite {
+                packages: vec![pkg_a.get_output_file(&pkg_a_name)],
+            },
+            output: PackageOutput::Tarball,
+            only_for_targets: None,
+            setup_hint: None,
+        };
+
+        let cfg = Config {
+            packages: BTreeMap::from([
+                (pkg_a_name.clone(), pkg_a.clone()),
+                (pkg_b_name.clone(), pkg_b.clone()),
+            ]),
+        };
+
+        let mut order = cfg.packages_to_build(&Target::default()).build_order();
+        // "pkg-a" comes first, because "pkg-b" depends on it.
+        assert_eq!(order.next(), Some(vec![(&pkg_a_name, &pkg_a)]));
+        assert_eq!(order.next(), Some(vec![(&pkg_b_name, &pkg_b)]));
+    }
+
+    // We're kinda limited by the topological-sort library here, as this is a documented
+    // behavior from [TopologicalSort::pop_all].
+    //
+    // Regardless, test that circular dependencies cause panics.
+    #[test]
+    #[should_panic(expected = "cyclic dependency in package manifest")]
+    fn test_cyclic_dependency() {
+        let pkg_a_name = String::from("pkg-a");
+        let pkg_b_name = String::from("pkg-b");
+        let pkg_a = Package {
+            service_name: String::from("a"),
+            source: PackageSource::Composite {
+                packages: vec![String::from("pkg-b.tar")],
+            },
+            output: PackageOutput::Tarball,
+            only_for_targets: None,
+            setup_hint: None,
+        };
+        let pkg_b = Package {
+            service_name: String::from("b"),
+            source: PackageSource::Composite {
+                packages: vec![String::from("pkg-a.tar")],
+            },
+            output: PackageOutput::Tarball,
+            only_for_targets: None,
+            setup_hint: None,
+        };
+
+        let cfg = Config {
+            packages: BTreeMap::from([
+                (pkg_a_name.clone(), pkg_a.clone()),
+                (pkg_b_name.clone(), pkg_b.clone()),
+            ]),
+        };
+
+        let mut order = cfg.packages_to_build(&Target::default()).build_order();
+        order.next();
+    }
+
+    // Make pkg-a depend on pkg-b.tar, but don't include pkg-b.tar anywhere.
+    //
+    // Ensure that we see an appropriate panic.
+    #[test]
+    #[should_panic(expected = "Could not find a package which creates 'pkg-b.tar'")]
+    fn test_missing_dependency() {
+        let pkg_a_name = String::from("pkg-a");
+        let pkg_a = Package {
+            service_name: String::from("a"),
+            source: PackageSource::Composite {
+                packages: vec![String::from("pkg-b.tar")],
+            },
+            output: PackageOutput::Tarball,
+            only_for_targets: None,
+            setup_hint: None,
+        };
+
+        let cfg = Config {
+            packages: BTreeMap::from([(pkg_a_name.clone(), pkg_a.clone())]),
+        };
+
+        let mut order = cfg.packages_to_build(&Target::default()).build_order();
+        order.next();
+    }
+}

src/package.rs

@@ -176,7 +176,7 @@ async fn add_package_to_zone_archive(
 /// the following path:
 ///
 /// <https://buildomat.eng.oxide.computer/public/file/oxidecomputer/REPO/SERIES/COMMIT/ARTIFACT>
-#[derive(Deserialize, Debug)]
+#[derive(Clone, Deserialize, Debug, PartialEq)]
 pub struct PrebuiltBlob {
     pub repo: String,
     pub series: String,
@@ -186,7 +186,7 @@ pub struct PrebuiltBlob {
 }
 
 /// Describes the origin of an externally-built package.
-#[derive(Deserialize, Debug)]
+#[derive(Clone, Deserialize, Debug, PartialEq)]
 #[serde(tag = "type", rename_all = "lowercase")]
 pub enum PackageSource {
     /// Describes a package which should be assembled locally.
@@ -257,7 +257,7 @@ impl PackageSource {
 }
 
 /// Describes the output format of the package.
-#[derive(Deserialize, Debug, Clone)]
+#[derive(Deserialize, Debug, Clone, PartialEq)]
 #[serde(tag = "type", rename_all = "lowercase")]
 pub enum PackageOutput {
     /// A complete zone image, ready to be deployed to the target.
@@ -274,7 +274,7 @@ pub enum PackageOutput {
 }
 
 /// A single package.
-#[derive(Deserialize, Debug)]
+#[derive(Clone, Deserialize, Debug, PartialEq)]
 pub struct Package {
     /// The name of the service name to be used on the target OS.
     pub service_name: String,
@@ -821,7 +821,7 @@ impl Package {
 }
 
 /// Describes configuration for a package which contains a Rust binary.
-#[derive(Deserialize, Debug)]
+#[derive(Clone, Deserialize, Debug, PartialEq)]
 pub struct RustPackage {
     /// The name of the compiled binary to be used.
     // TODO: Could be extrapolated to "produced build artifacts", we don't
@@ -869,7 +869,7 @@ impl RustPackage {
 }
 
 /// A string which can be modified with key-value pairs.
-#[derive(Deserialize, Debug)]
+#[derive(Clone, Deserialize, Debug, PartialEq)]
 pub struct InterpolatedString(String);
 
 impl InterpolatedString {
@@ -891,7 +891,10 @@ impl InterpolatedString {
             };
             let key = &input[..end_idx];
             let Some(value) = target.0.get(key) else {
-                bail!("Key '{key}' not found in target, but required in '{}'", self.0);
+                bail!(
+                    "Key '{key}' not found in target, but required in '{}'",
+                    self.0
+                );
             };
             output.push_str(&value);
             input = &input[end_idx + END_STR.len()..];
@@ -902,7 +905,7 @@ impl InterpolatedString {
 }
 
 /// A pair of paths, mapping from a directory on the host to the target.
-#[derive(Deserialize, Debug)]
+#[derive(Clone, Deserialize, Debug, PartialEq)]
 pub struct MappedPath {
     /// Source path.
     pub from: InterpolatedString,

tests/mod.rs

@@ -175,10 +175,16 @@ mod test {
         let cfg = config::parse("tests/service-e/cfg.toml").unwrap();
         let out = tempfile::tempdir().unwrap();
 
+        // Ask for the order of packages to-be-built
+        let packages = cfg.packages_to_build(&Target::default());
+        let mut build_order = packages.build_order();
+
         // Build the dependencies first.
-        let package_dependencies = ["pkg-1", "pkg-2"];
-        for package_name in package_dependencies {
-            let package = cfg.packages.get(package_name).unwrap();
+        let batch = build_order.next().expect("Missing dependency batch");
+        let mut batch_pkg_names: Vec<_> = batch.iter().map(|(name, _)| *name).collect();
+        batch_pkg_names.sort();
+        assert_eq!(batch_pkg_names, vec!["pkg-1", "pkg-2"]);
+        for (package_name, package) in batch {
             // Create the packaged file
             package
                 .create_for_target(&Target::default(), package_name, out.path())
@@ -187,7 +193,10 @@ mod test {
         }
 
         // Build the composite package
+        let batch = build_order.next().expect("Missing dependency batch");
+        let batch_pkg_names: Vec<_> = batch.iter().map(|(name, _)| *name).collect();
         let package_name = "pkg-3";
+        assert_eq!(batch_pkg_names, vec![package_name]);
         let package = cfg.packages.get(package_name).unwrap();
         package
             .create_for_target(&Target::default(), package_name, out.path())