Skip to content

ref: bundle exec-harness, memtrack, samply as codspeed tool subcommands#371

Open
art049 wants to merge 2 commits into
mainfrom
ref/codspeed-tool-subcommand
Open

ref: bundle exec-harness, memtrack, samply as codspeed tool subcommands#371
art049 wants to merge 2 commits into
mainfrom
ref/codspeed-tool-subcommand

Conversation

@art049
Copy link
Copy Markdown
Member

@art049 art049 commented May 24, 2026

Move samply from a flat hidden codspeed samply re-exec target to codspeed tool samply under a new hidden tool subcommand group, and add codspeed tool exec-harness + codspeed tool memtrack alongside it.

Both helpers were previously shipped as standalone binaries downloaded at runtime via pinned installer scripts. They now live entirely inside the main codspeed binary — the orchestrator re-execs current_exe tool exec-harness … / tool memtrack track … instead of relying on PATH, the BinaryPin entries + ensure_binary_installed flow are removed, and src/binary_installer/ is deleted (only mongo-tracer remained, and it uses its own download path).

Each crate keeps a run_cli(argv) entry shared between its standalone [[bin]] (kept for development) and the bundled subcommand, so the two paths can't drift. The standalone binaries are no longer released.

Platform notes

memtrack's eBPF code is Linux-only — the main crate enables the ebpf feature only via [target.'cfg(target_os = "linux")'.dependencies]. On macOS, codspeed tool memtrack parses but bails with a clear "only supported on Linux" error instead of a clap "unknown subcommand" message, keeping the CLI surface uniform across platforms.

CLI shape

The tool group is hidden and disables its own help, so the root CLI's global flags (--config, --config-name, --setup-cache-dir) no longer leak into wrapped binaries' --help output; --help flows through to samply/exec-harness/memtrack themselves. Tool short-circuits in cli::run() before any config loading so a broken ~/.config/codspeed/config.yaml doesn't block re-exec.

Areas worth a careful look

  • src/executor/tests.rs::wrap_with_exec_harness now locates target/<profile>/codspeed from the test binary's parent dir. This assumes cargo test for the workspace builds the codspeed bin alongside the tests, which it does in practice — but worth confirming in CI.
  • multi_targets::build_exec_targets_pipe_command now shell-quotes current_exe into a heredoc command. Paths with single quotes would still need extra care, but in practice the executable path doesn't contain them.

…ands

Move samply from a flat hidden `codspeed samply` re-exec target to
`codspeed tool samply` under a new hidden `tool` subcommand group, and
add `codspeed tool exec-harness` + `codspeed tool memtrack` alongside it.

Both helpers were previously shipped as standalone binaries downloaded
at runtime via pinned installer scripts. They now live entirely inside
the main `codspeed` binary: the orchestrator re-execs `current_exe tool
exec-harness …` / `tool memtrack track …` instead of relying on PATH,
the `BinaryPin` entries + `ensure_binary_installed` flow are removed,
and `src/binary_installer/` is deleted (only mongo-tracer remained, and
it uses its own download path).

Each crate keeps a `run_cli(argv)` entry shared between its standalone
`[[bin]]` (kept for development) and the bundled subcommand, so the two
paths can't drift. The standalone binaries are no longer released.

memtrack's eBPF code is Linux-only — the main crate enables the `ebpf`
feature only via `[target.'cfg(target_os = "linux")'.dependencies]`. On
macOS, `codspeed tool memtrack` parses but bails with a clear error
instead of a clap "unknown subcommand" message, keeping the CLI surface
uniform across platforms.

The `tool` group is hidden and disables its own help, so the root CLI's
global flags (`--config`, `--config-name`, `--setup-cache-dir`) no longer
leak into wrapped binaries' `--help` output; `--help` flows through to
samply/exec-harness/memtrack themselves. `Tool` short-circuits in
`cli::run()` before any config loading so a broken
`~/.config/codspeed/config.yaml` doesn't block re-exec.

Co-Authored-By: Claude <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 24, 2026

Greptile Summary

