Run setup/cleanup commands in a shell (#990)

src/benchmark/executor.rs

@@ -5,9 +5,7 @@ use std::process::ExitStatus;
 use crate::benchmark::measurement::Measurement;
 use crate::benchmark::measurement::Measurements;
 use crate::command::Command;
-use crate::options::{
-    CmdFailureAction, CommandInputPolicy, CommandOutputPolicy, Options, OutputStyleOption, Shell,
-};
+use crate::options::{CmdFailureAction, CommandOutputPolicy, Options, OutputStyleOption, Shell};
 use crate::output::progress_bar::get_progress_bar;
 use crate::quantity::{second, Information, Time, Zero};
 use crate::timer::execute_and_measure;
@@ -42,6 +40,21 @@ pub trait Executor {
         output_policy: &CommandOutputPolicy,
     ) -> Result<Measurement>;
 
+    /// Run a lifecycle command through a shell, always treating failures as errors.
+    fn run_intermediate_command_and_measure(
+        &self,
+        command: &Command<'_>,
+        iteration: BenchmarkIteration,
+        output_policy: &CommandOutputPolicy,
+    ) -> Result<Measurement> {
+        self.run_command_and_measure(
+            command,
+            iteration,
+            Some(CmdFailureAction::RaiseError),
+            output_policy,
+        )
+    }
+
     /// Perform a calibration of this executor. For example,
     /// when running commands through a shell, we need to
     /// measure the shell spawning time separately in order
@@ -58,13 +71,13 @@ pub trait Executor {
 fn run_command_and_measure_common(
     mut command: std::process::Command,
     iteration: BenchmarkIteration,
-    command_failure_action: CmdFailureAction,
-    command_input_policy: &CommandInputPolicy,
+    command_failure_action: Option<CmdFailureAction>,
+    options: &Options,
     command_output_policy: &CommandOutputPolicy,
     command_name: &str,
     collect_hardware_counters: bool,
 ) -> Result<Measurement> {
-    let stdin = command_input_policy.get_stdin()?;
+    let stdin = options.command_input_policy.get_stdin()?;
     let (stdout, stderr) = command_output_policy.get_stdout_stderr()?;
     command.stdin(stdin).stdout(stdout).stderr(stderr);
 
@@ -83,10 +96,14 @@ fn run_command_and_measure_common(
     if !measurement.exit_status.success() {
         use crate::util::exit_code::extract_exit_code;
 
-        let should_fail = match command_failure_action {
+        let show_ignore_failure_hint = command_failure_action.is_none();
+        let should_fail = match command_failure_action
+            .as_ref()
+            .unwrap_or(&options.command_failure_action)
+        {
             CmdFailureAction::RaiseError => true,
             CmdFailureAction::IgnoreAllFailures => false,
-            CmdFailureAction::IgnoreSpecificFailures(ref codes) => {
+            CmdFailureAction::IgnoreSpecificFailures(codes) => {
                 // Only fail if the exit code is not in the list of codes to ignore
                 if let Some(exit_code) = extract_exit_code(measurement.exit_status) {
                     !codes.contains(&exit_code)
@@ -105,15 +122,17 @@ fn run_command_and_measure_common(
                 BenchmarkIteration::Benchmark(0) => "the first benchmark run".to_string(),
                 BenchmarkIteration::Benchmark(i) => format!("benchmark iteration {i}"),
             };
-            bail!(
-                "{cause} in {when}. Use the '-i'/'--ignore-failure' option if you want to ignore this. \
-                Alternatively, use the '--show-output' option to debug what went wrong.",
-                cause=measurement.exit_status.code().map_or(
-                    "The process has been terminated by a signal".into(),
-                    |c| format!("Command terminated with non-zero exit code {c}")
-
-                ),
+            let cause = measurement.exit_status.code().map_or_else(
+                || "was terminated by a signal".to_owned(),
+                |code| format!("terminated with non-zero exit code {code}"),
             );
+            let hint = if show_ignore_failure_hint {
+                " Use the '-i'/'--ignore-failure' option if you want to ignore this. \
+                 Alternatively, use the '--show-output' option to debug what went wrong."
+            } else {
+                " Use the '--show-output' option to debug what went wrong."
+            };
+            bail!("Command '{command_name}' {cause} in {when}.{hint}");
         }
     }
 
@@ -141,14 +160,26 @@ impl Executor for RawExecutor<'_> {
         run_command_and_measure_common(
             command.get_command()?,
             iteration,
-            command_failure_action.unwrap_or_else(|| self.options.command_failure_action.clone()),
-            &self.options.command_input_policy,
+            command_failure_action,
+            self.options,
             output_policy,
             &command.get_command_line(),
             matches!(iteration, BenchmarkIteration::Benchmark(_)),
         )
     }
 
+    fn run_intermediate_command_and_measure(
+        &self,
+        command: &Command<'_>,
+        iteration: BenchmarkIteration,
+        output_policy: &CommandOutputPolicy,
+    ) -> Result<Measurement> {
+        // Keep shell startup in the hook's duration: raw benchmarks have no
+        // calibrated shell overhead to add back when estimating the run count.
+        ShellExecutor::new(&Shell::platform_default(), self.options)
+            .run_intermediate_command_and_measure(command, iteration, output_policy)
+    }
+
     fn calibrate(&mut self) -> Result<()> {
         Ok(())
     }
@@ -197,8 +228,8 @@ impl Executor for ShellExecutor<'_> {
         let mut measurement = run_command_and_measure_common(
             command_builder,
             iteration,
-            command_failure_action.unwrap_or_else(|| self.options.command_failure_action.clone()),
-            &self.options.command_input_policy,
+            command_failure_action,
+            self.options,
             output_policy,
             &command.get_command_line(),
             false,

src/benchmark/mod.rs

@@ -11,9 +11,7 @@ use crate::benchmark::benchmark_result::Parameter;
 use crate::benchmark::executor::BenchmarkIteration;
 use crate::benchmark::measurement::{Measurement, Measurements};
 use crate::command::Command;
-use crate::options::{
-    CmdFailureAction, CommandOutputPolicy, ExecutorKind, Options, OutputStyleOption,
-};
+use crate::options::{CommandOutputPolicy, ExecutorKind, Options, OutputStyleOption};
 use crate::outlier_detection::OUTLIER_THRESHOLD;
 use crate::output::console_writeln;
 use crate::output::progress_bar::{
@@ -55,22 +53,24 @@ impl<'a> Benchmark<'a> {
         }
     }
 
-    /// Run setup, cleanup, or preparation commands
+    /// Run setup, cleanup, preparation, or conclusion commands
     fn run_intermediate_command(
         &self,
         command: &Command<'_>,
-        error_output: &'static str,
+        kind: &'static str,
         output_policy: &CommandOutputPolicy,
         iteration: executor::BenchmarkIteration,
     ) -> Result<Measurement> {
         self.executor
-            .run_command_and_measure(
-                command,
-                iteration,
-                Some(CmdFailureAction::RaiseError),
-                output_policy,
-            )
-            .map_err(|_| anyhow!(error_output))
+            .run_intermediate_command_and_measure(command, iteration, output_policy)
+            .map_err(|error| {
+                let hint = if cfg!(windows) {
+                    "Append ' || exit /b 0' to the command if this failure can be ignored."
+                } else {
+                    "Append ' || true' to the command if this failure can be ignored."
+                };
+                anyhow!("The {kind} command failed: {error:#} {hint}")
+            })
     }
 
     /// Run the command specified by `--setup`.
@@ -85,14 +85,11 @@ impl<'a> Benchmark<'a> {
             .as_ref()
             .map(|setup_command| Command::new_parametrized(None, setup_command, parameters));
 
-        let error_output = "The setup command terminated with a non-zero exit code. \
-                            Append ' || true' to the command if you are sure that this can be ignored.";
-
         Ok(command
             .map(|cmd| {
                 self.run_intermediate_command(
                     &cmd,
-                    error_output,
+                    "setup",
                     output_policy,
                     BenchmarkIteration::NonBenchmarkRun,
                 )
@@ -113,14 +110,11 @@ impl<'a> Benchmark<'a> {
             .as_ref()
             .map(|cleanup_command| Command::new_parametrized(None, cleanup_command, parameters));
 
-        let error_output = "The cleanup command terminated with a non-zero exit code. \
-                            Append ' || true' to the command if you are sure that this can be ignored.";
-
         Ok(command
             .map(|cmd| {
                 self.run_intermediate_command(
                     &cmd,
-                    error_output,
+                    "cleanup",
                     output_policy,
                     BenchmarkIteration::NonBenchmarkRun,
                 )
@@ -136,10 +130,7 @@ impl<'a> Benchmark<'a> {
         output_policy: &CommandOutputPolicy,
         iteration: executor::BenchmarkIteration,
     ) -> Result<Measurement> {
-        let error_output = "The preparation command terminated with a non-zero exit code. \
-                            Append ' || true' to the command if you are sure that this can be ignored.";
-
-        self.run_intermediate_command(command, error_output, output_policy, iteration)
+        self.run_intermediate_command(command, "preparation", output_policy, iteration)
     }
 
     /// Run the command specified by `--conclude`.
@@ -149,10 +140,7 @@ impl<'a> Benchmark<'a> {
         output_policy: &CommandOutputPolicy,
         iteration: executor::BenchmarkIteration,
     ) -> Result<Measurement> {
-        let error_output = "The conclusion command terminated with a non-zero exit code. \
-                            Append ' || true' to the command if you are sure that this can be ignored.";
-
-        self.run_intermediate_command(command, error_output, output_policy, iteration)
+        self.run_intermediate_command(command, "conclusion", output_policy, iteration)
     }
 
     fn validate_measurement(&self, measurement: &Measurement) -> Result<()> {