diff --git a/crates/fbuild-cli/src/cli/deploy.rs b/crates/fbuild-cli/src/cli/deploy.rs index 92392f87f..e6b51338e 100644 --- a/crates/fbuild-cli/src/cli/deploy.rs +++ b/crates/fbuild-cli/src/cli/deploy.rs @@ -6,6 +6,7 @@ use crate::daemon_client::{ self, DaemonClient, DeployRequest, MonitorRequest, OperationResponse, TestEmuRequest, }; use crate::output; +use std::io::Write; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum CliEmulatorKind { @@ -214,13 +215,16 @@ pub async fn run_deploy( }; let resp = client.deploy(&req).await?; - if deploy_route == CliDeployRoute::Emulator(CliEmulatorKind::Qemu) - || deploy_route == CliDeployRoute::Emulator(CliEmulatorKind::Avr8js) - { - print_operation_streams(&resp); - } + // Physical deployers also return transport diagnostics in stdout/stderr + // (for example the RP2040 mass-storage and managed-picotool errors). Keep + // those visible instead of replaying streams only for emulator routes. + print_operation_streams(&resp); output::result(&resp.message); if !resp.success { + // process::exit skips normal destructor-based stdio flushing. Preserve + // the daemon's final stdout/stderr when fbuild is piped by automation. + let _ = std::io::stdout().flush(); + let _ = std::io::stderr().flush(); std::process::exit(resp.exit_code); } // Open browser for avr8js only when daemon returned a launch URL (non-headless mode) @@ -303,11 +307,10 @@ pub fn print_operation_streams(resp: &OperationResponse) { .as_deref() .filter(|text| !text.trim().is_empty()) { - // Operation stderr is in-progress diagnostic output from the emulator - // / build subprocess being replayed back. Route it through warn() so - // it shares the tracing level filter with progress(); --quiet hides it, - // colour/format flow through the same subscriber. - output::warn(stderr.trim_end_matches('\n')); + // This is final operation output returned by the daemon, not transient + // progress. It must remain visible under the default tracing filter and + // when the command exits non-zero. + output::diagnostic(stderr.trim_end_matches('\n')); } } diff --git a/crates/fbuild-cli/src/output.rs b/crates/fbuild-cli/src/output.rs index 5561c5791..7cc4ed5b6 100644 --- a/crates/fbuild-cli/src/output.rs +++ b/crates/fbuild-cli/src/output.rs @@ -1,28 +1,27 @@ //! Curated user-facing CLI output API. //! //! FastLED/fbuild#844 ("Bridge pair 12"). All user-facing output from -//! fbuild flows through one of these five functions. The matching +//! fbuild flows through one of these six functions. The matching //! `ban_print_in_production` dylint forbids `println!` / `eprintln!` //! in `crates/fbuild-cli/src/` and `crates/fbuild-build/src/` (this //! file is the sole exemption — it IS the bridge). //! //! The split is intentional: //! -//! | Fn | Sink | Use for | -//! |------------|-------------------|----------------------------------| -//! | `progress` | `tracing::info!` | "Building env X…", spinner-ish | -//! | `result` | `println!` | The final answer — what stdout-pipers see | -//! | `warn` | `tracing::warn!` | Non-fatal recoverables | -//! | `error` | `tracing::error!` | Fatal errors | -//! | `debug` | `tracing::debug!` | `--verbose` / `RUST_LOG=debug` | +//! | Fn | Sink | Use for | +//! |--------------|-------------------|----------------------------------| +//! | `progress` | `tracing::info!` | "Building env X…", spinner-ish | +//! | `result` | `println!` | The final answer — what stdout-pipers see | +//! | `diagnostic` | `eprintln!` | Final operation stderr returned by the daemon | +//! | `warn` | `tracing::warn!` | Non-fatal recoverables | +//! | `error` | `tracing::error!` | Fatal errors | +//! | `debug` | `tracing::debug!` | `--verbose` / `RUST_LOG=debug` | //! -//! Everything except `result` is routed through `tracing` so -//! `--color={auto,always,never}`, `--quiet`, and `--verbose` flags -//! flow through the level filter in one place. `result` stays on -//! `println!` because it's the only output that must survive -//! redirection / piping — `tracing` subscribers swallow it on -//! `--quiet` and that's the right behavior for everything *except* -//! the final answer. +//! Everything except `result` and `diagnostic` is routed through `tracing` so +//! `--color={auto,always,never}`, `--quiet`, and `--verbose` flow through the +//! level filter in one place. Final results and daemon diagnostics use stdout +//! and stderr directly so actionable operation output survives redirection, +//! piping, and `--quiet`. use std::fmt::Display; @@ -40,6 +39,13 @@ pub fn result(msg: impl Display) { println!("{msg}"); } +/// Final operation diagnostics returned by the daemon. Unlike transient +/// tracing warnings, these must remain visible with the default filter, +/// `--quiet`, and when stderr is redirected by automation. +pub fn diagnostic(msg: impl Display) { + eprintln!("{msg}"); +} + /// Non-fatal warning. Emits at `WARN`. Use for situations that don't /// stop the operation but the user should know about — deprecated /// flags, missing-but-not-required files, etc. diff --git a/crates/fbuild-cli/tests/test_emu_exit_code.rs b/crates/fbuild-cli/tests/test_emu_exit_code.rs index 68aa7e863..ac69aa704 100644 --- a/crates/fbuild-cli/tests/test_emu_exit_code.rs +++ b/crates/fbuild-cli/tests/test_emu_exit_code.rs @@ -100,6 +100,7 @@ fn write_response(stream: &mut TcpStream, status_line: &str, body: &str) { /// - GET /api/daemon/info — 200 with `source_mtime=0` so the CLI does not /// try to restart the "stale" daemon. /// - POST /api/test-emu — 500 + structured OperationResponse JSON. +/// - POST /api/deploy — 500 + an actionable structured deploy failure. fn spawn_mock_daemon(stop: Arc) -> u16 { let listener = TcpListener::bind("127.0.0.1:0").expect("bind ephemeral"); listener @@ -112,6 +113,7 @@ fn spawn_mock_daemon(stop: Arc) -> u16 { "{\"status\":\"healthy\",\"uptime_seconds\":1.0,\"version\":\"test\",\"pid\":1,\"source_mtime\":0.0}"; let info_body = "{\"status\":\"healthy\",\"uptime_seconds\":1.0,\"version\":\"test\",\"pid\":1,\"port\":0,\"dev_mode\":true,\"operation_in_progress\":false,\"daemon_state\":\"idle\",\"current_operation\":null,\"client_count\":0,\"spawner_cwd\":null,\"source_mtime\":0.0}"; let fail_body = "{\"success\":false,\"request_id\":\"mock-1\",\"message\":\"mock daemon: simulated test-emu failure\",\"exit_code\":0,\"output_file\":null,\"output_dir\":null,\"launch_url\":null,\"stdout\":null,\"stderr\":null}"; + let deploy_fail_body = "{\"success\":false,\"request_id\":\"mock-2\",\"message\":\"deploy failed\",\"exit_code\":1,\"output_file\":null,\"output_dir\":null,\"launch_url\":null,\"stdout\":null,\"stderr\":\"RP2040 UF2 transfer failed with Windows error 121\"}"; while !stop.load(Ordering::Relaxed) { match listener.accept() { @@ -134,6 +136,12 @@ fn spawn_mock_daemon(stop: Arc) -> u16 { "HTTP/1.1 500 Internal Server Error", fail_body, ); + } else if request_line.starts_with("POST /api/deploy") { + write_response( + &mut stream, + "HTTP/1.1 500 Internal Server Error", + deploy_fail_body, + ); } else { write_response(&mut stream, "HTTP/1.1 404 Not Found", "{}"); } @@ -160,6 +168,48 @@ fn make_test_project() -> tempfile::TempDir { dir } +#[test] +fn deploy_output_survives_nonzero_exit() { + let stop = Arc::new(AtomicBool::new(false)); + let port = spawn_mock_daemon(Arc::clone(&stop)); + + let project = make_test_project(); + let bin = env!("CARGO_BIN_EXE_fbuild"); + + // allow-direct-spawn: integration test driver that invokes the compiled fbuild binary. + let mut cmd = Command::new(bin); + cmd.args([ + "deploy", + project.path().to_str().expect("utf-8 path"), + "-e", + "uno", + "--skip-build", + ]) + .env("FBUILD_DAEMON_PORT", port.to_string()) + .env_remove("FBUILD_DEV_MODE") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let output = run_cli_with_timeout(cmd, Duration::from_secs(30)).expect("CLI wedged"); + + stop.store(true, Ordering::Relaxed); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + let code = output.status.code().unwrap_or(-1); + + assert_ne!(code, 0, "deploy failure must exit non-zero"); + assert!( + stdout.contains("deploy failed"), + "daemon failure message must survive process exit on stdout.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + stderr.contains("RP2040 UF2 transfer failed with Windows error 121"), + "actionable daemon failure must survive process exit on stderr.\nstdout: {stdout}\nstderr: {stderr}" + ); +} + #[test] fn test_emu_exits_non_zero_when_daemon_returns_failure() { let stop = Arc::new(AtomicBool::new(false));