From cd938a7800f792f0bebbd67fd5736581f550af58 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Mon, 7 Jul 2025 18:14:42 +0200 Subject: [PATCH 1/4] fix: multi-line commands in valgrind --- src/run/runner/valgrind/measure.rs | 39 +++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/src/run/runner/valgrind/measure.rs b/src/run/runner/valgrind/measure.rs index a0510e1f..4f39955b 100644 --- a/src/run/runner/valgrind/measure.rs +++ b/src/run/runner/valgrind/measure.rs @@ -56,7 +56,7 @@ fn create_run_script() -> anyhow::Result { // 1. The command to execute // 2. The path to the file where the exit code will be written const WRAPPER_SCRIPT: &str = r#"#!/bin/sh - (eval $1) + sh -c "$1" status=$? echo -n "$status" > "$2" "#; @@ -189,6 +189,10 @@ mod tests { #[test] fn test_run_wrapper_script() { + temp_env::with_var("TEST_ENV_VAR", "test_value".into(), || { + assert_eq!(safe_run("echo $TEST_ENV_VAR"), (0, 0)); + }); + assert_eq!(safe_run("ls"), (0, 0)); assert_eq!(safe_run("exit 0"), (0, 0)); assert_eq!(safe_run("exit 1"), (0, 1)); @@ -201,5 +205,38 @@ mod tests { assert_eq!(safe_run("test 1 = 2 && exit 42"), (0, 1)); assert_eq!(safe_run("test 1 = 1 || exit 42"), (0, 0)); assert_eq!(safe_run("test 1 = 2 || exit 42"), (0, 42)); + + const MULTILINE_ECHO_SCRIPT: &str = "echo \"Working\" +echo \"with\" +echo \"multiple lines\""; + + const MULTILINE_ECHO_WITH_SEMICOLONS: &str = "echo \"Working\"; +echo \"with\"; +echo \"multiple lines\";"; + + const ENV_VAR_VALIDATION_SCRIPT: &str = "export MY_ENV_VAR=\"Hello\" +output=$(echo \"$MY_ENV_VAR\") +if [ \"$output\" != \"Hello\" ]; then + echo \"Assertion failed: Expected 'Hello' but got '$output'\" + exit 1 +fi"; + + const ENV_VAR_VALIDATION_FAIL_SCRIPT: &str = "MY_ENV_VAR=\"Wrong\" +output=$(echo \"$MY_ENV_VAR\") +if [ \"$output\" != \"Hello\" ]; then + echo \"Assertion failed: Expected 'Hello' but got '$output'\" + exit 1 +fi"; + + const DIRECTORY_CHECK_SCRIPT: &str = "cd /tmp +# Check that the directory is actually changed +if [ $(basename $(pwd)) != \"tmp\" ]; then + exit 1 +fi"; + assert_eq!(safe_run(MULTILINE_ECHO_SCRIPT), (0, 0)); + assert_eq!(safe_run(MULTILINE_ECHO_WITH_SEMICOLONS), (0, 0)); + assert_eq!(safe_run(DIRECTORY_CHECK_SCRIPT), (0, 0)); + assert_eq!(safe_run(ENV_VAR_VALIDATION_SCRIPT), (0, 0)); + assert_eq!(safe_run(ENV_VAR_VALIDATION_FAIL_SCRIPT), (0, 1)); } } From da3af1cfe4d23267820934a4900a8261562ce804 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Mon, 7 Jul 2025 19:21:37 +0200 Subject: [PATCH 2/4] fix: only panic in upload for non-existing integration This makes it easier to test since we can skip the upload and just focus on the benchmark running. --- src/run/runner/wall_time/perf/mod.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/run/runner/wall_time/perf/mod.rs b/src/run/runner/wall_time/perf/mod.rs index 455721b2..ba435568 100644 --- a/src/run/runner/wall_time/perf/mod.rs +++ b/src/run/runner/wall_time/perf/mod.rs @@ -396,7 +396,7 @@ impl PerfRunner { } Ok(BenchmarkData { - integration: integration.context("Couldn't find integration metadata")?, + integration, bench_order_by_pid, symbols_by_pid, unwind_data_by_pid, @@ -406,7 +406,7 @@ impl PerfRunner { pub struct BenchmarkData { /// Name and version of the integration - integration: (String, String), + integration: Option<(String, String)>, bench_order_by_pid: HashMap>, symbols_by_pid: HashMap, @@ -426,7 +426,10 @@ impl BenchmarkData { } let metadata = PerfMetadata { - integration: self.integration.clone(), + integration: self + .integration + .clone() + .context("Couldn't find integration metadata")?, bench_order_by_pid: self.bench_order_by_pid.clone(), ignored_modules: { let mut to_ignore = vec![]; From 526f1a87ac00a8e6e98db23b0ef03d551e366cec Mon Sep 17 00:00:00 2001 From: not-matthias Date: Tue, 8 Jul 2025 15:46:45 +0200 Subject: [PATCH 3/4] fix: forward environment to systemd-run cmd --- src/run/runner/helpers/env.rs | 9 +++++++ src/run/runner/wall_time/executor.rs | 40 +++++++++++++++++++++------- src/run/runner/wall_time/perf/mod.rs | 16 ++++------- 3 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/run/runner/helpers/env.rs b/src/run/runner/helpers/env.rs index 9a5a6172..db745a06 100644 --- a/src/run/runner/helpers/env.rs +++ b/src/run/runner/helpers/env.rs @@ -17,3 +17,12 @@ pub fn get_base_injected_env( ), ]) } + +pub fn is_codspeed_debug_enabled() -> bool { + let log_level = std::env::var("CODSPEED_LOG") + .ok() + .and_then(|log_level| log_level.parse::().ok()) + .unwrap_or(log::LevelFilter::Info); + + log_level < log::LevelFilter::Debug +} diff --git a/src/run/runner/wall_time/executor.rs b/src/run/runner/wall_time/executor.rs index 5c58c18c..eea8ba18 100644 --- a/src/run/runner/wall_time/executor.rs +++ b/src/run/runner/wall_time/executor.rs @@ -3,14 +3,16 @@ use crate::prelude::*; use crate::run::RunnerMode; use crate::run::instruments::mongo_tracer::MongoTracer; use crate::run::runner::executor::Executor; -use crate::run::runner::helpers::env::get_base_injected_env; +use crate::run::runner::helpers::env::{get_base_injected_env, is_codspeed_debug_enabled}; use crate::run::runner::helpers::get_bench_command::get_bench_command; use crate::run::runner::helpers::run_command_with_log_pipe::run_command_with_log_pipe; use crate::run::runner::{ExecutorName, RunData}; use crate::run::{check_system::SystemInfo, config::Config}; use async_trait::async_trait; use std::fs::canonicalize; +use std::io::Write; use std::process::Command; +use tempfile::NamedTempFile; pub struct WallTimeExecutor { perf: Option, @@ -23,18 +25,36 @@ impl WallTimeExecutor { } } - fn walltime_bench_cmd(config: &Config, run_data: &RunData) -> Result { + fn walltime_bench_cmd(config: &Config, run_data: &RunData) -> Result<(NamedTempFile, String)> { let bench_cmd = get_bench_command(config)?; - let setenv = get_base_injected_env(RunnerMode::Walltime, &run_data.profile_folder) - .into_iter() - .map(|(env, value)| format!("--setenv={env}={value}")) - .join(" "); + let combined_env = std::env::vars() + .chain( + get_base_injected_env(RunnerMode::Walltime, &run_data.profile_folder) + .into_iter() + .map(|(k, v)| (k.into(), v)), + ) + .map(|(env, value)| format!("{env}={value}")) + .collect::>() + .join("\n"); + + let mut env_file = NamedTempFile::new()?; + env_file.write_all(combined_env.as_bytes())?; + + let quiet_flag = if is_codspeed_debug_enabled() { + "--quiet" + } else { + "" + }; + let uid = nix::unistd::Uid::current().as_raw(); let gid = nix::unistd::Gid::current().as_raw(); - Ok(format!( - "systemd-run --scope --slice=codspeed.slice --same-dir --uid={uid} --gid={gid} {setenv} -- {bench_cmd}" - )) + let cmd = format!( + "systemd-run {quiet_flag} --pipe --collect --wait --slice=codspeed.slice --same-dir --uid={uid} --gid={gid} --property=EnvironmentFile={} -- sh -c '{}'", + env_file.path().display(), + bench_cmd.replace("'", "\"") + ); + Ok((env_file, cmd)) } } @@ -66,7 +86,7 @@ impl Executor for WallTimeExecutor { cmd.current_dir(abs_cwd); } - let bench_cmd = Self::walltime_bench_cmd(config, run_data)?; + let (_env_file, bench_cmd) = Self::walltime_bench_cmd(config, run_data)?; let status = if let Some(perf) = &self.perf && config.enable_perf diff --git a/src/run/runner/wall_time/perf/mod.rs b/src/run/runner/wall_time/perf/mod.rs index ba435568..12c9133f 100644 --- a/src/run/runner/wall_time/perf/mod.rs +++ b/src/run/runner/wall_time/perf/mod.rs @@ -3,6 +3,7 @@ use crate::prelude::*; use crate::run::UnwindingMode; use crate::run::config::Config; +use crate::run::runner::helpers::env::is_codspeed_debug_enabled; use crate::run::runner::helpers::run_command_with_log_pipe::run_command_with_log_pipe_and_callback; use crate::run::runner::helpers::setup::run_with_sudo; use crate::run::runner::valgrind::helpers::ignored_objects_path::get_objects_path_to_ignore; @@ -157,17 +158,10 @@ impl PerfRunner { }; debug!("Using call graph mode: {cg_mode:?}"); - let quiet_flag = { - let log_level = std::env::var("CODSPEED_LOG") - .ok() - .and_then(|log_level| log_level.parse::().ok()) - .unwrap_or(log::LevelFilter::Info); - - if log_level < log::LevelFilter::Debug { - "--quiet" - } else { - "" - } + let quiet_flag = if is_codspeed_debug_enabled() { + "--quiet" + } else { + "" }; cmd.args([ From fe4de639b41105bee530a40b986cf8afa7ac5ace Mon Sep 17 00:00:00 2001 From: not-matthias Date: Tue, 8 Jul 2025 10:27:34 +0200 Subject: [PATCH 4/4] chore: add executor tests --- Cargo.lock | 55 ++++++++++++ Cargo.toml | 1 + src/run/runner/mod.rs | 2 + src/run/runner/tests.rs | 187 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 245 insertions(+) create mode 100644 src/run/runner/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 7f797444..f8fcc165 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -304,6 +304,7 @@ dependencies = [ "reqwest", "reqwest-middleware", "reqwest-retry", + "rstest", "serde", "serde_json", "serde_yaml", @@ -673,6 +674,12 @@ dependencies = [ "url", ] +[[package]] +name = "glob" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8d1add55171497b4705a648c6b583acafb01d58050a51727785f0b2c8e0a2b2" + [[package]] name = "gql_client" version = "1.0.7" @@ -1734,6 +1741,12 @@ version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c" +[[package]] +name = "relative-path" +version = "1.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba39f3699c378cd8970968dcbff9c43159ea4cfbd88d43c00b22f2ef10a435d2" + [[package]] name = "reqwest" version = "0.11.27" @@ -1826,12 +1839,48 @@ dependencies = [ "rand", ] +[[package]] +name = "rstest" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6fc39292f8613e913f7df8fa892b8944ceb47c247b78e1b1ae2f09e019be789d" +dependencies = [ + "rstest_macros", + "rustc_version", +] + +[[package]] +name = "rstest_macros" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1f168d99749d307be9de54d23fd226628d99768225ef08f6ffb52e0182a27746" +dependencies = [ + "cfg-if", + "glob", + "proc-macro2", + "quote", + "regex", + "relative-path", + "rustc_version", + "syn 2.0.96", + "unicode-ident", +] + [[package]] name = "rustc-demangle" version = "0.1.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "719b953e2095829ee67db738b3bfa9fa368c94900df327b3f07fe6e794d2fe1f" +[[package]] +name = "rustc_version" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cfcb3a22ef46e85b45de6ee7e79d063319ebb6594faafcf1c225ea92ab6e9b92" +dependencies = [ + "semver", +] + [[package]] name = "rustix" version = "0.38.43" @@ -1913,6 +1962,12 @@ dependencies = [ "libc", ] +[[package]] +name = "semver" +version = "1.0.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56e6fa9c48d24d85fb3de5ad847117517440f6beceb7798af16b4a87d616b8d0" + [[package]] name = "serde" version = "1.0.219" diff --git a/Cargo.toml b/Cargo.toml index 9aac9fd3..60df868a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,6 +62,7 @@ procfs = "0.17.0" temp-env = { version = "0.3.6", features = ["async_closure"] } insta = { version = "1.29.0", features = ["json", "redactions"] } test-with = { version = "0.15", default-features = false, features = [] } +rstest = { version = "0.25.0", default-features = false } [workspace.metadata.release] sign-tag = true diff --git a/src/run/runner/mod.rs b/src/run/runner/mod.rs index 9296f27d..978493b6 100644 --- a/src/run/runner/mod.rs +++ b/src/run/runner/mod.rs @@ -7,6 +7,8 @@ use super::{RunnerMode, config::Config}; mod executor; mod helpers; mod interfaces; +#[cfg(test)] +mod tests; mod valgrind; mod wall_time; diff --git a/src/run/runner/tests.rs b/src/run/runner/tests.rs new file mode 100644 index 00000000..ab668383 --- /dev/null +++ b/src/run/runner/tests.rs @@ -0,0 +1,187 @@ +use crate::run::check_system::SystemInfo; +use crate::run::config::Config; +use crate::run::runner::executor::Executor; +use crate::run::runner::interfaces::RunData; +use crate::run::runner::valgrind::executor::ValgrindExecutor; +use crate::run::{RunnerMode, runner::wall_time::executor::WallTimeExecutor}; +use tempfile::TempDir; +use tokio::sync::{OnceCell, Semaphore, SemaphorePermit}; + +const SIMPLE_ECHO_SCRIPT: &str = "echo 'Hello, World!'"; +const MULTILINE_ECHO_SCRIPT: &str = "echo \"Working\" +echo \"with\" +echo \"multiple lines\""; +const MULTILINE_ECHO_WITH_SEMICOLONS: &str = "echo \"Working\"; +echo \"with\"; +echo \"multiple lines\";"; +const DIRECTORY_CHECK_SCRIPT: &str = "cd /tmp +# Check that the directory is actually changed +if [ $(basename $(pwd)) != \"tmp\" ]; then + exit 1 +fi"; +const ENV_VAR_VALIDATION_SCRIPT: &str = " +output=$(echo \"$MY_ENV_VAR\") +if [ \"$output\" != \"Hello\" ]; then + echo \"Assertion failed: Expected 'Hello' but got '$output'\" + exit 1 +fi"; + +const TESTS: [&str; 5] = [ + SIMPLE_ECHO_SCRIPT, + MULTILINE_ECHO_SCRIPT, + MULTILINE_ECHO_WITH_SEMICOLONS, + DIRECTORY_CHECK_SCRIPT, + ENV_VAR_VALIDATION_SCRIPT, +]; + +async fn create_test_setup() -> (SystemInfo, RunData, TempDir) { + let system_info = SystemInfo::new().unwrap(); + + let temp_dir = TempDir::new().unwrap(); + let run_data = RunData { + profile_folder: temp_dir.path().to_path_buf(), + }; + (system_info, run_data, temp_dir) +} + +mod valgrind { + use super::*; + + async fn get_valgrind_executor() -> &'static ValgrindExecutor { + static VALGRIND_EXECUTOR: OnceCell = OnceCell::const_new(); + + VALGRIND_EXECUTOR + .get_or_init(|| async { + let executor = ValgrindExecutor; + let system_info = SystemInfo::new().unwrap(); + executor.setup(&system_info).await.unwrap(); + executor + }) + .await + } + + fn valgrind_config(command: &str) -> Config { + Config { + mode: RunnerMode::Instrumentation, + command: command.to_string(), + ..Config::test() + } + } + + #[rstest::rstest] + #[case(TESTS[0])] + #[case(TESTS[1])] + #[case(TESTS[2])] + #[case(TESTS[3])] + #[tokio::test] + async fn test_valgrind_executor(#[case] cmd: &str) { + let (system_info, run_data, _temp_dir) = create_test_setup().await; + let executor = get_valgrind_executor().await; + + let config = valgrind_config(cmd); + executor + .run(&config, &system_info, &run_data, &None) + .await + .unwrap(); + } + + #[rstest::rstest] + #[case("MY_ENV_VAR", "Hello", ENV_VAR_VALIDATION_SCRIPT)] + #[tokio::test] + async fn test_valgrind_executor_with_env( + #[case] env_var: &str, + #[case] env_value: &str, + #[case] cmd: &str, + ) { + let (system_info, run_data, _temp_dir) = create_test_setup().await; + let executor = get_valgrind_executor().await; + + temp_env::async_with_vars(&[(env_var, Some(env_value))], async { + let config = valgrind_config(cmd); + executor + .run(&config, &system_info, &run_data, &None) + .await + .unwrap(); + }) + .await; + } +} + +mod walltime { + use super::*; + + async fn get_walltime_executor() -> (SemaphorePermit<'static>, WallTimeExecutor) { + static WALLTIME_INIT: OnceCell<()> = OnceCell::const_new(); + static WALLTIME_SEMAPHORE: OnceCell = OnceCell::const_new(); + + WALLTIME_INIT + .get_or_init(|| async { + let executor = WallTimeExecutor::new(); + let system_info = SystemInfo::new().unwrap(); + executor.setup(&system_info).await.unwrap(); + }) + .await; + + // We can't execute multiple walltime executors in parallel because perf isn't thread-safe (yet). We have to + // use a semaphore to limit concurrent access. + let semaphore = WALLTIME_SEMAPHORE + .get_or_init(|| async { Semaphore::new(1) }) + .await; + let permit = semaphore.acquire().await.unwrap(); + + (permit, WallTimeExecutor::new()) + } + + fn walltime_config(command: &str, enable_perf: bool) -> Config { + Config { + mode: RunnerMode::Walltime, + command: command.to_string(), + enable_perf, + ..Config::test() + } + } + + #[rstest::rstest] + #[case(TESTS[0], false)] + #[case(TESTS[0], true)] + #[case(TESTS[1], false)] + #[case(TESTS[1], true)] + #[case(TESTS[2], false)] + #[case(TESTS[2], true)] + #[case(TESTS[3], false)] + #[case(TESTS[3], true)] + #[tokio::test] + async fn test_walltime_executor(#[case] cmd: &str, #[case] enable_perf: bool) { + let (system_info, run_data, _temp_dir) = create_test_setup().await; + let (_permit, executor) = get_walltime_executor().await; + + let config = walltime_config(cmd, enable_perf); + executor + .run(&config, &system_info, &run_data, &None) + .await + .unwrap(); + } + + #[rstest::rstest] + #[case("MY_ENV_VAR", "Hello", ENV_VAR_VALIDATION_SCRIPT, false)] + #[case("MY_ENV_VAR", "Hello", ENV_VAR_VALIDATION_SCRIPT, true)] + #[tokio::test] + async fn test_walltime_executor_with_env( + #[case] env_var: &str, + #[case] env_value: &str, + #[case] cmd: &str, + #[case] enable_perf: bool, + ) { + let (system_info, run_data, _temp_dir) = create_test_setup().await; + let (_permit, executor) = get_walltime_executor().await; + + temp_env::async_with_vars(&[(env_var, Some(env_value))], async { + let config = walltime_config(cmd, enable_perf); + executor + .run(&config, &system_info, &run_data, &None) + .await + .unwrap(); + }) + .await; + } +}