diff --git a/.github/workflows/check-ubuntu.yml b/.github/workflows/check-ubuntu.yml index 66325b86..1a4cd68a 100644 --- a/.github/workflows/check-ubuntu.yml +++ b/.github/workflows/check-ubuntu.yml @@ -29,5 +29,7 @@ jobs: run: soldr cargo check --workspace --all-targets - name: Clippy run: soldr cargo clippy --workspace --all-targets -- -D warnings + - name: Lint subprocess spawns + run: uv run python ci/find_direct_subprocess.py --fail - name: Test run: soldr cargo test --workspace diff --git a/.github/workflows/lint-subprocess.yml b/.github/workflows/lint-subprocess.yml new file mode 100644 index 00000000..7954e1a6 --- /dev/null +++ b/.github/workflows/lint-subprocess.yml @@ -0,0 +1,28 @@ +name: Lint subprocess spawns + +# Regression guard for FastLED/fbuild#141: every subprocess fbuild +# starts must flow through the fbuild-core::subprocess wrappers (which +# are backed by running-process-core). Direct std::process::Command / +# tokio::process::Command spawns are only allowed when annotated with +# an // allow-direct-spawn: marker. +# +# Keeps pipe-deadlock and containment-drift bugs from creeping back in. + +on: + push: + branches: [main] + pull_request: + branches: [main] + +env: + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" + +jobs: + lint-subprocess: + name: Lint subprocess spawns + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - uses: astral-sh/setup-uv@v3 + - name: Check no unannotated direct Command::new spawns + run: uv run python ci/find_direct_subprocess.py --fail diff --git a/ci/find_direct_subprocess.py b/ci/find_direct_subprocess.py index 4588bd8f..bf542f9a 100644 --- a/ci/find_direct_subprocess.py +++ b/ci/find_direct_subprocess.py @@ -2,26 +2,38 @@ # /// script # requires-python = ">=3.10" # /// -"""Find direct std::process / tokio::process spawn sites in fbuild crates. +"""Linter: forbid direct `std::process::Command::new` / `tokio::process::Command::new` +outside of documented hold-outs. -Goal: every subprocess that fbuild starts should go through -`running-process` (via the wrappers in `fbuild-core::subprocess` and -`fbuild-core::containment`). This script enumerates every remaining -direct `Command::new(...)` site so they can be migrated and eliminated -one PR at a time. +Every subprocess fbuild starts must go through the +`fbuild-core::subprocess` wrappers (which are themselves backed by +[`running-process`](https://github.com/zackees/running-process) so +that containment, concurrent pipe draining, and Windows-specific env +handling are implemented once and cannot drift. + +Tracked by FastLED/fbuild#141. A site is allowlisted by placing this marker on the same line or on the line immediately before the `Command::new(`: // allow-direct-spawn: -When this script reports zero non-allowlisted sites across the whole -workspace, delete it (and the marker comments) and rely on -`running-process` exclusively. Tracked by FastLED/fbuild#. +Intentional hold-outs currently allowed: +- Daemon spawns from CLI/Python/tests (daemon must outlive parent). +- zccache daemon bootstrap (independent lifecycle). +- containment module's own regression tests. +- Integration test harnesses that spawn binaries under test. +- tokio async streaming emulator handlers (QEMU, avr8js/node) where + NativeProcess's blocking API is unsuitable. +- tokio parallel async fan-out in the CLI (IWYU, clang-tidy) inside a + process that has no daemon containment group. + +Run in CI with `--fail` so any new direct spawn without a marker +breaks the build. Usage: uv run python ci/find_direct_subprocess.py # report - uv run python ci/find_direct_subprocess.py --fail # exit 1 if >0 + uv run python ci/find_direct_subprocess.py --fail # exit 1 if any uv run python ci/find_direct_subprocess.py --json # machine output """ @@ -125,7 +137,13 @@ def render_text(hits: list[Hit]) -> str: lines.append(f" allowlisted: {len(allowed)}") if pending: lines.append("") - lines.append("To migrate (no allow-direct-spawn marker):") + lines.append( + "NEW direct spawns without an `allow-direct-spawn: ` marker:" + ) + lines.append( + " (route via fbuild_core::subprocess::{run_command,run_command_passthrough}" + ) + lines.append(" or annotate with a one-line reason — see FastLED/fbuild#141)") for h in pending: rel = h.path.relative_to(REPO_ROOT) lines.append(f" {rel}:{h.line_no}: {h.text.strip()}") diff --git a/crates/fbuild-build/src/compiler.rs b/crates/fbuild-build/src/compiler.rs index c584353f..c22a8c55 100644 --- a/crates/fbuild-build/src/compiler.rs +++ b/crates/fbuild-build/src/compiler.rs @@ -417,13 +417,10 @@ fn compiler_identity(path: &Path) -> String { } fn compiler_version(path: &Path) -> String { - match std::process::Command::new(path) - .arg("-dumpversion") - .output() - { - Ok(output) if output.status.success() => { - String::from_utf8_lossy(&output.stdout).trim().to_string() - } + let program = path.to_string_lossy(); + let args = [program.as_ref(), "-dumpversion"]; + match fbuild_core::subprocess::run_command(&args, None, None, None) { + Ok(output) if output.success() => output.stdout.trim().to_string(), _ => String::new(), } } diff --git a/crates/fbuild-build/src/script_runtime.rs b/crates/fbuild-build/src/script_runtime.rs index 7b7935c4..cec0f8c8 100644 --- a/crates/fbuild-build/src/script_runtime.rs +++ b/crates/fbuild-build/src/script_runtime.rs @@ -7,7 +7,6 @@ use std::collections::HashMap; use std::path::Path; -use std::process::Command; use crate::flag_overlay::{ absolutize_if_relative, values_to_args, BuildOverlay, LanguageExtraFlags, LinkExtraFlags, @@ -89,47 +88,37 @@ pub fn resolve_extra_script_overlay( )) })?; - let mut command = Command::new(&python[0]); - if python.len() > 1 { - command.args(&python[1..]); - } - command - .arg(&harness_path) - .arg(&input_path) - .current_dir(project_dir) - .stdin(std::process::Stdio::null()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()); // Route the spawn through the daemon's containment group so a // daemon crash mid-evaluation doesn't leave a python child running - // in the background. See FastLED/fbuild#32. - let child = fbuild_core::containment::spawn_contained(&mut command).map_err(|e| { - fbuild_core::FbuildError::BuildFailed(format!( - "failed to run extra_scripts runtime via '{}': {}", - python.join(" "), - e - )) - })?; - let output = child.wait_with_output().map_err(|e| { - fbuild_core::FbuildError::BuildFailed(format!( - "failed to collect extra_scripts runtime output: {}", - e - )) - })?; + // in the background. See FastLED/fbuild#32. Uses fbuild_core's + // NativeProcess-backed runner to drain stdout/stderr concurrently. + let harness_path_str = harness_path.to_string_lossy(); + let input_path_str = input_path.to_string_lossy(); + let mut argv: Vec<&str> = python.iter().map(|s| s.as_str()).collect(); + argv.push(harness_path_str.as_ref()); + argv.push(input_path_str.as_ref()); + let output = fbuild_core::subprocess::run_command(&argv, Some(project_dir), None, None) + .map_err(|e| { + fbuild_core::FbuildError::BuildFailed(format!( + "failed to run extra_scripts runtime via '{}': {}", + python.join(" "), + e + )) + })?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + if !output.success() { + let stderr = output.stderr.trim(); return Err(fbuild_core::FbuildError::BuildFailed(format!( "extra_scripts runtime failed: {}\nRecommendation: use --platformio for this project.", if stderr.is_empty() { - format!("process exited with status {}", output.status) + format!("process exited with status {}", output.exit_code) } else { - stderr + stderr.to_string() } ))); } - let runtime: ScriptRuntimeResult = serde_json::from_slice(&output.stdout).map_err(|e| { + let runtime: ScriptRuntimeResult = serde_json::from_str(&output.stdout).map_err(|e| { fbuild_core::FbuildError::BuildFailed(format!( "failed to parse extra_scripts runtime output: {}", e @@ -288,12 +277,10 @@ fn find_python() -> Option> { }; for candidate in candidates { - let mut command = Command::new(candidate[0]); - if candidate.len() > 1 { - command.args(&candidate[1..]); - } - if let Ok(output) = command.arg("--version").output() { - if output.status.success() { + let mut argv: Vec<&str> = candidate.to_vec(); + argv.push("--version"); + if let Ok(output) = fbuild_core::subprocess::run_command(&argv, None, None, None) { + if output.success() { return Some(candidate.iter().map(|s| (*s).to_string()).collect()); } } diff --git a/crates/fbuild-build/src/zccache.rs b/crates/fbuild-build/src/zccache.rs index 42e8efde..d540ec62 100644 --- a/crates/fbuild-build/src/zccache.rs +++ b/crates/fbuild-build/src/zccache.rs @@ -132,6 +132,7 @@ pub fn ensure_running(zccache: &Path) { // is a no-op when it's already running, and either way the zccache // daemon must survive the fbuild daemon — so this spawn stays out // of the containment group. + // allow-direct-spawn: zccache daemon must outlive the fbuild daemon. let mut cmd = std::process::Command::new(zccache); cmd.arg("start") .stdout(std::process::Stdio::null()) diff --git a/crates/fbuild-build/tests/zccache_hit_across_workspace_rename.rs b/crates/fbuild-build/tests/zccache_hit_across_workspace_rename.rs index 6df21cfd..132b6bb4 100644 --- a/crates/fbuild-build/tests/zccache_hit_across_workspace_rename.rs +++ b/crates/fbuild-build/tests/zccache_hit_across_workspace_rename.rs @@ -263,6 +263,7 @@ fn compile_fake_zccache(root: &Path) -> PathBuf { fs::write(&source, FAKE_ZCCACHE).unwrap(); let rustc = env::var_os("RUSTC").unwrap_or_else(|| "rustc".into()); + // allow-direct-spawn: test helper compiling a throwaway rustc binary. let status = Command::new(rustc) .arg(&source) .arg("-o") diff --git a/crates/fbuild-cli/src/daemon_client.rs b/crates/fbuild-cli/src/daemon_client.rs index 57245495..eecb32ef 100644 --- a/crates/fbuild-cli/src/daemon_client.rs +++ b/crates/fbuild-cli/src/daemon_client.rs @@ -858,6 +858,7 @@ pub async fn ensure_daemon_running() -> fbuild_core::Result<()> { /// Spawn a single daemon process instance. async fn spawn_daemon_process() -> fbuild_core::Result<()> { let daemon_exe = "fbuild-daemon"; + // allow-direct-spawn: daemon must outlive the CLI; see INTENTIONALLY DETACHED comment below. let mut cmd = tokio::process::Command::new(daemon_exe); if fbuild_paths::is_dev_mode() { diff --git a/crates/fbuild-cli/src/main.rs b/crates/fbuild-cli/src/main.rs index 719c8c5b..32bed660 100644 --- a/crates/fbuild-cli/src/main.rs +++ b/crates/fbuild-cli/src/main.rs @@ -888,12 +888,11 @@ fn shell_tokenize(s: &str) -> Vec { /// Find the `pio` binary. Checks PATH first, then the fbuild cache. fn find_pio() -> fbuild_core::Result { // Check PATH - if let Ok(output) = std::process::Command::new(if cfg!(windows) { "where" } else { "which" }) - .arg("pio") - .output() - { - if output.status.success() { - let path = String::from_utf8_lossy(&output.stdout) + let locator = if cfg!(windows) { "where" } else { "which" }; + if let Ok(output) = fbuild_core::subprocess::run_command(&[locator, "pio"], None, None, None) { + if output.success() { + let path = output + .stdout .lines() .next() .unwrap_or("") @@ -929,16 +928,14 @@ fn find_pio() -> fbuild_core::Result { /// Run a PlatformIO command with real-time output streaming. fn run_pio_command(args: &[&str]) -> fbuild_core::Result<()> { let pio = find_pio()?; - let status = std::process::Command::new(&pio) - .args(args) - .stdin(std::process::Stdio::inherit()) - .stdout(std::process::Stdio::inherit()) - .stderr(std::process::Stdio::inherit()) - .status() + let pio_str = pio.to_string_lossy(); + let mut argv: Vec<&str> = vec![pio_str.as_ref()]; + argv.extend_from_slice(args); + let code = fbuild_core::subprocess::run_command_passthrough(&argv, None, None, None) .map_err(|e| fbuild_core::FbuildError::Other(format!("failed to run pio: {}", e)))?; - if !status.success() { - std::process::exit(status.code().unwrap_or(1)); + if code != 0 { + std::process::exit(code); } Ok(()) } @@ -1015,23 +1012,22 @@ fn pio_monitor( } fn open_in_browser(url: &str) -> fbuild_core::Result<()> { - let status = if cfg!(target_os = "windows") { - std::process::Command::new("cmd") - .args(["/c", "start", "", url]) - .status() + let args: Vec<&str> = if cfg!(target_os = "windows") { + vec!["cmd", "/c", "start", "", url] } else if cfg!(target_os = "macos") { - std::process::Command::new("open").arg(url).status() + vec!["open", url] } else { - std::process::Command::new("xdg-open").arg(url).status() - } - .map_err(|e| fbuild_core::FbuildError::Other(format!("failed to launch browser: {}", e)))?; + vec!["xdg-open", url] + }; + let output = fbuild_core::subprocess::run_command(&args, None, None, None) + .map_err(|e| fbuild_core::FbuildError::Other(format!("failed to launch browser: {}", e)))?; - if status.success() { + if output.success() { Ok(()) } else { Err(fbuild_core::FbuildError::Other(format!( "browser launcher exited with status {}", - status + output.exit_code ))) } } @@ -1780,7 +1776,9 @@ async fn run_iwyu( } } - // Cache miss — run IWYU + // Cache miss — run IWYU. Parallel async fan-out inside the CLI binary + // (no daemon containment group in this process). + // allow-direct-spawn: parallel async fan-out in CLI; no containment group here. let mut cmd = tokio::process::Command::new(tool.as_ref()); cmd.arg("-p").arg(p_dir.as_ref()); cmd.arg("-Xiwyu").arg("--no_comments"); @@ -2033,6 +2031,7 @@ async fn run_clang_tool( let extra = extra_owned.clone(); let handle = tokio::spawn(async move { let _permit = sem.acquire().await.unwrap(); + // allow-direct-spawn: parallel async fan-out (clang-tidy) in CLI binary. let mut cmd = tokio::process::Command::new(tool.as_ref()); cmd.arg("-p").arg(pd.as_ref()); for arg in &extra { @@ -2756,30 +2755,26 @@ fn run_daemon_kill_all(force: bool) -> fbuild_core::Result<()> { } fn kill_process(pid: u32, force: bool) -> fbuild_core::Result<()> { - let output = if cfg!(windows) { - let mut cmd = std::process::Command::new("taskkill"); + let pid_str = pid.to_string(); + let argv: Vec<&str> = if cfg!(windows) { if force { - cmd.arg("/F"); + vec!["taskkill", "/F", "/PID", &pid_str] + } else { + vec!["taskkill", "/PID", &pid_str] } - cmd.arg("/PID").arg(pid.to_string()); - cmd.output() } else { let signal = if force { "-9" } else { "-TERM" }; - std::process::Command::new("kill") - .arg(signal) - .arg(pid.to_string()) - .output() + vec!["kill", signal, &pid_str] }; - let output = output.map_err(|e| { + let output = fbuild_core::subprocess::run_command(&argv, None, None, None).map_err(|e| { fbuild_core::FbuildError::Other(format!("failed to execute kill command: {}", e)) })?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); + if !output.success() { return Err(fbuild_core::FbuildError::Other(format!( "kill failed: {}", - stderr.trim() + output.stderr.trim() ))); } Ok(()) @@ -2787,15 +2782,22 @@ fn kill_process(pid: u32, force: bool) -> fbuild_core::Result<()> { fn find_daemon_pids() -> fbuild_core::Result> { if cfg!(windows) { - let output = std::process::Command::new("tasklist") - .args(["/FI", "IMAGENAME eq fbuild-daemon.exe", "/FO", "CSV", "/NH"]) - .output() - .map_err(|e| { - fbuild_core::FbuildError::Other(format!("failed to run tasklist: {}", e)) - })?; - let stdout = String::from_utf8_lossy(&output.stdout); + let output = fbuild_core::subprocess::run_command( + &[ + "tasklist", + "/FI", + "IMAGENAME eq fbuild-daemon.exe", + "/FO", + "CSV", + "/NH", + ], + None, + None, + None, + ) + .map_err(|e| fbuild_core::FbuildError::Other(format!("failed to run tasklist: {}", e)))?; let mut pids = Vec::new(); - for line in stdout.lines() { + for line in output.stdout.lines() { // CSV format: "image name","PID","session name","session#","mem usage" if line.contains("fbuild-daemon") { let fields: Vec<&str> = line.split(',').collect(); @@ -2809,12 +2811,15 @@ fn find_daemon_pids() -> fbuild_core::Result> { } Ok(pids) } else { - let output = std::process::Command::new("pgrep") - .args(["-f", "fbuild-daemon"]) - .output() - .map_err(|e| fbuild_core::FbuildError::Other(format!("failed to run pgrep: {}", e)))?; - let stdout = String::from_utf8_lossy(&output.stdout); - let pids: Vec = stdout + let output = fbuild_core::subprocess::run_command( + &["pgrep", "-f", "fbuild-daemon"], + None, + None, + None, + ) + .map_err(|e| fbuild_core::FbuildError::Other(format!("failed to run pgrep: {}", e)))?; + let pids: Vec = output + .stdout .lines() .filter_map(|line| line.trim().parse().ok()) .collect(); diff --git a/crates/fbuild-cli/tests/test_emu_exit_code.rs b/crates/fbuild-cli/tests/test_emu_exit_code.rs index 123b76c1..7e8a3335 100644 --- a/crates/fbuild-cli/tests/test_emu_exit_code.rs +++ b/crates/fbuild-cli/tests/test_emu_exit_code.rs @@ -137,6 +137,7 @@ fn test_emu_exits_non_zero_when_daemon_returns_failure() { // Drive the CLI at the mock daemon. We clear FBUILD_DEV_MODE so the // CLI sticks to prod-mode path assumptions, and pin // FBUILD_DAEMON_PORT so the client calls 127.0.0.1:. + // allow-direct-spawn: integration test driver that invokes the compiled fbuild binary. let output = Command::new(bin) .args([ "test-emu", diff --git a/crates/fbuild-core/src/containment.rs b/crates/fbuild-core/src/containment.rs index bebabf58..dbdf2763 100644 --- a/crates/fbuild-core/src/containment.rs +++ b/crates/fbuild-core/src/containment.rs @@ -436,10 +436,12 @@ mod tests { // able to spawn processes — this preserves behaviour for the // CLI binary and for unit tests. let mut cmd = if cfg!(windows) { + // allow-direct-spawn: this IS the containment module's own test of spawn_contained. let mut c = Command::new("cmd"); c.args(["/C", "echo", "hello"]); c } else { + // allow-direct-spawn: this IS the containment module's own test of spawn_contained. let mut c = Command::new("echo"); c.arg("hello"); c @@ -484,10 +486,12 @@ mod tests { // reproduces the original bug. let build_cmd = || { let mut cmd = if cfg!(windows) { + // allow-direct-spawn: regression test for this module's own containment behaviour. let mut c = Command::new("cmd"); c.args(["/C", "echo", "ok"]); c } else { + // allow-direct-spawn: regression test for this module's own containment behaviour. let mut c = Command::new("echo"); c.arg("ok"); c diff --git a/crates/fbuild-core/src/subprocess.rs b/crates/fbuild-core/src/subprocess.rs index 96d39ab1..162eb88a 100644 --- a/crates/fbuild-core/src/subprocess.rs +++ b/crates/fbuild-core/src/subprocess.rs @@ -1,12 +1,28 @@ -//! Subprocess runner with timeout support. +//! Subprocess runner backed by `running-process-core`. //! -//! On Windows, uses CREATE_NO_WINDOW to prevent compiler processes from -//! spawning visible console windows. +//! Every synchronous spawn in fbuild flows through this module. We use +//! [`running_process_core::NativeProcess`] so that stdout and stderr are +//! drained concurrently from the moment the child starts — the manual +//! drain loop that preceded this module deadlocked the moment a compiler +//! filled its stderr pipe (see FastLED/fbuild#141). +//! +//! On Windows we still: +//! * prepend the executable's directory to PATH so GCC's `cc1plus` can +//! find its sibling DLLs; and +//! * strip MSYS/MSYS2 env vars that would otherwise poison native +//! Windows toolchain binaries. +//! +//! Containment is honoured via `ProcessConfig::containment = Some(...)` +//! when the daemon has installed the global containment group. CLI +//! binaries and unit tests run uncontained just as before. use std::path::Path; -use std::process::{Command, Output, Stdio}; use std::time::Duration; +use running_process_core::{ + CommandSpec, Containment, NativeProcess, ProcessConfig, ProcessError, StderrMode, StdinMode, +}; + use crate::{FbuildError, Result}; /// Output from a subprocess invocation. @@ -31,86 +47,224 @@ pub fn run_command( env: Option<&[(&str, &str)]>, timeout: Option, ) -> Result { + let config = build_config(args, cwd, env, /*capture=*/ true, StdinMode::Null)?; + run_captured(config, args, timeout) +} + +/// Run an external command with inherited stdin/stdout/stderr (no +/// capture). Intended for pass-through cases like the `pio` CLI +/// delegation where users expect the tool's live output. +/// +/// Returns the exit code. +pub fn run_command_passthrough( + args: &[&str], + cwd: Option<&Path>, + env: Option<&[(&str, &str)]>, + timeout: Option, +) -> Result { + let config = build_config(args, cwd, env, /*capture=*/ false, StdinMode::Inherit)?; + let process = NativeProcess::new(config); + process.start().map_err(|e| spawn_err(args, e))?; + match process.wait(timeout) { + Ok(code) => Ok(code), + Err(ProcessError::Timeout) => { + let _ = process.kill(); + Err(FbuildError::Timeout(format!( + "command timed out after {}s", + timeout.map(|d| d.as_secs()).unwrap_or(0) + ))) + } + Err(e) => Err(FbuildError::Other(format!( + "command {:?} failed: {}", + args, e + ))), + } +} + +fn run_captured( + config: ProcessConfig, + args: &[&str], + timeout: Option, +) -> Result { + let process = NativeProcess::new(config); + process.start().map_err(|e| spawn_err(args, e))?; + let exit_code = match process.wait(timeout) { + Ok(code) => code, + Err(ProcessError::Timeout) => { + let _ = process.kill(); + return Err(FbuildError::Timeout(format!( + "command timed out after {}s", + timeout.map(|d| d.as_secs()).unwrap_or(0) + ))); + } + Err(e) => { + return Err(FbuildError::Other(format!( + "command {:?} failed: {}", + args, e + ))) + } + }; + + let stdout = join_lines(process.captured_stdout()); + let stderr = join_lines(process.captured_stderr()); + + Ok(ToolOutput { + stdout, + stderr, + exit_code, + }) +} + +fn build_config( + args: &[&str], + cwd: Option<&Path>, + env: Option<&[(&str, &str)]>, + capture: bool, + stdin_mode: StdinMode, +) -> Result { if args.is_empty() { return Err(FbuildError::Other("empty command".to_string())); } - let mut cmd = build_command(args); + let argv: Vec = args.iter().map(|s| (*s).to_string()).collect(); - if let Some(dir) = cwd { - cmd.current_dir(dir); - } + // Build the environment the child will see. Windows needs PATH + // rewriting (prepend exe dir) and optional MSYS-var stripping; Unix + // only needs overlay vars applied. When no changes are required + // leave `env = None` so the child inherits the parent environment + // verbatim (matching the pre-migration behaviour). + let env_vec = compute_env(args[0], env); - if let Some(vars) = env { - for (k, v) in vars { - cmd.env(k, v); + #[cfg(windows)] + let creationflags = { + const CREATE_NO_WINDOW: u32 = 0x08000000; + Some(CREATE_NO_WINDOW) + }; + #[cfg(not(windows))] + let creationflags: Option = None; + + Ok(ProcessConfig { + command: CommandSpec::Argv(argv), + cwd: cwd.map(|p| p.to_path_buf()), + env: env_vec, + capture, + stderr_mode: StderrMode::Pipe, + creationflags, + create_process_group: false, + stdin_mode, + nice: None, + containment: if crate::containment::is_initialised() { + Some(Containment::Contained) + } else { + None + }, + }) +} + +fn join_lines(lines: Vec>) -> String { + // NativeProcess returns one Vec per line (CR/LF stripped). Join + // with '\n' and add a trailing newline when non-empty so the result + // matches the shape of the previous `String::from_utf8_lossy(&raw)` + // output closely enough for downstream parsers (which mostly call + // `.trim()` or `.lines()`). + if lines.is_empty() { + return String::new(); + } + let mut out = String::new(); + for (idx, line) in lines.iter().enumerate() { + if idx > 0 { + out.push('\n'); } + out.push_str(&String::from_utf8_lossy(line)); } + out.push('\n'); + out +} - if let Some(timeout) = timeout { - run_with_timeout(cmd, timeout) - } else { - run_no_timeout(cmd) - } +fn spawn_err(args: &[&str], e: ProcessError) -> FbuildError { + FbuildError::Other(format!("failed to spawn {:?}: {}", args, e)) } -/// Build a `Command` with platform-specific settings. +/// Build the env vector to pass to the child. /// -/// On Windows: adds the executable's directory to PATH so child processes -/// (like GCC's cc1plus) can find shared libraries (libgcc_s_seh-1.dll, etc.) -/// that live alongside the main binary. Also strips MSYS/MSYS2 environment -/// variables when detected, to prevent interference with native Windows -/// toolchain binaries. -fn build_command(args: &[&str]) -> Command { - let mut cmd = Command::new(args[0]); - cmd.args(&args[1..]); - +/// * On Unix: when `overlay` is Some, merge it into the current +/// environment and return `Some(vec)`. Otherwise return `None` so the +/// child inherits `std::env::vars()` transparently. +/// * On Windows: always construct a full env vector when we need to +/// rewrite PATH (to prepend the exe directory) or strip MSYS vars. +/// This mirrors the behaviour of the pre-migration code, which called +/// `cmd.env("PATH", …)` and `cmd.env_remove(...)` on a command that +/// otherwise inherited the current env. +fn compute_env(program: &str, overlay: Option<&[(&str, &str)]>) -> Option> { #[cfg(windows)] { - use std::os::windows::process::CommandExt; - const CREATE_NO_WINDOW: u32 = 0x08000000; - cmd.creation_flags(CREATE_NO_WINDOW); + let mut env_map: std::collections::BTreeMap = std::env::vars().collect(); - // Add the executable's parent directory to PATH so that child processes - // (e.g., cc1plus launched by g++) can find DLLs in the same bin/ dir. - if let Some(exe_dir) = Path::new(args[0]).parent() { - let exe_dir_str = exe_dir.to_string_lossy(); + // Prepend the executable's directory to PATH so that child + // processes (e.g., cc1plus launched by g++) can find DLLs in + // the same bin/ dir. + if let Some(exe_dir) = Path::new(program).parent() { + let exe_dir_str = exe_dir.to_string_lossy().to_string(); if !exe_dir_str.is_empty() { - let current_path = std::env::var("PATH").unwrap_or_default(); - cmd.env("PATH", format!("{};{}", exe_dir_str, current_path)); + let current_path = env_map + .get("PATH") + .or_else(|| env_map.get("Path")) + .cloned() + .unwrap_or_default(); + env_map.insert( + "PATH".to_string(), + format!("{};{}", exe_dir_str, current_path), + ); } } // Strip MSYS/MSYS2 environment variables that interfere with - // native Windows toolchain binaries finding their internal tools. - if is_msys_environment() { - strip_msys_env(&mut cmd); + // native Windows toolchain binaries finding their internal + // tools. + if is_msys_environment(&env_map) { + strip_msys_env(&mut env_map); + } + + if let Some(vars) = overlay { + for (k, v) in vars { + env_map.insert((*k).to_string(), (*v).to_string()); + } } - } - cmd.stdin(Stdio::null()); - cmd.stdout(Stdio::piped()); - cmd.stderr(Stdio::piped()); - cmd + Some(env_map.into_iter().collect()) + } + #[cfg(not(windows))] + { + let _ = program; + match overlay { + Some(vars) if !vars.is_empty() => { + let mut env_map: std::collections::BTreeMap = + std::env::vars().collect(); + for (k, v) in vars { + env_map.insert((*k).to_string(), (*v).to_string()); + } + Some(env_map.into_iter().collect()) + } + _ => None, + } + } } -/// Check if we're running inside an MSYS/MSYS2 environment. #[cfg(windows)] -fn is_msys_environment() -> bool { - std::env::var("MSYSTEM").is_ok() || std::env::var("MSYS").is_ok() +fn is_msys_environment(env_map: &std::collections::BTreeMap) -> bool { + env_map.contains_key("MSYSTEM") || env_map.contains_key("MSYS") } -/// Strip MSYS-specific environment variables and rebuild PATH without MSYS dirs. +/// Strip MSYS-specific environment variables and rebuild PATH without +/// MSYS dirs. /// -/// Matches Python's `get_pio_safe_env()` in `pio_env.py`: strips variables with -/// prefixes (MSYS*, MINGW*, CHERE*, ORIGINAL_PATH*), exact shell/terminal keys, -/// and PATH entries starting with "/" (MSYS-style paths). +/// Matches Python's `get_pio_safe_env()` in `pio_env.py`: strips vars +/// with prefixes (MSYS*, MINGW*, CHERE*, ORIGINAL_PATH*), exact +/// shell/terminal keys, and PATH entries starting with "/" (MSYS-style +/// paths). #[cfg(windows)] -fn strip_msys_env(cmd: &mut Command) { - // Prefixes to strip (matches Python's strip_prefixes) +fn strip_msys_env(env_map: &mut std::collections::BTreeMap) { let strip_prefixes: &[&str] = &["MSYS", "MINGW", "CHERE", "ORIGINAL_PATH"]; - - // Exact keys to strip (matches Python's strip_exact) let strip_exact: &[&str] = &[ "SHELL", "SHLVL", @@ -130,29 +284,24 @@ fn strip_msys_env(cmd: &mut Command) { "CONFIG_SITE", ]; - // Collect keys to remove (prefix-based + exact) - let keys_to_remove: Vec = std::env::vars() - .filter_map(|(k, _)| { - let should_strip = strip_prefixes.iter().any(|prefix| k.starts_with(prefix)) - || strip_exact.contains(&k.as_str()); - if should_strip { - Some(k) - } else { - None - } + let keys_to_remove: Vec = env_map + .keys() + .filter(|k| { + strip_prefixes.iter().any(|prefix| k.starts_with(prefix)) + || strip_exact.contains(&k.as_str()) }) + .cloned() .collect(); - - for key in &keys_to_remove { - cmd.env_remove(key); + for key in keys_to_remove { + env_map.remove(&key); } - // Clean PATH: remove MSYS-style entries (start with "/") and dirs containing msys/usr - if let Ok(path) = std::env::var("PATH") { + // Clean PATH: remove MSYS-style entries (start with "/") and dirs + // containing msys/usr. + if let Some(path) = env_map.get("PATH").cloned() { let filtered: Vec<&str> = path .split(';') .filter(|p| { - // Remove MSYS-style paths (start with "/") if p.starts_with('/') { return false; } @@ -160,89 +309,7 @@ fn strip_msys_env(cmd: &mut Command) { !lower.contains("\\msys") && !lower.contains("/msys") && !lower.contains("/usr/") }) .collect(); - cmd.env("PATH", filtered.join(";")); - } -} - -fn run_no_timeout(mut cmd: Command) -> Result { - // Route the spawn through the process-wide containment group so the - // resulting child (and any grandchildren it forks) dies with the - // daemon. Falls back to an uncontained spawn when the global group - // is not initialised (CLI binary, unit tests). See FastLED/fbuild#32. - // - // `wait_with_output` drains stdout and stderr concurrently on - // background threads before waiting, which is required to avoid a - // pipe-buffer deadlock when the child writes >64 KB to either stream - // (e.g. ESP32 GCC stderr during a full build). - let child = crate::containment::spawn_contained(&mut cmd)?; - let output = child.wait_with_output()?; - Ok(output_to_tool_output(output)) -} - -fn run_with_timeout(mut cmd: Command, timeout: Duration) -> Result { - // See `run_no_timeout`: route through containment. - let mut child = crate::containment::spawn_contained(&mut cmd)?; - - // Drain stdout and stderr concurrently on background threads. Reading - // serially would deadlock the moment either pipe's ~64 KB OS buffer - // fills — the child then blocks on `write()` forever, and we sit in - // the poll loop until the timeout fires. See FastLED/fbuild regression - // from the containment rewrite. - let stdout_handle = child.stdout.take().map(|mut s| { - std::thread::spawn(move || { - let mut buf = Vec::new(); - std::io::Read::read_to_end(&mut s, &mut buf).ok(); - buf - }) - }); - let stderr_handle = child.stderr.take().map(|mut s| { - std::thread::spawn(move || { - let mut buf = Vec::new(); - std::io::Read::read_to_end(&mut s, &mut buf).ok(); - buf - }) - }); - - let timeout_ms = timeout.as_millis() as u64; - let start = std::time::Instant::now(); - - let status = loop { - match child.try_wait()? { - Some(status) => break status, - None => { - if start.elapsed().as_millis() as u64 > timeout_ms { - let _ = child.kill(); - // Drop reader threads' joins — they'll finish once the - // kill closes the pipes, but we're already erroring. - return Err(FbuildError::Timeout(format!( - "command timed out after {}s", - timeout.as_secs() - ))); - } - std::thread::sleep(Duration::from_millis(50)); - } - } - }; - - let stdout = stdout_handle - .and_then(|h| h.join().ok()) - .unwrap_or_default(); - let stderr = stderr_handle - .and_then(|h| h.join().ok()) - .unwrap_or_default(); - - Ok(ToolOutput { - stdout: String::from_utf8_lossy(&stdout).to_string(), - stderr: String::from_utf8_lossy(&stderr).to_string(), - exit_code: status.code().unwrap_or(-1), - }) -} - -fn output_to_tool_output(output: Output) -> ToolOutput { - ToolOutput { - stdout: String::from_utf8_lossy(&output.stdout).to_string(), - stderr: String::from_utf8_lossy(&output.stderr).to_string(), - exit_code: output.status.code().unwrap_or(-1), + env_map.insert("PATH".to_string(), filtered.join(";")); } } @@ -253,7 +320,7 @@ mod tests { #[test] fn run_echo() { let args = if cfg!(windows) { - vec!["cmd", "/C", "echo", "hello"] + vec!["cmd", "/C", "echo hello"] } else { vec!["echo", "hello"] }; @@ -274,12 +341,6 @@ mod tests { assert!(result.is_err()); } - #[test] - fn run_with_cwd() { - let result = run_command(&["pwd"], Some(Path::new("/")), None, None); - let _ = result; - } - #[test] fn tool_output_success() { let output = ToolOutput { @@ -296,4 +357,17 @@ mod tests { }; assert!(!output.success()); } + + #[test] + fn run_captures_stderr() { + // Verify that stderr is captured independently from stdout. + let args = if cfg!(windows) { + vec!["cmd", "/C", "echo err 1>&2"] + } else { + vec!["sh", "-c", "echo err 1>&2"] + }; + let result = run_command(&args, None, None, None).unwrap(); + assert!(result.success()); + assert!(result.stderr.contains("err"), "got: {:?}", result); + } } diff --git a/crates/fbuild-daemon/src/bin/containment_harness.rs b/crates/fbuild-daemon/src/bin/containment_harness.rs index c5596105..241bd451 100644 --- a/crates/fbuild-daemon/src/bin/containment_harness.rs +++ b/crates/fbuild-daemon/src/bin/containment_harness.rs @@ -45,6 +45,7 @@ fn run_parent() { // Spawn the child via the contained-spawn helper. let self_exe = std::env::current_exe().expect("current_exe"); + // allow-direct-spawn: integration-test harness exercising spawn_contained itself. let mut cmd = std::process::Command::new(&self_exe); cmd.arg("child") .stdin(std::process::Stdio::null()) @@ -96,6 +97,7 @@ fn run_child() { // So here we use a plain `spawn()` and the grandchild still ends // up in the parent's containment group. let self_exe = std::env::current_exe().expect("current_exe"); + // allow-direct-spawn: integration-test harness verifying grandchild containment inheritance. let mut cmd = std::process::Command::new(&self_exe); cmd.arg("grandchild") .stdin(std::process::Stdio::null()) diff --git a/crates/fbuild-daemon/src/handlers/emulator.rs b/crates/fbuild-daemon/src/handlers/emulator.rs index 5615f56c..713ed4f7 100644 --- a/crates/fbuild-daemon/src/handlers/emulator.rs +++ b/crates/fbuild-daemon/src/handlers/emulator.rs @@ -760,6 +760,7 @@ async fn run_avr8js_headless( ))); } + // allow-direct-spawn: tokio streaming emulator; blocking NativeProcess unsuitable. let mut cmd = tokio::process::Command::new(node_path); cmd.arg(script_path) .arg("--hex") @@ -1008,6 +1009,7 @@ async fn run_qemu_process( args: &[String], options: RunQemuOptions<'_>, ) -> fbuild_core::Result { + // allow-direct-spawn: tokio streaming QEMU emulator; blocking NativeProcess unsuitable. let mut cmd = tokio::process::Command::new(qemu_path); cmd.args(args) .stdin(Stdio::null()) diff --git a/crates/fbuild-daemon/tests/port_recovery.rs b/crates/fbuild-daemon/tests/port_recovery.rs index 16d7aa84..5ccc3973 100644 --- a/crates/fbuild-daemon/tests/port_recovery.rs +++ b/crates/fbuild-daemon/tests/port_recovery.rs @@ -35,6 +35,7 @@ fn daemon_rebinds_cleanly_after_hard_kill_with_open_connection() { let bin = env!("CARGO_BIN_EXE_fbuild-daemon"); // 1) Spawn the first daemon. + // allow-direct-spawn: test driver spawns the real fbuild-daemon binary under test. let mut d1 = Command::new(bin) .env("FBUILD_DAEMON_PORT", port.to_string()) .stdout(Stdio::null()) @@ -61,6 +62,7 @@ fn daemon_rebinds_cleanly_after_hard_kill_with_open_connection() { #[cfg(windows)] { // taskkill /F is the Windows equivalent of SIGKILL. + // allow-direct-spawn: test driver hard-killing the daemon process under test. let _ = Command::new("taskkill") .args(["/F", "/PID", &d1.id().to_string()]) .status(); @@ -71,6 +73,7 @@ fn daemon_rebinds_cleanly_after_hard_kill_with_open_connection() { // a short window. Spawn a second daemon and assert it binds. // With a *correct* fix (graceful shutdown + SO_EXCLUSIVEADDRUSE // on Windows) this should succeed without permissive REUSEADDR. + // allow-direct-spawn: test driver spawns the real fbuild-daemon binary under test. let mut d2 = Command::new(bin) .env("FBUILD_DAEMON_PORT", port.to_string()) .stdout(Stdio::piped()) diff --git a/crates/fbuild-daemon/tests/process_containment.rs b/crates/fbuild-daemon/tests/process_containment.rs index 5f7c44e5..8680021d 100644 --- a/crates/fbuild-daemon/tests/process_containment.rs +++ b/crates/fbuild-daemon/tests/process_containment.rs @@ -38,6 +38,7 @@ fn daemon_children_die_when_daemon_dies() { let harness = env!("CARGO_BIN_EXE_containment_harness"); // Start the parent role. + // allow-direct-spawn: integration-test driver invoking the containment harness binary. let mut parent = Command::new(harness) .arg("parent") .stdin(Stdio::null()) @@ -165,6 +166,7 @@ fn kill_hard(pid: u32) -> std::io::Result<()> { fn kill_hard(pid: u32) -> std::io::Result<()> { // `taskkill /F` is the standard Windows hard-kill and works for // arbitrary PIDs without DLL shenanigans. + // allow-direct-spawn: test driver using taskkill to hard-kill a test subject. let status = Command::new("taskkill") .args(["/F", "/PID", &pid.to_string()]) .stdout(Stdio::null()) diff --git a/crates/fbuild-python/src/lib.rs b/crates/fbuild-python/src/lib.rs index 5d0b9ba2..fbf5027e 100644 --- a/crates/fbuild-python/src/lib.rs +++ b/crates/fbuild-python/src/lib.rs @@ -622,6 +622,7 @@ impl Daemon { // Python interpreter process, which has no global containment // group, so `spawn()` is already uncontained; see the matching // comment in fbuild-cli/src/daemon_client.rs. + // allow-direct-spawn: daemon must outlive the Python interpreter. let mut cmd = std::process::Command::new("fbuild-daemon"); if fbuild_paths::is_dev_mode() { cmd.arg("--dev"); @@ -1565,6 +1566,7 @@ impl AsyncDaemon { // INTENTIONALLY DETACHED (FastLED/fbuild#32): see the // matching comment in `Daemon::ensure_running` above. + // allow-direct-spawn: daemon must outlive the Python interpreter. let mut cmd = std::process::Command::new("fbuild-daemon"); if dev_mode { cmd.arg("--dev");