Rework command execution structure and make rayon optional (since it's not necessarily faster

due to workspace wide lockfile contention)

.cargo/config.toml

@@ -1,5 +1,6 @@
 [alias]
 xtask = "run --package xtask --"
+pxtask = "run --package xtask --features rayon --"
 
 [target.thumbv6m-none-eabi]
 runner = "qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel"

xtask/Cargo.toml

@@ -9,6 +9,5 @@ anyhow = "1.0.43"
 clap = { version = "4", features = ["derive"] }
 pretty_env_logger = "0.4.0"
 log = "0.4.17"
-rayon = "1.6.1"
+rayon = { version = "1.6.1", optional = true }
 diffy = "0.3.0"
-exitcode = "1.1.2"

xtask/src/argument_parsing.rs

@@ -275,12 +275,22 @@ pub struct PackageOpt {
 }
 
 impl PackageOpt {
+    #[cfg(not(feature = "rayon"))]
     pub fn packages(&self) -> impl Iterator<Item = Package> {
         self.package
             .map(|p| vec![p])
             .unwrap_or(Package::all())
             .into_iter()
     }
+
+    #[cfg(feature = "rayon")]
+    pub fn packages(&self) -> impl rayon::prelude::ParallelIterator<Item = Package> {
+        use rayon::prelude::*;
+        self.package
+            .map(|p| vec![p])
+            .unwrap_or(Package::all())
+            .into_par_iter()
+    }
 }
 
 #[derive(Args, Debug, Clone)]

xtask/src/cargo_commands.rs

@@ -1,11 +1,145 @@
 use crate::{
     argument_parsing::{Backends, BuildOrCheck, ExtraArguments, Globals, PackageOpt, TestMetadata},
     command::{BuildMode, CargoCommand},
-    command_parser,
+    command_parser, RunResult,
 };
-use log::error;
+use log::{error, info, Level};
+
+#[cfg(feature = "rayon")]
 use rayon::prelude::*;
 
+use iters::*;
+
+enum FinalRunResult<'c> {
+    Success(CargoCommand<'c>, RunResult),
+    Failed(CargoCommand<'c>, RunResult),
+    CommandError(anyhow::Error),
+}
+
+fn run_and_convert<'a>(
+    (global, command, overwrite): (&Globals, CargoCommand<'a>, bool),
+) -> FinalRunResult<'a> {
+    // Run the command
+    let result = command_parser(global, &command, overwrite);
+    match result {
+        // If running the command succeeded without looking at any of the results,
+        // log the data and see if the actual execution was succesfull too.
+        Ok(result) => {
+            if result.exit_status.success() {
+                FinalRunResult::Success(command, result)
+            } else {
+                FinalRunResult::Failed(command, result)
+            }
+        }
+        // If it didn't and some IO error occured, just panic
+        Err(e) => FinalRunResult::CommandError(e),
+    }
+}
+
+fn handle_results(results: Vec<FinalRunResult>) -> anyhow::Result<()> {
+    let errors = results.iter().filter_map(|r| {
+        if let FinalRunResult::Failed(c, r) = r {
+            Some((c, r))
+        } else {
+            None
+        }
+    });
+
+    let successes = results.iter().filter_map(|r| {
+        if let FinalRunResult::Success(c, r) = r {
+            Some((c, r))
+        } else {
+            None
+        }
+    });
+
+    let log_stdout_stderr = |level: Level| {
+        move |(command, result): (&CargoCommand, &RunResult)| {
+            let stdout = &result.stdout;
+            let stderr = &result.stderr;
+            if !stdout.is_empty() && !stderr.is_empty() {
+                log::log!(
+                    level,
+                    "Command output for {command}\nStdout:\n{stdout}\nStderr:\n{stderr}"
+                );
+            } else if !stdout.is_empty() {
+                log::log!(
+                    level,
+                    "Command output for {command}\nStdout:\n{}",
+                    stdout.trim_end()
+                );
+            } else if !stderr.is_empty() {
+                log::log!(
+                    level,
+                    "Command output for {command}\nStderr:\n{}",
+                    stderr.trim_end()
+                );
+            }
+        }
+    };
+
+    successes.clone().for_each(log_stdout_stderr(Level::Debug));
+    errors.clone().for_each(log_stdout_stderr(Level::Error));
+
+    successes.for_each(|(cmd, _)| {
+        info!("Succesfully executed {cmd}");
+    });
+
+    errors.clone().for_each(|(cmd, _)| {
+        error!("Command {cmd} failed");
+    });
+
+    if errors.count() != 0 {
+        Err(anyhow::anyhow!("Some commands failed."))
+    } else {
+        Ok(())
+    }
+}
+
+pub trait CoalescingRunning {
+    /// Run all the commands in this iterator, and coalesce the results into
+    /// one error (if any individual commands failed)
+    fn run_and_coalesce(self) -> anyhow::Result<()>;
+}
+
+#[cfg(not(feature = "rayon"))]
+mod iters {
+    use super::*;
+
+    pub fn examples_iter(examples: &[String]) -> impl Iterator<Item = &String> {
+        examples.into_iter()
+    }
+
+    impl<'g, 'c, I> CoalescingRunning for I
+    where
+        I: Iterator<Item = (&'g Globals, CargoCommand<'c>, bool)>,
+    {
+        fn run_and_coalesce(self) -> anyhow::Result<()> {
+            let results: Vec<_> = self.map(run_and_convert).collect();
+            handle_results(results)
+        }
+    }
+}
+
+#[cfg(feature = "rayon")]
+mod iters {
+    use super::*;
+
+    pub fn examples_iter(examples: &[String]) -> impl ParallelIterator<Item = &String> {
+        examples.into_par_iter()
+    }
+
+    impl<'g, 'c, I> CoalescingRunning for I
+    where
+        I: ParallelIterator<Item = (&'g Globals, CargoCommand<'c>, bool)>,
+    {
+        fn run_and_coalesce(self) -> anyhow::Result<()> {
+            let results: Vec<_> = self.map(run_and_convert).collect();
+            handle_results(results)
+        }
+    }
+}
+
 /// Cargo command to either build or check
 pub fn cargo(
     globals: &Globals,
@@ -14,7 +148,7 @@ pub fn cargo(
     package: &PackageOpt,
     backend: Backends,
 ) -> anyhow::Result<()> {
-    package.packages().for_each(|package| {
+    let runner = package.packages().map(|package| {
         let target = backend.to_target();
 
         let features = package.extract_features(target, backend);
@@ -44,13 +178,11 @@ pub fn cargo(
                 mode: BuildMode::Release,
             },
         };
-        let res = command_parser(globals, &command, false);
+
-        if let Err(e) = res {
+        (globals, command, false)
-            error!("{e}");
-        }
     });
 
-    Ok(())
+    runner.run_and_coalesce()
 }
 
 /// Cargo command to either build or check all examples
@@ -63,7 +195,7 @@ pub fn cargo_example(
     backend: Backends,
     examples: &[String],
 ) -> anyhow::Result<()> {
-    examples.into_par_iter().for_each(|example| {
+    let runner = examples_iter(examples).map(|example| {
         let features = Some(backend.to_target().and_features(backend.to_rtic_feature()));
 
         let command = match operation {
@@ -82,13 +214,9 @@ pub fn cargo_example(
                 mode: BuildMode::Release,
             },
         };
-
+        (globals, command, false)
-        if let Err(err) = command_parser(globals, &command, false) {
-            error!("{err}");
-        }
     });
-
+    runner.run_and_coalesce()
-    Ok(())
 }
 
 /// Run cargo clippy on selected package
@@ -98,27 +226,23 @@ pub fn cargo_clippy(
     package: &PackageOpt,
     backend: Backends,
 ) -> anyhow::Result<()> {
-    package.packages().for_each(|p| {
+    let runner = package.packages().map(|p| {
         let target = backend.to_target();
         let features = p.extract_features(target, backend);
 
-        let res = command_parser(
+        (
             globals,
-            &CargoCommand::Clippy {
+            CargoCommand::Clippy {
                 cargoarg,
                 package: Some(p),
                 target,
                 features,
             },
             false,
-        );
+        )
-
-        if let Err(e) = res {
-            error!("{e}")
-        }
     });
 
-    Ok(())
+    runner.run_and_coalesce()
 }
 
 /// Run cargo fmt on selected package
@@ -128,23 +252,18 @@ pub fn cargo_format(
     package: &PackageOpt,
     check_only: bool,
 ) -> anyhow::Result<()> {
-    package.packages().for_each(|p| {
+    let runner = package.packages().map(|p| {
-        let res = command_parser(
+        (
             globals,
-            &CargoCommand::Format {
+            CargoCommand::Format {
                 cargoarg,
                 package: Some(p),
                 check_only,
             },
             false,
-        );
+        )
-
-        if let Err(e) = res {
-            error!("{e}")
-        }
     });
-
+    runner.run_and_coalesce()
-    Ok(())
 }
 
 /// Run cargo doc
@@ -176,26 +295,24 @@ pub fn cargo_test(
     package: &PackageOpt,
     backend: Backends,
 ) -> anyhow::Result<()> {
-    package.packages().for_each(|p| {
+    package
-        let cmd = &TestMetadata::match_package(p, backend);
+        .packages()
-        if let Err(err) = command_parser(globals, cmd, false) {
+        .map(|p| (globals, TestMetadata::match_package(p, backend), false))
-            error!("{err}")
+        .run_and_coalesce()
-        }
-    });
-
-    Ok(())
 }
 
 /// Use mdbook to build the book
-pub fn cargo_book(globals: &Globals, arguments: &Option<ExtraArguments>) -> anyhow::Result<()> {
+pub fn cargo_book(
+    globals: &Globals,
+    arguments: &Option<ExtraArguments>,
+) -> anyhow::Result<RunResult> {
     command_parser(
         globals,
         &CargoCommand::Book {
             arguments: arguments.clone(),
         },
         false,
-    )?;
+    )
-    Ok(())
 }
 
 /// Run examples
@@ -211,33 +328,31 @@ pub fn run_test(
     let target = backend.to_target();
     let features = Some(target.and_features(backend.to_rtic_feature()));
 
-    examples.into_par_iter().for_each(|example| {
+    examples_iter(examples)
-        let cmd = CargoCommand::ExampleBuild {
+        .map(|example| {
-            cargoarg: &Some("--quiet"),
+            let cmd = CargoCommand::ExampleBuild {
-            example,
+                cargoarg: &Some("--quiet"),
-            target,
+                example,
-            features: features.clone(),
+                target,
-            mode: BuildMode::Release,
+                features: features.clone(),
-        };
+                mode: BuildMode::Release,
+            };
 
-        if let Err(err) = command_parser(globals, &cmd, false) {
+            if let Err(err) = command_parser(globals, &cmd, false) {
-            error!("{err}");
+                error!("{err}");
-        }
+            }
 
-        let cmd = CargoCommand::Qemu {
+            let cmd = CargoCommand::Qemu {
-            cargoarg,
+                cargoarg,
-            example,
+                example,
-            target,
+                target,
-            features: features.clone(),
+                features: features.clone(),
-            mode: BuildMode::Release,
+                mode: BuildMode::Release,
-        };
+            };
 
-        if let Err(err) = command_parser(globals, &cmd, overwrite) {
+            (globals, cmd, overwrite)
-            error!("{err}");
+        })
-        }
+        .run_and_coalesce()
-    });
-
-    Ok(())
 }
 
 /// Check the binary sizes of examples
@@ -251,7 +366,7 @@ pub fn build_and_check_size(
     let target = backend.to_target();
     let features = Some(target.and_features(backend.to_rtic_feature()));
 
-    examples.into_par_iter().for_each(|example| {
+    let runner = examples_iter(examples).map(|example| {
         // Make sure the requested example(s) are built
         let cmd = CargoCommand::ExampleBuild {
             cargoarg: &Some("--quiet"),
@@ -272,10 +387,8 @@ pub fn build_and_check_size(
             mode: BuildMode::Release,
             arguments: arguments.clone(),
         };
-        if let Err(err) = command_parser(globals, &cmd, false) {
+        (globals, cmd, false)
-            error!("{err}");
-        }
     });
 
-    Ok(())
+    runner.run_and_coalesce()
 }

xtask/src/command.rs

@@ -108,6 +108,14 @@ pub enum CargoCommand<'a> {
     },
 }
 
+impl core::fmt::Display for CargoCommand<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let executable = self.executable();
+        let args = self.args().join(" ");
+        write!(f, "\"{executable} {args}\"")
+    }
+}
+
 impl<'a> CargoCommand<'a> {
     fn command(&self) -> &str {
         match self {
@@ -460,12 +468,7 @@ impl fmt::Display for BuildMode {
 }
 
 pub fn run_command(command: &CargoCommand, stderr_mode: OutputMode) -> anyhow::Result<RunResult> {
-    let command_display = command.executable();
+    debug!("👟 {command}");
-    let args = command.args();
-
-    let full_command = format!("\"{command_display}\" {}", args.join(" "));
-
-    debug!("👟 {full_command}");
 
     let result = Command::new(command.executable())
         .args(command.args())
@@ -479,7 +482,6 @@ pub fn run_command(command: &CargoCommand, stderr_mode: OutputMode) -> anyhow::R
 
     Ok(RunResult {
         exit_status,
-        full_command,
         stdout,
         stderr,
     })

xtask/src/main.rs

@@ -3,7 +3,6 @@ mod build;
 mod cargo_commands;
 mod command;
 
-use anyhow::bail;
 use argument_parsing::{ExtraArguments, Globals, Package};
 use clap::Parser;
 use command::OutputMode;
@@ -15,7 +14,6 @@ use std::{
     fs::File,
     io::prelude::*,
     path::{Path, PathBuf},
-    process,
     process::ExitStatus,
     str,
 };
@@ -72,7 +70,6 @@ const ARMV8MMAIN: Target = Target::new("thumbv8m.main-none-eabi", false);
 #[derive(Debug, Clone)]
 pub struct RunResult {
     exit_status: ExitStatus,
-    full_command: String,
     stdout: String,
     stderr: String,
 }
@@ -125,7 +122,9 @@ fn main() -> anyhow::Result<()> {
     // check the name of `env::current_dir()` because people might clone it into a different name)
     let probably_running_from_repo_root = Path::new("./xtask").exists();
     if !probably_running_from_repo_root {
-        bail!("xtasks can only be executed from the root of the `rtic` repository");
+        return Err(anyhow::anyhow!(
+            "xtasks can only be executed from the root of the `rtic` repository"
+        ));
     }
 
     let examples: Vec<_> = std::fs::read_dir("./rtic/examples")?
@@ -195,10 +194,10 @@ fn main() -> anyhow::Result<()> {
                     \n{examples:#?}\n\
              By default if example flag is emitted, all examples are tested.",
             );
-            process::exit(exitcode::USAGE);
+            return Err(anyhow::anyhow!("Incorrect usage"));
         } else {
+            examples_to_run
         }
-        examples_to_run
     };
 
     init_build_dir()?;
@@ -299,7 +298,11 @@ fn main() -> anyhow::Result<()> {
 }
 
 // run example binary `example`
-fn command_parser(glob: &Globals, command: &CargoCommand, overwrite: bool) -> anyhow::Result<()> {
+fn command_parser(
+    glob: &Globals,
+    command: &CargoCommand,
+    overwrite: bool,
+) -> anyhow::Result<RunResult> {
     let output_mode = if glob.stderr_inherited {
         OutputMode::Inherited
     } else {
@@ -338,8 +341,9 @@ fn command_parser(glob: &Globals, command: &CargoCommand, overwrite: bool) -> an
                 };
             } else {
                 run_successful(&cargo_run_result, &expected_output_file)?;
-            }
+            };
-            Ok(())
+
+            Ok(cargo_run_result)
         }
         CargoCommand::Format { .. }
         | CargoCommand::ExampleCheck { .. }
@@ -352,30 +356,7 @@ fn command_parser(glob: &Globals, command: &CargoCommand, overwrite: bool) -> an
         | CargoCommand::Book { .. }
         | CargoCommand::ExampleSize { .. } => {
             let cargo_result = run_command(command, output_mode)?;
-            let command = cargo_result.full_command;
+            Ok(cargo_result)
-            if let Some(exit_code) = cargo_result.exit_status.code() {
-                if exit_code != exitcode::OK {
-                    error!("Command {command} failed.");
-                    error!("Exit code: {exit_code}");
-
-                    if !cargo_result.stdout.is_empty() {
-                        info!("{}", cargo_result.stdout);
-                    }
-                    if !cargo_result.stderr.is_empty() {
-                        error!("{}", cargo_result.stderr);
-                    }
-                    process::exit(exit_code);
-                } else {
-                    if !cargo_result.stdout.is_empty() {
-                        info!("{}", cargo_result.stdout);
-                    }
-                    if !cargo_result.stderr.is_empty() {
-                        info!("{}", cargo_result.stderr);
-                    }
-                }
-            }
-
-            Ok(())
         }
     }
 }