Skip to content

Conversation

@GuillaumeLagrange
Copy link
Contributor

@GuillaumeLagrange GuillaumeLagrange commented Nov 25, 2025

  • Depends on the monorepo pr for the new endpoint
  • Refactor to move out of the run module the whole executor logic
  • Create a new ExecutionContext that holds all the data required by the executor and other parts to work
  • Factorize the logic between exec and run while keeping some stuff modular
  • Parse the perf map during wt teardown if not outputing pipedata
    • This is temporary, while the upstream linux-perf-data cannot parse the pipedata format, ideally we remove the /proc/<pid>/maps parsing logic altogether

Some of the logic would still need to be refactored to be perfectly clean, but the PR is starting to be quite huge already

CI fails because of unrelated version mismatch within the repo (the plan job), should not affect review

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch 2 times, most recently from 1df9f70 to e597063 Compare November 25, 2025 05:16
Copy link
Member

@not-matthias not-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good! I like the new structure 👍

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch 11 times, most recently from 31266e0 to f39e236 Compare December 2, 2025 11:56
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch 9 times, most recently from 192b8db to 65e94e3 Compare December 8, 2025 15:55
@not-matthias not-matthias requested a review from Copilot December 8, 2025 16:24
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new exec subcommand to run arbitrary Rust binaries with CodSpeed instrumentation, and performs a substantial refactoring to support this feature. The changes separate executor logic from the run module, introduce a unified ExecutionContext, and modify perf handling to support both streaming (pipedata) and file-based (perf.data) formats.

Key Changes:

  • Added exec subcommand with corresponding harness binary for wrapping commands
  • Refactored executor logic from run::runner to dedicated executor module
  • Created ExecutionContext to consolidate execution state (config, system info, provider, logger)
  • Modified perf handling to conditionally output .pipedata (streaming) vs .perf.data (file-based) format
  • Moved run_environment and instruments modules to top-level

Reviewed changes

Copilot reviewed 118 out of 150 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/executor/mod.rs Core executor module with shared execution logic
src/executor/execution_context.rs New ExecutionContext for consolidating execution state
src/executor/config.rs Moved and refactored Config from run module
src/exec/mod.rs New exec subcommand implementation
src/run/mod.rs Refactored to use new executor infrastructure
src/run_environment/* Moved from run::run_environment to top level
src/executor/wall_time/perf/* Perf handling with pipedata vs file-based modes
crates/exec-harness/* New harness binary for wrapping arbitrary commands
src/app.rs Added exec subcommand to CLI
Various test files Updated to work with new ExecutionContext pattern

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@not-matthias
Copy link
Member

Looks good overall, it's mostly just about how we can reduce duplication while also keeping clarity. Feel free to challenge any comment, I'll let you decide where it does and doesn't make sense.

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch 4 times, most recently from 2fa3fdd to b503745 Compare December 10, 2025 00:49
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch from b503745 to abede20 Compare December 10, 2025 10:21
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch 5 times, most recently from f90c630 to 5727e04 Compare December 15, 2025 13:46
Copy link
Member

@adriencaccia adriencaccia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but as you said the refactor is not complete, there are still a lot of stuff defined in run that is being used in other modules at the same level.
Let's make sure you have an issue for this, which describe the idea state we want this repo to end up in

This binary is in charge of providing a minimal instrument-hooks wrapper
around the command it is given.
Refactor aims to extract common behavior between exec and run.
It mostly moves out the run::Config into the executor module, as well
as moving the run_environment module to the root of the repo.

The main aim is to offload the overloaded run module that has outgrown
its initial purpose of a clap subcommand.
This is a quick fix for now, that will need to be addressed when we
start allowing multiple projects etc.
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch from e5fec24 to 1dd86c6 Compare December 16, 2025 11:39
@GuillaumeLagrange GuillaumeLagrange merged commit 1dd86c6 into main Dec 16, 2025
14 of 15 checks passed
@GuillaumeLagrange GuillaumeLagrange deleted the cod-1723-run-an-hello-world-rust-binary-in-the-runner-in-walltime branch December 16, 2025 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants