From aebd1771f98a6ce05b633244ff39a5f056fb1278 Mon Sep 17 00:00:00 2001 From: moha-bekh Date: Mon, 21 Sep 2026 05:31:54 -0400 Subject: [PATCH] refactor(executor): share the yes/no confirmation between the setup steps Two setup steps ask before installing something on the user's machine: the valgrind-codspeed source build, for the distributions we publish no package for, and the Homebrew bash samply needs on macOS, since the Apple-signed /bin/bash cannot be profiled. Each carried its own copy of the same logic: skip the question with no TTY, print the explanation and the question on stderr, read a line, count an empty answer as a yes. Move both onto confirm_default_yes, taking the valgrind prompt as the reference implementation. The macOS question is now asked with the progress bar suspended, so a running spinner can no longer overwrite it. Everything else keeps its shape: the valgrind caller still decides on CODSPEED_VALGRIND_BUILD_FROM_SOURCE first and treats a decline as a legitimate answer, the macOS one still fails on a decline. Closes COD-3537 Co-Authored-By: Claude Opus 5 (1M context) --- src/executor/helpers/confirm.rs | 19 +++++++++++ src/executor/helpers/mod.rs | 1 + src/executor/valgrind/build_from_source.rs | 33 +++---------------- src/executor/wall_time/profiler/samply/mod.rs | 23 +++---------- 4 files changed, 30 insertions(+), 46 deletions(-) create mode 100644 src/executor/helpers/confirm.rs diff --git a/src/executor/helpers/confirm.rs b/src/executor/helpers/confirm.rs new file mode 100644 index 000000000..30afb365b --- /dev/null +++ b/src/executor/helpers/confirm.rs @@ -0,0 +1,19 @@ +use crate::local_logger::{IS_TTY, suspend_progress_bar}; +use crate::prelude::*; +use console::Term; + +pub fn confirm_default_yes(explanation: &str, question: &str) -> bool { + if !*IS_TTY { + debug!("Not attached to a terminal, accepting without asking: {question}"); + return true; + } + + suspend_progress_bar(|| { + eprintln!("{explanation}"); + eprint!("\n{question} [Y/n] "); + + let line = Term::stderr().read_line().unwrap_or_default(); + let answer = line.trim(); + answer.is_empty() || answer.eq_ignore_ascii_case("y") || answer.eq_ignore_ascii_case("yes") + }) +} diff --git a/src/executor/helpers/mod.rs b/src/executor/helpers/mod.rs index 721ec697e..084302a1b 100644 --- a/src/executor/helpers/mod.rs +++ b/src/executor/helpers/mod.rs @@ -2,6 +2,7 @@ pub mod apt; #[cfg(target_os = "linux")] pub mod capabilities; pub mod command; +pub mod confirm; pub mod debug_file; pub mod detect_executable; pub mod env; diff --git a/src/executor/valgrind/build_from_source.rs b/src/executor/valgrind/build_from_source.rs index 9e4dbc75a..4622493ce 100644 --- a/src/executor/valgrind/build_from_source.rs +++ b/src/executor/valgrind/build_from_source.rs @@ -10,12 +10,11 @@ //! minutes and installs system-wide, so an interactive user is asked first. use crate::executor::helpers::command::CommandBuilder; +use crate::executor::helpers::confirm::confirm_default_yes; use crate::executor::helpers::run_command_with_log_pipe::run_command_with_log_pipe; use crate::executor::helpers::run_with_sudo::wrap_with_sudo; use crate::local_logger::rolling_buffer::{activate_rolling_buffer, deactivate_rolling_buffer}; -use crate::local_logger::{IS_TTY, suspend_progress_bar}; use crate::prelude::*; -use console::Term; use std::env; use std::ffi::OsStr; use std::path::Path; @@ -139,9 +138,7 @@ async fn install_build(source_dir: &Path) -> Result<()> { /// Decision, in order: /// /// - [`BUILD_FROM_SOURCE_ENV`] set to `true` or `false`: that answer, unconditionally; -/// - not a TTY (CI, unattended runs): build, since nobody is there to answer and -/// failing the run outright is the worse outcome; -/// - otherwise: ask, defaulting to building when the answer is empty. +/// - otherwise: ask, which on a run with no terminal means building without asking. /// /// Declining is a legitimate choice, not a failure: the caller then points at a /// manual installation, which is what happens on a failed build too. @@ -159,33 +156,13 @@ pub(super) fn is_wanted() -> bool { Err(_) => {} } - if !*IS_TTY { - debug!("Not attached to a terminal, building valgrind from source without asking"); - return true; - } - - suspend_progress_bar(prompt_for_source_build) -} - -/// Ask whether to build valgrind from source, defaulting to yes on an empty answer. -/// -/// Mirrors the confirmation the walltime executor uses before installing bash: the -/// question goes to stderr so it stays visible whatever the caller does with stdout. -fn prompt_for_source_build() -> bool { - eprintln!( + let accepted = confirm_default_yes( "CodSpeed can build valgrind-codspeed from source for this system. It clones the sources \ into a temporary directory, compiles them (a few minutes) and installs them system-wide \ with sudo. Declining leaves the installation to you, see \ - https://github.com/CodSpeedHQ/valgrind-codspeed" + https://github.com/CodSpeedHQ/valgrind-codspeed", + "Build valgrind-codspeed from source now?", ); - eprint!("\nBuild valgrind-codspeed from source now? [Y/n] "); - - let line = Term::stderr().read_line().unwrap_or_default(); - let answer = line.trim(); - - // Default to yes on empty input (just pressing Enter), as the `[Y/n]` prompt announces. - let accepted = - answer.is_empty() || answer.eq_ignore_ascii_case("y") || answer.eq_ignore_ascii_case("yes"); if !accepted { info!( "Skipping the source build. Set {BUILD_FROM_SOURCE_ENV}=true to build without being asked" diff --git a/src/executor/wall_time/profiler/samply/mod.rs b/src/executor/wall_time/profiler/samply/mod.rs index 3d77e7ade..4b72336a7 100644 --- a/src/executor/wall_time/profiler/samply/mod.rs +++ b/src/executor/wall_time/profiler/samply/mod.rs @@ -235,26 +235,13 @@ fn bash_in_path_is_compatible() -> anyhow::Result { #[cfg(target_os = "macos")] fn confirm_bash_install() -> anyhow::Result<()> { - use crate::local_logger::IS_TTY; - use console::Term; + use crate::executor::helpers::confirm::confirm_default_yes; - // Non-interactive (CI): just install - if !*IS_TTY { - return Ok(()); - } - - eprintln!( - "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." + let accepted = confirm_default_yes( + "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.", + "Run `brew install bash` now?", ); - eprint!("\nRun `brew install bash` now? [Y/n] "); - let line = Term::stderr().read_line().unwrap_or_default(); - let answer = line.trim(); - - // Default to yes on empty input (just pressing Enter). - if !(answer.is_empty() - || answer.eq_ignore_ascii_case("y") - || answer.eq_ignore_ascii_case("yes")) - { + if !accepted { bail!("Declined; cannot continue without an unsigned bash"); } Ok(())