This PR bundles exec-harness, memtrack, and samply as subcommands under a new hidden codspeed tool group, replacing the previous approach of downloading standalone binaries at runtime via installer scripts. The binary_installer module and the MemtrackInstaller/ExecHarnessInstaller BinaryPin entries are deleted; the orchestrator and memory executor now re-exec current_exe tool exec-harness … and current_exe tool memtrack track … respectively.

  • Bundling: exec-harness and memtrack each expose a shared run_cli(argv) entry point used by both the standalone [[bin]] and the tool subcommand, so the two paths can't drift independently.
  • Platform gating: memtrack's eBPF dependency is Linux-only; on macOS the tool memtrack subcommand compiles but bails with an actionable error instead of silently failing as an unknown subcommand.
  • Shell-command safety: A new CommandBuilder::to_shell_command() method replaces to_string_lossy() when embedding the executable path in a shell heredoc, failing fast on non-UTF-8 paths instead of silently mangling them.

Confidence Score: 5/5

This PR is safe to merge. The bundling approach is well-structured, platform gating is correct, and prior issues with the DRAIN_EVENTS static and lossy path embedding have been addressed in this change.

The refactor correctly re-routes exec-harness and memtrack invocations through current_exe tool … instead of relying on separately installed binaries. The Arc per-call drain signal, the new to_shell_command() guard, and the cfg-gated memtrack stubs all close the issues that were present in the prior design. No logic bugs were found in the changed paths.

src/executor/tests.rs — the new workspace_binary helper asserts (panics) if the codspeed binary hasn't been built before cargo test is run; CI job ordering should ensure the binary exists.

Important Files Changed

Filename Overview
src/cli/tool/mod.rs New hidden tool subcommand group; get_command_builder properly used by both multi_targets.rs and executor.rs — neither arm is dead code.
crates/memtrack/src/cli.rs Shared CLI entry for memtrack; fixes the prior static DRAIN_EVENTS issue by using Arc per call, safe for multiple invocations in the same process.
crates/exec-harness/src/lib.rs Shared run_cli entry for exec-harness; uses try_init().ok() for env_logger so the bundled and standalone paths don't conflict on logger initialization.
src/executor/helpers/command.rs Adds to_shell_command() that fails on non-UTF-8 paths, replacing the lossy conversion used when building shell heredoc commands.
src/cli/exec/multi_targets.rs Builds exec-harness heredoc command via ToolCommand::ExecHarness.get_command_builder().to_shell_command() instead of a hardcoded string; correctly embeds - for stdin-mode.
src/executor/memory/executor.rs Builds memtrack invocation via ToolCommand::Memtrack.get_command_builder() instead of the deleted MEMTRACK_COMMAND constant; tool_status() now returns None and setup() is a no-op.
src/executor/tests.rs Adds workspace_binary to locate the codspeed binary one level above the test deps dir; wrap_with_exec_harness now invokes the actual workspace binary, requiring it to be pre-built before running tests.
src/cli/tool/memtrack.rs Proper #[cfg(target_os = linux)] / #[cfg(not(target_os = linux))] split; non-Linux builds get a clear bail message instead of a clap unknown-subcommand error.
src/binary_installer/mod.rs Deleted entirely; ensure_binary_installed for memtrack and exec-harness is no longer needed since both are now bundled.

Reviews (2): Last reviewed commit: "--wip-- [skip ci]" | Re-trigger Greptile

Comment thread src/cli/exec/multi_targets.rs Outdated
Comment thread src/cli/tool/mod.rs
Comment on lines +49 to +56
ToolCommand::ExecHarness(args) => {
builder.arg("exec-harness");
builder.args(args.args.iter().cloned());
}
ToolCommand::Memtrack(args) => {
builder.arg("memtrack");
builder.args(args.args.iter().cloned());
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 ExecHarness and Memtrack arms of get_command_builder are dead code

ToolCommand::Samply is the only variant whose get_command_builder() is actually called (from wall_time/profiler/samply/mod.rs). The memory executor builds its memtrack invocation inline in executor.rs, and multi_targets builds the exec-harness heredoc command directly — neither goes through this method. As the two paths evolve independently they could diverge silently. Consider either routing executor.rs and multi_targets.rs through get_command_builder() for consistency, or removing the dead arms (and the method if only Samply uses it) to avoid the mismatch risk.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 24, 2026

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing ref/codspeed-tool-subcommand (d513311) with main (f4d8493)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

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.

1 participant