fix: execute pre- and post-bench scripts for non-perf walltime runner

Author: not-matthias

src/run/runner/wall_time/executor.rs

@@ -11,9 +11,58 @@ use crate::run::{check_system::SystemInfo, config::Config};
 use async_trait::async_trait;
 use std::fs::canonicalize;
 use std::io::Write;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 use tempfile::NamedTempFile;
 
+struct HookScriptsGuard {
+    post_bench_script: PathBuf,
+}
+
+impl HookScriptsGuard {
+    fn execute_script_from_path<P: AsRef<Path>>(path: P) -> anyhow::Result<()> {
+        let path = path.as_ref();
+        if !path.exists() || !path.is_file() {
+            debug!("Script not found or not a file: {}", path.display());
+            return Ok(());
+        }
+
+        let output = Command::new("bash").args([&path]).output()?;
+        if !output.status.success() {
+            debug!("stdout: {}", String::from_utf8_lossy(&output.stdout));
+            debug!("stderr: {}", String::from_utf8_lossy(&output.stderr));
+            bail!("Failed to execute script: {}", path.display());
+        }
+
+        Ok(())
+    }
+
+    pub fn setup_with_scripts<P: AsRef<Path>>(pre_bench_script: P, post_bench_script: P) -> Self {
+        if let Err(e) = Self::execute_script_from_path(pre_bench_script.as_ref()) {
+            debug!("Failed to execute pre-bench script: {e}");
+        }
+
+        Self {
+            post_bench_script: post_bench_script.as_ref().to_path_buf(),
+        }
+    }
+
+    pub fn setup() -> Self {
+        Self::setup_with_scripts(
+            "/usr/local/bin/codspeed-pre-bench",
+            "/usr/local/bin/codspeed-post-bench",
+        )
+    }
+}
+
+impl Drop for HookScriptsGuard {
+    fn drop(&mut self) {
+        if let Err(e) = Self::execute_script_from_path(&self.post_bench_script) {
+            debug!("Failed to execute post-bench script: {e}");
+        }
+    }
+}
+
 pub struct WallTimeExecutor {
     perf: Option<PerfRunner>,
 }
@@ -86,17 +135,20 @@ impl Executor for WallTimeExecutor {
             cmd.current_dir(abs_cwd);
         }
 
