--wip-- [skip ci]

GuillaumeLagrange · GuillaumeLagrange · commit 2fa3fdde10cc · 2025-12-10T01:41:57.000+01:00
diff --git a/crates/exec-harness/src/main.rs b/crates/exec-harness/src/main.rs
@@ -1,4 +1,7 @@
 use crate::walltime::WalltimeResults;
+use anyhow::Context;
+use anyhow::Result;
+use anyhow::bail;
 use clap::Parser;
 use codspeed::instrument_hooks::InstrumentHooks;
 use codspeed::walltime_results::WalltimeBenchmark;
@@ -19,7 +22,7 @@ struct Args {
     command: Vec<String>,
 }
 
-fn main() {
+fn main() -> Result<()> {
     let args = Args::parse();
 
     if args.command.is_empty() {
@@ -31,18 +34,15 @@ fn main() {
     let bench_name = args.name.unwrap_or_else(|| {
         // Extract filename from command path
         let cmd = &args.command[0];
-        std::path::Path::new(cmd)
-            .file_name()
-            .and_then(|n| n.to_str())
-            .map(|s| s.to_string())
-            .unwrap_or_else(|| "exec_benchmark".to_string())
+        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: Change this to avoid impersonating `codspeed-rust`
+    // TODO: Stop impersonating codspeed-rust 🥸
     hooks
         .set_integration("codspeed-rust", env!("CARGO_PKG_VERSION"))
         .unwrap();
@@ -53,36 +53,24 @@ fn main() {
     hooks.start_benchmark().unwrap();
     for _ in 0..NUM_ITERATIONS {
         // Spawn the command
-        let mut child = match process::Command::new(&args.command[0])
+        let mut child = process::Command::new(&args.command[0])
             .args(&args.command[1..])
             .spawn()
-        {
-            Ok(child) => child,
-            Err(e) => {
-                eprintln!("Failed to spawn command: {e}");
-                process::exit(1);
-            }
-        };
+            .context("Failed to spawn command")?;
+
         // Start monotonic timer for this iteration
         let bench_start = InstrumentHooks::current_timestamp();
 
         // Wait for the process to complete
-        let status = match child.wait() {
-            Ok(status) => status,
-            Err(e) => {
-                eprintln!("Failed to wait for command: {e}");
-                process::exit(1);
-            }
-        };
+        let status = child.wait().context("Failed to wait for command")?;
 
         // Measure elapsed time
         let bench_end = InstrumentHooks::current_timestamp();
         hooks.add_benchmark_timestamps(bench_start, bench_end);
 
         // Exit immediately if any iteration fails
         if !status.success() {
-            eprintln!("Command failed with exit code: {:?}", status.code());
-            process::exit(status.code().unwrap_or(1));
+            bail!("Command failed with exit code: {:?}", status.code());
         }
 
         // Calculate and store the elapsed time in nanoseconds
@@ -113,4 +101,6 @@ fn main() {
                 .unwrap_or_else(|_| std::env::current_dir().unwrap().join(".codspeed")),
         )
         .expect("Failed to save walltime results");
+
+    Ok(())
 }
diff --git a/crates/exec-harness/src/walltime.rs b/crates/exec-harness/src/walltime.rs
@@ -32,6 +32,7 @@ impl WalltimeResults {
                 type_: "walltime".to_string(),
             },
             creator: Creator {
+                // TODO: Stop impersonating codspeed-rust 🥸
                 name: "codspeed-rust".to_string(),
                 version: env!("CARGO_PKG_VERSION").to_string(),
                 pid: std::process::id(),
diff --git a/src/executor/config.rs b/src/executor/config.rs
@@ -1,3 +1,4 @@
+use crate::exec::DEFAULT_REPOSITORY_NAME;
 use crate::instruments::Instruments;
 use crate::prelude::*;
 use crate::run::{RunArgs, UnwindingMode};
@@ -150,11 +151,9 @@ impl TryFrom<crate::exec::ExecArgs> for Config {
             .repository
             .map(|repo| RepositoryOverride::from_arg(repo, args.shared.provider))
             .transpose()?
-            //FIXME: This is ignored in the project upload endpoint, but it's easier to provide
-            //a default override here to make the LocalProvider work without extra changes.
             .unwrap_or_else(|| RepositoryOverride {
                 owner: "projects".to_string(),
-                repository: "local-run".to_string(),
+                repository: DEFAULT_REPOSITORY_NAME.to_string(),
                 repository_provider: RepositoryProvider::GitHub,
             });
 
diff --git a/src/executor/tests.rs b/src/executor/tests.rs
@@ -221,7 +221,7 @@ mod walltime {
 
         WALLTIME_INIT
             .get_or_init(|| async {
-                let executor = WallTimeExecutor::new(true);
+                let executor = WallTimeExecutor::new();
                 let system_info = SystemInfo::new().unwrap();
                 executor.setup(&system_info, None).await.unwrap();
             })
@@ -234,7 +234,7 @@ mod walltime {
             .await;
         let permit = semaphore.acquire().await.unwrap();
 
-        (permit, WallTimeExecutor::new(true))
+        (permit, WallTimeExecutor::new())
     }
 
     fn walltime_config(command: &str, enable_perf: bool) -> Config {
diff --git a/src/executor/wall_time/executor.rs b/src/executor/wall_time/executor.rs
@@ -94,7 +94,13 @@ pub struct WallTimeExecutor {
 }
 
 impl WallTimeExecutor {
-    pub fn new(output_pipedata: bool) -> Self {
+    pub fn new() -> Self {
+        Self {
+            perf: cfg!(target_os = "linux").then(|| PerfRunner::new(true)),
+        }
+    }
+
+    pub fn new_with_output_pipedata(output_pipedata: bool) -> Self {
         Self {
             perf: cfg!(target_os = "linux").then(|| PerfRunner::new(output_pipedata)),
         }
diff --git a/src/executor/wall_time/perf/mod.rs b/src/executor/wall_time/perf/mod.rs
@@ -49,6 +49,8 @@ const PERF_PIPEDATA_FILE_NAME: &str = "perf.pipedata";
 pub struct PerfRunner {
     benchmark_data: OnceCell<BenchmarkData>,
     /// Whether to output the perf data to a streamable .pipedata file
+    /// This can be removed once we have upstreamed the the linux-perf-data crate changes to parse
+    /// from pipedata directly, to only support pipedata.
     output_pipedata: bool,
 }
 
@@ -167,11 +169,15 @@ impl PerfRunner {
         // Output the perf data to the profile folder
         let perf_data_file_path = self.get_perf_file_path(profile_folder);
 
-        let raw_command = format!(
-            "set -o pipefail && {} | cat > {}",
-            &cmd_builder.as_command_line(),
-            perf_data_file_path.to_string_lossy()
-        );
+        let raw_command = if self.output_pipedata {
+            format!(
+                "set -o pipefail && {} | cat > {}",
+                &cmd_builder.as_command_line(),
+                perf_data_file_path.to_string_lossy()
+            )
+        } else {
+            cmd_builder.as_command_line()
+        };
         let mut wrapped_builder = CommandBuilder::new("bash");
         wrapped_builder.args(["-c", &raw_command]);
 
@@ -270,7 +276,7 @@ impl PerfRunner {
 
             match cmd {
                 FifoCommand::CurrentBenchmark { pid, uri } => {
-                    bench_order_by_timestamp.push((current_time(), uri.clone()));
+                    bench_order_by_timestamp.push((current_time(), uri));
                     bench_pids.insert(pid);
 
                     #[cfg(target_os = "linux")]
diff --git a/src/executor/wall_time/perf/parse_perf_file.rs b/src/executor/wall_time/perf/parse_perf_file.rs
@@ -1,5 +1,7 @@
 use super::perf_map::ProcessSymbols;
 use super::unwind_data::UnwindDataExt;
+use crate::executor::helpers::run_with_sudo::run_with_sudo;
+use crate::prelude::*;
 use libc::pid_t;
 use linux_perf_data::PerfFileReader;
 use linux_perf_data::PerfFileRecord;
@@ -8,26 +10,40 @@ use runner_shared::unwind_data::UnwindData;
 use std::collections::HashMap;
 use std::path::Path;
 
-use crate::prelude::*;
-
 pub struct MemmapRecordsOutput {
     pub symbols_by_pid: HashMap<pid_t, ProcessSymbols>,
     pub unwind_data_by_pid: HashMap<pid_t, Vec<UnwindData>>,
 }
 
 pub(super) fn parse_for_memmap2<P: AsRef<Path>>(perf_file_path: P) -> Result<MemmapRecordsOutput> {
-    let reader = std::fs::File::open(perf_file_path).unwrap();
     let mut symbols_by_pid = HashMap::<pid_t, ProcessSymbols>::new();
     let mut unwind_data_by_pid = HashMap::<pid_t, Vec<UnwindData>>::new();
 
-    // TODO: Handle pipedata here once linux_perf_data supports it upstream
+    //FIXME: Remove this once again when we parse directly from pipedata
+    {
+        let tmp_perf_file_path = perf_file_path.as_ref().to_string_lossy();
+
+        // 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()
+                ),
+                &tmp_perf_file_path,
+            ],
+        )?;
+    }
+    let reader = std::fs::File::open(perf_file_path.as_ref()).unwrap();
+
     let PerfFileReader {
         mut perf_file,
         mut record_iter,
     } = PerfFileReader::parse_file(reader)?;
 
-    const PROT_EXEC: u32 = 0x4;
-
     while let Some(record) = record_iter.next_record(&mut perf_file).unwrap() {
         let PerfFileRecord::EventRecord { record, .. } = record else {
             continue;
@@ -59,7 +75,7 @@ pub(super) fn parse_for_memmap2<P: AsRef<Path>>(perf_file_path: P) -> Result<Mem
 
         if record_path_string.starts_with("[") && record_path_string.ends_with("]") {
             // Skip special mappings
-            debug!(
+            trace!(
                 "Skipping special mapping: {} - {:x}-{:x}",
                 record_path_string, record.address, end_addr
             );
@@ -76,7 +92,7 @@ pub(super) fn parse_for_memmap2<P: AsRef<Path>>(perf_file_path: P) -> Result<Mem
             record.protection,
         );
 
-        if record.protection & PROT_EXEC == 0 {
+        if record.protection as i32 & libc::PROT_EXEC == 0 {
             continue;
         }
 

Original file line number	Diff line number	Diff line change
`@@ -94,7 +94,13 @@ pub struct WallTimeExecutor {`
`94`	`94`	`}`
`95`	`95`
`96`	`96`	`impl WallTimeExecutor {`
`97`		`- pub fn new(output_pipedata: bool) -> Self {`
	`97`	`+ pub fn new() -> Self {`
	`98`	`+ Self {`
	`99`	`+ perf: cfg!(target_os = "linux").then(\|\| PerfRunner::new(true)),`
	`100`	`+ }`
	`101`	`+ }`
	`102`	`+`
	`103`	`+ pub fn new_with_output_pipedata(output_pipedata: bool) -> Self {`
`98`	`104`	`Self {`
`99`	`105`	`perf: cfg!(target_os = "linux").then(\|\| PerfRunner::new(output_pipedata)),`
`100`	`106`	`}`