-
Notifications
You must be signed in to change notification settings - Fork 6
Feat: add the exec subcommand to run an arbitrary rust binary #165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: add the exec subcommand to run an arbitrary rust binary #165
Conversation
1df9f70 to
e597063
Compare
not-matthias
left a comment
There was a problem hiding this 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 👍
31266e0 to
f39e236
Compare
192b8db to
65e94e3
Compare
There was a problem hiding this 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
execsubcommand with corresponding harness binary for wrapping commands - Refactored executor logic from
run::runnerto dedicatedexecutormodule - Created
ExecutionContextto consolidate execution state (config, system info, provider, logger) - Modified perf handling to conditionally output
.pipedata(streaming) vs.perf.data(file-based) format - Moved
run_environmentandinstrumentsmodules 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.
|
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. |
2fa3fdd to
b503745
Compare
b503745 to
abede20
Compare
f90c630 to
5727e04
Compare
adriencaccia
left a comment
There was a problem hiding this 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.
e5fec24 to
1dd86c6
Compare
runmodule the whole executor logicExecutionContextthat holds all the data required by the executor and other parts to workexecandrunwhile keeping some stuff modularpipedataformat, ideally we remove the/proc/<pid>/mapsparsing logic altogetherSome 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