-        let (_env_file, bench_cmd) = Self::walltime_bench_cmd(config, run_data)?;
+        let status = {
+            let _guard = HookScriptsGuard::setup();
 
-        let status = if let Some(perf) = &self.perf
-            && config.enable_perf
-        {
-            perf.run(cmd, &bench_cmd, config).await
-        } else {
-            cmd.args(["sh", "-c", &bench_cmd]);
-            debug!("cmd: {cmd:?}");
+            let (_env_file, bench_cmd) = Self::walltime_bench_cmd(config, run_data)?;
+            if let Some(perf) = &self.perf
+                && config.enable_perf
+            {
+                perf.run(cmd, &bench_cmd, config).await
+            } else {
+                cmd.args(["sh", "-c", &bench_cmd]);
+                debug!("cmd: {cmd:?}");
 
-            run_command_with_log_pipe(cmd).await
+                run_command_with_log_pipe(cmd).await
+            }
         };
 
         let status =
@@ -127,3 +179,51 @@ impl Executor for WallTimeExecutor {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use tempfile::NamedTempFile;
+
+    use super::*;
+    use std::{
+        io::{Read, Write},
+        os::unix::fs::PermissionsExt,
+    };
+
+    #[test]
+    fn test_env_guard_no_crash() {
+        fn create_run_script(content: &str) -> anyhow::Result<NamedTempFile> {
+            let rwx = std::fs::Permissions::from_mode(0o777);
+            let mut script_file = tempfile::Builder::new()
+                .suffix(".sh")
+                .permissions(rwx)
+                .keep(true)
+                .tempfile()?;
+            script_file.write_all(content.as_bytes())?;
+
+            Ok(script_file)
+        }
+
+        let mut tmp_dst = tempfile::NamedTempFile::new().unwrap();
+
+        let pre_script = create_run_script(&format!(
+            "#!/usr/bin/env bash\necho \"pre\" >> {}",
+            tmp_dst.path().display()
+        ))
+        .unwrap();
+        let post_script = create_run_script(&format!(
+            "#!/usr/bin/env bash\necho \"post\" >> {}",
+            tmp_dst.path().display()
+        ))
+        .unwrap();
+
+        {
+            let _guard =
+                HookScriptsGuard::setup_with_scripts(pre_script.path(), post_script.path());
+        }
+
+        let mut result = String::new();
+        tmp_dst.read_to_string(&mut result).unwrap();
+        assert_eq!(result, "pre\npost\n");
+    }
+}

src/run/runner/wall_time/perf/mod.rs

@@ -15,7 +15,7 @@ use metadata::PerfMetadata;
 use perf_map::ProcessSymbols;
 use shared::Command as FifoCommand;
 use std::collections::HashSet;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::process::Command;
 use std::time::Duration;
 use std::{cell::OnceCell, collections::HashMap, process::ExitStatus};
@@ -34,55 +34,6 @@ pub mod unwind_data;
 
 const PERF_DATA_PREFIX: &str = "perf.data.";
 
-struct EnvGuard {
-    post_bench_script: PathBuf,
-}
-
-impl EnvGuard {
-    fn execute_script_from_path<P: AsRef<Path>>(path: P) -> anyhow::Result<()> {
-        let path = path.as_ref();
-        if !path.exists() || !path.is_file() {
-            warn!("Script not found or not a file: {}", path.display());
-            return Ok(());
-        }
-
-        let output = Command::new("bash").args([&path]).output()?;
-        if !output.status.success() {
-            info!("stdout: {}", String::from_utf8_lossy(&output.stdout));
-            error!("stderr: {}", String::from_utf8_lossy(&output.stderr));
-            bail!("Failed to execute script: {}", path.display());
-        }
-
-        Ok(())
-    }
-
-    pub fn setup_with_scripts<P: AsRef<Path>>(pre_bench_script: P, post_bench_script: P) -> Self {
-        if let Err(e) = Self::execute_script_from_path(pre_bench_script.as_ref()) {
-            warn!("Failed to execute pre-bench script: {e}");
-            println!("asdf: {e}");
-        }
-
-        Self {
-            post_bench_script: post_bench_script.as_ref().to_path_buf(),
-        }
-    }
-
-    pub fn setup() -> Self {
-        Self::setup_with_scripts(
-            "/usr/local/bin/codspeed-pre-bench",
-            "/usr/local/bin/codspeed-post-bench",
-        )
-    }
-}
-
-impl Drop for EnvGuard {
-    fn drop(&mut self) {
-        if let Err(e) = Self::execute_script_from_path(&self.post_bench_script) {
-            warn!("Failed to execute post-bench script: {e}");
-        }
-    }
-}
-
 pub struct PerfRunner {
     perf_dir: TempDir,
     benchmark_data: OnceCell<BenchmarkData>,
@@ -182,11 +133,7 @@ impl PerfRunner {
 
             Ok(())
         };
-
-        {
-            let _guard = EnvGuard::setup();
-            run_command_with_log_pipe_and_callback(cmd, on_process_started).await
-        }
+        run_command_with_log_pipe_and_callback(cmd, on_process_started).await
     }
 
     pub async fn save_files_to(&self, profile_folder: &PathBuf) -> anyhow::Result<()> {
@@ -456,49 +403,3 @@ impl BenchmarkData {
         self.bench_order_by_pid.values().map(|v| v.len()).sum()
     }
 }
-#[cfg(test)]
-mod tests {
-    use tempfile::NamedTempFile;
-
-    use super::*;
-    use std::{
-        io::{Read, Write},
-        os::unix::fs::PermissionsExt,
-    };
-
-    #[test]
-    fn test_env_guard_no_crash() {
-        fn create_run_script(content: &str) -> anyhow::Result<NamedTempFile> {
-            let rwx = std::fs::Permissions::from_mode(0o777);
-            let mut script_file = tempfile::Builder::new()
-                .suffix(".sh")
-                .permissions(rwx)
-                .keep(true)
-                .tempfile()?;
-            script_file.write_all(content.as_bytes())?;
-
-            Ok(script_file)
-        }
-
-        let mut tmp_dst = tempfile::NamedTempFile::new().unwrap();
-
-        let pre_script = create_run_script(&format!(
-            "#!/usr/bin/env bash\necho \"pre\" >> {}",
-            tmp_dst.path().display()
-        ))
-        .unwrap();
-        let post_script = create_run_script(&format!(
-            "#!/usr/bin/env bash\necho \"post\" >> {}",
-            tmp_dst.path().display()
-        ))
-        .unwrap();
-
-        {
-            let _guard = EnvGuard::setup_with_scripts(pre_script.path(), post_script.path());
-        }
-
-        let mut result = String::new();
-        tmp_dst.read_to_string(&mut result).unwrap();
-        assert_eq!(result, "pre\npost\n");
-    }
-}