From 525703358bf52b74a01d9b0c04680d33621d60cd Mon Sep 17 00:00:00 2001 From: datdenkikniet Date: Sat, 15 Apr 2023 12:21:11 +0200 Subject: [PATCH] Rework command execution structure and make rayon optional (since it's not necessarily faster due to workspace wide lockfile contention) --- .cargo/config.toml | 3 +- xtask/Cargo.toml | 3 +- xtask/src/argument_parsing.rs | 10 ++ xtask/src/cargo_commands.rs | 261 ++++++++++++++++++++++++---------- xtask/src/command.rs | 16 ++- xtask/src/main.rs | 47 ++---- 6 files changed, 223 insertions(+), 117 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index d70faef427..0a62466ad2 100644 --- a/.cargo/config.toml +++ b/.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" @@ -10,4 +11,4 @@ runner = "qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semiho [target.'cfg(all(target_arch = "arm", target_os = "none"))'] rustflags = [ "-C", "link-arg=-Tlink.x", -] \ No newline at end of file +] diff --git a/xtask/Cargo.toml b/xtask/Cargo.toml index 5609480bfa..9e565fa470 100644 --- a/xtask/Cargo.toml +++ b/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" diff --git a/xtask/src/argument_parsing.rs b/xtask/src/argument_parsing.rs index 7284fc553e..e653f9a0ec 100644 --- a/xtask/src/argument_parsing.rs +++ b/xtask/src/argument_parsing.rs @@ -275,12 +275,22 @@ pub struct PackageOpt { } impl PackageOpt { + #[cfg(not(feature = "rayon"))] pub fn packages(&self) -> impl Iterator { self.package .map(|p| vec![p]) .unwrap_or(Package::all()) .into_iter() } + + #[cfg(feature = "rayon")] + pub fn packages(&self) -> impl rayon::prelude::ParallelIterator { + use rayon::prelude::*; + self.package + .map(|p| vec![p]) + .unwrap_or(Package::all()) + .into_par_iter() + } } #[derive(Args, Debug, Clone)] diff --git a/xtask/src/cargo_commands.rs b/xtask/src/cargo_commands.rs index 2a15b3c163..e88f31e970 100644 --- a/xtask/src/cargo_commands.rs +++ b/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) -> 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 { + examples.into_iter() + } + + impl<'g, 'c, I> CoalescingRunning for I + where + I: Iterator, 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 { + examples.into_par_iter() + } + + impl<'g, 'c, I> CoalescingRunning for I + where + I: ParallelIterator, 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 { - error!("{e}"); - } + + (globals, command, false) }); - 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, }, }; - - if let Err(err) = command_parser(globals, &command, false) { - error!("{err}"); - } + (globals, command, false) }); - - Ok(()) + runner.run_and_coalesce() } /// 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 res = command_parser( + let runner = package.packages().map(|p| { + ( globals, - &CargoCommand::Format { + CargoCommand::Format { cargoarg, package: Some(p), check_only, }, false, - ); - - if let Err(e) = res { - error!("{e}") - } + ) }); - - Ok(()) + runner.run_and_coalesce() } /// Run cargo doc @@ -176,26 +295,24 @@ pub fn cargo_test( package: &PackageOpt, backend: Backends, ) -> anyhow::Result<()> { - package.packages().for_each(|p| { - let cmd = &TestMetadata::match_package(p, backend); - if let Err(err) = command_parser(globals, cmd, false) { - error!("{err}") - } - }); - - Ok(()) + package + .packages() + .map(|p| (globals, TestMetadata::match_package(p, backend), false)) + .run_and_coalesce() } /// Use mdbook to build the book -pub fn cargo_book(globals: &Globals, arguments: &Option) -> anyhow::Result<()> { +pub fn cargo_book( + globals: &Globals, + arguments: &Option, +) -> anyhow::Result { 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| { - let cmd = CargoCommand::ExampleBuild { - cargoarg: &Some("--quiet"), - example, - target, - features: features.clone(), - mode: BuildMode::Release, - }; + examples_iter(examples) + .map(|example| { + let cmd = CargoCommand::ExampleBuild { + cargoarg: &Some("--quiet"), + example, + target, + features: features.clone(), + mode: BuildMode::Release, + }; - if let Err(err) = command_parser(globals, &cmd, false) { - error!("{err}"); - } + if let Err(err) = command_parser(globals, &cmd, false) { + error!("{err}"); + } - let cmd = CargoCommand::Qemu { - cargoarg, - example, - target, - features: features.clone(), - mode: BuildMode::Release, - }; + let cmd = CargoCommand::Qemu { + cargoarg, + example, + target, + features: features.clone(), + mode: BuildMode::Release, + }; - if let Err(err) = command_parser(globals, &cmd, overwrite) { - error!("{err}"); - } - }); - - Ok(()) + (globals, cmd, overwrite) + }) + .run_and_coalesce() } /// 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) { - error!("{err}"); - } + (globals, cmd, false) }); - Ok(()) + runner.run_and_coalesce() } diff --git a/xtask/src/command.rs b/xtask/src/command.rs index 32ca9c854d..359a7f9fd0 100644 --- a/xtask/src/command.rs +++ b/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 { - let command_display = command.executable(); - let args = command.args(); - - let full_command = format!("\"{command_display}\" {}", args.join(" ")); - - debug!("👟 {full_command}"); + debug!("👟 {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, }) diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 89ca0feb1f..a50657d0a2 100644 --- a/xtask/src/main.rs +++ b/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 { 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; - 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(()) + Ok(cargo_result) } } }