diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0cd3fdfe..a3b552d6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,11 @@ jobs: - uses: actions/checkout@v3 with: lfs: true - - uses: moonrepo/setup-rust@v1 + + - name: 'Install rust-toolchain.toml' + run: rustup toolchain install + # We use Swatinem/rust-cache to cache cargo registry, index and target in this job + - uses: Swatinem/rust-cache@v2 # Install memtrack for the memory integration tests - name: Install dependencies required for libbpf-sys (vendored feature) @@ -31,6 +35,9 @@ jobs: - name: Install memtrack run: | cargo install --path crates/memtrack --locked + - name: Install exec-harness + run: | + cargo install --path crates/exec-harness --locked - run: cargo test --all --exclude memtrack --exclude exec-harness diff --git a/crates/exec-harness/src/main.rs b/crates/exec-harness/src/main.rs index d755e3cb..df574795 100644 --- a/crates/exec-harness/src/main.rs +++ b/crates/exec-harness/src/main.rs @@ -1,11 +1,11 @@ use crate::prelude::*; use crate::walltime::WalltimeResults; use clap::Parser; -use codspeed::instrument_hooks::InstrumentHooks; use runner_shared::walltime_results::WalltimeBenchmark; use std::path::PathBuf; mod prelude; +mod uri; mod walltime; #[derive(Parser, Debug)] @@ -15,7 +15,7 @@ mod walltime; about = "CodSpeed exec harness - wraps commands with performance instrumentation" )] struct Args { - /// Optional benchmark name (defaults to command filename) + /// Optional benchmark name, else the command will be used as the name #[arg(long)] name: Option, @@ -23,6 +23,7 @@ struct Args { execution_args: walltime::WalltimeExecutionArgs, /// The command and arguments to execute + #[arg(trailing_var_arg = true, allow_hyphen_values = true)] command: Vec, } @@ -38,22 +39,10 @@ fn main() -> Result<()> { bail!("Error: No command provided"); } - // Derive benchmark name from command if not provided - let bench_name = args.name.unwrap_or_else(|| { - // Extract filename from command path - let cmd = &args.command[0]; - std::path::Path::new(cmd).to_string_lossy().into_owned() - }); - - // TODO: Better URI generation - let bench_uri = format!("standalone_run::{bench_name}"); - - let hooks = InstrumentHooks::instance(); - - // TODO(COD-1736): Stop impersonating codspeed-rust 🥸 - hooks - .set_integration("codspeed-rust", env!("CARGO_PKG_VERSION")) - .unwrap(); + let uri::NameAndUri { + name: bench_name, + uri: bench_uri, + } = uri::generate_name_and_uri(&args.name, &args.command); // Build execution options from CLI args let execution_options: walltime::ExecutionOptions = args.execution_args.try_into()?; diff --git a/crates/exec-harness/src/uri.rs b/crates/exec-harness/src/uri.rs new file mode 100644 index 00000000..1e387a02 --- /dev/null +++ b/crates/exec-harness/src/uri.rs @@ -0,0 +1,26 @@ +use crate::prelude::*; + +pub(crate) struct NameAndUri { + pub(crate) name: String, + pub(crate) uri: String, +} + +/// Maximum length for benchmark name to avoid excessively long URIs +/// Should be removed once we have structured metadata around benchmarks +const MAX_NAME_LENGTH: usize = 1024 - 100; + +pub(crate) fn generate_name_and_uri(name: &Option, command: &[String]) -> NameAndUri { + let mut name = name.clone().unwrap_or_else(|| command.join(" ")); + let uri = format!("exec_harness::{name}"); + + if name.len() > MAX_NAME_LENGTH { + warn!( + "Benchmark name is too long ({} characters). Truncating to {} characters.", + name.len(), + MAX_NAME_LENGTH + ); + name.truncate(MAX_NAME_LENGTH); + } + + NameAndUri { name, uri } +} diff --git a/crates/exec-harness/tests/integration_test.rs b/crates/exec-harness/tests/integration_test.rs index 0d2e541a..b9f8839b 100644 --- a/crates/exec-harness/tests/integration_test.rs +++ b/crates/exec-harness/tests/integration_test.rs @@ -321,3 +321,126 @@ fn test_single_long_execution() -> Result<()> { Ok(()) } + +/// Test that a command with shell operators (&&) works correctly when passed as a single argument +#[test] +fn test_command_with_shell_operators() -> Result<()> { + let exec_opts = exec_harness::walltime::ExecutionOptions::try_from( + exec_harness::walltime::WalltimeExecutionArgs { + warmup_time: Some("0s".to_string()), + max_time: None, + min_time: None, + max_rounds: Some(1), + min_rounds: None, + }, + )?; + + let tmpdir = TempDir::new()?; + let marker_file = tmpdir.path().join("marker.txt"); + + // This simulates: bash -c "echo first && echo second > marker.txt" + // The entire "echo first && echo second > marker.txt" should be passed as one argument to -c + let cmd = format!("echo first && echo second > {}", marker_file.display()); + + let times = exec_harness::walltime::perform( + "test::shell_operators".to_string(), + vec!["bash".to_string(), "-c".to_string(), cmd], + &exec_opts, + )?; + + assert_eq!(times.len(), 1, "Expected exactly 1 iteration"); + + // Verify that the second command (after &&) was executed + assert!( + marker_file.exists(), + "Marker file should exist - the second part of && was not executed" + ); + + let content = std::fs::read_to_string(&marker_file)?; + assert_eq!( + content.trim(), + "second", + "Marker file should contain 'second'" + ); + + Ok(()) +} + +/// Test that a command with pipes works correctly +#[test] +fn test_command_with_pipes() -> Result<()> { + let exec_opts = exec_harness::walltime::ExecutionOptions::try_from( + exec_harness::walltime::WalltimeExecutionArgs { + warmup_time: Some("0s".to_string()), + max_time: None, + min_time: None, + max_rounds: Some(1), + min_rounds: None, + }, + )?; + + let tmpdir = TempDir::new()?; + let output_file = tmpdir.path().join("output.txt"); + + // This simulates: bash -c "echo 'hello world' | tr 'a-z' 'A-Z' > output.txt" + let cmd = format!( + "echo 'hello world' | tr 'a-z' 'A-Z' > {}", + output_file.display() + ); + + let times = exec_harness::walltime::perform( + "test::pipes".to_string(), + vec!["bash".to_string(), "-c".to_string(), cmd], + &exec_opts, + )?; + + assert_eq!(times.len(), 1, "Expected exactly 1 iteration"); + + // Verify that the pipe worked correctly + let content = std::fs::read_to_string(&output_file)?; + assert_eq!( + content.trim(), + "HELLO WORLD", + "Pipe should have transformed text to uppercase" + ); + + Ok(()) +} + +/// Test that a command with quotes in the argument works correctly +#[test] +fn test_command_with_embedded_quotes() -> Result<()> { + let exec_opts = exec_harness::walltime::ExecutionOptions::try_from( + exec_harness::walltime::WalltimeExecutionArgs { + warmup_time: Some("0s".to_string()), + max_time: None, + min_time: None, + max_rounds: Some(1), + min_rounds: None, + }, + )?; + + let tmpdir = TempDir::new()?; + let output_file = tmpdir.path().join("output.txt"); + + // This simulates: bash -c "echo 'hello world' > output.txt" + let cmd = format!("echo 'hello world' > {}", output_file.display()); + + let times = exec_harness::walltime::perform( + "test::embedded_quotes".to_string(), + vec!["bash".to_string(), "-c".to_string(), cmd], + &exec_opts, + )?; + + assert_eq!(times.len(), 1, "Expected exactly 1 iteration"); + + // Verify that the quoted string was preserved + let content = std::fs::read_to_string(&output_file)?; + assert_eq!( + content.trim(), + "hello world", + "Quoted string should be preserved" + ); + + Ok(()) +} diff --git a/src/exec/mod.rs b/src/exec/mod.rs index c3340bcf..36599193 100644 --- a/src/exec/mod.rs +++ b/src/exec/mod.rs @@ -14,9 +14,24 @@ mod poll_results; /// We temporarily force this name for all exec runs pub const DEFAULT_REPOSITORY_NAME: &str = "local-runs"; -pub const EXEC_HARNESS_COMMAND: &str = "exec-harness"; +const EXEC_HARNESS_COMMAND: &str = "exec-harness"; const EXEC_HARNESS_VERSION: &str = "1.0.0"; +/// Wraps a command with exec-harness and the given walltime arguments. +/// +/// This produces a shell command string like: +/// `exec-harness --warmup-time 1s --max-rounds 10 sleep 0.1` +pub fn wrap_with_exec_harness( + walltime_args: &exec_harness::walltime::WalltimeExecutionArgs, + command: &[String], +) -> String { + shell_words::join( + std::iter::once(EXEC_HARNESS_COMMAND) + .chain(walltime_args.to_cli_args().iter().map(|s| s.as_str())) + .chain(command.iter().map(|s| s.as_str())), + ) +} + #[derive(Args, Debug)] pub struct ExecArgs { #[command(flatten)] diff --git a/src/executor/config.rs b/src/executor/config.rs index 1eee92a0..362a6c1f 100644 --- a/src/executor/config.rs +++ b/src/executor/config.rs @@ -1,4 +1,4 @@ -use crate::exec::EXEC_HARNESS_COMMAND; +use crate::exec::wrap_with_exec_harness; use crate::instruments::Instruments; use crate::prelude::*; use crate::run::{RunArgs, UnwindingMode}; @@ -135,12 +135,7 @@ impl TryFrom for Config { let upload_url = Url::parse(&raw_upload_url) .map_err(|e| anyhow!("Invalid upload URL: {raw_upload_url}, {e}"))?; - let wrapped_command = std::iter::once(EXEC_HARNESS_COMMAND.to_owned()) - // Forward exec-harness arguments - .chain(args.walltime_args.to_cli_args()) - .chain(args.command) - .collect::>() - .join(" "); + let wrapped_command = wrap_with_exec_harness(&args.walltime_args, &args.command); Ok(Self { upload_url, diff --git a/src/executor/tests.rs b/src/executor/tests.rs index 70f46437..8cf30fe1 100644 --- a/src/executor/tests.rs +++ b/src/executor/tests.rs @@ -109,6 +109,16 @@ const ENV_TESTS: [(&str, &str); 8] = [ #[case(TESTS[5])] fn test_cases(#[case] cmd: &str) {} +// Exec-harness currently does not support the inline multi command scripts +#[template] +#[rstest::rstest] +#[case(TESTS[0])] +#[case(TESTS[1])] +#[case(TESTS[2])] +fn exec_harness_test_cases() -> Vec<&'static str> { + EXEC_HARNESS_COMMANDS.to_vec() +} + #[template] #[rstest::rstest] #[case(ENV_TESTS[0])] @@ -346,6 +356,37 @@ fi }) .await; } + + // Ensure that the walltime executor works with the exec-harness + #[apply(exec_harness_test_cases)] + #[rstest::rstest] + #[test_log::test(tokio::test)] + async fn test_exec_harness(#[case] cmd: &str) { + use crate::exec::wrap_with_exec_harness; + use exec_harness::walltime::WalltimeExecutionArgs; + + let (_permit, executor) = get_walltime_executor().await; + + let walltime_args = WalltimeExecutionArgs { + warmup_time: Some("0s".to_string()), + max_time: None, + min_time: None, + max_rounds: Some(3), + min_rounds: None, + }; + + let cmd = cmd.split(" ").map(|s| s.to_owned()).collect::>(); + let wrapped_command = wrap_with_exec_harness(&walltime_args, &cmd); + + // Unset GITHUB_ACTIONS to force LocalProvider which supports repository_override + temp_env::async_with_vars(&[("GITHUB_ACTIONS", None::<&str>)], async { + let config = walltime_config(&wrapped_command, true); + dbg!(&config); + let (execution_context, _temp_dir) = create_test_setup(config).await; + executor.run(&execution_context, &None).await.unwrap(); + }) + .await; + } } mod memory {