cleaner way of ensuring package manager events are ran in succession

Author: JustinBacher

app/src/commands/apply.rs

@@ -86,7 +86,7 @@ impl ComtryaCommand for Apply {
         let contexts = runtime.contexts.clone();
         let manifest_path = self
             .manifest_path(runtime)
-            .inspect(|path| debug!("Load manifests from path: {path:#?}"))?;
+            .inspect(|path| debug!("Load manifests from path: {:#?}", path))?;
         let manifests = load(manifest_path, &contexts);
         let manifest_manager = {
             let graph = DependencyGraph::new(manifests, &contexts, runtime).await?;
@@ -96,41 +96,42 @@ impl ComtryaCommand for Apply {
         let dry_run = self.dry_run;
         let password_manager = &runtime.password_manager;
 
-        println!("Total: {}", manifest_manager.get_ordered_manifests().len());
         for manifest in manifest_manager.get_ordered_manifests() {
             // Need to clone all these because they'll be in their own threads
             let label = self.label.clone();
             let contexts = contexts.clone();
-            let password_manager = password_manager.clone();
+            let pm = password_manager.clone();
             let manifest_manager = Arc::clone(&manifest_manager);
+            let name = manifest.get_name();
 
             workers.spawn(async move {
-                for successor in manifest_manager.get_successors(&manifest).await {
-                    // May need to remove this unwrap and handle the option instead
-                    if !successor.barrier.as_ref().unwrap().wait(true).await {
-                        return Err(anyhow!(
-                            "Debug unable to run manifest {manifest:?} due dependency failure."
-                        ));
-                    }
-                }
-
-                let exec_result = manifest
-                    .execute(
-                        dry_run,
-                        label.clone(),
-                        &contexts.clone(),
-                        password_manager.clone(),
-                    )
-                    .await;
-
+                // Wait on own barrier, if a dependency fails this is how it find's out
                 manifest
                     .barrier
                     .as_ref()
-                    .unwrap()
-                    .wait(exec_result.is_ok())
-                    .await;
+                    .with_context(|| format!("Cannot lock manifest '{}' for execution", name))?
+                    .wait(true)
+                    .await
+                    .then_some(())
+                    .context(format!("Skipping manifest '{}' Dependancy(s) failed", name))?;
+
+                let result = manifest.execute(dry_run, label, &contexts, pm).await;
+
+                // Inform successors (dependants) of pass or fail
+                for successor in manifest_manager
+                    .get_successors(&manifest)
+                    .await
+                    .context(format!("Cannot resolve dependants for manifest '{}'", name))?
+                {
+                    successor
+                        .barrier
+                        .as_ref()
+                        .context(format!("Cannot mark manifest '{}' completed", name))?
+                        .wait(result.is_ok())
+                        .await;
+                }
 
-                exec_result.context("Manifest {manifest:?} failed. {e}")
+                Ok(())
             });
         }
 

app/src/config/mod.rs

@@ -81,7 +81,7 @@ where
 }
 
 pub(crate) fn load_config(args: &GlobalArgs) -> Result<Config> {
-    match lib_config(&args) {
+    match lib_config(args) {
         Ok(config) => match args.manifest_directory.clone() {
             Some(manifest_path) => Ok(Config {
                 manifest_paths: vec![manifest_path],

app/src/main.rs

@@ -10,7 +10,6 @@ use comtrya_lib::manifests;
 use clap::Parser;
 use tracing::{error, Level};
 
-#[allow(unused_imports)]
 use tracing_subscriber::{fmt::writer::MakeWriterExt, layer::SubscriberExt, FmtSubscriber};
 
 mod commands;

app/src/utils/dependency_graph.rs

@@ -14,7 +14,6 @@ use tracing::{error, trace};
 
 use crate::Runtime;
 use comtrya_lib::{
-    actions::Actions,
     contexts::Contexts,
     manifests::{DependencyBarrier, Manifest},
     utilities::{get_privilege_provider, password_manager::PasswordManager},
@@ -41,7 +40,7 @@ impl DependencyGraph {
         let mut should_ask_for_pass = false;
         let mut dependency_map = Vec::new();
 
-        for (_, manifest) in manifests.iter_mut() {
+        for manifest in manifests.values_mut() {
             manifest.barrier = Some(DependencyBarrier::new(manifest.depends.len() + 1));
             let node = this.add_manifest(manifest.clone()).await;
             this.name_to_idx.insert(manifest.get_name(), node);
@@ -54,7 +53,7 @@ impl DependencyGraph {
                 let dep_prefix = name.rsplit_once('.').map(|(n, _)| n).unwrap_or(&name);
                 let dependency_name = dependency_name.replace("./", &format!("{dep_prefix}."));
 
-                let Some(dependency_manifest) = manifests.get(&dependency_name) else {
+                let Some(dependency) = manifests.get(&dependency_name) else {
                     return error!(
                         message = "Unresolved dependency",
                         dependency = dependency_name
@@ -64,28 +63,20 @@ impl DependencyGraph {
                 trace!(
                     message = "Dependency Registered",
                     from = name,
-                    to = dependency_manifest.get_name()
+                    to = dependency.get_name()
                 );
 
-                dependency_map.push((node, this.name_to_idx[&dependency_manifest.get_name()]));
+                dependency_map.push((node, this.name_to_idx[&dependency.get_name()]));
             });
 
-            if !should_ask_for_pass {
-                should_ask_for_pass = manifest.actions.iter().any(|action| match action {
-                    Actions::CommandRun(cva) => cva.action.privileged,
-                    Actions::PackageInstall(_) | Actions::PackageRepository(_) => true,
-                    _ => false,
-                });
-            }
+            should_ask_for_pass |= manifest.actions.iter().any(|action| action.is_privileged());
         }
 
-        for (from, to) in dependency_map {
-            this.graph.add_edge(from, to, ());
+        for (from, to) in &dependency_map {
+            this.graph.add_edge(*from, *to, ());
         }
 
         if should_ask_for_pass {
-            debug!("Should be prompting for password. Asking now");
-
             let mut password_manager =
                 PasswordManager::new(get_privilege_provider(contexts).as_deref())?;
             password_manager.prompt("Please enter password:")?;
@@ -117,15 +108,17 @@ impl DependencyGraph {
         *idx
     }
 
-    pub async fn get_successors(&self, manifest: &LockedManifest) -> Vec<LockedManifest> {
-        self.graph
-            .neighbors_directed(self.get_node_from_manifest(manifest).await, Incoming)
+    pub async fn get_successors(&self, manifest: &LockedManifest) -> Option<Vec<LockedManifest>> {
+        let manifests = self
+            .graph
+            .neighbors_directed(self.get_node_from_manifest(manifest).await?, Incoming)
             .map(|node| Arc::clone(&self.graph[node].clone()))
-            .collect()
+            .collect();
+
+        Some(manifests)
     }
 
-    pub async fn get_node_from_manifest(&self, manifest: &LockedManifest) -> NodeIndex {
-        // let join_set = JoinSet::new();
-        *self.name_to_idx.get(&manifest.get_name()).unwrap()
+    pub async fn get_node_from_manifest(&self, manifest: &LockedManifest) -> Option<NodeIndex> {
+        self.name_to_idx.get(&manifest.get_name()).copied()
     }
 }

app/src/utils/mod.rs

@@ -1,25 +1,3 @@
 mod dependency_graph;
-pub use dependency_graph::DependencyGraph;
-use std::iter::from_fn;
-
-pub trait ZipLongest<T>: Iterator<Item = T> {
-    fn zip_longest<U, I>(self, other: I) -> impl Iterator<Item = (Option<T>, Option<U>)>
-    where
-        Self: Sized,
-        I: IntoIterator<Item = U>;
-}
 
-impl<T, I: Iterator<Item = T>> ZipLongest<T> for I {
-    fn zip_longest<U, J>(self, other: J) -> impl Iterator<Item = (Option<T>, Option<U>)>
-    where
-        J: IntoIterator<Item = U>,
-    {
-        let mut a = self.fuse();
-        let mut b = other.into_iter().fuse();
-
-        from_fn(move || match (a.next(), b.next()) {
-            (None, None) => None,
-            (a, b) => Some((a, b)),
-        })
-    }
-}
+pub use dependency_graph::DependencyGraph;

lib/src/actions/command/run.rs

@@ -62,6 +62,10 @@ impl Action for RunCommand {
             ))],
         }])
     }
+
+    fn is_privileged(&self) -> bool {
+        self.privileged
+    }
 }
 
 #[cfg(test)]

lib/src/actions/file/link.rs

@@ -92,22 +92,19 @@ impl FileLink {
         }];
 
         if let Ok(paths) = std::fs::read_dir(from) {
-            paths.for_each(|path| {
-                if let Ok(path) = path {
-                    let p = path.path();
-
-                    if let Some(file_name) = p.file_name() {
-                        steps.push(Step {
-                            atom: Box::new(Link {
-                                source: p.clone(),
-                                target: to.join(file_name),
-                            }),
-                            initializers: vec![Ensure(Box::new(FileExists(p.clone())))],
-                            finalizers: vec![],
-                        })
-                    }
-                }
-            })
+            steps.extend(paths.filter_map(|path| {
+                let p = path.ok()?.path();
+                let file_name = p.file_name()?;
+
+                Some(Step {
+                    atom: Box::new(Link {
+                        source: p.clone(),
+                        target: to.join(file_name),
+                    }),
+                    initializers: vec![Ensure(Box::new(FileExists(p.clone())))],
+                    finalizers: vec![],
+                })
+            }))
         }
 
         steps
@@ -209,7 +206,6 @@ mod tests {
             actions: vec![],
             depends: vec![],
             name: None,
-            dag_index: None,
             ..Default::default()
         };
 
@@ -265,7 +261,6 @@ mod tests {
             actions: vec![],
             depends: vec![],
             name: None,
-            dag_index: None,
             ..Default::default()
         };
 

lib/src/actions/file/mod.rs

@@ -7,7 +7,7 @@ pub mod unarchive;
 
 use crate::actions::Action;
 use crate::manifests::Manifest;
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, Context, Result};
 use normpath::PathExt;
 use serde::{de::Error, Deserialize, Deserializer};
 use std::path::PathBuf;
@@ -16,20 +16,16 @@ pub trait FileAction: Action {
     fn resolve(&self, manifest: &Manifest, path: &str) -> anyhow::Result<PathBuf> {
         Ok(manifest
             .root_dir
-            .clone()
-            .ok_or_else(|| anyhow!("Failed because manifest has no root_dir"))?
+            .as_ref()
+            .context("Failed because manifest has no root_dir")?
             .join("files")
             .join(path)
             .normalize()
-            .map_err(|e| {
-                anyhow!(
-                    "Resolution of {} failed in manifest {} because {}",
-                    path.to_string(),
-                    manifest
-                        .name
-                        .as_ref()
-                        .unwrap_or(&"cannot extract manifest name".to_string()),
-                    e.to_string()
+            .with_context(|| {
+                format!(
+                    "Resolution of {} failed in manifest {}",
+                    path,
+                    manifest.get_name()
                 )
             })?
             .as_path()
@@ -45,7 +41,7 @@ pub trait FileAction: Action {
             .join("files")
             .join(path);
 
-        std::fs::read(file_path.clone()).map_err(|e| match e.kind() {
+        std::fs::read(&*file_path).map_err(|e| match e.kind() {
             ErrorKind::NotFound => anyhow!(
                 "Failed because {} was not found",
                 file_path.to_string_lossy()

lib/src/actions/group/add.rs

@@ -20,8 +20,12 @@ impl Action for GroupAdd {
 
         let mut atoms: Vec<Step> = vec![];
 
-        atoms.append(&mut provider.add_group(&variant, &contexts));
+        atoms.append(&mut provider.add_group(&variant, contexts));
 
         Ok(atoms)
     }
+
+    fn is_privileged(&self) -> bool {
+        true
+    }
 }