Skip to content

Commit b3c84c5

Browse files
committed
fix(walltime): use brew-installed bash for samply on macOS
samply can't profile macOS's Apple-signed /bin/bash (and Python launched through it), so it needs an unsigned (ad-hoc-signed) bash. On macOS we now: - Probe the `bash` first on PATH with `codesign -dv` and only intervene when it lacks `Signature=adhoc` (i.e. system bash or another signed build). If a compatible bash is already on PATH (e.g. Homebrew's), we skip the brew dance entirely and leave PATH untouched. - When intervention is needed: - Require Homebrew to be installed (bail with a pointer to brew.sh otherwise); installing it ourselves would be too invasive. - Skip the install if `brew list --formula --quiet bash` reports bash is already present. - In a TTY, prompt the user before running `brew install bash` (default Y, accepts y/yes/Enter). In CI (non-TTY), install silently. - Suppress brew's stdio unless CODSPEED_LOG=debug; on failure the captured output is included in the error message. - Prepend `$(brew --prefix)/bin` to the spawned samply command's PATH so brew's unsigned bash wins over other `bash` entries earlier on PATH (e.g. nix-profile's signed bash). Only the samply child process tree sees this PATH — the parent shell is untouched. Adds: - `executor/helpers/homebrew` module with `ensure_installed`, `is_installed`, `prefix`, and `install`. - `CommandBuilder::env()` to set env vars on the spawned process.
1 parent 174c524 commit b3c84c5

4 files changed

Lines changed: 199 additions & 2 deletions

File tree

src/executor/helpers/command.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,17 @@ impl CommandBuilder {
4848
self
4949
}
5050

51+
#[allow(dead_code)] // currently only used behind cfg(target_os = "macos")
52+
pub fn env<K, V>(&mut self, key: K, value: V) -> &mut Self
53+
where
54+
K: AsRef<OsStr>,
55+
V: AsRef<OsStr>,
56+
{
57+
self.envs
58+
.insert(key.as_ref().to_owned(), value.as_ref().to_owned());
59+
self
60+
}
61+
5162
pub fn current_dir<D>(&mut self, dir: D)
5263
where
5364
D: AsRef<OsStr>,

src/executor/helpers/homebrew.rs

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
//! Thin wrappers around the `brew` CLI for macOS-only setup paths.
2+
3+
use crate::executor::helpers::env::is_codspeed_debug_enabled;
4+
use crate::prelude::*;
5+
use std::path::PathBuf;
6+
use std::process::{Command, Stdio};
7+
8+
/// Fail unless `brew` is on `PATH`. We intentionally do not install Homebrew
9+
/// ourselves — that's too invasive a side effect for a profiler setup step.
10+
pub fn ensure_installed() -> Result<()> {
11+
let installed = Command::new("which")
12+
.arg("brew")
13+
.output()
14+
.is_ok_and(|o| o.status.success());
15+
if !installed {
16+
bail!("Homebrew is required but was not found on PATH");
17+
}
18+
Ok(())
19+
}
20+
21+
/// Return Homebrew's install prefix (`/opt/homebrew` on Apple Silicon,
22+
/// `/usr/local` on Intel). Shells out to `brew --prefix` rather than hardcoding
23+
/// so we don't have to guess the architecture.
24+
pub fn prefix() -> Result<PathBuf> {
25+
let output = Command::new("brew")
26+
.arg("--prefix")
27+
.output()
28+
.context("failed to spawn `brew --prefix`")?;
29+
if !output.status.success() {
30+
bail!("`brew --prefix` exited with status {}", output.status);
31+
}
32+
let path = String::from_utf8(output.stdout)
33+
.context("`brew --prefix` returned non-UTF-8 output")?
34+
.trim()
35+
.to_owned();
36+
Ok(PathBuf::from(path))
37+
}
38+
39+
/// Check whether a brew formula is already installed. Uses `brew list <pkg>`,
40+
/// which is local-only (no network/API hit) and returns non-zero when missing.
41+
pub fn is_installed(package: &str) -> bool {
42+
Command::new("brew")
43+
.args(["list", "--formula", "--quiet", package])
44+
.output()
45+
.is_ok_and(|o| o.status.success())
46+
}
47+
48+
/// Run `brew install <package>`. Idempotent: brew exits 0 when the formula
49+
/// is already installed, so callers don't need to pre-check.
50+
pub fn install(package: &str) -> Result<()> {
51+
// Bypass the logger here: `info!` goes through the spinner-suspend path
52+
// which buffers until the spinner ticks, so the message would only show
53+
// up after brew returns. We want the user to see it before brew starts.
54+
eprintln!("Installing {package} via Homebrew...");
55+
56+
// Check the user-facing debug knob rather than log::max_level(); the
57+
// latter is forced to Trace by the runner's file logger and can't
58+
// distinguish "user wants debug output" from "captured to runner.log".
59+
let stdio = || {
60+
if is_codspeed_debug_enabled() {
61+
Stdio::inherit()
62+
} else {
63+
Stdio::piped()
64+
}
65+
};
66+
let output = Command::new("brew")
67+
.args(["install", package])
68+
.stdout(stdio())
69+
.stderr(stdio())
70+
.output()
71+
.with_context(|| format!("failed to spawn `brew install {package}`"))?;
72+
if !output.status.success() {
73+
bail!(
74+
"`brew install {package}` exited with status {}\nstdout:\n{}\nstderr:\n{}",
75+
output.status,
76+
String::from_utf8_lossy(&output.stdout),
77+
String::from_utf8_lossy(&output.stderr),
78+
);
79+
}
80+
Ok(())
81+
}

src/executor/helpers/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ pub mod detect_executable;
44
pub mod env;
55
pub mod get_bench_command;
66
pub mod harvest_perf_maps_for_pids;
7+
#[cfg(target_os = "macos")]
8+
pub mod homebrew;
79
pub mod introspected_golang;
810
pub mod introspected_nodejs;
911
pub mod profile_folder;

src/executor/wall_time/profiler/samply/mod.rs

Lines changed: 105 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,20 @@ pub struct SamplyProfiler {
2727
/// returns — samply writes the file itself — but we hold onto it so future
2828
/// `finalize` work (e.g. validation, conversion) has the path on hand.
2929
output_path: Option<PathBuf>,
30+
/// macOS only: set in [`Profiler::setup`] when the `bash` resolved on PATH
31+
/// is Apple-signed and samply can't profile it, so [`Profiler::wrap_command`]
32+
/// must prepend brew's bin dir to PATH.
33+
#[cfg(target_os = "macos")]
34+
needs_brew_bash: std::cell::Cell<bool>,
3035
}
3136

3237
impl SamplyProfiler {
3338
pub fn new() -> Self {
34-
Self { output_path: None }
39+
Self {
40+
output_path: None,
41+
#[cfg(target_os = "macos")]
42+
needs_brew_bash: std::cell::Cell::new(false),
43+
}
3544
}
3645
}
3746

@@ -42,7 +51,28 @@ impl Profiler for SamplyProfiler {
4251
_system_info: &SystemInfo,
4352
_setup_cache_dir: Option<&Path>,
4453
) -> anyhow::Result<()> {
45-
ensure_linux_profiling_sysctls()
54+
ensure_linux_profiling_sysctls()?;
55+
56+
// samply can't profile Apple-signed bash. Only do the brew dance if the
57+
// bash that samply would actually exec (the first `bash` on PATH) is
58+
// signed; if a compatible (ad-hoc-signed) bash is already first on PATH,
59+
// we're done.
60+
#[cfg(target_os = "macos")]
61+
{
62+
use crate::executor::helpers::homebrew;
63+
if bash_in_path_is_compatible()? {
64+
return Ok(());
65+
}
66+
67+
self.needs_brew_bash.set(true);
68+
homebrew::ensure_installed()?;
69+
if !homebrew::is_installed("bash") {
70+
confirm_bash_install()?;
71+
homebrew::install("bash")?;
72+
}
73+
}
74+
75+
Ok(())
4676
}
4777

4878
async fn wrap_command(
@@ -71,6 +101,23 @@ impl Profiler for SamplyProfiler {
71101
.get_command_builder()?;
72102

73103
cmd_builder.wrap_with(samply_builder);
104+
105+
// If `setup` decided the bash on PATH is Apple-signed, prepend brew's
106+
// bin so samply's spawned shell resolves to the ad-hoc-signed brew bash
107+
// instead. Only the samply child's PATH is touched.
108+
#[cfg(target_os = "macos")]
109+
if self.needs_brew_bash.get() {
110+
use crate::executor::helpers::homebrew;
111+
let brew_bin = homebrew::prefix()?.join("bin");
112+
let existing = std::env::var_os("PATH").unwrap_or_default();
113+
let mut new_path = std::ffi::OsString::from(brew_bin);
114+
if !existing.is_empty() {
115+
new_path.push(":");
116+
new_path.push(&existing);
117+
}
118+
cmd_builder.env("PATH", new_path);
119+
}
120+
74121
self.output_path = Some(output_path);
75122
Ok(cmd_builder)
76123
}
@@ -114,3 +161,59 @@ impl Profiler for SamplyProfiler {
114161
Ok(())
115162
}
116163
}
164+
165+
/// Return `true` if the first `bash` on `PATH` can be profiled by samply.
166+
/// Compatible bashes (e.g. Homebrew's) are ad-hoc-signed and show
167+
/// `Signature=adhoc`; the system `/bin/bash` is signed with an `Authority=`
168+
/// line and is incompatible. Anything we can't classify is treated as
169+
/// incompatible so we err on the side of installing the brew bash.
170+
#[cfg(target_os = "macos")]
171+
fn bash_in_path_is_compatible() -> anyhow::Result<bool> {
172+
use std::process::Command;
173+
174+
let which = Command::new("/usr/bin/which")
175+
.arg("bash")
176+
.output()
177+
.context("failed to spawn `which bash`")?;
178+
if !which.status.success() {
179+
// No bash on PATH at all — samply will fail. Force the brew install
180+
// path so we end up with one.
181+
return Ok(false);
182+
}
183+
let bash_path = String::from_utf8_lossy(&which.stdout).trim().to_owned();
184+
185+
// `codesign -dv` writes to stderr.
186+
let codesign = Command::new("/usr/bin/codesign")
187+
.args(["-dv", "--verbose=2", &bash_path])
188+
.output()
189+
.context("failed to spawn `codesign`")?;
190+
let info = String::from_utf8_lossy(&codesign.stderr);
191+
Ok(info.contains("Signature=adhoc") || info.contains("flags=0x2(adhoc)"))
192+
}
193+
194+
#[cfg(target_os = "macos")]
195+
fn confirm_bash_install() -> anyhow::Result<()> {
196+
use crate::local_logger::IS_TTY;
197+
use console::Term;
198+
199+
// Non-interactive (CI): just install
200+
if !*IS_TTY {
201+
return Ok(());
202+
}
203+
204+
eprintln!(
205+
"CodSpeed depends on bash for benchmark execution, but can't use /bin/bash because system executables are signed in a way that prevents profiling. Because of this, we need to install bash with Homebrew. This is a one-time setup, your system bash is untouched."
206+
);
207+
eprint!("\nRun `brew install bash` now? [Y/n] ");
208+
let line = Term::stderr().read_line().unwrap_or_default();
209+
let answer = line.trim();
210+
211+
// Default to yes on empty input (just pressing Enter).
212+
if !(answer.is_empty()
213+
|| answer.eq_ignore_ascii_case("y")
214+
|| answer.eq_ignore_ascii_case("yes"))
215+
{
216+
bail!("Declined; cannot continue without an unsigned bash");
217+
}
218+
Ok(())
219+
}

0 commit comments

Comments
 (0)