From 1eea8c6e103f7e5e9299ebe28519848114dcc22a Mon Sep 17 00:00:00 2001 From: Guillaume Lagrange Date: Wed, 18 Mar 2026 16:43:47 +0100 Subject: [PATCH 1/3] fix: do not run teardown if executor's run failed --- src/executor/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/executor/mod.rs b/src/executor/mod.rs index e7c70026..2dc6b572 100644 --- a/src/executor/mod.rs +++ b/src/executor/mod.rs @@ -114,7 +114,7 @@ pub async fn run_executor( None }; - let run_result = executor.run(execution_context, &mongo_tracer).await; + executor.run(execution_context, &mongo_tracer).await?; // TODO: refactor and move directly in the Instruments struct as a `stop` method if let Some(mut mongo_tracer) = mongo_tracer { @@ -123,9 +123,6 @@ pub async fn run_executor( debug!("Tearing down the executor"); executor.teardown(execution_context).await?; - // Propagate any run error after cleanup - run_result?; - orchestrator .logger .persist_log_to_profile_folder(&execution_context.profile_folder)?; From 7fd0abe3cb3031bd5048fd7cd23e57f9233a9f0b Mon Sep 17 00:00:00 2001 From: Guillaume Lagrange Date: Wed, 18 Mar 2026 17:31:24 +0100 Subject: [PATCH 2/3] refactor: extract config discovery into `DiscoveredProjectConfig` Move config file discovery logic from `ProjectConfig` into a dedicated `DiscoveredProjectConfig` struct that preserves the config file path. This removes the side-effect of `set_current_dir()` during discovery and instead exposes `config_dir()` for callers to resolve paths relative to the config file location. Also stop merging `working_directory` from config in the merger since it needs path resolution relative to the config file directory, which the merger doesn't have access to. --- src/cli/mod.rs | 14 ++-- src/cli/run/mod.rs | 6 +- src/executor/tests.rs | 1 - src/project_config/discover.rs | 102 +++++++++++++++++++++++++++++ src/project_config/merger.rs | 112 ++++++++++--------------------- src/project_config/mod.rs | 116 ++------------------------------- src/project_config/tests.rs | 28 ++++---- 7 files changed, 167 insertions(+), 212 deletions(-) create mode 100644 src/project_config/discover.rs diff --git a/src/cli/mod.rs b/src/cli/mod.rs index e2f9b60f..e76929ac 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -15,7 +15,7 @@ use crate::{ config::CodSpeedConfig, local_logger::{CODSPEED_U8_COLOR_CODE, init_local_logger}, prelude::*, - project_config::ProjectConfig, + project_config::DiscoveredProjectConfig, }; use clap::{ Parser, Subcommand, @@ -97,9 +97,11 @@ pub async fn run() -> Result<()> { CodSpeedConfig::load_with_override(cli.config_name.as_deref(), cli.oauth_token.as_deref())?; let api_client = CodSpeedAPIClient::try_from((&cli, &codspeed_config))?; - // Discover project configuration file (this may change the working directory) - let project_config = - ProjectConfig::discover_and_load(cli.config.as_deref(), &std::env::current_dir()?)?; + // Discover project configuration file + let discovered_config = DiscoveredProjectConfig::discover_and_load( + cli.config.as_deref(), + &std::env::current_dir()?, + )?; // In the context of the CI, it is likely that a ~ made its way here without being expanded by the shell let setup_cache_dir = cli @@ -121,7 +123,7 @@ pub async fn run() -> Result<()> { *args, &api_client, &codspeed_config, - project_config.as_ref(), + discovered_config.as_ref(), setup_cache_dir, ) .await? @@ -131,7 +133,7 @@ pub async fn run() -> Result<()> { *args, &api_client, &codspeed_config, - project_config.as_ref(), + discovered_config.as_ref().map(|d| &d.config), setup_cache_dir, ) .await? diff --git a/src/cli/run/mod.rs b/src/cli/run/mod.rs index 17c993f4..0c96d139 100644 --- a/src/cli/run/mod.rs +++ b/src/cli/run/mod.rs @@ -5,8 +5,8 @@ use crate::executor; use crate::executor::config::{self, OrchestratorConfig, RepositoryOverride}; use crate::instruments::Instruments; use crate::prelude::*; -use crate::project_config::ProjectConfig; use crate::project_config::merger::ConfigMerger; +use crate::project_config::{DiscoveredProjectConfig, ProjectConfig}; use crate::run_environment::interfaces::RepositoryProvider; use crate::upload::poll_results::PollResultsOptions; use clap::{Args, ValueEnum}; @@ -152,13 +152,13 @@ pub async fn run( args: RunArgs, api_client: &CodSpeedAPIClient, codspeed_config: &CodSpeedConfig, - project_config: Option<&ProjectConfig>, + discovered_config: Option<&DiscoveredProjectConfig>, setup_cache_dir: Option<&Path>, ) -> Result<()> { let output_json = args.message_format == Some(MessageFormat::Json); + let project_config = discovered_config.map(|d| &d.config); let args = args.merge_with_project_config(project_config); - let run_target = if args.command.is_empty() { // No command provided - check for targets in project config let targets = project_config diff --git a/src/executor/tests.rs b/src/executor/tests.rs index 49fa6ce0..db982741 100644 --- a/src/executor/tests.rs +++ b/src/executor/tests.rs @@ -370,7 +370,6 @@ fi // 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(); }) diff --git a/src/project_config/discover.rs b/src/project_config/discover.rs new file mode 100644 index 00000000..201f26c1 --- /dev/null +++ b/src/project_config/discover.rs @@ -0,0 +1,102 @@ +use crate::prelude::*; +use std::path::{Path, PathBuf}; + +use super::ProjectConfig; + +/// Config file names in priority order +const CONFIG_FILENAMES: &[&str] = &[ + "codspeed.yaml", + "codspeed.yml", + ".codspeed.yaml", + ".codspeed.yml", +]; + +/// A project configuration paired with the path it was loaded from. +#[derive(Debug)] +#[allow(dead_code)] +pub struct DiscoveredProjectConfig { + pub config: ProjectConfig, + pub path: PathBuf, +} + +impl DiscoveredProjectConfig { + /// Discover and load project configuration file + /// + /// # Search Strategy + /// 1. If `config_path_override` is provided, load from that path only (error if not found) + /// 2. Otherwise, search for config files in current directory and upward to git root + /// 3. Try filenames in priority order: codspeed.yaml, codspeed.yml, .codspeed.yaml, .codspeed.yml + pub fn discover_and_load( + config_path_override: Option<&Path>, + current_dir: &Path, + ) -> Result> { + // Case 1: Explicit --config path provided + if let Some(config_path) = config_path_override { + let config = ProjectConfig::load_from_path(config_path) + .with_context(|| format!("Failed to load config from {}", config_path.display()))?; + + return Ok(Some(DiscoveredProjectConfig { + config, + path: config_path.to_path_buf(), + })); + } + + // Case 2: Search for config files + let search_dirs = Self::get_search_directories(current_dir)?; + + for dir in search_dirs { + for filename in CONFIG_FILENAMES { + let candidate_path = dir.join(filename); + if candidate_path.exists() { + debug!("Found config file at {}", candidate_path.display()); + let config = ProjectConfig::load_from_path(&candidate_path)?; + + return Ok(Some(DiscoveredProjectConfig { + config, + path: candidate_path, + })); + } + } + } + + // No config found - this is OK + Ok(None) + } + + /// Returns the directory containing the config file. + #[allow(dead_code)] + pub fn config_dir(&self) -> Option { + let canonical_path = self + .path + .canonicalize() + .unwrap_or_else(|_| self.path.clone()); + canonical_path.parent().map(|p| p.to_path_buf()) + } + + /// Get list of directories to search for config files + /// + /// Returns directories from current_dir upward to git root (if in a git repo) + fn get_search_directories(current_dir: &Path) -> Result> { + let mut dirs = vec![current_dir.to_path_buf()]; + + // Try to find git repository root + if let Some(git_root) = crate::cli::run::helpers::find_repository_root(current_dir) { + // Add parent directories up to git root + let mut dir = current_dir.to_path_buf(); + while let Some(parent) = dir.parent() { + if parent == git_root { + if !dirs.contains(&git_root) { + dirs.push(git_root.clone()); + } + break; + } + if !dirs.contains(&parent.to_path_buf()) { + dirs.push(parent.to_path_buf()); + } + dir = parent.to_path_buf(); + } + } + + Ok(dirs) + } +} diff --git a/src/project_config/merger.rs b/src/project_config/merger.rs index 760e18e1..3d9e7590 100644 --- a/src/project_config/merger.rs +++ b/src/project_config/merger.rs @@ -41,18 +41,12 @@ impl ConfigMerger { /// Note: Some fields like upload_url, token, repository are CLI-only and not in config. pub fn merge_shared_args( cli: &ExecAndRunSharedArgs, - config_opts: Option<&ProjectOptions>, + _config_opts: Option<&ProjectOptions>, ) -> ExecAndRunSharedArgs { - let mut merged = cli.clone(); - - // Merge working_directory if not set via CLI - if merged.working_directory.is_none() { - if let Some(opts) = config_opts { - merged.working_directory = opts.working_directory.clone(); - } - } - - merged + // Note: working_directory is NOT merged here because config paths need to be + // resolved relative to the config file directory. This resolution is handled + // by the caller (e.g., `codspeed run`) which has access to the config file path. + cli.clone() } /// Helper to merge Option values with precedence: CLI > config > None @@ -67,6 +61,29 @@ mod tests { use crate::cli::PerfRunArgs; use crate::runner_mode::RunnerMode; + fn make_cli(working_directory: Option<&str>) -> ExecAndRunSharedArgs { + ExecAndRunSharedArgs { + upload_url: None, + token: None, + repository: None, + provider: None, + working_directory: working_directory.map(|s| s.to_string()), + mode: vec![RunnerMode::Walltime], + simulation_tool: None, + profile_folder: None, + skip_upload: false, + skip_run: false, + skip_setup: false, + allow_empty: false, + go_runner_version: None, + show_full_output: false, + perf_run_args: PerfRunArgs { + enable_perf: true, + perf_unwinding_mode: None, + }, + } + } + #[test] fn test_merge_walltime_all_from_cli() { let cli = WalltimeExecutionArgs { @@ -146,27 +163,7 @@ mod tests { #[test] fn test_merge_shared_args_working_directory_from_cli() { - let cli = ExecAndRunSharedArgs { - upload_url: None, - token: None, - repository: None, - provider: None, - working_directory: Some("./cli-dir".to_string()), - mode: vec![RunnerMode::Walltime], - simulation_tool: None, - profile_folder: None, - skip_upload: false, - skip_run: false, - skip_setup: false, - allow_empty: false, - go_runner_version: None, - show_full_output: false, - perf_run_args: PerfRunArgs { - enable_perf: true, - perf_unwinding_mode: None, - }, - }; - + let cli = make_cli(Some("./cli-dir")); let config = ProjectOptions { walltime: None, working_directory: Some("./config-dir".to_string()), @@ -179,28 +176,8 @@ mod tests { } #[test] - fn test_merge_shared_args_working_directory_from_config() { - let cli = ExecAndRunSharedArgs { - upload_url: None, - token: None, - repository: None, - provider: None, - working_directory: None, - mode: vec![RunnerMode::Walltime], - simulation_tool: None, - profile_folder: None, - skip_upload: false, - skip_run: false, - skip_setup: false, - allow_empty: false, - go_runner_version: None, - show_full_output: false, - perf_run_args: PerfRunArgs { - enable_perf: true, - perf_unwinding_mode: None, - }, - }; - + fn test_merge_shared_args_working_directory_not_merged_from_config() { + let cli = make_cli(None); let config = ProjectOptions { walltime: None, working_directory: Some("./config-dir".to_string()), @@ -208,40 +185,21 @@ mod tests { let merged = ConfigMerger::merge_shared_args(&cli, Some(&config)); - // Config working_directory should be used - assert_eq!(merged.working_directory, Some("./config-dir".to_string())); + // Config working_directory is NOT merged — resolution is handled by the caller + // relative to the config file directory. + assert_eq!(merged.working_directory, None); // Mode stays as CLI value assert_eq!(merged.mode, vec![RunnerMode::Walltime]); } #[test] fn test_merge_shared_args_no_config() { - let cli = ExecAndRunSharedArgs { - upload_url: None, - token: None, - repository: None, - provider: None, - working_directory: Some("./dir".to_string()), - mode: vec![RunnerMode::Simulation], - simulation_tool: None, - profile_folder: None, - skip_upload: false, - skip_run: false, - skip_setup: false, - allow_empty: false, - go_runner_version: None, - show_full_output: false, - perf_run_args: PerfRunArgs { - enable_perf: false, - perf_unwinding_mode: None, - }, - }; + let cli = make_cli(Some("./dir")); let merged = ConfigMerger::merge_shared_args(&cli, None); // Should be identical to CLI assert_eq!(merged.working_directory, Some("./dir".to_string())); - assert_eq!(merged.mode, vec![RunnerMode::Simulation]); } #[test] diff --git a/src/project_config/mod.rs b/src/project_config/mod.rs index 4bb426d5..727e1965 100644 --- a/src/project_config/mod.rs +++ b/src/project_config/mod.rs @@ -1,125 +1,17 @@ use crate::prelude::*; use std::fs; -use std::path::{Path, PathBuf}; +use std::path::Path; +mod discover; mod interfaces; pub mod merger; +pub use discover::*; pub use interfaces::*; -/// Config file names in priority order -const CONFIG_FILENAMES: &[&str] = &[ - "codspeed.yaml", - "codspeed.yml", - ".codspeed.yaml", - ".codspeed.yml", -]; - impl ProjectConfig { - /// Discover and load project configuration file - /// - /// # Search Strategy - /// 1. If `config_path_override` is provided, load from that path only (error if not found) - /// 2. Otherwise, search for config files in current directory and upward to git root - /// 3. Try filenames in priority order: codspeed.yaml, codspeed.yml, .codspeed.yaml, .codspeed.yml - /// 4. If a config is found in a parent directory, changes the working directory to that location - /// - /// # Arguments - /// * `config_path_override` - Explicit path to config file (from --config flag) - /// * `current_dir` - Directory to start searching from - /// - /// # Returns - /// * `Ok(Some(config))` - Config found and loaded successfully - /// * `Ok(None)` - No config file found - /// * `Err(_)` - Error loading or parsing config - pub fn discover_and_load( - config_path_override: Option<&Path>, - current_dir: &Path, - ) -> Result> { - // Case 1: Explicit --config path provided - if let Some(config_path) = config_path_override { - let config = Self::load_from_path(config_path) - .with_context(|| format!("Failed to load config from {}", config_path.display()))?; - let canonical_path = config_path - .canonicalize() - .unwrap_or_else(|_| config_path.to_path_buf()); - - // Change working directory if config was found in a different directory - Self::change_to_config_directory(&canonical_path, current_dir)?; - - return Ok(Some(config)); - } - - // Case 2: Search for config files - let search_dirs = Self::get_search_directories(current_dir)?; - - for dir in search_dirs { - for filename in CONFIG_FILENAMES { - let candidate_path = dir.join(filename); - if candidate_path.exists() { - debug!("Found config file at {}", candidate_path.display()); - let config = Self::load_from_path(&candidate_path)?; - let canonical_path = candidate_path.canonicalize().unwrap_or(candidate_path); - - // Change working directory if config was found in a different directory - Self::change_to_config_directory(&canonical_path, current_dir)?; - - return Ok(Some(config)); - } - } - } - - // No config found - this is OK - Ok(None) - } - - /// Get list of directories to search for config files - /// - /// Returns directories from current_dir upward to git root (if in a git repo) - fn get_search_directories(current_dir: &Path) -> Result> { - let mut dirs = vec![current_dir.to_path_buf()]; - - // Try to find git repository root - if let Some(git_root) = crate::cli::run::helpers::find_repository_root(current_dir) { - // Add parent directories up to git root - let mut dir = current_dir.to_path_buf(); - while let Some(parent) = dir.parent() { - if parent == git_root { - if !dirs.contains(&git_root) { - dirs.push(git_root.clone()); - } - break; - } - if !dirs.contains(&parent.to_path_buf()) { - dirs.push(parent.to_path_buf()); - } - dir = parent.to_path_buf(); - } - } - - Ok(dirs) - } - - /// Change working directory to the directory containing the config file - fn change_to_config_directory(config_path: &Path, original_dir: &Path) -> Result<()> { - let config_dir = config_path - .parent() - .context("Config file has no parent directory")?; - - if config_dir != original_dir { - std::env::set_current_dir(config_dir)?; - debug!( - "Changed working directory from {} to {}", - original_dir.display(), - config_dir.display() - ); - } - - Ok(()) - } - /// Load and parse config from a specific path - fn load_from_path(path: &Path) -> Result { + pub(crate) fn load_from_path(path: &Path) -> Result { let config_content = fs::read(path) .with_context(|| format!("Failed to read config file at {}", path.display()))?; diff --git a/src/project_config/tests.rs b/src/project_config/tests.rs index 095254fc..321b8b13 100644 --- a/src/project_config/tests.rs +++ b/src/project_config/tests.rs @@ -162,11 +162,12 @@ options: ) .unwrap(); - let config = ProjectConfig::discover_and_load(Some(&config_path), temp_dir.path()).unwrap(); + let discovered = + DiscoveredProjectConfig::discover_and_load(Some(&config_path), temp_dir.path()).unwrap(); - assert!(config.is_some()); - let config = config.unwrap(); - assert!(config.options.is_some()); + assert!(discovered.is_some()); + let discovered = discovered.unwrap(); + assert!(discovered.config.options.is_some()); } #[test] @@ -174,7 +175,7 @@ fn test_discover_with_explicit_path_not_found() { let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("missing.yaml"); - let result = ProjectConfig::discover_and_load(Some(&config_path), temp_dir.path()); + let result = DiscoveredProjectConfig::discover_and_load(Some(&config_path), temp_dir.path()); assert!(result.is_err()); } @@ -192,9 +193,9 @@ options: ) .unwrap(); - let config = ProjectConfig::discover_and_load(None, temp_dir.path()).unwrap(); + let discovered = DiscoveredProjectConfig::discover_and_load(None, temp_dir.path()).unwrap(); - assert!(config.is_some()); + assert!(discovered.is_some()); } #[test] @@ -220,18 +221,19 @@ options: ) .unwrap(); - let config = ProjectConfig::discover_and_load(None, temp_dir.path()).unwrap(); + let discovered = DiscoveredProjectConfig::discover_and_load(None, temp_dir.path()).unwrap(); - assert!(config.is_some()); - // Note: We can no longer verify which file was loaded since we don't return the path - // The priority is still enforced but not testable without checking the filesystem + assert!(discovered.is_some()); + let discovered = discovered.unwrap(); + // Verify the .yaml file was picked over .yml (priority order) + assert!(discovered.path.ends_with("codspeed.yaml")); } #[test] fn test_discover_no_config_found() { let temp_dir = TempDir::new().unwrap(); - let config = ProjectConfig::discover_and_load(None, temp_dir.path()).unwrap(); - assert!(config.is_none()); + let discovered = DiscoveredProjectConfig::discover_and_load(None, temp_dir.path()).unwrap(); + assert!(discovered.is_none()); } #[test] From f630ed5a37cf7b0b83cbb3e2d65a182557f2ea21 Mon Sep 17 00:00:00 2001 From: Guillaume Lagrange Date: Wed, 18 Mar 2026 17:33:09 +0100 Subject: [PATCH 3/3] fix: resolve working_directory relative to config file, not CWD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When running `codspeed run` with config targets, the `working-directory` option is now resolved relative to the config file's directory instead of the current working directory. When no `working-directory` is set, it defaults to the config file's directory. For `codspeed run -- command` and `codspeed exec`, the config's `working-directory` is explicitly ignored — only the `--working-directory` CLI flag is used. A warning is emitted if `--working-directory` is passed in config targets mode. --- Cargo.lock | 83 ++++++++ Cargo.toml | 2 + src/cli/exec/mod.rs | 4 - src/cli/run/mod.rs | 56 ++++-- src/project_config/discover.rs | 2 - src/project_config/merger.rs | 83 +------- tests/working_directory.rs | 342 +++++++++++++++++++++++++++++++++ 7 files changed, 467 insertions(+), 105 deletions(-) create mode 100644 tests/working_directory.rs diff --git a/Cargo.lock b/Cargo.lock index 9dc18974..1af4db41 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -143,6 +143,21 @@ version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7c02d123df017efcdfbd739ef81735b36c5ba83ec3c59c80a9d7ecc718f92e50" +[[package]] +name = "assert_cmd" +version = "2.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a686bbee5efb88a82df0621b236e74d925f470e5445d3220a5648b892ec99c9" +dependencies = [ + "anstyle", + "bstr", + "libc", + "predicates", + "predicates-core", + "predicates-tree", + "wait-timeout", +] + [[package]] name = "astral-tokio-tar" version = "0.5.6" @@ -557,6 +572,7 @@ version = "4.11.1" dependencies = [ "addr2line", "anyhow", + "assert_cmd", "astral-tokio-tar", "async-compression", "async-trait", @@ -584,6 +600,7 @@ dependencies = [ "nix 0.31.1", "object 0.36.7", "open", + "predicates", "procfs", "rand", "rayon", @@ -775,6 +792,12 @@ dependencies = [ "powerfmt", ] +[[package]] +name = "difflib" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8" + [[package]] name = "digest" version = "0.10.7" @@ -952,6 +975,15 @@ dependencies = [ "miniz_oxide", ] +[[package]] +name = "float-cmp" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b09cf3155332e944990140d967ff5eceb70df778b34f77d8075db46e4704e6d8" +dependencies = [ + "num-traits", +] + [[package]] name = "fnv" version = "1.0.7" @@ -2132,6 +2164,12 @@ dependencies = [ "minimal-lexical", ] +[[package]] +name = "normalize-line-endings" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" + [[package]] name = "ntapi" version = "0.4.1" @@ -2495,6 +2533,36 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "predicates" +version = "3.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ada8f2932f28a27ee7b70dd6c1c39ea0675c55a36879ab92f3a715eaa1e63cfe" +dependencies = [ + "anstyle", + "difflib", + "float-cmp", + "normalize-line-endings", + "predicates-core", + "regex", +] + +[[package]] +name = "predicates-core" +version = "1.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cad38746f3166b4031b1a0d39ad9f954dd291e7854fcc0eed52ee41a0b50d144" + +[[package]] +name = "predicates-tree" +version = "1.0.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0de1b847b39c8131db0467e9df1ff60e6d0562ab8e9a16e568ad0fdb372e2f2" +dependencies = [ + "predicates-core", + "termtree", +] + [[package]] name = "prettyplease" version = "0.2.37" @@ -3740,6 +3808,12 @@ dependencies = [ "windows-sys 0.60.2", ] +[[package]] +name = "termtree" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f50febec83f5ee1df3015341d8bd429f2d1cc62bcba7ea2076759d315084683" + [[package]] name = "test-log" version = "0.2.19" @@ -4287,6 +4361,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "wait-timeout" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ac3b126d3914f9849036f826e054cbabdc8519970b8998ddaf3b5bd3c65f11" +dependencies = [ + "libc", +] + [[package]] name = "want" version = "0.3.1" diff --git a/Cargo.toml b/Cargo.toml index ab1c8758..0fd492a7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -84,6 +84,8 @@ test-with = { version = "0.15", default-features = false, features = [] } rstest = { version = "0.25.0", default-features = false } rstest_reuse = "0.7.0" shell-quote = "0.7.2" +assert_cmd = "2.0.16" +predicates = "3.1.4" [workspace] members = [ diff --git a/src/cli/exec/mod.rs b/src/cli/exec/mod.rs index 9a0a79aa..3d6a77ed 100644 --- a/src/cli/exec/mod.rs +++ b/src/cli/exec/mod.rs @@ -40,10 +40,6 @@ impl ExecArgs { /// CLI arguments take precedence over config values. pub fn merge_with_project_config(mut self, project_config: Option<&ProjectConfig>) -> Self { if let Some(project_config) = project_config { - // Merge shared args - self.shared = - ConfigMerger::merge_shared_args(&self.shared, project_config.options.as_ref()); - // Merge walltime args self.walltime_args = ConfigMerger::merge_walltime_options( &self.walltime_args, project_config diff --git a/src/cli/run/mod.rs b/src/cli/run/mod.rs index 0c96d139..045c53f0 100644 --- a/src/cli/run/mod.rs +++ b/src/cli/run/mod.rs @@ -5,8 +5,7 @@ use crate::executor; use crate::executor::config::{self, OrchestratorConfig, RepositoryOverride}; use crate::instruments::Instruments; use crate::prelude::*; -use crate::project_config::merger::ConfigMerger; -use crate::project_config::{DiscoveredProjectConfig, ProjectConfig}; +use crate::project_config::DiscoveredProjectConfig; use crate::run_environment::interfaces::RepositoryProvider; use crate::upload::poll_results::PollResultsOptions; use clap::{Args, ValueEnum}; @@ -40,19 +39,6 @@ pub struct RunArgs { pub command: Vec, } -impl RunArgs { - /// Merge CLI args with project config if available - /// - /// CLI arguments take precedence over config values. - pub fn merge_with_project_config(mut self, project_config: Option<&ProjectConfig>) -> Self { - if let Some(project_config) = project_config { - self.shared = - ConfigMerger::merge_shared_args(&self.shared, project_config.options.as_ref()); - } - self - } -} - #[derive(ValueEnum, Clone, Debug, PartialEq)] pub enum MessageFormat { Json, @@ -158,7 +144,6 @@ pub async fn run( let output_json = args.message_format == Some(MessageFormat::Json); let project_config = discovered_config.map(|d| &d.config); - let args = args.merge_with_project_config(project_config); let run_target = if args.command.is_empty() { // No command provided - check for targets in project config let targets = project_config @@ -183,6 +168,8 @@ pub async fn run( match run_target { RunTarget::SingleCommand(args) => { + // SingleCommand: working_directory comes from --working-directory CLI flag only. + // Config file's working-directory is NOT used. let command = args.command.join(" "); let config = build_orchestrator_config( args, @@ -192,6 +179,7 @@ pub async fn run( }], PollResultsOptions::for_run(output_json), )?; + let orchestrator = executor::Orchestrator::new(config, codspeed_config, api_client).await?; @@ -208,10 +196,44 @@ pub async fn run( targets, default_walltime, } => { + // ConfigTargets: working_directory is resolved relative to config file dir. + // If --working-directory CLI flag is passed, ignore it with a warning. + if args.shared.working_directory.is_some() { + // Intentionally using eprintln! because logger has not been initialized yet. + eprintln!( + "Warning: The --working-directory flag is ignored when running targets from the config file. \ + Use the `working-directory` option in the config file instead." + ); + } + + // Resolve working_directory relative to config file directory + let resolved_working_directory = + if let Some(config_dir) = discovered_config.and_then(|d| d.config_dir()) { + let root_wd = project_config + .and_then(|c| c.options.as_ref()) + .and_then(|o| o.working_directory.as_ref()); + + match root_wd { + Some(wd) => { + let wd_path = Path::new(wd); + if wd_path.is_absolute() { + Some(wd.clone()) + } else { + Some(config_dir.join(wd).to_string_lossy().into_owned()) + } + } + None => Some(config_dir.to_string_lossy().into_owned()), + } + } else { + None + }; + let benchmark_targets = super::exec::multi_targets::build_benchmark_targets(targets, default_walltime)?; - let config = + let mut config = build_orchestrator_config(args, benchmark_targets, PollResultsOptions::for_exec())?; + config.working_directory = resolved_working_directory; + super::exec::execute_config(config, api_client, codspeed_config, setup_cache_dir) .await?; } diff --git a/src/project_config/discover.rs b/src/project_config/discover.rs index 201f26c1..beae3718 100644 --- a/src/project_config/discover.rs +++ b/src/project_config/discover.rs @@ -13,7 +13,6 @@ const CONFIG_FILENAMES: &[&str] = &[ /// A project configuration paired with the path it was loaded from. #[derive(Debug)] -#[allow(dead_code)] pub struct DiscoveredProjectConfig { pub config: ProjectConfig, pub path: PathBuf, @@ -64,7 +63,6 @@ impl DiscoveredProjectConfig { } /// Returns the directory containing the config file. - #[allow(dead_code)] pub fn config_dir(&self) -> Option { let canonical_path = self .path diff --git a/src/project_config/merger.rs b/src/project_config/merger.rs index 3d9e7590..e5f78630 100644 --- a/src/project_config/merger.rs +++ b/src/project_config/merger.rs @@ -1,7 +1,6 @@ -use crate::cli::ExecAndRunSharedArgs; use exec_harness::walltime::WalltimeExecutionArgs; -use super::{ProjectOptions, WalltimeOptions}; +use super::WalltimeOptions; /// Handles merging of CLI arguments with project configuration /// @@ -35,20 +34,6 @@ impl ConfigMerger { } } - /// Merge shared args with project config options - /// - /// CLI arguments take precedence over config values. - /// Note: Some fields like upload_url, token, repository are CLI-only and not in config. - pub fn merge_shared_args( - cli: &ExecAndRunSharedArgs, - _config_opts: Option<&ProjectOptions>, - ) -> ExecAndRunSharedArgs { - // Note: working_directory is NOT merged here because config paths need to be - // resolved relative to the config file directory. This resolution is handled - // by the caller (e.g., `codspeed run`) which has access to the config file path. - cli.clone() - } - /// Helper to merge Option values with precedence: CLI > config > None fn merge_option(cli_value: &Option, config_value: Option<&T>) -> Option { cli_value.clone().or_else(|| config_value.cloned()) @@ -58,31 +43,6 @@ impl ConfigMerger { #[cfg(test)] mod tests { use super::*; - use crate::cli::PerfRunArgs; - use crate::runner_mode::RunnerMode; - - fn make_cli(working_directory: Option<&str>) -> ExecAndRunSharedArgs { - ExecAndRunSharedArgs { - upload_url: None, - token: None, - repository: None, - provider: None, - working_directory: working_directory.map(|s| s.to_string()), - mode: vec![RunnerMode::Walltime], - simulation_tool: None, - profile_folder: None, - skip_upload: false, - skip_run: false, - skip_setup: false, - allow_empty: false, - go_runner_version: None, - show_full_output: false, - perf_run_args: PerfRunArgs { - enable_perf: true, - perf_unwinding_mode: None, - }, - } - } #[test] fn test_merge_walltime_all_from_cli() { @@ -161,47 +121,6 @@ mod tests { assert_eq!(merged.min_rounds, None); } - #[test] - fn test_merge_shared_args_working_directory_from_cli() { - let cli = make_cli(Some("./cli-dir")); - let config = ProjectOptions { - walltime: None, - working_directory: Some("./config-dir".to_string()), - }; - - let merged = ConfigMerger::merge_shared_args(&cli, Some(&config)); - - // CLI working_directory should win - assert_eq!(merged.working_directory, Some("./cli-dir".to_string())); - } - - #[test] - fn test_merge_shared_args_working_directory_not_merged_from_config() { - let cli = make_cli(None); - let config = ProjectOptions { - walltime: None, - working_directory: Some("./config-dir".to_string()), - }; - - let merged = ConfigMerger::merge_shared_args(&cli, Some(&config)); - - // Config working_directory is NOT merged — resolution is handled by the caller - // relative to the config file directory. - assert_eq!(merged.working_directory, None); - // Mode stays as CLI value - assert_eq!(merged.mode, vec![RunnerMode::Walltime]); - } - - #[test] - fn test_merge_shared_args_no_config() { - let cli = make_cli(Some("./dir")); - - let merged = ConfigMerger::merge_shared_args(&cli, None); - - // Should be identical to CLI - assert_eq!(merged.working_directory, Some("./dir".to_string())); - } - #[test] fn test_merge_option_helper() { // CLI value wins diff --git a/tests/working_directory.rs b/tests/working_directory.rs new file mode 100644 index 00000000..e434934a --- /dev/null +++ b/tests/working_directory.rs @@ -0,0 +1,342 @@ +// Integration tests for working_directory resolution. +// +// These tests require sudo access (walltime mode uses systemd-run), +// so they are gated behind the GITHUB_ACTIONS env var. + +#[test_with::env(GITHUB_ACTIONS)] +mod tests { + use assert_cmd::Command; + use predicates::str::contains; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::path::Path; + use std::sync::Mutex; + + /// Tests use systemd-run/perf which cannot run concurrently. + static SERIAL: Mutex<()> = Mutex::new(()); + + fn codspeed_cmd(cwd: &Path) -> Command { + let mut cmd = Command::cargo_bin("codspeed").unwrap(); + cmd.current_dir(cwd); + cmd.env("CODSPEED_TOKEN", "test-token"); + cmd + } + + /// Create a bash script that validates CWD matches the expected absolute path. + fn write_cwd_check_script(path: &Path, expected_dir: &Path) { + let expected = expected_dir.to_string_lossy(); + let content = format!( + r#"#!/bin/bash +ACTUAL=$(pwd) +EXPECTED="{expected}" +if [ "$ACTUAL" != "$EXPECTED" ]; then + echo "FAIL: expected $EXPECTED, got $ACTUAL" >&2 + exit 1 +fi +"# + ); + fs::write(path, content).unwrap(); + fs::set_permissions(path, fs::Permissions::from_mode(0o755)).unwrap(); + } + + /// Write a codspeed.yml config file. + fn write_config(path: &Path, content: &str) { + fs::write(path, content).unwrap(); + } + + // ------------------------------------------------------------------- + // Config targets tests (exec targets via exec-harness) + // ------------------------------------------------------------------- + + /// Config targets — root working_directory resolves relative to config dir. + #[test] + fn config_targets_root_working_directory_resolves_relative_to_config_dir() { + let _lock = SERIAL.lock().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let subdir = root.join("subdir"); + fs::create_dir_all(&subdir).unwrap(); + let run_from = root.join("run-from-here"); + fs::create_dir_all(&run_from).unwrap(); + + let script = subdir.join("check_cwd.sh"); + write_cwd_check_script(&script, &subdir); + + let config_path = root.join("codspeed.yaml"); + write_config( + &config_path, + &format!( + r#" +options: + working-directory: subdir +benchmarks: + - exec: {script} + options: + max-rounds: 1 + warmup-time: "0s" +"#, + script = script.display(), + ), + ); + + codspeed_cmd(&run_from) + .args([ + "--config", + &config_path.to_string_lossy(), + "run", + "-m", + "walltime", + "--skip-upload", + "--show-full-output", + ]) + .assert() + .success(); + } + + /// Config targets — no working_directory defaults to config dir. + #[test] + fn config_targets_no_working_directory_defaults_to_config_dir() { + let _lock = SERIAL.lock().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let run_from = root.join("run-from-here"); + fs::create_dir_all(&run_from).unwrap(); + + let script = root.join("check_cwd.sh"); + write_cwd_check_script(&script, root); + + let config_path = root.join("codspeed.yaml"); + write_config( + &config_path, + &format!( + r#" +benchmarks: + - exec: {script} + options: + max-rounds: 1 + warmup-time: "0s" +"#, + script = script.display(), + ), + ); + + codspeed_cmd(&run_from) + .args([ + "--config", + &config_path.to_string_lossy(), + "run", + "-m", + "walltime", + "--skip-upload", + "--show-full-output", + ]) + .assert() + .success(); + } + + /// Config targets — `--working-directory` CLI flag is ignored with warning. + /// The warning is emitted via eprintln before the logger is initialized, + /// so we check stderr for it. + #[test] + fn config_targets_cli_working_directory_flag_is_ignored_with_warning() { + let _lock = SERIAL.lock().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let wrong_dir = root.join("wrong-dir"); + fs::create_dir_all(&wrong_dir).unwrap(); + + let script = root.join("check_cwd.sh"); + write_cwd_check_script(&script, root); + + let config_path = root.join("codspeed.yaml"); + write_config( + &config_path, + &format!( + r#" +benchmarks: + - exec: {script} + options: + max-rounds: 1 + warmup-time: "0s" +"#, + script = script.display(), + ), + ); + + codspeed_cmd(root) + .args([ + "--config", + &config_path.to_string_lossy(), + "run", + "-m", + "walltime", + "--skip-upload", + "--show-full-output", + "--working-directory", + &wrong_dir.to_string_lossy(), + ]) + .assert() + .success() + .stderr(contains("--working-directory flag is ignored")); + } + + // ------------------------------------------------------------------- + // SingleCommand (codspeed run -- command) tests + // + // SingleCommand uses entrypoint mode — plain scripts don't produce + // walltime results, so we use --allow-empty and check CWD indirectly + // via script exit code. + // ------------------------------------------------------------------- + + /// SingleCommand — `--working-directory` CLI flag is used. + #[test] + fn single_command_cli_working_directory_is_used() { + let _lock = SERIAL.lock().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let target_dir = root.join("target-dir"); + fs::create_dir_all(&target_dir).unwrap(); + + let script = root.join("check_cwd.sh"); + write_cwd_check_script(&script, &target_dir); + + codspeed_cmd(root) + .args([ + "run", + "-m", + "walltime", + "--skip-upload", + "--show-full-output", + "--allow-empty", + "--working-directory", + &target_dir.to_string_lossy(), + "--", + &script.to_string_lossy(), + ]) + .assert() + .success(); + } + + /// SingleCommand — config file's working_directory is NOT used. + #[test] + fn single_command_config_working_directory_is_not_used() { + let _lock = SERIAL.lock().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let wrong_dir = root.join("wrong-dir"); + fs::create_dir_all(&wrong_dir).unwrap(); + + let config_path = root.join("codspeed.yaml"); + write_config( + &config_path, + r#" +options: + working-directory: wrong-dir +"#, + ); + + // The script checks that CWD is root (where we launch from), not wrong-dir + let script = root.join("check_cwd.sh"); + write_cwd_check_script(&script, root); + + codspeed_cmd(root) + .args([ + "--config", + &config_path.to_string_lossy(), + "run", + "-m", + "walltime", + "--skip-upload", + "--show-full-output", + "--allow-empty", + "--", + &script.to_string_lossy(), + ]) + .assert() + .success(); + } + + // ------------------------------------------------------------------- + // Exec command tests (uses exec-harness) + // ------------------------------------------------------------------- + + /// Exec command — `--working-directory` CLI flag is used. + #[test] + fn exec_command_cli_working_directory_is_used() { + let _lock = SERIAL.lock().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let target_dir = root.join("target-dir"); + fs::create_dir_all(&target_dir).unwrap(); + + let script = root.join("check_cwd.sh"); + write_cwd_check_script(&script, &target_dir); + + codspeed_cmd(root) + .args([ + "exec", + "-m", + "walltime", + "--skip-upload", + "--show-full-output", + "--working-directory", + &target_dir.to_string_lossy(), + "--warmup-time", + "0s", + "--max-rounds", + "1", + "--", + &script.to_string_lossy(), + ]) + .assert() + .success(); + } + + /// Exec command — config file's working_directory is NOT used. + #[test] + fn exec_command_config_working_directory_is_not_used() { + let _lock = SERIAL.lock().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + + let wrong_dir = root.join("wrong-dir"); + fs::create_dir_all(&wrong_dir).unwrap(); + + let config_path = root.join("codspeed.yaml"); + write_config( + &config_path, + r#" +options: + working-directory: wrong-dir +"#, + ); + + let script = root.join("check_cwd.sh"); + write_cwd_check_script(&script, root); + + codspeed_cmd(root) + .args([ + "--config", + &config_path.to_string_lossy(), + "exec", + "-m", + "walltime", + "--skip-upload", + "--show-full-output", + "--warmup-time", + "0s", + "--max-rounds", + "1", + "--", + &script.to_string_lossy(), + ]) + .assert() + .success(); + } +}