From 5e0489742cb11ca184e906d5baa0056ed8c6b276 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Thu, 13 Nov 2025 12:44:48 +0100 Subject: [PATCH 1/4] fix: create perf.pipedata in temporary file to avoid issues with golang --- src/run/runner/tests.rs | 45 ++++++++++++++++++++++++++++ src/run/runner/wall_time/executor.rs | 8 +++-- src/run/runner/wall_time/perf/mod.rs | 19 ++++++++++-- 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/run/runner/tests.rs b/src/run/runner/tests.rs index 5dd895bf..c1de1a3d 100644 --- a/src/run/runner/tests.rs +++ b/src/run/runner/tests.rs @@ -186,6 +186,8 @@ mod valgrind { } mod walltime { + use std::io::Read; + use super::*; async fn get_walltime_executor() -> (SemaphorePermit<'static>, WallTimeExecutor) { @@ -297,4 +299,47 @@ fi let result = executor.run(&config, &system_info, &run_data, &None).await; assert!(result.is_err(), "Command should fail"); } + + #[tokio::test] + async fn test_walltime_executor_works_with_go() { + let system_info = SystemInfo::new().unwrap(); + let profile_dir = TempDir::new().unwrap().into_path(); + let run_data = RunData { + profile_folder: profile_dir.clone(), + }; + + let (_permit, executor) = get_walltime_executor().await; + + // NOTE: Even though `go test` doesn't work because we don't have benchmarks it should still + // create a few perf events that are written to perf.pipedata. + //``` + // [DEBUG go.sh] Called with arguments: test -bench=. + // [DEBUG go.sh] Number of arguments: 2 + // [DEBUG go.sh] Detected 'test' command, routing to go-runner + // [DEBUG go.sh] Using go-runner at: /home/not-matthias/.cargo/bin/codspeed-go-runner + // [DEBUG go.sh] Full command: RUST_LOG=info /home/not-matthias/.cargo/bin/codspeed-go-runner test -bench=. + // Error: Failed to execute 'go list': [DEBUG go.sh] Called with arguments: list -test -compiled -json ./... + // [DEBUG go.sh] Number of arguments: 5 + // [DEBUG go.sh] Detected non-test command ('list'), routing to standard go binary + // [DEBUG go.sh] Full command: /nix/store/k1kn1c59ss7nq6agdppzq3krwmi3xqy4-go-1.25.2/bin/go list -test -compiled -json ./... + // pattern ./...: directory prefix . does not contain main module or its selected dependencies + // + // [ perf record: Woken up 4 times to write data ] + // [ perf record: Captured and wrote 0.200 MB - ] + // ``` + let config = walltime_config("go test -bench=.", true); + + let _result = executor.run(&config, &system_info, &run_data, &None).await; + + let perf_runner = executor.perf_runner(); + let perf_data_path = perf_runner.perf_file().path(); + assert!(perf_data_path.exists(), "perf.pipedata should exist"); + + // Assert it starts with PERFPIPEPERFILE2 + let mut file = std::fs::File::open(perf_data_path).unwrap(); + let expected = b"PERFILE2"; + let mut actual = [0u8; 8]; + file.read_exact(&mut actual).unwrap(); + assert_eq!(actual, *expected); + } } diff --git a/src/run/runner/wall_time/executor.rs b/src/run/runner/wall_time/executor.rs index f26334d0..20bb9c31 100644 --- a/src/run/runner/wall_time/executor.rs +++ b/src/run/runner/wall_time/executor.rs @@ -98,6 +98,11 @@ impl WallTimeExecutor { } } + #[cfg(test)] + pub fn perf_runner(&self) -> &PerfRunner { + self.perf.as_ref().expect("PerfRunner is not available") + } + fn walltime_bench_cmd( config: &Config, run_data: &RunData, @@ -191,8 +196,7 @@ impl Executor for WallTimeExecutor { if let Some(perf) = &self.perf && config.enable_perf { - perf.run(cmd_builder, config, &run_data.profile_folder) - .await + perf.run(cmd_builder, config).await } else { let cmd = wrap_with_sudo(cmd_builder)?.build(); debug!("cmd: {cmd:?}"); diff --git a/src/run/runner/wall_time/perf/mod.rs b/src/run/runner/wall_time/perf/mod.rs index 7e4c8220..e4cb1dcb 100644 --- a/src/run/runner/wall_time/perf/mod.rs +++ b/src/run/runner/wall_time/perf/mod.rs @@ -29,6 +29,7 @@ use std::collections::HashSet; use std::path::Path; use std::time::Duration; use std::{cell::OnceCell, collections::HashMap, process::ExitStatus}; +use tempfile::NamedTempFile; mod jit_dump; mod setup; @@ -45,9 +46,15 @@ const PERF_DATA_FILE_NAME: &str = "perf.pipedata"; pub struct PerfRunner { benchmark_data: OnceCell, + perf_file: NamedTempFile, } impl PerfRunner { + #[cfg(test)] + pub fn perf_file(&self) -> &NamedTempFile { + &self.perf_file + } + pub async fn setup_environment( system_info: &crate::run::check_system::SystemInfo, setup_cache_dir: Option<&Path>, @@ -82,6 +89,7 @@ impl PerfRunner { pub fn new() -> Self { Self { benchmark_data: OnceCell::new(), + perf_file: NamedTempFile::new().unwrap(), } } @@ -89,7 +97,6 @@ impl PerfRunner { &self, mut cmd_builder: CommandBuilder, config: &Config, - profile_folder: &Path, ) -> anyhow::Result { let perf_fifo = PerfFifo::new()?; let runner_fifo = RunnerFifo::new()?; @@ -146,8 +153,8 @@ impl PerfRunner { ]); cmd_builder.wrap_with(perf_wrapper_builder); - // Copy the perf data to the profile folder - let perf_data_file_path = profile_folder.join(PERF_DATA_FILE_NAME); + // Get the perf data file path from the stored tempfile + let perf_data_file_path = self.perf_file.path(); let raw_command = format!( "set -o pipefail && {} | cat > {}", @@ -178,6 +185,12 @@ impl PerfRunner { pub async fn save_files_to(&self, profile_folder: &Path) -> anyhow::Result<()> { let start = std::time::Instant::now(); + // Copy perf data from tempfile to profile folder + let dest_path = profile_folder.join(PERF_DATA_FILE_NAME); + std::fs::copy(&self.perf_file, &dest_path) + .context("Failed to copy perf.pipedata from tempfile to profile folder")?; + debug!("Copied perf.pipedata to {}", dest_path.display()); + let bench_data = self .benchmark_data .get() From 745a466b2139c087b9724fd71791ceef3d36dc15 Mon Sep 17 00:00:00 2001 From: not-matthias Date: Thu, 13 Nov 2025 13:25:15 +0100 Subject: [PATCH 2/4] fixup: chown the temporary perf.pipedata file --- src/run/runner/wall_time/perf/mod.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/run/runner/wall_time/perf/mod.rs b/src/run/runner/wall_time/perf/mod.rs index e4cb1dcb..9938445f 100644 --- a/src/run/runner/wall_time/perf/mod.rs +++ b/src/run/runner/wall_time/perf/mod.rs @@ -185,6 +185,20 @@ impl PerfRunner { pub async fn save_files_to(&self, profile_folder: &Path) -> anyhow::Result<()> { let start = std::time::Instant::now(); + // We ran perf with sudo, so we have to change the ownership of the perf.data + run_with_sudo( + "chown", + [ + "-R", + &format!( + "{}:{}", + nix::unistd::Uid::current(), + nix::unistd::Gid::current() + ), + &self.perf_file.path().to_string_lossy(), + ], + )?; + // Copy perf data from tempfile to profile folder let dest_path = profile_folder.join(PERF_DATA_FILE_NAME); std::fs::copy(&self.perf_file, &dest_path) From e08063a04688a0ca3ad2394e3bdb5d654633b46d Mon Sep 17 00:00:00 2001 From: not-matthias Date: Thu, 13 Nov 2025 13:29:31 +0100 Subject: [PATCH 3/4] fixup: fix typo in comment --- src/run/runner/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/run/runner/tests.rs b/src/run/runner/tests.rs index c1de1a3d..d2a9b64e 100644 --- a/src/run/runner/tests.rs +++ b/src/run/runner/tests.rs @@ -335,7 +335,7 @@ fi let perf_data_path = perf_runner.perf_file().path(); assert!(perf_data_path.exists(), "perf.pipedata should exist"); - // Assert it starts with PERFPIPEPERFILE2 + // Assert it starts with PERFILE2 let mut file = std::fs::File::open(perf_data_path).unwrap(); let expected = b"PERFILE2"; let mut actual = [0u8; 8]; From 9a7a4bd0ccbfa68ba1a1116cc22b8fba8b9703da Mon Sep 17 00:00:00 2001 From: not-matthias Date: Thu, 13 Nov 2025 13:40:00 +0100 Subject: [PATCH 4/4] fixup: try to chown before writing --- src/run/runner/wall_time/perf/mod.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/run/runner/wall_time/perf/mod.rs b/src/run/runner/wall_time/perf/mod.rs index 9938445f..69ccad1b 100644 --- a/src/run/runner/wall_time/perf/mod.rs +++ b/src/run/runner/wall_time/perf/mod.rs @@ -156,6 +156,19 @@ impl PerfRunner { // Get the perf data file path from the stored tempfile let perf_data_file_path = self.perf_file.path(); + // Ensure we have the permissions to write to the tempfile when running with sudo + run_with_sudo( + "chown", + [ + &format!( + "{}:{}", + nix::unistd::Uid::current(), + nix::unistd::Gid::current() + ), + &perf_data_file_path.to_string_lossy().to_string(), + ], + )?; + let raw_command = format!( "set -o pipefail && {} | cat > {}", &cmd_builder.as_command_line(),