diff --git a/Cargo.lock b/Cargo.lock index f91154f8a..5ed4b4e26 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1232,6 +1232,7 @@ dependencies = [ "async-trait", "libc", "prost", + "reqwest", "running-process", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index 05a545148..284b72198 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,18 @@ exclude = [ "dylints/cli_no_build_deploy_direct_use", "dylints/require_multi_thread_flavor_when_spawning", "dylints/ban_std_sync_mutex_in_async", + # FastLED/fbuild#844 bridge sweep — 10 net-new lints landing + # together with the matching internal bridge APIs. + "dylints/ban_bare_reqwest", + "dylints/ban_std_fs_in_async", + "dylints/ban_tokio_fs_direct_import", + "dylints/ban_std_thread_sleep", + "dylints/ban_std_mpsc_in_async_reachable", + "dylints/ban_tokio_mpsc_direct_import", + "dylints/ban_std_fs_canonicalize", + "dylints/ban_runtime_new_outside_main", + "dylints/ban_poison_panic", + "dylints/ban_print_in_production", ] [workspace.metadata.dylint] diff --git a/ci/hooks/crate_guard.py b/ci/hooks/crate_guard.py index e189548ed..033d4b868 100644 --- a/ci/hooks/crate_guard.py +++ b/ci/hooks/crate_guard.py @@ -70,6 +70,20 @@ "dylints/cli_no_build_deploy_direct_use", "dylints/require_multi_thread_flavor_when_spawning", "dylints/ban_std_sync_mutex_in_async", + # FastLED/fbuild#844 bridge sweep — 10 new dylints landing + # together with the matching internal bridge APIs in + # fbuild-core (http, fs, time, channel, path::canonicalize_existing) + # and fbuild-cli/src/output.rs. + "dylints/ban_bare_reqwest", + "dylints/ban_std_fs_in_async", + "dylints/ban_tokio_fs_direct_import", + "dylints/ban_std_thread_sleep", + "dylints/ban_std_mpsc_in_async_reachable", + "dylints/ban_tokio_mpsc_direct_import", + "dylints/ban_std_fs_canonicalize", + "dylints/ban_runtime_new_outside_main", + "dylints/ban_poison_panic", + "dylints/ban_print_in_production", } ) diff --git a/crates/fbuild-build/src/avr/avr_linker.rs b/crates/fbuild-build/src/avr/avr_linker.rs index 6f9be326a..89065867b 100644 --- a/crates/fbuild-build/src/avr/avr_linker.rs +++ b/crates/fbuild-build/src/avr/avr_linker.rs @@ -181,8 +181,7 @@ impl Linker for AvrLinker { let args = self.build_link_args(objects, &linker_archives, output_dir, &elf_path, extra); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::avr", "link: {}", args.join(" ")); } let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); diff --git a/crates/fbuild-build/src/build_info.rs b/crates/fbuild-build/src/build_info.rs index 4c20eb116..4bebb8055 100644 --- a/crates/fbuild-build/src/build_info.rs +++ b/crates/fbuild-build/src/build_info.rs @@ -288,7 +288,24 @@ pub fn emit_build_info(project_dir: &Path, env_name: &str, info: &BuildInfo) -> continue; } } - if let Err(e) = std::fs::write(path, &json) { + // Atomic write — FastLED/fbuild#844 bridge pair 6. + // `emit_build_info` is a sync function called from sync build + // pipeline code, but always under the daemon's tokio runtime. + // Bridge to the async `write_atomic` via `block_in_place`. Same + // pattern as `fbuild_packages::toolchain::esp32_metadata`. + let write_res = + if let Ok(handle) = tokio::runtime::Handle::try_current() { + tokio::task::block_in_place(|| { + handle.block_on(fbuild_core::fs::write_atomic(path, json.as_bytes())) + }) + } else { + // No runtime — happens in unit tests of this module. + // Fall back to plain `std::fs::write`; the integration + // path always has a runtime so the atomic guarantee is + // preserved where it matters. + std::fs::write(path, &json) + }; + if let Err(e) = write_res { tracing::warn!("failed to write {}: {}", path.display(), e); } } diff --git a/crates/fbuild-build/src/build_output.rs b/crates/fbuild-build/src/build_output.rs index f6fd12fa7..b891491be 100644 --- a/crates/fbuild-build/src/build_output.rs +++ b/crates/fbuild-build/src/build_output.rs @@ -7,12 +7,11 @@ use std::path::Path; use std::time::Instant; +use fbuild_core::channel::UnboundedSender; use fbuild_core::{BuildLog, MemoryRegion, SizeInfo, SymbolMap}; /// Create a [`BuildLog`], optionally wired to a real-time streaming sender. -pub fn create_build_log( - sender: Option>, -) -> BuildLog { +pub fn create_build_log(sender: Option>) -> BuildLog { match sender { Some(s) => BuildLog::with_sender(s), None => BuildLog::new(), @@ -21,7 +20,7 @@ pub fn create_build_log( /// Create a [`BuildLog`] with elapsed-time prefixes from the given epoch. pub fn create_build_log_with_epoch( - sender: Option>, + sender: Option>, epoch: Instant, ) -> BuildLog { match sender { diff --git a/crates/fbuild-build/src/ch32v/ch32v_linker.rs b/crates/fbuild-build/src/ch32v/ch32v_linker.rs index 0ea71d0f8..9f6f6c0ca 100644 --- a/crates/fbuild-build/src/ch32v/ch32v_linker.rs +++ b/crates/fbuild-build/src/ch32v/ch32v_linker.rs @@ -107,8 +107,7 @@ impl Linker for Ch32vLinker { args.extend(extra.libs.iter().cloned()); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::ch32v", "link: {}", args.join(" ")); } let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); diff --git a/crates/fbuild-build/src/esp8266/esp8266_linker.rs b/crates/fbuild-build/src/esp8266/esp8266_linker.rs index 3b13ac199..f391b86c2 100644 --- a/crates/fbuild-build/src/esp8266/esp8266_linker.rs +++ b/crates/fbuild-build/src/esp8266/esp8266_linker.rs @@ -230,8 +230,7 @@ impl Linker for Esp8266Linker { args.push("-Wl,--end-group".to_string()); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::esp8266", "link: {}", args.join(" ")); } let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); diff --git a/crates/fbuild-build/src/generic_arm/arm_linker.rs b/crates/fbuild-build/src/generic_arm/arm_linker.rs index a65da35c7..867937159 100644 --- a/crates/fbuild-build/src/generic_arm/arm_linker.rs +++ b/crates/fbuild-build/src/generic_arm/arm_linker.rs @@ -148,8 +148,7 @@ impl Linker for ArmLinker { let args = self.build_link_args(objects, archives, &elf_path, &map_path, extra); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::generic_arm", "link: {}", args.join(" ")); } // GCC LTO temp dir for MSYS-safe paths — see FastLED/fbuild#261. diff --git a/crates/fbuild-build/src/lib.rs b/crates/fbuild-build/src/lib.rs index f623cfe69..f24208510 100644 --- a/crates/fbuild-build/src/lib.rs +++ b/crates/fbuild-build/src/lib.rs @@ -166,12 +166,15 @@ pub struct BuildParams { pub compiledb_only: bool, /// Optional sender for streaming build log lines in real-time. /// - /// Uses `tokio::sync::mpsc::UnboundedSender` so the orchestrator (running - /// on a tokio runtime) and the WebSocket forwarder can share one channel + /// Uses `fbuild_core::channel::UnboundedSender` (the workspace bridge over + /// `tokio::sync::mpsc::UnboundedSender`) so the orchestrator (running on a + /// tokio runtime) and the WebSocket forwarder can share one channel /// without a sync→async bridge — `UnboundedSender::send` is sync and safe /// to call from blocking code, while the receive side is awaited from the - /// async daemon handler (fbuild#818). - pub log_sender: Option>, + /// async daemon handler (fbuild#818). Routed via `fbuild_core::channel` + /// to satisfy the workspace `ban_tokio_mpsc_direct_import` dylint + /// (fbuild#844). + pub log_sender: Option>, /// When true, run symbol-level memory analysis after linking. pub symbol_analysis: bool, /// Optional path to write the symbol analysis report to. diff --git a/crates/fbuild-build/src/nrf52/nrf52_linker.rs b/crates/fbuild-build/src/nrf52/nrf52_linker.rs index 5d695714d..3e4ca7715 100644 --- a/crates/fbuild-build/src/nrf52/nrf52_linker.rs +++ b/crates/fbuild-build/src/nrf52/nrf52_linker.rs @@ -114,8 +114,7 @@ impl Linker for Nrf52Linker { args.extend(extra.libs.iter().cloned()); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::nrf52", "link: {}", args.join(" ")); } // GCC LTO temp dir for MSYS-safe paths — see FastLED/fbuild#261. diff --git a/crates/fbuild-build/src/renesas/renesas_linker.rs b/crates/fbuild-build/src/renesas/renesas_linker.rs index a61ebf913..850e02936 100644 --- a/crates/fbuild-build/src/renesas/renesas_linker.rs +++ b/crates/fbuild-build/src/renesas/renesas_linker.rs @@ -119,8 +119,7 @@ impl Linker for RenesasLinker { args.extend(extra.libs.iter().cloned()); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::renesas", "link: {}", args.join(" ")); } // GCC LTO temp dir for MSYS-safe paths — see FastLED/fbuild#261. diff --git a/crates/fbuild-build/src/sam/sam_linker.rs b/crates/fbuild-build/src/sam/sam_linker.rs index 33685a63e..f38ff297b 100644 --- a/crates/fbuild-build/src/sam/sam_linker.rs +++ b/crates/fbuild-build/src/sam/sam_linker.rs @@ -135,8 +135,7 @@ impl Linker for SamLinker { args.extend(extra.libs.iter().cloned()); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::sam", "link: {}", args.join(" ")); } // GCC LTO temp dir for MSYS-safe paths — see FastLED/fbuild#261. diff --git a/crates/fbuild-build/src/script_runtime.rs b/crates/fbuild-build/src/script_runtime.rs index a7590d135..aa67f7ff4 100644 --- a/crates/fbuild-build/src/script_runtime.rs +++ b/crates/fbuild-build/src/script_runtime.rs @@ -69,12 +69,16 @@ pub async fn resolve_extra_script_overlay( ) })?; - let temp_dir = tempfile::tempdir().map_err(|e| { - fbuild_core::FbuildError::BuildFailed(format!( - "failed to create temporary directory for extra_scripts runtime: {}", - e - )) - })?; + // FastLED/fbuild#844 (bridge pair 10): rooted under + // `~/.fbuild/{dev|prod}/tmp/script-runtime/` so the harness sidecar + // is reachable from a single user-visible directory. + let temp_dir = + tempfile::tempdir_in(fbuild_paths::temp_subdir("script-runtime")).map_err(|e| { + fbuild_core::FbuildError::BuildFailed(format!( + "failed to create temporary directory for extra_scripts runtime: {}", + e + )) + })?; let harness_path = temp_dir.path().join("fbuild_lite_scons_harness.py"); let input_path = temp_dir.path().join("input.json"); std::fs::write(&harness_path, HARNESS).map_err(|e| { diff --git a/crates/fbuild-build/src/silabs/silabs_linker.rs b/crates/fbuild-build/src/silabs/silabs_linker.rs index 2fc8adf89..f67a22b4a 100644 --- a/crates/fbuild-build/src/silabs/silabs_linker.rs +++ b/crates/fbuild-build/src/silabs/silabs_linker.rs @@ -122,8 +122,7 @@ impl Linker for SilabsLinker { args.push("-Wl,--end-group".to_string()); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::silabs", "link: {}", args.join(" ")); } // GCC LTO temp dir for MSYS-safe paths — see FastLED/fbuild#261. diff --git a/crates/fbuild-build/src/teensy/teensy_linker.rs b/crates/fbuild-build/src/teensy/teensy_linker.rs index 4547e5260..40182a9cc 100644 --- a/crates/fbuild-build/src/teensy/teensy_linker.rs +++ b/crates/fbuild-build/src/teensy/teensy_linker.rs @@ -139,8 +139,7 @@ impl Linker for TeensyLinker { let args = self.build_link_args(objects, archives, &elf_path, extra); if self.verbose { - eprintln!("link: {}", args.join(" ")); - tracing::info!("link: {}", args.join(" ")); + tracing::debug!(target: "fbuild_build::linker::teensy", "link: {}", args.join(" ")); } // Redirect GCC LTO temp files into a forward-slashed, fbuild-owned diff --git a/crates/fbuild-cli/src/cli/bloat_lookup.rs b/crates/fbuild-cli/src/cli/bloat_lookup.rs index aedff1701..fbba0ed0f 100644 --- a/crates/fbuild-cli/src/cli/bloat_lookup.rs +++ b/crates/fbuild-cli/src/cli/bloat_lookup.rs @@ -17,6 +17,8 @@ use fbuild_core::symbol_analysis::{ }; use fbuild_core::{FbuildError, Result}; +use crate::output; + use super::symbols_cmd::resolve_tool_paths_public; #[allow(clippy::too_many_arguments)] @@ -81,30 +83,34 @@ pub async fn run_bloat_lookup( match report.find_symbol(&query) { SymbolLookup::Hit(sym) => { if json { - let out = serde_json::to_string_pretty(sym) + let rendered = serde_json::to_string_pretty(sym) .map_err(|e| FbuildError::Other(format!("json serialize: {e}")))?; - println!("{out}"); + output::result(rendered); } else { - print!("{}", format_symbol_block(sym, &report)); + // format_symbol_block already terminates lines with '\n'; strip + // the trailing newline so result()'s newline doesn't double up. + output::result(format_symbol_block(sym, &report).trim_end_matches('\n')); } Ok(()) } SymbolLookup::Ambiguous(candidates) => { if json { let payload: Vec<&FineGrainedSymbol> = candidates; - let out = serde_json::to_string_pretty(&payload) + let rendered = serde_json::to_string_pretty(&payload) .map_err(|e| FbuildError::Other(format!("json serialize: {e}")))?; - println!("{out}"); + output::result(rendered); } else { - eprintln!( + output::error(format!( "ambiguous: {} symbols match `{}`:", candidates.len(), query_str - ); + )); for c in &candidates { - eprintln!(" {} B {}", c.size, c.demangled); + output::error(format!(" {} B {}", c.size, c.demangled)); } - eprintln!("re-run with a more specific --symbol value (or use --symbol-mangled)."); + output::error( + "re-run with a more specific --symbol value (or use --symbol-mangled).", + ); } Err(FbuildError::BuildFailed(format!( "ambiguous symbol query `{query_str}`" diff --git a/crates/fbuild-cli/src/cli/bringup.rs b/crates/fbuild-cli/src/cli/bringup.rs index 6f1183460..6fb60d287 100644 --- a/crates/fbuild-cli/src/cli/bringup.rs +++ b/crates/fbuild-cli/src/cli/bringup.rs @@ -33,6 +33,8 @@ use clap::Args; use fbuild_core::{FbuildError, Result}; use fbuild_serial::boards::{family_for_vid_pid, vcom_for_env, BoardFamily}; +use crate::output; + /// `fbuild bringup` CLI args. Pluggable RPC method / payload / /// expected-result, with per-board defaults supplied via the env /// argument's board JSON (`bringup.{method, payload, expected_result}`). @@ -161,7 +163,9 @@ impl std::fmt::Display for BringupResult { /// Top-level entry — dispatcher calls this. pub fn run_bringup(args: BringupArgs) -> Result<()> { let result = run_bringup_inner(&args)?; - println!("{result}"); + // BringupResult's Display impl already terminates with '\n'; strip the + // trailing newline so result()'s newline doesn't double up. + output::result(result.to_string().trim_end_matches('\n')); if !result.is_passing() && !args.dry_run { return Err(FbuildError::DeployFailed(format!( "bring-up failed: {result}" diff --git a/crates/fbuild-cli/src/cli/build.rs b/crates/fbuild-cli/src/cli/build.rs index dff0b2e0f..9486ed3ee 100644 --- a/crates/fbuild-cli/src/cli/build.rs +++ b/crates/fbuild-cli/src/cli/build.rs @@ -1,6 +1,7 @@ //! `fbuild build` handler and a couple of path / browser helpers. use crate::daemon_client::{self, BuildRequest, DaemonClient}; +use crate::output; pub async fn open_in_browser(url: &str) -> fbuild_core::Result<()> { let args: Vec<&str> = if cfg!(target_os = "windows") { @@ -67,14 +68,14 @@ pub async fn run_build( .as_deref() .or_else(|| config.get_default_environment()) .unwrap_or("default"); - println!("Environment: {}", env_name); - println!("Daemon is running. Dry-run complete."); + output::result(format!("Environment: {}", env_name)); + output::result("Daemon is running. Dry-run complete."); return Ok(()); } let client = DaemonClient::new(); if verbose { - eprintln!("{}", daemon_client::runtime_diagnostic()); + output::debug(daemon_client::runtime_diagnostic()); } let profile = if release { @@ -87,10 +88,10 @@ pub async fn run_build( let generate_compiledb = target.as_deref() == Some("compiledb"); if generate_compiledb { let env_label = environment.as_deref().unwrap_or("default"); - println!( + output::progress(format!( "Generating compile_commands.json for environment: {}...", env_label - ); + )); } let (caller_pid, caller_cwd) = daemon_client::caller_info(); let req = BuildRequest { @@ -121,7 +122,7 @@ pub async fn run_build( let resp = client.build_streaming(&req).await?; let stream_elapsed = stream_start.elapsed(); if !resp.message.is_empty() { - println!("{}", resp.message); + output::result(&resp.message); } if perf_enabled { let summary = format!( @@ -131,18 +132,21 @@ pub async fn run_build( cli_start.elapsed().as_millis(), ); tracing::info!(target: "fbuild_cli::perf_log", "{}", summary); - eprintln!("{}", summary); + output::debug(&summary); } if !resp.success { if !verbose { - eprintln!("{}", daemon_client::runtime_diagnostic()); + output::error(daemon_client::runtime_diagnostic()); } std::process::exit(resp.exit_code); } if generate_compiledb { let db_path = std::path::Path::new(&project_dir).join("compile_commands.json"); if db_path.exists() { - println!("compile_commands.json written to {}", db_path.display()); + output::result(format!( + "compile_commands.json written to {}", + db_path.display() + )); } } Ok(()) diff --git a/crates/fbuild-cli/src/cli/clang_tools.rs b/crates/fbuild-cli/src/cli/clang_tools.rs index 8f77ec5e2..f7a653f8d 100644 --- a/crates/fbuild-cli/src/cli/clang_tools.rs +++ b/crates/fbuild-cli/src/cli/clang_tools.rs @@ -2,6 +2,8 @@ //! `iwyu` (include-what-you-use), plus a couple of cache + filter helpers //! used by them. +use crate::output; + use super::build::{normalize_path, run_build}; /// Run IWYU (include-what-you-use) analysis with ESP32 cross-compilation support. @@ -26,15 +28,18 @@ pub async fn run_iwyu( fbuild_packages::toolchain::ClangComponentKind::Iwyu, ); let tool_path = component.get_binary("include-what-you-use").await?; - println!("Using include-what-you-use: {}", tool_path.display()); + output::progress(format!( + "Using include-what-you-use: {}", + tool_path.display() + )); // Step 2: Generate compile_commands.json via fbuild daemon (skip if it already exists) let project_path = std::path::Path::new(&project_dir); let db_path = project_path.join("compile_commands.json"); if db_path.exists() { - println!("Using existing compile_commands.json"); + output::progress("Using existing compile_commands.json"); } else { - println!("Generating compile_commands.json..."); + output::progress("Generating compile_commands.json..."); run_build( project_dir.clone(), environment.clone(), @@ -95,17 +100,20 @@ pub async fn run_iwyu( .collect(); if source_entries.is_empty() { - println!("No source files found in compile_commands.json under src/"); + output::result("No source files found in compile_commands.json under src/"); return Ok(()); } // Step 4: Find GCC toolchain builtin include dirs let gcc_includes = fbuild_packages::toolchain::clang::find_gcc_builtin_include_dirs(); if !gcc_includes.is_empty() { - println!("Found {} GCC builtin include dir(s)", gcc_includes.len()); + output::progress(format!( + "Found {} GCC builtin include dir(s)", + gcc_includes.len() + )); if verbose { for inc in &gcc_includes { - println!(" {}", inc.display()); + output::debug(format!(" {}", inc.display())); } } } @@ -203,13 +211,16 @@ pub async fn run_iwyu( let iwyu_json = serde_json::to_string_pretty(&iwyu_entries).map_err(|e| { fbuild_core::FbuildError::Other(format!("failed to serialize IWYU compile database: {}", e)) })?; - std::fs::write(&iwyu_db_path, iwyu_json).map_err(|e| { - fbuild_core::FbuildError::Other(format!( - "failed to write {}: {}", - iwyu_db_path.display(), - e - )) - })?; + // Atomic write — FastLED/fbuild#844 bridge pair 6 (IWYU compile DB). + fbuild_core::fs::write_atomic(&iwyu_db_path, iwyu_json) + .await + .map_err(|e| { + fbuild_core::FbuildError::Other(format!( + "failed to write {}: {}", + iwyu_db_path.display(), + e + )) + })?; // Step 6: Set up zccache-style content-addressed cache for IWYU results. // Cache key = blake3(source_content + iwyu_entry_json) per file. let cache_dir = iwyu_dir_path.join("cache"); @@ -227,10 +238,10 @@ pub async fn run_iwyu( }) .collect(); - println!( + output::progress(format!( "Running include-what-you-use on {} source file(s)...", source_entries.len() - ); + )); // Step 7: Run IWYU in parallel with caching let jobs = std::thread::available_parallelism() @@ -341,7 +352,10 @@ pub async fn run_iwyu( Ok(stderr) => { let filtered = filter_iwyu_output(&stderr, &src_path); if !filtered.trim().is_empty() { - print!("{}", filtered); + // filter_iwyu_output already terminates each line with '\n'; strip + // the final trailing newline so result()'s own newline doesn't + // double up. + output::result(filtered.trim_end_matches('\n')); total_suggestions += filtered .lines() .filter(|l| l.contains("should add") || l.contains("should remove")) @@ -349,20 +363,23 @@ pub async fn run_iwyu( } } Err(e) => { - eprintln!("failed to run include-what-you-use on {}: {}", file, e); + output::error(format!( + "failed to run include-what-you-use on {}: {}", + file, e + )); failed_files.push(file); } } } - println!("\n--- include-what-you-use summary ---"); - println!("Suggestions: {}", total_suggestions); - println!( + output::result("\n--- include-what-you-use summary ---"); + output::result(format!("Suggestions: {}", total_suggestions)); + output::result(format!( "Cache: {} hit(s), {} miss(es)", cache_hits, cache_misses - ); + )); if !failed_files.is_empty() { - println!("Failed: {} file(s)", failed_files.len()); + output::result(format!("Failed: {} file(s)", failed_files.len())); } if !failed_files.is_empty() { @@ -473,10 +490,14 @@ pub async fn run_clang_tool( // Step 1: Ensure tool is installed let component = fbuild_packages::toolchain::ClangComponent::new(kind); let tool_path = component.get_binary(binary_name).await?; - println!("Using {}: {}", binary_name, tool_path.display()); + output::progress(format!( + "Using {}: {}", + binary_name, + tool_path.display() + )); // Step 2: Generate compile_commands.json via fbuild daemon - println!("Generating compile_commands.json..."); + output::progress("Generating compile_commands.json..."); run_build( project_dir.clone(), environment.clone(), @@ -521,20 +542,20 @@ pub async fn run_clang_tool( .collect(); if source_files.is_empty() { - println!("No source files found in compile_commands.json under src/"); + output::result("No source files found in compile_commands.json under src/"); return Ok(()); } - println!( + output::progress(format!( "Running {} on {} source file(s)...", binary_name, source_files.len() - ); + )); // Step 4: Run tool in parallel with ncpus * 2 let jobs = std::thread::available_parallelism() .map(|n| n.get() * 2) .unwrap_or(4); - println!("Using {} parallel jobs", jobs); + output::progress(format!("Using {} parallel jobs", jobs)); let semaphore = std::sync::Arc::new(tokio::sync::Semaphore::new(jobs)); let project_dir_arc = std::sync::Arc::new(project_dir.clone()); @@ -588,12 +609,14 @@ pub async fn run_clang_tool( .await .map_err(|e| fbuild_core::FbuildError::Other(format!("task join error: {}", e)))?; match result { - Ok(output) => { - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + Ok(out) => { + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); let combined = format!("{}{}", stdout, stderr); if !combined.trim().is_empty() { - print!("{}", combined); + // clang-tidy already terminates each line with '\n'; strip the + // final trailing newline so result()'s newline doesn't double up. + output::result(combined.trim_end_matches('\n')); } for line in combined.lines() { if line.contains("warning:") { @@ -605,17 +628,17 @@ pub async fn run_clang_tool( } } Err(e) => { - eprintln!("failed to run {} on {}: {}", binary_name, file, e); + output::error(format!("failed to run {} on {}: {}", binary_name, file, e)); failed_files.push(file); } } } - println!("\n--- {} summary ---", binary_name); - println!("Warnings: {}", total_warnings); - println!("Errors: {}", total_errors); + output::result(format!("\n--- {} summary ---", binary_name)); + output::result(format!("Warnings: {}", total_warnings)); + output::result(format!("Errors: {}", total_errors)); if !failed_files.is_empty() { - println!("Failed: {} file(s)", failed_files.len()); + output::result(format!("Failed: {} file(s)", failed_files.len())); } if total_errors > 0 || !failed_files.is_empty() { diff --git a/crates/fbuild-cli/src/cli/clangd_config.rs b/crates/fbuild-cli/src/cli/clangd_config.rs index 7f154e382..72c6241c5 100644 --- a/crates/fbuild-cli/src/cli/clangd_config.rs +++ b/crates/fbuild-cli/src/cli/clangd_config.rs @@ -9,6 +9,8 @@ //! `.vscode/settings.json`, and `.vscode/extensions.json` at the project root. //! It does not touch the build pipeline. +use crate::output; + use super::build::{normalize_path, run_build}; /// Generate clangd / VS Code configuration for the project's default env. @@ -22,14 +24,14 @@ pub async fn run_clangd_config( // Step 1: Resolve the environment name (explicit -e wins, else default). let env_name = resolve_env_name(project_path, environment)?; - println!("Using environment: {}", env_name); + output::progress(format!("Using environment: {}", env_name)); // Step 2: Ensure compile_commands.json exists at the project root. let db_path = project_path.join("compile_commands.json"); if db_path.exists() { - println!("Using existing compile_commands.json"); + output::progress("Using existing compile_commands.json"); } else { - println!("Generating compile_commands.json..."); + output::progress("Generating compile_commands.json..."); run_build( project_dir.clone(), Some(env_name.clone()), @@ -56,7 +58,7 @@ pub async fn run_clangd_config( // Step 3: Pull the real cross-compiler path out of the compile database. let compiler = extract_compiler_path(&db_path)?; let query_driver = compiler_query_driver_glob(&compiler); - println!("Detected compiler: {}", compiler); + output::progress(format!("Detected compiler: {}", compiler)); // Step 4: Write .clangd let clangd_path = project_path.join(".clangd"); @@ -80,35 +82,38 @@ pub async fn run_clangd_config( })?; // Step 6: Write .vscode/extensions.json only if it does not already exist. + // Atomic write — FastLED/fbuild#844 bridge pair 6 (state-file write). let extensions_path = vscode_dir.join("extensions.json"); let wrote_extensions = if extensions_path.exists() { false } else { - std::fs::write(&extensions_path, render_extensions_json()).map_err(|e| { - fbuild_core::FbuildError::Other(format!( - "failed to write {}: {}", - extensions_path.display(), - e - )) - })?; + fbuild_core::fs::write_atomic(&extensions_path, render_extensions_json()) + .await + .map_err(|e| { + fbuild_core::FbuildError::Other(format!( + "failed to write {}: {}", + extensions_path.display(), + e + )) + })?; true }; // Step 7: Summary. - println!("\nWrote clangd configuration:"); - println!(" {}", db_path.display()); - println!(" {}", clangd_path.display()); - println!(" {}", settings_path.display()); + output::result("\nWrote clangd configuration:"); + output::result(format!(" {}", db_path.display())); + output::result(format!(" {}", clangd_path.display())); + output::result(format!(" {}", settings_path.display())); if wrote_extensions { - println!(" {}", extensions_path.display()); + output::result(format!(" {}", extensions_path.display())); } else { - println!( + output::result(format!( " {} (left unchanged — already exists)", extensions_path.display() - ); + )); } - println!("\nInstall the clangd extension (llvm-vs-code-extensions.vscode-clangd),"); - println!("then run \"clangd: Restart language server\" in VS Code to pick up the config."); + output::result("\nInstall the clangd extension (llvm-vs-code-extensions.vscode-clangd),"); + output::result("then run \"clangd: Restart language server\" in VS Code to pick up the config."); Ok(()) } diff --git a/crates/fbuild-cli/src/cli/compile_many.rs b/crates/fbuild-cli/src/cli/compile_many.rs index 45b92678f..accce6fe6 100644 --- a/crates/fbuild-cli/src/cli/compile_many.rs +++ b/crates/fbuild-cli/src/cli/compile_many.rs @@ -1,6 +1,8 @@ //! `fbuild compile-many` (FastLED/fbuild#238) and the thin `fbuild ci` //! adapter that maps `pio ci` flags onto it. +use crate::output; + /// Separator used to join `PLATFORMIO_LIB_EXTRA_DIRS` entries. /// /// PlatformIO follows `PATH`-style conventions: ';' on Windows, ':' elsewhere. @@ -143,21 +145,21 @@ pub async fn run_compile_many(args: CompileManyArgs) -> fbuild_core::Result<()> let effective_sketch = req .sketch_jobs .unwrap_or_else(fbuild_build::compile_many::default_sketch_jobs); - println!( + output::progress(format!( "compile-many: board={} sketches={} framework_jobs={} sketch_jobs={}", board, sketches.len(), effective_framework, effective_sketch, - ); + )); // `compile_many` is async (driving per-stage tokio fanout). Await it // directly — the runtime is already multi-threaded. let result = compile_many(req).await?; // Per-sketch result map suitable for the bench summary. - println!(); - println!("compile-many results:"); + output::result(""); + output::result("compile-many results:"); for r in &result.results { let stage_label = match r.stage { Stage::Stage1Framework => "stage1", @@ -169,7 +171,7 @@ pub async fn run_compile_many(args: CompileManyArgs) -> fbuild_core::Result<()> .as_ref() .map(|p| p.display().to_string()) .unwrap_or_else(|| "-".to_string()); - println!( + output::result(format!( " [{}] {} ({:.2}s) {} log={} {}", stage_label, status, @@ -177,25 +179,24 @@ pub async fn run_compile_many(args: CompileManyArgs) -> fbuild_core::Result<()> r.sketch.display(), log_str, r.message, - ); + )); } - println!(); - println!( + output::result(""); + output::result(format!( "compile-many summary: stage1={}/{:.2}s stage2={}/{:.2}s total={:.2}s", result.stage1_count, result.stage1_secs, result.stage2_count, result.stage2_secs, result.total_secs, - ); + )); if diag_stage2 { for r in result .results .iter() .filter(|r| r.stage == Stage::Stage2Sketch) { - println!( - "{}", + output::result( serde_json::json!({ "type": "stage2", "worker": r.worker_index, @@ -207,6 +208,7 @@ pub async fn run_compile_many(args: CompileManyArgs) -> fbuild_core::Result<()> "build_secs": r.build_time_secs, "log": r.log_path.as_ref().map(|p| p.display().to_string()), }) + .to_string(), ); } } diff --git a/crates/fbuild-cli/src/cli/daemon_cmd.rs b/crates/fbuild-cli/src/cli/daemon_cmd.rs index ab83de42f..269279e89 100644 --- a/crates/fbuild-cli/src/cli/daemon_cmd.rs +++ b/crates/fbuild-cli/src/cli/daemon_cmd.rs @@ -3,6 +3,7 @@ //! with the purge subcommand. use crate::daemon_client::{self, DaemonClient}; +use crate::output; use super::args::DaemonAction; use super::purge::format_size; @@ -13,7 +14,7 @@ pub async fn run_daemon(action: DaemonAction) -> fbuild_core::Result<()> { match action { DaemonAction::Stop => { if !client.health().await { - println!("daemon is not running"); + output::result("daemon is not running"); return Ok(()); } client.shutdown().await?; @@ -21,57 +22,66 @@ pub async fn run_daemon(action: DaemonAction) -> fbuild_core::Result<()> { for _ in 0..50 { tokio::time::sleep(std::time::Duration::from_millis(100)).await; if !client.health().await { - println!("daemon stopped"); + output::result("daemon stopped"); return Ok(()); } } - println!("daemon stop requested (may still be shutting down)"); + output::result("daemon stop requested (may still be shutting down)"); } DaemonAction::Status => { if client.health().await { match client.daemon_info().await { Ok(info) => { let uptime = format_uptime(info.uptime_seconds); - println!("daemon is running at {}", fbuild_paths::get_daemon_url()); - println!(" PID: {}", info.pid); - println!(" Port: {}", info.port); - println!(" Uptime: {}", uptime); - println!(" Version: {}", info.version); - println!(" Mode: {}", if info.dev_mode { "dev" } else { "prod" }); - println!(" State: {}", info.daemon_state); + output::result(format!( + "daemon is running at {}", + fbuild_paths::get_daemon_url() + )); + output::result(format!(" PID: {}", info.pid)); + output::result(format!(" Port: {}", info.port)); + output::result(format!(" Uptime: {}", uptime)); + output::result(format!(" Version: {}", info.version)); + output::result(format!( + " Mode: {}", + if info.dev_mode { "dev" } else { "prod" } + )); + output::result(format!(" State: {}", info.daemon_state)); if info.operation_in_progress { if let Some(ref op) = info.current_operation { - println!(" Operation: {}", op); + output::result(format!(" Operation: {}", op)); } else { - println!(" Operation: (in progress)"); + output::result(" Operation: (in progress)"); } } if let Some(ref install) = info.dependency_install { - println!( + output::result(format!( " Install: {} {} ({}, {})", install.name, install.version.as_deref().unwrap_or(""), install.phase, install.role - ); - println!(" {}", install.message); + )); + output::result(format!(" {}", install.message)); } if info.client_count > 0 { - println!(" Clients: {}", info.client_count); + output::result(format!(" Clients: {}", info.client_count)); } if let Some(ref cwd) = info.spawner_cwd { - println!(" Spawned from: {}", cwd); + output::result(format!(" Spawned from: {}", cwd)); } if let Some(mtime) = info.source_mtime { - println!(" Binary mtime: {:.0}", mtime); + output::result(format!(" Binary mtime: {:.0}", mtime)); } } Err(_) => { - println!("daemon is running at {}", fbuild_paths::get_daemon_url()); + output::result(format!( + "daemon is running at {}", + fbuild_paths::get_daemon_url() + )); } } } else { - println!("daemon is not running"); + output::result("daemon is not running"); } } DaemonAction::Restart => { @@ -87,7 +97,7 @@ pub async fn run_daemon(action: DaemonAction) -> fbuild_core::Result<()> { } // Start fresh daemon_client::ensure_daemon_running().await?; - println!("daemon restarted"); + output::result("daemon restarted"); } DaemonAction::List => { run_daemon_list(&client).await?; @@ -197,56 +207,74 @@ fn run_daemon_running_process(json: bool) -> fbuild_core::Result<()> { "fallback_reason": fallback_reason, }, }); - let output = serde_json::to_string_pretty(&payload) + let rendered = serde_json::to_string_pretty(&payload) .map_err(|e| fbuild_core::FbuildError::Other(format!("json serialize: {e}")))?; - println!("{output}"); + output::result(rendered); return Ok(()); } - println!("running-process broker adoption"); - println!(" Service: {}", rp::SERVICE_NAME); - println!(" Isolation (local): {}", rp::BROKER_ISOLATION); - println!( + output::result("running-process broker adoption"); + output::result(format!(" Service: {}", rp::SERVICE_NAME)); + output::result(format!(" Isolation (local): {}", rp::BROKER_ISOLATION)); + output::result(format!( " Isolation (CI): EXPLICIT_INSTANCE \"{}\"", rp::CI_TRUSTED_INSTANCE - ); - println!(" Min version: {}", rp::MIN_VERSION); - println!( + )); + output::result(format!(" Min version: {}", rp::MIN_VERSION)); + output::result(format!( " Payload protocol: {:#06X}", rp::FBUILD_PAYLOAD_PROTOCOL - ); - println!(" fbuild proto version: {}", rp::FBUILD_PROTOCOL_VERSION); - println!(" Cache schema version: {}", rp::CACHE_SCHEMA_VERSION); - println!(" Encoding lane: json-direct + prost-broker (parity-tested)"); - println!( + )); + output::result(format!( + " fbuild proto version: {}", + rp::FBUILD_PROTOCOL_VERSION + )); + output::result(format!( + " Cache schema version: {}", + rp::CACHE_SCHEMA_VERSION + )); + output::result(" Encoding lane: json-direct + prost-broker (parity-tested)"); + output::result(format!( " Service definition: {}", service_definition_path.display() - ); - println!(" Selected path: {selected_path}"); - println!(" Mode: {live_mode}"); + )); + output::result(format!(" Selected path: {selected_path}")); + output::result(format!(" Mode: {live_mode}")); if let Some(endpoint) = negotiated_endpoint { - println!(" Negotiated endpoint: {endpoint}"); + output::result(format!(" Negotiated endpoint: {endpoint}")); } if let Some(version) = negotiated_daemon_version { - println!(" Negotiated version: {version}"); + output::result(format!(" Negotiated version: {version}")); } if let Some(reason) = fallback_reason { - println!(" Fallback reason: {reason}"); + output::result(format!(" Fallback reason: {reason}")); } - println!(" Daemon endpoint: {}", fbuild_paths::get_daemon_url()); - println!(" Daemon binary: {}", daemon_candidate.display()); - println!( + output::result(format!( + " Daemon endpoint: {}", + fbuild_paths::get_daemon_url() + )); + output::result(format!( + " Daemon binary: {}", + daemon_candidate.display() + )); + output::result(format!( " Daemon binary exists: {}", if daemon_candidate_exists { "yes" } else { "no" } - ); - println!(" Cache roots:"); - println!(" - artifact: {}", cache_roots.artifact.display()); - println!(" - index: {}", cache_roots.index.display()); - println!(" - temp: {}", cache_roots.temp.display()); - println!(" - log: {}", cache_roots.log.display()); - println!(" - lock: {}", cache_roots.lock.display()); - println!(" - runtime: {}", cache_roots.runtime.display()); - println!(" - config: {}", cache_roots.config.display()); + )); + output::result(" Cache roots:"); + output::result(format!( + " - artifact: {}", + cache_roots.artifact.display() + )); + output::result(format!(" - index: {}", cache_roots.index.display())); + output::result(format!(" - temp: {}", cache_roots.temp.display())); + output::result(format!(" - log: {}", cache_roots.log.display())); + output::result(format!(" - lock: {}", cache_roots.lock.display())); + output::result(format!( + " - runtime: {}", + cache_roots.runtime.display() + )); + output::result(format!(" - config: {}", cache_roots.config.display())); Ok(()) } @@ -265,28 +293,31 @@ pub async fn run_daemon_list(client: &DaemonClient) -> fbuild_core::Result<()> { match client.daemon_info().await { Ok(info) => { let uptime = format_uptime(info.uptime_seconds); - println!("fbuild daemon (running)"); - println!(" PID: {}", info.pid); - println!(" Port: {}", info.port); - println!(" Uptime: {}", uptime); - println!(" Version: {}", info.version); - println!(" Mode: {}", if info.dev_mode { "dev" } else { "prod" }); + output::result("fbuild daemon (running)"); + output::result(format!(" PID: {}", info.pid)); + output::result(format!(" Port: {}", info.port)); + output::result(format!(" Uptime: {}", uptime)); + output::result(format!(" Version: {}", info.version)); + output::result(format!( + " Mode: {}", + if info.dev_mode { "dev" } else { "prod" } + )); } Err(e) => { - println!("daemon is running but info unavailable: {}", e); + output::result(format!("daemon is running but info unavailable: {}", e)); } } } else { - println!("no daemon is running"); + output::result("no daemon is running"); // Check for stale PID file let pid_file = fbuild_paths::get_daemon_pid_file(); if pid_file.exists() { if let Ok(contents) = std::fs::read_to_string(&pid_file) { - println!( + output::result(format!( " (stale PID file: {} — PID {})", pid_file.display(), contents.trim() - ); + )); } } } @@ -294,18 +325,18 @@ pub async fn run_daemon_list(client: &DaemonClient) -> fbuild_core::Result<()> { // Also scan for orphan processes let pids = find_daemon_pids().await?; if pids.len() > 1 { - println!("\nwarning: multiple fbuild-daemon processes detected:"); + output::warn("multiple fbuild-daemon processes detected:"); for pid in &pids { - println!(" PID {}", pid); + output::result(format!(" PID {}", pid)); } - println!("use 'fbuild daemon kill-all' to clean up"); + output::result("use 'fbuild daemon kill-all' to clean up"); } Ok(()) } pub async fn run_daemon_locks(client: &DaemonClient) -> fbuild_core::Result<()> { if !client.health().await { - println!("daemon is not running"); + output::result("daemon is not running"); return Ok(()); } @@ -313,35 +344,35 @@ pub async fn run_daemon_locks(client: &DaemonClient) -> fbuild_core::Result<()> // Display port locks if status.port_locks.is_empty() { - println!("Port Locks: (none)"); + output::result("Port Locks: (none)"); } else { - println!("Port Locks:"); + output::result("Port Locks:"); for lock in &status.port_locks { let state = if lock.is_held { "HELD" } else { "FREE" }; let writer = lock.writer_client_id.as_deref().unwrap_or("none"); - println!( + output::result(format!( " {} [{}] open={} writer={} readers={}", lock.port, state, lock.is_open, writer, lock.reader_count - ); + )); } } // Display project locks if status.project_locks.is_empty() { - println!("Project Locks: (none)"); + output::result("Project Locks: (none)"); } else { - println!("Project Locks:"); + output::result("Project Locks:"); for lock in &status.project_locks { let state = if lock.is_held { "HELD" } else { "FREE" }; - println!(" {} [{}]", lock.project_dir, state); + output::result(format!(" {} [{}]", lock.project_dir, state)); } } if !status.stale_locks.is_empty() { - println!( - "\nWarning: {} stale lock(s) detected. Use 'fbuild daemon clear-locks' to clear.", + output::warn(format!( + "{} stale lock(s) detected. Use 'fbuild daemon clear-locks' to clear.", status.stale_locks.len() - ); + )); } Ok(()) @@ -349,14 +380,14 @@ pub async fn run_daemon_locks(client: &DaemonClient) -> fbuild_core::Result<()> pub async fn run_daemon_clear_locks(client: &DaemonClient) -> fbuild_core::Result<()> { if !client.health().await { - println!("daemon is not running"); + output::result("daemon is not running"); return Ok(()); } let result = client.clear_locks().await?; - println!("{}", result.message); + output::result(&result.message); if result.cleared_count > 0 { - println!("Cleared {} lock(s)", result.cleared_count); + output::result(format!("Cleared {} lock(s)", result.cleared_count)); } Ok(()) } @@ -369,7 +400,7 @@ pub async fn run_daemon_cache_stats(client: &DaemonClient) -> fbuild_core::Resul let stats = dc.stats().map_err(|e| { fbuild_core::FbuildError::Other(format!("failed to read cache stats: {}", e)) })?; - println!("{}", stats); + output::result(format!("{}", stats)); } Err(e) => { return Err(fbuild_core::FbuildError::Other(format!( @@ -388,17 +419,26 @@ pub async fn run_daemon_cache_stats(client: &DaemonClient) -> fbuild_core::Resul stats.message.as_deref().unwrap_or("unknown error") ))); } - println!("Disk Cache Statistics:"); - println!(" Entries: {}", stats.entry_count); - println!(" Installed: {}", format_size(stats.installed_bytes)); - println!(" Archives: {}", format_size(stats.archive_bytes)); - println!(" Total: {}", format_size(stats.total_bytes)); - println!( + output::result("Disk Cache Statistics:"); + output::result(format!(" Entries: {}", stats.entry_count)); + output::result(format!( + " Installed: {}", + format_size(stats.installed_bytes) + )); + output::result(format!( + " Archives: {}", + format_size(stats.archive_bytes) + )); + output::result(format!(" Total: {}", format_size(stats.total_bytes))); + output::result(format!( " Watermarks: {} high / {} low", format_size(stats.high_watermark), format_size(stats.low_watermark) - ); - println!(" Archive budget: {}", format_size(stats.archive_budget)); + )); + output::result(format!( + " Archive budget: {}", + format_size(stats.archive_budget) + )); Ok(()) } @@ -422,26 +462,32 @@ pub async fn run_daemon_gc(client: &DaemonClient) -> fbuild_core::Result<()> { result.message.as_deref().unwrap_or("unknown error") ))); } - println!("GC complete:"); - println!( + output::result("GC complete:"); + output::result(format!( " Installed evicted: {} ({})", result.installed_evicted, format_size(result.installed_bytes_freed) - ); - println!( + )); + output::result(format!( " Archives evicted: {} ({})", result.archives_evicted, format_size(result.archive_bytes_freed) - ); - println!( + )); + output::result(format!( " Total freed: {}", format_size(result.total_bytes_freed) - ); + )); if result.orphan_files_removed > 0 { - println!(" Orphan files removed: {}", result.orphan_files_removed); + output::result(format!( + " Orphan files removed: {}", + result.orphan_files_removed + )); } if result.orphan_rows_cleaned > 0 { - println!(" Orphan rows cleaned: {}", result.orphan_rows_cleaned); + output::result(format!( + " Orphan rows cleaned: {}", + result.orphan_rows_cleaned + )); } Ok(()) } @@ -451,29 +497,35 @@ pub fn print_gc_report(report: &fbuild_packages::disk_cache::GcReport) { && report.orphan_files_removed == 0 && report.orphan_rows_cleaned == 0 { - println!("GC: nothing to clean up"); + output::result("GC: nothing to clean up"); return; } - println!("GC complete:"); - println!( + output::result("GC complete:"); + output::result(format!( " Installed evicted: {} ({})", report.installed_evicted, format_size(report.installed_bytes_freed) - ); - println!( + )); + output::result(format!( " Archives evicted: {} ({})", report.archives_evicted, format_size(report.archive_bytes_freed) - ); - println!( + )); + output::result(format!( " Total freed: {}", format_size(report.total_bytes_freed()) - ); + )); if report.orphan_files_removed > 0 { - println!(" Orphan files removed: {}", report.orphan_files_removed); + output::result(format!( + " Orphan files removed: {}", + report.orphan_files_removed + )); } if report.orphan_rows_cleaned > 0 { - println!(" Orphan rows cleaned: {}", report.orphan_rows_cleaned); + output::result(format!( + " Orphan rows cleaned: {}", + report.orphan_rows_cleaned + )); } } @@ -494,7 +546,7 @@ pub async fn run_daemon_kill( }; kill_process(target_pid, force).await?; - println!("killed daemon (PID {})", target_pid); + output::result(format!("killed daemon (PID {})", target_pid)); let _ = std::fs::remove_file(fbuild_paths::get_daemon_pid_file()); Ok(()) } @@ -520,7 +572,7 @@ pub fn read_pid_from_file() -> fbuild_core::Result { pub async fn run_daemon_kill_all(force: bool) -> fbuild_core::Result<()> { let pids = find_daemon_pids().await?; if pids.is_empty() { - println!("no fbuild-daemon processes found"); + output::result("no fbuild-daemon processes found"); return Ok(()); } @@ -528,17 +580,17 @@ pub async fn run_daemon_kill_all(force: bool) -> fbuild_core::Result<()> { for pid in &pids { match kill_process(*pid, force).await { Ok(()) => { - println!("killed daemon (PID {})", pid); + output::result(format!("killed daemon (PID {})", pid)); killed += 1; } Err(e) => { - eprintln!("failed to kill PID {}: {}", pid, e); + output::error(format!("failed to kill PID {}: {}", pid, e)); } } } let _ = std::fs::remove_file(fbuild_paths::get_daemon_pid_file()); - println!("killed {} daemon(s)", killed); + output::result(format!("killed {} daemon(s)", killed)); Ok(()) } diff --git a/crates/fbuild-cli/src/cli/deploy.rs b/crates/fbuild-cli/src/cli/deploy.rs index 1a84fd405..48ec7e514 100644 --- a/crates/fbuild-cli/src/cli/deploy.rs +++ b/crates/fbuild-cli/src/cli/deploy.rs @@ -5,6 +5,7 @@ use super::build::open_in_browser; use crate::daemon_client::{ self, DaemonClient, DeployRequest, MonitorRequest, OperationResponse, TestEmuRequest, }; +use crate::output; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum CliEmulatorKind { @@ -215,7 +216,7 @@ pub async fn run_deploy( { print_operation_streams(&resp); } - println!("{}", resp.message); + output::result(&resp.message); if !resp.success { std::process::exit(resp.exit_code); } @@ -223,8 +224,8 @@ pub async fn run_deploy( if deploy_route == CliDeployRoute::Emulator(CliEmulatorKind::Avr8js) { if let Some(url) = resp.launch_url.as_deref() { if let Err(e) = open_in_browser(url).await { - eprintln!("warning: failed to open browser: {}", e); - eprintln!("open this URL manually: {}", url); + output::warn(format!("failed to open browser: {}", e)); + output::warn(format!("open this URL manually: {}", url)); } } } @@ -265,7 +266,7 @@ pub async fn run_test_emu( let resp = client.test_emu(&req).await?; print_operation_streams(&resp); - println!("{}", resp.message); + output::result(&resp.message); if !resp.success { // Guarantee a non-zero exit when the daemon reports failure. A // structured error response carries `exit_code`, but if the @@ -288,20 +289,21 @@ pub fn print_operation_streams(resp: &OperationResponse) { .as_deref() .filter(|text| !text.trim().is_empty()) { - print!("{}", stdout); - if !stdout.ends_with('\n') { - println!(); - } + // result() always appends one '\n'; strip a trailing newline so we don't + // double up. If there was no trailing newline, result() supplies the one + // the caller would have added with println!(). + output::result(stdout.trim_end_matches('\n')); } if let Some(stderr) = resp .stderr .as_deref() .filter(|text| !text.trim().is_empty()) { - eprint!("{}", stderr); - if !stderr.ends_with('\n') { - eprintln!(); - } + // 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')); } } @@ -337,7 +339,7 @@ pub async fn run_monitor( }; let resp = client.monitor(&req).await?; - println!("{}", resp.message); + output::result(&resp.message); if !resp.success { std::process::exit(resp.exit_code); } diff --git a/crates/fbuild-cli/src/cli/device.rs b/crates/fbuild-cli/src/cli/device.rs index 1f5c8354e..23342a0b4 100644 --- a/crates/fbuild-cli/src/cli/device.rs +++ b/crates/fbuild-cli/src/cli/device.rs @@ -3,6 +3,7 @@ use crate::daemon_client::{ self, DaemonClient, DeviceLeaseConflictResponse, DeviceLeaseInfoResponse, }; +use crate::output; use super::args::DeviceAction; @@ -14,14 +15,14 @@ pub async fn run_device(action: DeviceAction) -> fbuild_core::Result<()> { DeviceAction::List { refresh } => { let resp = client.list_devices(refresh).await?; if resp.devices.is_empty() { - println!("no devices found"); + output::result("no devices found"); return Ok(()); } - println!( + output::result(format!( "{:<20} {:<12} {:<12} {:<24} DESCRIPTION", "PORT", "DEVICE ID", "LEASE", "HOLDER" - ); - println!("{}", "-".repeat(88)); + )); + output::result("-".repeat(88)); for dev in &resp.devices { let id = dev.device_id.as_deref().unwrap_or("-"); let lease = lease_summary(dev.exclusive_lease.as_ref(), dev.monitor_count); @@ -36,21 +37,21 @@ pub async fn run_device(action: DeviceAction) -> fbuild_core::Result<()> { // older than the resolver wiring), fall back to the raw // description so behavior is identical to pre-resolver. let pretty = device_pretty_name(dev); - println!( + output::result(format!( "{:<20} {:<12} {:<12} {:<24} {}", dev.port, id, lease, holder, device_description(&pretty, dev.previous_port.as_deref()) - ); + )); } - println!("\n{} device(s) found", resp.devices.len()); + output::result(format!("\n{} device(s) found", resp.devices.len())); } DeviceAction::Status { port } => { let resp = client.device_status(&port).await?; if !resp.success { - eprintln!("error: {}", resp.description); + output::error(&resp.description); return Ok(()); } let connected = if resp.is_connected { @@ -58,38 +59,38 @@ pub async fn run_device(action: DeviceAction) -> fbuild_core::Result<()> { } else { "disconnected" }; - println!(" {}", resp.port); - println!(" Device ID: {}", resp.device_id); + output::result(format!(" {}", resp.port)); + output::result(format!(" Device ID: {}", resp.device_id)); if let Some(ref vendor) = resp.vendor_name { - println!(" Vendor: {}", vendor); + output::result(format!(" Vendor: {}", vendor)); } if let Some(ref product) = resp.product_name { - println!(" Product: {}", product); + output::result(format!(" Product: {}", product)); } - println!(" Description: {}", resp.description); + output::result(format!(" Description: {}", resp.description)); if let Some(ref serial) = resp.serial_number { - println!(" Serial: {}", serial); + output::result(format!(" Serial: {}", serial)); } if let Some(ref previous_port) = resp.previous_port { - println!(" Previous port: {}", previous_port); + output::result(format!(" Previous port: {}", previous_port)); } - println!(" Status: {}", connected); - println!( + output::result(format!(" Status: {}", connected)); + output::result(format!( " Available: {}", if resp.available_for_exclusive { "yes" } else { "no" } - ); + )); if let Some(ref holder) = resp.exclusive_holder { - println!(" Exclusive holder: {}", holder); + output::result(format!(" Exclusive holder: {}", holder)); } if let Some(ref lease) = resp.exclusive_lease { print_lease(" Exclusive lease", lease); } if resp.monitor_count > 0 { - println!(" Monitor sessions: {}", resp.monitor_count); + output::result(format!(" Monitor sessions: {}", resp.monitor_count)); for lease in &resp.monitor_leases { print_lease(" Monitor lease", lease); } @@ -105,12 +106,12 @@ pub async fn run_device(action: DeviceAction) -> fbuild_core::Result<()> { .device_lease(&port, &lease_type, &description, track_serial) .await?; if resp.success { - println!("lease acquired on '{}'", port); + output::result(format!("lease acquired on '{}'", port)); if let Some(ref id) = resp.lease_id { - println!(" lease_id: {}", id); + output::result(format!(" lease_id: {}", id)); } } else { - eprintln!("error: {}", resp.message); + output::error(&resp.message); if let Some(ref conflict) = resp.conflict { print_conflict(conflict); } @@ -119,17 +120,17 @@ pub async fn run_device(action: DeviceAction) -> fbuild_core::Result<()> { DeviceAction::Release { port, lease_id } => { let resp = client.device_release(&port, lease_id.as_deref()).await?; if resp.success { - println!("{}", resp.message); + output::result(&resp.message); } else { - eprintln!("error: {}", resp.message); + output::error(&resp.message); } } DeviceAction::Take { port, reason } => { let resp = client.device_preempt(&port, &reason).await?; if resp.success { - println!("{}", resp.message); + output::result(&resp.message); } else { - eprintln!("error: {}", resp.message); + output::error(&resp.message); } } } @@ -146,24 +147,33 @@ fn lease_summary(exclusive: Option<&DeviceLeaseInfoResponse>, monitor_count: usi } fn print_lease(label: &str, lease: &DeviceLeaseInfoResponse) { - println!("{}:", label); - println!(" lease_id: {}", lease.lease_id); - println!(" type: {}", lease.lease_type); - println!(" client_id: {}", lease.client_id); - println!(" description: {}", empty_dash(&lease.description)); - println!(" acquired_at: {:.3}", lease.acquired_at); - println!(" track_serial: {}", lease.track_serial); + output::result(format!("{}:", label)); + output::result(format!(" lease_id: {}", lease.lease_id)); + output::result(format!(" type: {}", lease.lease_type)); + output::result(format!(" client_id: {}", lease.client_id)); + output::result(format!( + " description: {}", + empty_dash(&lease.description) + )); + output::result(format!(" acquired_at: {:.3}", lease.acquired_at)); + output::result(format!(" track_serial: {}", lease.track_serial)); } fn print_conflict(conflict: &DeviceLeaseConflictResponse) { - eprintln!( + output::error(format!( " holder: {} ({})", conflict.holder.client_id, empty_dash(&conflict.holder.description) - ); - eprintln!(" lease_id: {}", conflict.holder.lease_id); - eprintln!(" device: {} {}", conflict.port, conflict.device_id); - eprintln!(" description: {}", empty_dash(&conflict.description)); + )); + output::error(format!(" lease_id: {}", conflict.holder.lease_id)); + output::error(format!( + " device: {} {}", + conflict.port, conflict.device_id + )); + output::error(format!( + " description: {}", + empty_dash(&conflict.description) + )); } fn empty_dash(value: &str) -> &str { diff --git a/crates/fbuild-cli/src/cli/dispatch.rs b/crates/fbuild-cli/src/cli/dispatch.rs index 715754fea..74a37b9a7 100644 --- a/crates/fbuild-cli/src/cli/dispatch.rs +++ b/crates/fbuild-cli/src/cli/dispatch.rs @@ -3,7 +3,7 @@ use clap::Parser; -use crate::{daemon_client, lib_select, mcp}; +use crate::{daemon_client, lib_select, mcp, output}; use super::args::{resolve_project_dir, rewrite_args, BloatCmd, Cli, Commands}; use super::bloat_lookup::run_bloat_lookup; @@ -48,14 +48,14 @@ pub async fn async_main() { // Handle Ctrl+C with exit code 130 (standard POSIX SIGINT behavior, matches Python) ctrlc::set_handler(move || { - eprintln!("\nInterrupted"); + output::warn("Interrupted"); std::process::exit(130); }) .ok(); // Notify when running in dev mode (matches Python behavior) if std::env::var("FBUILD_DEV_MODE").is_ok_and(|v| v == "1") { - eprintln!("FBUILD_DEV_MODE=1 (dev mode: port 8865, ~/.fbuild/dev/)"); + output::progress("FBUILD_DEV_MODE=1 (dev mode: port 8865, ~/.fbuild/dev/)"); } // Extract top-level project_dir before matching (since match partially moves cli) @@ -465,10 +465,10 @@ pub async fn async_main() { sketches, }) => { if let Some(bd) = &build_dir { - eprintln!( - "warning: --build-dir {} is accepted for pio ci compatibility but not yet honored; outputs go to .fbuild/build/...", + output::warn(format!( + "--build-dir {} is accepted for pio ci compatibility but not yet honored; outputs go to .fbuild/build/...", bd - ); + )); } let normalized = normalize_ci_sketches(&sketches); let pio_env = build_ci_pio_env(&libs, project_conf.as_deref()); @@ -535,7 +535,7 @@ pub async fn async_main() { }; if let Err(e) = result { - eprintln!("error: {}", e); + output::error(format!("{}", e)); std::process::exit(1); } } diff --git a/crates/fbuild-cli/src/cli/graph_cmd.rs b/crates/fbuild-cli/src/cli/graph_cmd.rs index ae695f229..11826f16a 100644 --- a/crates/fbuild-cli/src/cli/graph_cmd.rs +++ b/crates/fbuild-cli/src/cli/graph_cmd.rs @@ -18,6 +18,8 @@ use fbuild_build::symbol_analyzer::{ use fbuild_core::symbol_analysis::{BackrefGraph, GraphConfig, GraphDepth}; use fbuild_core::{FbuildError, Result}; +use crate::output as cli_output; + use super::symbols_cmd::resolve_tool_paths_public; #[allow(clippy::too_many_arguments)] @@ -85,14 +87,16 @@ pub async fn run_bloat_graph( std::fs::write(&path, &dot).map_err(|e| { FbuildError::Io(std::io::Error::new(e.kind(), format!("write {path}: {e}"))) })?; - println!( + cli_output::result(format!( "Wrote back-reference graph for {symbol} to {path} ({} nodes, {} edges)", graph.nodes.len(), graph.edges.len() - ); + )); } None => { - print!("{dot}"); + // `dot` is the final answer (DOT graph text) — emit verbatim. Strip + // any trailing newline so result()'s newline doesn't double up. + cli_output::result(dot.trim_end_matches('\n')); } } Ok(()) diff --git a/crates/fbuild-cli/src/cli/lnk.rs b/crates/fbuild-cli/src/cli/lnk.rs index ddd7e6699..0b8f4c34d 100644 --- a/crates/fbuild-cli/src/cli/lnk.rs +++ b/crates/fbuild-cli/src/cli/lnk.rs @@ -4,6 +4,8 @@ //! - `check` — verify every cached blob's sha256 (no network) //! - `add` — fetch a URL once, hash it, write a new .lnk pointing at it +use crate::output; + use super::args::LnkAction; pub async fn run_lnk( @@ -34,7 +36,7 @@ pub async fn run_lnk( let root = resolve_root(project_dir, top_level_project_dir); let discovered = scan_for_lnk(&root)?; if discovered.is_empty() { - println!("no .lnk files found under {}", root.display()); + output::result(format!("no .lnk files found under {}", root.display())); return Ok(()); } let cache = open_cache()?; @@ -44,23 +46,23 @@ pub async fn run_lnk( match fbuild_packages::lnk::resolve(&d.lnk, &cache) { Ok(r) => { ok += 1; - println!( + output::result(format!( "ok {} → {} ({})", d.path.display(), r.path.display(), d.lnk.sha256 - ); + )); } Err(e) => { failed += 1; - eprintln!("FAIL {}: {}", d.path.display(), e); + output::error(format!("FAIL {}: {}", d.path.display(), e)); } } } - println!( + output::result(format!( "\nlnk pull: {ok} ok, {failed} failed (of {})", discovered.len() - ); + )); if failed > 0 { std::process::exit(1); } @@ -71,7 +73,7 @@ pub async fn run_lnk( let root = resolve_root(project_dir, top_level_project_dir); let discovered = scan_for_lnk(&root)?; if discovered.is_empty() { - println!("no .lnk files found under {}", root.display()); + output::result(format!("no .lnk files found under {}", root.display())); return Ok(()); } let cache = open_cache()?; @@ -93,20 +95,20 @@ pub async fn run_lnk( })?; let Some(entry) = entry else { missing += 1; - println!( + output::result(format!( "MISSING {} (run `fbuild lnk pull` to fetch)", d.path.display() - ); + )); continue; }; let blob_path = PathBuf::from(entry.archive_path.unwrap_or_default()); if !blob_path.exists() { missing += 1; - println!( + output::result(format!( "MISSING {} (cache index points at {} which is gone)", d.path.display(), blob_path.display() - ); + )); continue; } let bytes = std::fs::read(&blob_path).map_err(|e| { @@ -120,32 +122,35 @@ pub async fn run_lnk( let actual = format!("{:x}", h.finalize()); if actual == d.lnk.sha256 { ok += 1; - println!("ok {}", d.path.display()); + output::result(format!("ok {}", d.path.display())); } else { mismatched += 1; - println!( + output::result(format!( "BAD {} (expected {}, got {})", d.path.display(), d.lnk.sha256, actual - ); + )); } } - println!( + output::result(format!( "\nlnk check: {ok} ok, {missing} missing, {mismatched} mismatched (of {})", discovered.len() - ); + )); if mismatched > 0 || missing > 0 { std::process::exit(1); } Ok(()) } - LnkAction::Add { url, output } => { + LnkAction::Add { + url, + output: output_arg, + } => { // Determine output path before downloading so we fail early on a // bad output spec. let basename = url.rsplit('/').next().unwrap_or("blob"); - let output_path = match output { + let output_path = match output_arg { Some(p) => PathBuf::from(p), None => PathBuf::from(format!("{basename}.lnk")), }; @@ -208,12 +213,12 @@ pub async fn run_lnk( })?; f.write_all(b"\n").ok(); - println!( + output::result(format!( "wrote {} ({} bytes, sha256={})", output_path.display(), bytes.len(), sha - ); + )); Ok(()) } } diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index 2cf50ecbe..079a06f72 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -28,6 +28,8 @@ use clap::Subcommand; use fbuild_core::{FbuildError, Result}; use std::time::Duration; +use crate::output; + #[derive(Subcommand)] pub enum PortAction { /// Enumerate every visible serial port; for each, render two rows — @@ -63,7 +65,9 @@ fn run_scan(offline: bool) -> Result<()> { let ports = serialport::available_ports() .map_err(|e| FbuildError::SerialError(format!("serial port enumeration failed: {e}")))?; let rendered = render_scan(&ports); - print!("{rendered}"); + // render_scan terminates every row with '\n'; strip the trailing newline + // so result()'s newline doesn't double up. + output::result(rendered.trim_end_matches('\n')); Ok(()) } @@ -126,10 +130,9 @@ fn fetch_overlay_to(path: &std::path::Path) -> std::result::Result<(), String> { } fn fetch_overlay_to_inner(path: &std::path::Path) -> std::result::Result<(), String> { - let client = reqwest::blocking::Client::builder() - .timeout(Duration::from_secs(15)) - .build() - .map_err(|e| format!("client build: {e}"))?; + // FastLED/fbuild#844: route the OS-thread blocking client through the + // shared bridge so all reqwest construction has one source of truth. + let client = fbuild_core::http::blocking_client(Duration::from_secs(15)); fetch_overlay_to_inner_with_client(path, &client, fbuild_core::usb::USB_VIDS_PROTO_ZSTD_URL) } diff --git a/crates/fbuild-cli/src/cli/purge.rs b/crates/fbuild-cli/src/cli/purge.rs index 1326dab85..6ab01b78f 100644 --- a/crates/fbuild-cli/src/cli/purge.rs +++ b/crates/fbuild-cli/src/cli/purge.rs @@ -2,6 +2,7 @@ //! formatting helpers they share with the daemon subcommands. use crate::daemon_client::DaemonClient; +use crate::output; use super::daemon_cmd::print_gc_report; @@ -16,26 +17,32 @@ pub async fn run_purge_gc() -> fbuild_core::Result<()> { result.message.as_deref().unwrap_or("unknown error") ))); } - println!("GC complete (via daemon):"); - println!( + output::result("GC complete (via daemon):"); + output::result(format!( " Installed evicted: {} ({})", result.installed_evicted, format_size(result.installed_bytes_freed) - ); - println!( + )); + output::result(format!( " Archives evicted: {} ({})", result.archives_evicted, format_size(result.archive_bytes_freed) - ); - println!( + )); + output::result(format!( " Total freed: {}", format_size(result.total_bytes_freed) - ); + )); if result.orphan_files_removed > 0 { - println!(" Orphan files removed: {}", result.orphan_files_removed); + output::result(format!( + " Orphan files removed: {}", + result.orphan_files_removed + )); } if result.orphan_rows_cleaned > 0 { - println!(" Orphan rows cleaned: {}", result.orphan_rows_cleaned); + output::result(format!( + " Orphan rows cleaned: {}", + result.orphan_rows_cleaned + )); } return Ok(()); } @@ -83,7 +90,7 @@ pub fn run_purge( // Purge specific cache subdirectory (e.g., environment name) let path = cache_root.join(t); if !path.exists() { - eprintln!("target not found: {}", path.display()); + output::error(format!("target not found: {}", path.display())); return Ok(()); } purge_dir(&path, dry_run)?; @@ -94,29 +101,40 @@ pub fn run_purge( pub fn purge_dir(path: &std::path::Path, dry_run: bool) -> fbuild_core::Result<()> { if !path.exists() { - println!("nothing to purge: {}", path.display()); + output::result(format!("nothing to purge: {}", path.display())); return Ok(()); } let size = dir_size(path); if dry_run { - println!("would remove: {} ({})", path.display(), format_size(size)); + output::result(format!( + "would remove: {} ({})", + path.display(), + format_size(size) + )); } else { std::fs::remove_dir_all(path).map_err(|e| { fbuild_core::FbuildError::Other(format!("failed to remove {}: {}", path.display(), e)) })?; - println!("removed: {} ({})", path.display(), format_size(size)); + output::result(format!( + "removed: {} ({})", + path.display(), + format_size(size) + )); } Ok(()) } pub fn list_cached_packages(cache_root: &std::path::Path) -> fbuild_core::Result<()> { if !cache_root.exists() { - println!("No cached packages found at {}", cache_root.display()); - println!("\nUsage:"); - println!(" fbuild purge all Remove all cached packages"); - println!(" fbuild purge project Remove project build artifacts (.fbuild/)"); - println!(" fbuild purge Remove specific cache subdirectory"); - println!(" fbuild purge ... --dry-run Show what would be removed"); + output::result(format!( + "No cached packages found at {}", + cache_root.display() + )); + output::result("\nUsage:"); + output::result(" fbuild purge all Remove all cached packages"); + output::result(" fbuild purge project Remove project build artifacts (.fbuild/)"); + output::result(" fbuild purge Remove specific cache subdirectory"); + output::result(" fbuild purge ... --dry-run Show what would be removed"); return Ok(()); } @@ -150,22 +168,22 @@ pub fn list_cached_packages(cache_root: &std::path::Path) -> fbuild_core::Result packages.sort_by(|a, b| a.0.cmp(&b.0)); if !packages.is_empty() { - println!("{}:", type_name.to_string_lossy().to_uppercase()); + output::result(format!("{}:", type_name.to_string_lossy().to_uppercase())); for (name, size) in &packages { - println!(" {} ({})", name, format_size(*size)); + output::result(format!(" {} ({})", name, format_size(*size))); total_size += size; total_count += 1; } - println!(); + output::result(""); } } - println!( + output::result(format!( "Total: {} package(s), {}", total_count, format_size(total_size) - ); - println!("\nUse 'fbuild purge all' to remove all, or 'fbuild purge ' for specific."); + )); + output::result("\nUse 'fbuild purge all' to remove all, or 'fbuild purge ' for specific."); Ok(()) } diff --git a/crates/fbuild-cli/src/cli/reset.rs b/crates/fbuild-cli/src/cli/reset.rs index 428bbd5f2..37ec2aba9 100644 --- a/crates/fbuild-cli/src/cli/reset.rs +++ b/crates/fbuild-cli/src/cli/reset.rs @@ -1,5 +1,7 @@ //! `fbuild reset` — reboot a device without re-flashing. +use crate::output; + pub fn run_reset( project_dir: String, environment: Option, @@ -39,14 +41,14 @@ pub fn run_reset( fbuild_core::FbuildError::SerialError("no serial port specified (use --port)".to_string()) })?; - println!("resetting {} device on {}...", platform, port); + output::progress(format!("resetting {} device on {}...", platform, port)); match fbuild_deploy::reset::reset_device(platform, &port, verbose)? { true => { - println!("device reset successful"); + output::result("device reset successful"); Ok(()) } false => { - eprintln!("device reset failed"); + output::error("device reset failed"); std::process::exit(1); } } diff --git a/crates/fbuild-cli/src/cli/serial_probe.rs b/crates/fbuild-cli/src/cli/serial_probe.rs index a34d8cdb7..6deac62f4 100644 --- a/crates/fbuild-cli/src/cli/serial_probe.rs +++ b/crates/fbuild-cli/src/cli/serial_probe.rs @@ -37,6 +37,8 @@ use clap::Subcommand; use fbuild_core::{FbuildError, Result}; use fbuild_serial::boards::{board_hint, family_for_vid_pid, vcom_for_env, BoardFamily}; +use crate::output; + #[derive(Subcommand)] pub enum SerialAction { /// Port-probe utilities — list, find, read. @@ -114,7 +116,7 @@ fn list_ports() -> Result<()> { .map_err(|e| FbuildError::SerialError(format!("serial port enumeration failed: {e}")))?; if ports.is_empty() { - eprintln!("no serial ports visible"); + output::warn("no serial ports visible"); return Ok(()); } @@ -138,15 +140,17 @@ fn print_port_summary(port: &serialport::SerialPortInfo) { } else { format!("ser={serial} ") }; - println!( + output::result(format!( "{name:<10} {vid:04X}:{pid:04X} {serial_field}{product} {hint}", vid = info.vid, pid = info.pid, - ); + )); + } + serialport::SerialPortType::PciPort => output::result(format!("{name:<10} [PCI]")), + serialport::SerialPortType::BluetoothPort => { + output::result(format!("{name:<10} [Bluetooth]")) } - serialport::SerialPortType::PciPort => println!("{name:<10} [PCI]"), - serialport::SerialPortType::BluetoothPort => println!("{name:<10} [Bluetooth]"), - serialport::SerialPortType::Unknown => println!("{name:<10} [Unknown]"), + serialport::SerialPortType::Unknown => output::result(format!("{name:<10} [Unknown]")), } } @@ -180,7 +184,7 @@ fn find_port(vid_pid: Option<&str>, env: Option<&str>) -> Result<()> { for port in ports { if let serialport::SerialPortType::UsbPort(info) = &port.port_type { if (info.vid, info.pid) == target { - println!("{}", port.port_name); + output::result(&port.port_name); return Ok(()); } } @@ -232,10 +236,10 @@ fn read_port(port_name: &str, baud: u32, seconds: f64, send: Option<&str>) -> Re .write_request_to_send(rts) .map_err(|e| FbuildError::SerialError(format!("set RTS={rts} on `{port_name}`: {e}")))?; - eprintln!( + output::progress(format!( "probe read: port={port_name} baud={baud} family={family:?} \ dtr={dtr} rts={rts} seconds={seconds}" - ); + )); if let Some(payload) = send { let bytes = unescape_send(payload); diff --git a/crates/fbuild-cli/src/cli/show.rs b/crates/fbuild-cli/src/cli/show.rs index b6caf94ff..c5a0fa466 100644 --- a/crates/fbuild-cli/src/cli/show.rs +++ b/crates/fbuild-cli/src/cli/show.rs @@ -1,11 +1,16 @@ //! `fbuild show ` plus the daemon log tail used by both `show //! daemon` and `daemon monitor`. +use crate::output; + pub fn run_show(target: &str, follow: bool, lines: usize) -> fbuild_core::Result<()> { match target { "daemon" => show_daemon_logs(follow, lines), other => { - eprintln!("unknown show target: '{}' (available: daemon)", other); + output::error(format!( + "unknown show target: '{}' (available: daemon)", + other + )); std::process::exit(1); } } @@ -14,8 +19,11 @@ pub fn run_show(target: &str, follow: bool, lines: usize) -> fbuild_core::Result pub fn show_daemon_logs(follow: bool, initial_lines: usize) -> fbuild_core::Result<()> { let log_path = fbuild_paths::get_daemon_log_file(); if !log_path.exists() { - eprintln!("daemon log file not found: {}", log_path.display()); - eprintln!("the daemon may not have been started yet"); + output::error(format!( + "daemon log file not found: {}", + log_path.display() + )); + output::error("the daemon may not have been started yet"); return Ok(()); } @@ -26,7 +34,7 @@ pub fn show_daemon_logs(follow: bool, initial_lines: usize) -> fbuild_core::Resu let all_lines: Vec<&str> = content.lines().collect(); let start = all_lines.len().saturating_sub(initial_lines); for line in &all_lines[start..] { - println!("{}", line); + output::result(*line); } if !follow { @@ -34,7 +42,10 @@ pub fn show_daemon_logs(follow: bool, initial_lines: usize) -> fbuild_core::Resu } // Follow mode: poll for new content - println!("--- following {} (Ctrl+C to stop) ---", log_path.display()); + output::progress(format!( + "--- following {} (Ctrl+C to stop) ---", + log_path.display() + )); let mut pos = content.len() as u64; loop { std::thread::sleep(std::time::Duration::from_millis(100)); @@ -46,7 +57,9 @@ pub fn show_daemon_logs(follow: bool, initial_lines: usize) -> fbuild_core::Resu let _ = file.seek(std::io::SeekFrom::Start(pos)); let mut buf = String::new(); if file.read_to_string(&mut buf).is_ok() && !buf.is_empty() { - print!("{}", buf); + // Buffered tail content may end with '\n'; strip it so + // result()'s appended newline doesn't double up. + output::result(buf.trim_end_matches('\n')); } pos = current_len; } diff --git a/crates/fbuild-cli/src/cli/symbols_cmd.rs b/crates/fbuild-cli/src/cli/symbols_cmd.rs index 7d0c237ae..95d12fa67 100644 --- a/crates/fbuild-cli/src/cli/symbols_cmd.rs +++ b/crates/fbuild-cli/src/cli/symbols_cmd.rs @@ -24,6 +24,8 @@ use fbuild_build::symbol_analyzer::{ }; use fbuild_core::{FbuildError, Result}; +use crate::output; + use super::graph_cmd::parse_graph_config; #[allow(clippy::too_many_arguments)] @@ -91,14 +93,14 @@ pub async fn run_symbols( let mut wrote_anything = false; if let Some(json_path) = json_out { - write_json(&report, &json_path)?; - println!( + write_json(&report, &json_path).await?; + output::result(format!( "Wrote {} symbols to {} (flash={} B, ram={} B)", report.symbols.len(), json_path, report.total_flash, report.total_ram - ); + )); wrote_anything = true; } @@ -112,7 +114,7 @@ pub async fn run_symbols( })?; let json_target = dir.join("report.json"); let md_target = dir.join("report.md"); - write_json(&report, &json_target.to_string_lossy())?; + write_json(&report, &json_target.to_string_lossy()).await?; let graph_config = parse_graph_config( &graph_depth, graph_fan_out, @@ -152,7 +154,7 @@ pub async fn run_symbols( }, )? }; - println!( + output::result(format!( "Wrote {} symbols to {} and {} (flash={} B, ram={} B); {} sidecar graphs", report.symbols.len(), json_target.display(), @@ -160,29 +162,32 @@ pub async fn run_symbols( report.total_flash, report.total_ram, sidecar_count, - ); + )); wrote_anything = true; } if !wrote_anything { - println!("{}", format_text_report(&report, top)); + output::result(format_text_report(&report, top)); } Ok(()) } -fn write_json( +async fn write_json( report: &fbuild_core::symbol_analysis::FineGrainedSymbolMap, json_path: &str, ) -> Result<()> { let json = serde_json::to_string_pretty(report) .map_err(|e| FbuildError::Other(format!("json serialize: {e}")))?; - std::fs::write(json_path, json).map_err(|e| { - FbuildError::Io(std::io::Error::new( - e.kind(), - format!("write {json_path}: {e}"), - )) - })?; + // Atomic write — FastLED/fbuild#844 bridge pair 6 (symbol report). + fbuild_core::fs::write_atomic(json_path, json) + .await + .map_err(|e| { + FbuildError::Io(std::io::Error::new( + e.kind(), + format!("write {json_path}: {e}"), + )) + })?; Ok(()) } diff --git a/crates/fbuild-cli/src/daemon_client.rs b/crates/fbuild-cli/src/daemon_client.rs index 0625f612f..a4c6715d2 100644 --- a/crates/fbuild-cli/src/daemon_client.rs +++ b/crates/fbuild-cli/src/daemon_client.rs @@ -10,7 +10,10 @@ use serde::Serialize; mod types; pub use types::*; -const LONG_OPERATION_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(1800); +/// Daemon long-operation cap — re-exported as a local alias so existing +/// call sites read naturally. Backed by `fbuild_core::time::DAEMON_LONG_OP_TIMEOUT` +/// (30 min) per FastLED/fbuild#844. +const LONG_OPERATION_TIMEOUT: std::time::Duration = fbuild_core::time::DAEMON_LONG_OP_TIMEOUT; /// Percent-encode a port name for use in a URL path segment. fn encode_port(port: &str) -> String { @@ -202,14 +205,15 @@ pub struct DaemonClient { impl DaemonClient { pub fn new() -> Self { - // 100ms connect_timeout fails fast when the daemon is not running - // (ECONNREFUSED returns instantly on Windows and Linux but reqwest - // would otherwise wait for the full request timeout before surfacing - // the error). - let client = reqwest::Client::builder() - .connect_timeout(std::time::Duration::from_millis(100)) - .build() - .unwrap_or_else(|_| reqwest::Client::new()); + // FastLED/fbuild#844: route through the shared HTTP bridge so this + // crate has zero direct reqwest construction. Per-call + // `.timeout(...)` deadlines on each endpoint (already set on + // `health`, `health_full`, `list_devices`, etc.) bound the + // fast-fail behavior the previous 100ms connect_timeout provided — + // ECONNREFUSED still returns instantly on Windows/Linux, and a + // slow / non-responsive daemon trips the per-request timeout + // (typically 2-10s) rather than the 30s default connect_timeout. + let client = fbuild_core::http::client().clone(); Self { base_url: fbuild_paths::get_daemon_url(), client, @@ -220,7 +224,7 @@ impl DaemonClient { pub async fn health(&self) -> bool { self.client .get(format!("{}/health", self.base_url)) - .timeout(std::time::Duration::from_secs(2)) + .timeout(fbuild_core::time::SHORT_HTTP_TIMEOUT) .send() .await .map(|r| r.status().is_success()) @@ -231,7 +235,7 @@ impl DaemonClient { pub async fn health_full(&self) -> Option { self.client .get(format!("{}/health", self.base_url)) - .timeout(std::time::Duration::from_secs(2)) + .timeout(fbuild_core::time::SHORT_HTTP_TIMEOUT) .send() .await .ok()? @@ -388,7 +392,7 @@ impl DaemonClient { let resp = self .client .get(format!("{}/api/daemon/info", self.base_url)) - .timeout(std::time::Duration::from_secs(2)) + .timeout(fbuild_core::time::SHORT_HTTP_TIMEOUT) .send() .await .map_err(|e| fbuild_core::FbuildError::DaemonError(format!("request failed: {}", e)))?; @@ -429,7 +433,7 @@ impl DaemonClient { let resp = self .client .get(format!("{}/api/locks/status", self.base_url)) - .timeout(std::time::Duration::from_secs(5)) + .timeout(fbuild_core::time::MEDIUM_HTTP_TIMEOUT) .send() .await .map_err(|e| fbuild_core::FbuildError::DaemonError(format!("request failed: {}", e)))?; @@ -545,7 +549,7 @@ impl DaemonClient { let resp = self .client .post(format!("{}/api/locks/clear", self.base_url)) - .timeout(std::time::Duration::from_secs(5)) + .timeout(fbuild_core::time::MEDIUM_HTTP_TIMEOUT) .send() .await .map_err(|e| fbuild_core::FbuildError::DaemonError(format!("request failed: {}", e)))?; @@ -560,7 +564,7 @@ impl DaemonClient { let resp = self .client .get(format!("{}/api/cache/stats", self.base_url)) - .timeout(std::time::Duration::from_secs(5)) + .timeout(fbuild_core::time::MEDIUM_HTTP_TIMEOUT) .send() .await .map_err(|e| fbuild_core::FbuildError::DaemonError(format!("request failed: {}", e)))?; diff --git a/crates/fbuild-cli/src/lib_select.rs b/crates/fbuild-cli/src/lib_select.rs index 11dfa753e..0d68e5938 100644 --- a/crates/fbuild-cli/src/lib_select.rs +++ b/crates/fbuild-cli/src/lib_select.rs @@ -28,6 +28,10 @@ use walkdir::{DirEntry, WalkDir}; /// Top-level entry point. Returns a process exit code. pub fn run(project_dir: &Path, env: Option<&str>, explain: bool, json: bool) -> i32 { + // FastLED/fbuild#844 sync-context allowlist: `fbuild lib-select` + // is a synchronous diagnostic CLI entry point dispatched directly + // by clap, with no tokio runtime in scope. File is allowlisted in + // `dylints/ban_std_fs_canonicalize/src/allowlist.txt`. let project_dir = match std::fs::canonicalize(project_dir) { Ok(p) => p, Err(err) => { diff --git a/crates/fbuild-cli/src/main.rs b/crates/fbuild-cli/src/main.rs index b53f34027..e539ead32 100644 --- a/crates/fbuild-cli/src/main.rs +++ b/crates/fbuild-cli/src/main.rs @@ -2,6 +2,7 @@ mod cli; mod daemon_client; mod lib_select; mod mcp; +mod output; fn main() { // Trampoline through a larger-stack thread: Windows' default 1 MB main-thread diff --git a/crates/fbuild-cli/src/output.rs b/crates/fbuild-cli/src/output.rs new file mode 100644 index 000000000..5561c5791 --- /dev/null +++ b/crates/fbuild-cli/src/output.rs @@ -0,0 +1,60 @@ +//! 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 +//! `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` | +//! +//! 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. + +use std::fmt::Display; + +/// In-progress narration. Emits at `INFO` so a default-verbosity user +/// sees it; `--quiet` hides it. Examples: "Building env esp32dev…", +/// "Downloaded xtensa-esp32-elf-12.2.0". +pub fn progress(msg: impl Display) { + tracing::info!("{msg}"); +} + +/// The final answer. The only output that must survive `--quiet` and +/// pipe redirection (e.g. `fbuild show fqbn esp32dev | xargs ...`). +/// Use sparingly — most output is `progress`, not `result`. +pub fn result(msg: impl Display) { + println!("{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. +pub fn warn(msg: impl Display) { + tracing::warn!("{msg}"); +} + +/// Fatal error. Emits at `ERROR`. The caller is expected to also +/// surface a non-zero exit code; this function does NOT exit. +pub fn error(msg: impl Display) { + tracing::error!("{msg}"); +} + +/// Debug output. Emits at `DEBUG`. Hidden by default; surfaces with +/// `--verbose` or `RUST_LOG=debug`. +pub fn debug(msg: impl Display) { + tracing::debug!("{msg}"); +} diff --git a/crates/fbuild-core/Cargo.toml b/crates/fbuild-core/Cargo.toml index 17943a6fb..93dcf54bc 100644 --- a/crates/fbuild-core/Cargo.toml +++ b/crates/fbuild-core/Cargo.toml @@ -39,6 +39,11 @@ tokio = { workspace = true } # the #813 Phase A foundation so downstream crates can `use async_trait` # without each one redeclaring the workspace dep. async-trait = { workspace = true } +# Shared HTTP client bridge (FastLED/fbuild#844). The `client()` +# constructor is hoisted from fbuild-packages so every reqwest call site +# in the workspace has one source of truth. See `src/http.rs` and the +# matching `ban_bare_reqwest` dylint. +reqwest = { workspace = true } # `libc::setpgid` / `libc::prctl` for the Unix `pre_exec` hook used by # `containment::tokio_spawn::configure`. See FastLED/fbuild#32. diff --git a/crates/fbuild-core/src/build_log.rs b/crates/fbuild-core/src/build_log.rs index d2b425e1d..5a210b009 100644 --- a/crates/fbuild-core/src/build_log.rs +++ b/crates/fbuild-core/src/build_log.rs @@ -7,9 +7,9 @@ //! When an `epoch` is set, every line pushed is automatically prefixed with //! the elapsed time since that epoch (e.g. `" 0.46 compiling foo.cpp"`). +use crate::channel::UnboundedSender; use std::collections::VecDeque; use std::time::Instant; -use tokio::sync::mpsc::UnboundedSender; /// Centralized build output log. /// diff --git a/crates/fbuild-core/src/channel.rs b/crates/fbuild-core/src/channel.rs new file mode 100644 index 000000000..28cfde8d5 --- /dev/null +++ b/crates/fbuild-core/src/channel.rs @@ -0,0 +1,40 @@ +//! Channel bridge — wraps `tokio::sync::mpsc`. +//! +//! FastLED/fbuild#844 (bridge sweep). All async channels in the +//! workspace flow through this module so the workspace has one source +//! of truth for the channel surface. The two matching dylints are: +//! +//! - `ban_std_mpsc_in_async_reachable` — `std::sync::mpsc::*` doesn't +//! integrate with the tokio reactor; blocking `recv()` from inside +//! an `async fn` starves the worker. +//! - `ban_tokio_mpsc_direct_import` — direct `tokio::sync::mpsc` +//! imports bypass this curated surface. +//! +//! The renames (`channel` → `bounded`, `unbounded_channel` → +//! `unbounded`) match the conventions zccache and a handful of other +//! Rust services use — the upstream names are awkward when both are in +//! scope. + +pub use tokio::sync::mpsc::{ + channel as bounded, unbounded_channel as unbounded, Receiver, Sender, UnboundedReceiver, + UnboundedSender, +}; + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn bounded_round_trip() { + let (tx, mut rx) = bounded::(4); + tx.send(7).await.unwrap(); + assert_eq!(rx.recv().await, Some(7)); + } + + #[tokio::test] + async fn unbounded_round_trip() { + let (tx, mut rx) = unbounded::(); + tx.send(42).unwrap(); + assert_eq!(rx.recv().await, Some(42)); + } +} diff --git a/crates/fbuild-core/src/fs.rs b/crates/fbuild-core/src/fs.rs new file mode 100644 index 000000000..565269658 --- /dev/null +++ b/crates/fbuild-core/src/fs.rs @@ -0,0 +1,217 @@ +//! Async filesystem bridge. +//! +//! FastLED/fbuild#844 (bridge sweep). All async filesystem access in +//! the workspace flows through this module so: +//! +//! 1. The workspace has **one** source of truth for `tokio::fs` — +//! swapping the backend (e.g. an `io_uring` impl) is a one-file +//! change. +//! 2. `std::fs::*` inside `async fn` is lint-banned +//! (`ban_std_fs_in_async`) because blocking the tokio worker on +//! filesystem I/O is a latency landmine. +//! 3. Direct `tokio::fs` imports are lint-banned +//! (`ban_tokio_fs_direct_import`) so the curated surface here stays +//! the canonical entry point. +//! +//! For `std::fs::*` in synchronous, non-`async` code paths (the +//! occasional `OnceLock` init, the synchronous main of a binary, +//! `#[cfg(test)]` blocks) the lint scope is narrow enough that direct +//! `std::fs` use is fine. The `spawn_blocking` escape hatch covers the +//! rare case where a synchronous filesystem call from inside async is +//! genuinely correct. +//! +//! ## Atomic writes +//! +//! [`write_atomic`] writes to a sibling temp path, `fsync`s, then +//! atomic-renames into place. Required for every state-file write that +//! corruption would block a rebuild (build fingerprint JSON, framework +//! discovery cache, etc.) — see FastLED/fbuild#844 "Bridge pair 6". + +// Curated re-export of the `tokio::fs` surface fbuild uses. Adding new +// items here is preferable to having callers `use tokio::fs` directly — +// the matching `ban_tokio_fs_direct_import` dylint enforces this. +pub use tokio::fs::{ + canonicalize, copy, create_dir, create_dir_all, hard_link, metadata, read, read_dir, + read_link, read_to_string, remove_dir, remove_dir_all, remove_file, rename, + set_permissions, symlink_metadata, write, DirEntry, File, OpenOptions, ReadDir, +}; + +use std::path::Path; +use std::sync::atomic::{AtomicU64, Ordering}; + +/// Process-monotonic nonce for atomic-write tempfile names. Combined +/// with the PID this guarantees uniqueness even if two threads race on +/// the same target path. +static WRITE_ATOMIC_NONCE: AtomicU64 = AtomicU64::new(0); + +/// Write a file atomically. Writes to `.tmp..`, +/// `fsync`s the file (and on POSIX the parent dir), then atomic-renames +/// into place. +/// +/// On POSIX `rename(2)` is atomic on the same filesystem. On Windows +/// tokio's `rename` lowers to `MoveFileExW` with +/// `MOVEFILE_REPLACE_EXISTING`, which is atomic on NTFS/ReFS. If a +/// caller is writing across filesystems, atomicity degrades to "at +/// most one of the two files exists after the write" — still strictly +/// better than the corrupt-mid-write hazard of `tokio::fs::write`. +/// +/// Use this for every state-file write that corruption would block a +/// rebuild — build fingerprint JSON, framework discovery cache, +/// clangd config, symbol caches, etc. See FastLED/fbuild#844 +/// "Bridge pair 6" for the migration list. +pub async fn write_atomic( + path: impl AsRef, + content: impl AsRef<[u8]>, +) -> std::io::Result<()> { + let path = path.as_ref(); + let nonce = WRITE_ATOMIC_NONCE.fetch_add(1, Ordering::Relaxed); + let pid = std::process::id(); + + // Build the temp path: `.tmp..`. Sibling of the + // target so the rename stays on the same filesystem. + let mut tmp_name = path + .file_name() + .map(|s| s.to_os_string()) + .unwrap_or_default(); + if tmp_name.is_empty() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "write_atomic: target path has no file name", + )); + } + tmp_name.push(format!(".tmp.{pid}.{nonce}")); + let tmp_path = match path.parent() { + Some(parent) => parent.join(&tmp_name), + None => std::path::PathBuf::from(&tmp_name), + }; + + // Ensure parent dir exists. Mirrors `tokio::fs::write`'s contract + // (which doesn't create parents) — leave that policy to the + // caller, but fail loudly if parent is missing rather than silently + // dropping the write into the void. + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() { + // Don't auto-create — if the parent is missing that's a + // caller bug worth surfacing. Just probe. + match tokio::fs::metadata(parent).await { + Ok(_) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + format!( + "write_atomic: parent directory does not exist: {}", + parent.display() + ), + )); + } + Err(e) => return Err(e), + } + } + } + + // Write + fsync the temp file via the async surface. `sync_all` + // on tokio's File lowers to the OS sync (`fsync` / `FlushFileBuffers`) + // through the tokio blocking pool — same syscall, just dispatched + // off the reactor thread. + { + use tokio::io::AsyncWriteExt; + let mut file = tokio::fs::File::create(&tmp_path).await?; + let write_res = file.write_all(content.as_ref()).await; + if let Err(e) = write_res { + let _ = tokio::fs::remove_file(&tmp_path).await; + return Err(e); + } + let sync_res = file.sync_all().await; + if let Err(e) = sync_res { + let _ = tokio::fs::remove_file(&tmp_path).await; + return Err(e); + } + // Drop the handle before rename — Windows requires the file to + // be closed (or opened FILE_SHARE_DELETE, which `tokio::fs` + // does not request) for `MoveFileExW` to succeed. + drop(file); + } + + // Atomic rename. On NTFS this is `MoveFileExW(..., + // MOVEFILE_REPLACE_EXISTING)`. On POSIX it's plain `rename(2)`. + if let Err(e) = tokio::fs::rename(&tmp_path, path).await { + let _ = tokio::fs::remove_file(&tmp_path).await; + return Err(e); + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[tokio::test] + async fn write_atomic_creates_file_with_expected_content() { + let dir = tempdir().unwrap(); + let target = dir.path().join("state.json"); + write_atomic(&target, b"{\"k\":1}").await.unwrap(); + let got = tokio::fs::read(&target).await.unwrap(); + assert_eq!(got, b"{\"k\":1}"); + } + + #[tokio::test] + async fn write_atomic_overwrites_existing_file() { + let dir = tempdir().unwrap(); + let target = dir.path().join("state.json"); + tokio::fs::write(&target, b"old").await.unwrap(); + write_atomic(&target, b"new").await.unwrap(); + let got = tokio::fs::read(&target).await.unwrap(); + assert_eq!(got, b"new"); + } + + #[tokio::test] + async fn write_atomic_does_not_leave_tempfile_on_success() { + let dir = tempdir().unwrap(); + let target = dir.path().join("state.json"); + write_atomic(&target, b"hello").await.unwrap(); + // No sibling `.tmp..` left behind. + let mut entries = tokio::fs::read_dir(dir.path()).await.unwrap(); + let mut count = 0; + while let Some(entry) = entries.next_entry().await.unwrap() { + count += 1; + let name = entry.file_name(); + let name = name.to_string_lossy(); + assert!( + !name.contains(".tmp."), + "leftover tempfile: {name}" + ); + } + assert_eq!(count, 1); + } + + #[tokio::test] + async fn write_atomic_errors_when_parent_missing() { + let dir = tempdir().unwrap(); + let target = dir.path().join("does_not_exist").join("state.json"); + let err = write_atomic(&target, b"x").await.unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); + } + + #[tokio::test] + async fn write_atomic_handles_concurrent_writes() { + // Two concurrent writes to the same target must each complete + // successfully (last-writer-wins) and the file must end up + // with one of the two payloads, never a torn write. + let dir = tempdir().unwrap(); + let target = dir.path().join("state.json"); + let t1 = { + let target = target.clone(); + tokio::spawn(async move { write_atomic(&target, b"AAAAA").await }) + }; + let t2 = { + let target = target.clone(); + tokio::spawn(async move { write_atomic(&target, b"BBBBB").await }) + }; + t1.await.unwrap().unwrap(); + t2.await.unwrap().unwrap(); + let got = tokio::fs::read(&target).await.unwrap(); + assert!(got == b"AAAAA" || got == b"BBBBB", "torn write: {got:?}"); + } +} diff --git a/crates/fbuild-core/src/http.rs b/crates/fbuild-core/src/http.rs new file mode 100644 index 000000000..e4c290356 --- /dev/null +++ b/crates/fbuild-core/src/http.rs @@ -0,0 +1,108 @@ +//! Shared HTTP client bridge. +//! +//! FastLED/fbuild#844 (bridge sweep) hoists `fbuild_packages::http::client()` +//! here so every reqwest construction in the workspace has one source of +//! truth. Lives in `fbuild-core` because every crate already depends on +//! `fbuild-core` (it's the bottom of the dependency graph). The matching +//! `ban_bare_reqwest` dylint forbids direct `reqwest::Client::new()` / +//! `reqwest::get` / `reqwest::ClientBuilder` outside this module. +//! +//! ## API surface +//! +//! - [`client()`] — process-shared async [`reqwest::Client`] (300s total, +//! 30s connect). Use this for every default-timeout call. +//! - [`client_with_timeout`] — per-call total-timeout override. Allocates +//! a fresh client; do not call in a tight loop. +//! - [`blocking_client`] — for the rare OS-thread case (e.g. the +//! `port_scan` worker pool that intentionally isolates reqwest off the +//! tokio reactor). +//! +//! ## Timeouts +//! +//! Defaults are long enough for slow CDN downloads of multi-MB toolchain +//! archives and short enough that a wedged server doesn't pin a +//! fbuild-daemon worker indefinitely. + +use std::sync::OnceLock; +use std::time::Duration; + +use reqwest::Client; + +/// Default per-request timeout. 5 minutes — sized for toolchain archive +/// downloads. +pub const DEFAULT_TIMEOUT: Duration = Duration::from_secs(300); + +/// Default connect timeout. 30 seconds — tight enough to surface DNS / +/// network breakage quickly, loose enough that a slow handshake doesn't +/// kill a download. +pub const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(30); + +static CLIENT: OnceLock = OnceLock::new(); + +/// Process-shared async [`reqwest::Client`]. Lazily built on first call. +/// +/// All HTTP traffic in fbuild flows through this client unless a per-call +/// timeout override is required (use [`client_with_timeout`] for that). +pub fn client() -> &'static Client { + CLIENT.get_or_init(|| { + Client::builder() + .timeout(DEFAULT_TIMEOUT) + .connect_timeout(DEFAULT_CONNECT_TIMEOUT) + .build() + .expect("reqwest client builder should never fail with these settings") + }) +} + +/// Build a client with a custom total timeout (for callers that need a +/// tighter deadline, e.g. a registry ping that should fail fast). +/// +/// Allocates a fresh client per call. Do not invoke in a tight loop — +/// reuse the returned client if multiple requests share the same +/// deadline. +pub fn client_with_timeout(total: Duration) -> Client { + Client::builder() + .timeout(total) + .connect_timeout(DEFAULT_CONNECT_TIMEOUT) + .build() + .expect("reqwest client builder should never fail with valid settings") +} + +/// Build a blocking [`reqwest::blocking::Client`] for the rare OS-thread +/// case. Used by `port_scan` so reqwest's blocking machinery stays off +/// the tokio reactor. +/// +/// Most callers should use [`client()`] (async) instead. The blocking +/// surface is intentionally narrow — if you find yourself reaching for +/// it inside an `async fn`, you almost certainly want [`client()`]. +pub fn blocking_client(total: Duration) -> reqwest::blocking::Client { + reqwest::blocking::Client::builder() + .timeout(total) + .connect_timeout(DEFAULT_CONNECT_TIMEOUT) + .build() + .expect("reqwest blocking client builder should never fail with valid settings") +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The shared client is stable across calls — same pointer. + #[test] + fn client_is_shared() { + let a = client(); + let b = client(); + assert!(std::ptr::eq(a, b)); + } + + /// `client_with_timeout` returns a freshly-built client. + #[test] + fn client_with_timeout_builds() { + let _c = client_with_timeout(Duration::from_secs(10)); + } + + /// `blocking_client` builds successfully — the OS-thread path. + #[test] + fn blocking_client_builds() { + let _c = blocking_client(Duration::from_secs(10)); + } +} diff --git a/crates/fbuild-core/src/lib.rs b/crates/fbuild-core/src/lib.rs index b588976ed..c60e1e8c5 100644 --- a/crates/fbuild-core/src/lib.rs +++ b/crates/fbuild-core/src/lib.rs @@ -7,16 +7,20 @@ //! - Size info parsing (avr-size / arm-none-eabi-size output) pub mod build_log; +pub mod channel; pub mod compiler_flags; pub mod containment; pub mod elapsed; pub mod emulator; +pub mod fs; +pub mod http; pub mod install_status; pub mod path; pub mod response_file; pub mod shell_split; pub mod subprocess; pub mod symbol_analysis; +pub mod time; pub mod usb; pub use build_log::BuildLog; diff --git a/crates/fbuild-core/src/path.rs b/crates/fbuild-core/src/path.rs index 16d324662..81b186acc 100644 --- a/crates/fbuild-core/src/path.rs +++ b/crates/fbuild-core/src/path.rs @@ -233,6 +233,52 @@ impl<'de> Deserialize<'de> for NormalizedPath { } } +/// Strip Windows extended-length / UNC prefix from a path. +/// +/// On Unix this is a no-op (returns the path unchanged). On Windows +/// it strips `\\?\` (extended-length) and `\\?\UNC\` (UNC normalized) +/// prefixes that `std::fs::canonicalize` injects, so cache keys and +/// log lines stay readable. See FastLED/fbuild#844 "Bridge pair 5". +#[must_use] +pub fn strip_unc_prefix(p: &Path) -> PathBuf { + #[cfg(windows)] + { + let s = p.to_string_lossy(); + // `\\?\UNC\server\share\...` → `\\server\share\...` + if let Some(rest) = s.strip_prefix(r"\\?\UNC\") { + let mut out = String::from(r"\\"); + out.push_str(rest); + return PathBuf::from(out); + } + // `\\?\C:\...` → `C:\...` + if let Some(rest) = s.strip_prefix(r"\\?\") { + return PathBuf::from(rest); + } + PathBuf::from(s.into_owned()) + } + #[cfg(not(windows))] + { + p.to_path_buf() + } +} + +/// Canonicalize an existing path, stripping the Windows UNC prefix and +/// wrapping in [`NormalizedPath`]. +/// +/// FastLED/fbuild#844 (bridge sweep, "Bridge pair 5"). All +/// `std::fs::canonicalize` / `tokio::fs::canonicalize` call sites +/// migrate to this so the workspace has one source of truth for +/// "canonical form" — `\\?\` prefix stripped, slashes normalized, +/// case-folded on case-insensitive platforms. +/// +/// Errors if the path does not exist or cannot be canonicalized +/// (matches `std::fs::canonicalize` semantics). +pub async fn canonicalize_existing(p: impl AsRef) -> std::io::Result { + let canon = tokio::fs::canonicalize(p.as_ref()).await?; + let stripped = strip_unc_prefix(&canon); + Ok(NormalizedPath::from(stripped)) +} + /// Normalize a path by resolving `.` and `..` components without /// touching the filesystem (no symlink resolution). /// @@ -499,4 +545,61 @@ mod tests { let n = NormalizedPath::new("/usr/bin/nm"); assert!(takes_path(&n)); } + + /// On non-Windows `strip_unc_prefix` is a no-op. + #[test] + #[cfg(not(windows))] + fn strip_unc_prefix_is_noop_on_unix() { + assert_eq!( + strip_unc_prefix(Path::new("/usr/bin/nm")), + PathBuf::from("/usr/bin/nm") + ); + } + + /// On Windows the extended-length prefix is stripped. + #[test] + #[cfg(windows)] + fn strip_unc_prefix_strips_extended_length_on_windows() { + assert_eq!( + strip_unc_prefix(Path::new(r"\\?\C:\Users\test")), + PathBuf::from(r"C:\Users\test") + ); + } + + /// On Windows the UNC-normalized prefix collapses to plain UNC. + #[test] + #[cfg(windows)] + fn strip_unc_prefix_collapses_unc_form_on_windows() { + assert_eq!( + strip_unc_prefix(Path::new(r"\\?\UNC\server\share\dir")), + PathBuf::from(r"\\server\share\dir") + ); + } + + /// `canonicalize_existing` returns a `NormalizedPath` that matches + /// the canonical form of an existing file. The temp dir trick is + /// the smallest "existing path" available in a unit test. + #[tokio::test] + async fn canonicalize_existing_returns_normalized_path() { + let dir = tempfile::tempdir().unwrap(); + let file = dir.path().join("probe.txt"); + std::fs::write(&file, b"hi").unwrap(); + let canon = canonicalize_existing(&file).await.unwrap(); + // Round-trip through `as_path` and back to `NormalizedPath` + // produces the same key. + assert_eq!( + canon.key(), + NormalizedPath::new(canon.as_path()).key() + ); + } + + /// `canonicalize_existing` propagates a `NotFound` for a missing + /// path — same contract as `tokio::fs::canonicalize`. + #[tokio::test] + async fn canonicalize_existing_errors_on_missing() { + let err = canonicalize_existing("/no/such/path/__fbuild_test_missing__") + .await + .unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); + } } diff --git a/crates/fbuild-core/src/time.rs b/crates/fbuild-core/src/time.rs new file mode 100644 index 000000000..2937b989d --- /dev/null +++ b/crates/fbuild-core/src/time.rs @@ -0,0 +1,77 @@ +//! Time bridge — tokio `sleep` + named `Duration` constants. +//! +//! FastLED/fbuild#844 (bridge sweep). All async sleeps in the workspace +//! flow through this module. The matching `ban_std_thread_sleep` dylint +//! forbids `std::thread::sleep` (which blocks the tokio worker). +//! +//! Named `Duration` constants live here so the workspace doesn't have +//! 335 distinct `Duration::from_secs(...)` literals with no shared +//! meaning. The set below covers the top 80% of the audit found in +//! #844 — extend as new categories emerge. + +pub use tokio::time::{interval, sleep, timeout, Duration}; + +// ---- HTTP timeouts ---- + +/// Short HTTP timeout — fast-fail registry pings, health checks. +pub const SHORT_HTTP_TIMEOUT: Duration = Duration::from_secs(2); + +/// Medium HTTP timeout — daemon-internal JSON RPC, status polls. +pub const MEDIUM_HTTP_TIMEOUT: Duration = Duration::from_secs(5); + +/// Long HTTP timeout — toolchain manifest downloads. +pub const LONG_HTTP_TIMEOUT: Duration = Duration::from_secs(15); + +// ---- Deploy / build deadlines ---- + +/// Post-deploy serial-port recovery deadline (3 s fast-poll). See +/// `Deployer::post_deploy_recovery` in `crates/fbuild-deploy/`. +pub const POST_DEPLOY_RECOVERY_DEADLINE: Duration = Duration::from_secs(3); + +/// Daemon long-operation cap (30 min). Anything longer than this is a +/// bug, not a slow user — surfaces with a structured timeout error so +/// the daemon doesn't pin a worker indefinitely. +pub const DAEMON_LONG_OP_TIMEOUT: Duration = Duration::from_secs(1800); + +/// "Real" build timeout — 15 min. Covers a cold-cache full-tree compile +/// of any board fbuild supports. +pub const REAL_BUILD_TIMEOUT: Duration = Duration::from_secs(900); + +// ---- Poll intervals ---- + +/// 50 ms poll — the lightest poll loop fbuild uses. Reserve for +/// readiness checks (port file appears, daemon is up). +pub const POLL_50MS: Duration = Duration::from_millis(50); + +/// 100 ms poll — standard interactive-feel poll. +pub const POLL_100MS: Duration = Duration::from_millis(100); + +/// 200 ms poll — background poll where the user isn't actively waiting. +pub const POLL_200MS: Duration = Duration::from_millis(200); + +/// 500 ms poll — slow background poll for long-running operations. +pub const POLL_500MS: Duration = Duration::from_millis(500); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn http_timeouts_are_monotonic() { + assert!(SHORT_HTTP_TIMEOUT < MEDIUM_HTTP_TIMEOUT); + assert!(MEDIUM_HTTP_TIMEOUT < LONG_HTTP_TIMEOUT); + } + + #[test] + fn poll_intervals_are_monotonic() { + assert!(POLL_50MS < POLL_100MS); + assert!(POLL_100MS < POLL_200MS); + assert!(POLL_200MS < POLL_500MS); + } + + #[test] + fn deploy_recovery_is_shorter_than_real_build() { + assert!(POST_DEPLOY_RECOVERY_DEADLINE < REAL_BUILD_TIMEOUT); + assert!(REAL_BUILD_TIMEOUT < DAEMON_LONG_OP_TIMEOUT); + } +} diff --git a/crates/fbuild-core/src/usb/data.rs b/crates/fbuild-core/src/usb/data.rs index 4884b6a85..9d1e5d4dd 100644 --- a/crates/fbuild-core/src/usb/data.rs +++ b/crates/fbuild-core/src/usb/data.rs @@ -223,7 +223,7 @@ fn proto_to_map(db: UsbVidDatabase) -> HashMap { /// the daemon could in principle skip the file dance — primary user is the /// resolver's own test suite. pub(crate) fn install_online_cache_map(map: HashMap) { - let mut guard = ONLINE_MAP.write().unwrap(); + let mut guard = ONLINE_MAP.write().unwrap_or_else(|e| e.into_inner()); *guard = Some(map); } diff --git a/crates/fbuild-daemon/src/device_manager.rs b/crates/fbuild-daemon/src/device_manager.rs index 6bbdee2a4..ff855917f 100644 --- a/crates/fbuild-daemon/src/device_manager.rs +++ b/crates/fbuild-daemon/src/device_manager.rs @@ -206,7 +206,7 @@ impl DeviceManager { /// *recently enough* — we just don't need one on every deploy. pub fn refresh_devices_if_stale(&self, max_age: std::time::Duration) -> bool { { - let last = self.last_refresh_at.lock().unwrap(); + let last = self.last_refresh_at.lock().unwrap_or_else(|e| e.into_inner()); if let Some(t) = *last { if t.elapsed() < max_age { return false; @@ -281,11 +281,11 @@ impl DeviceManager { .collect(); self.refresh_from_discovered(discovered); - *self.last_refresh_at.lock().unwrap() = Some(Instant::now()); + *self.last_refresh_at.lock().unwrap_or_else(|e| e.into_inner()) = Some(Instant::now()); } fn refresh_from_discovered(&self, discovered: Vec) { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); let now = Self::now_unix(); // Mark all devices as disconnected first. Track the previous @@ -326,7 +326,7 @@ impl DeviceManager { state.product_name = device.product_name; state.serial_number = device.serial_number; if let Some(previous_port) = state.previous_port.clone() { - self.recent_port_moves.lock().unwrap().push(DevicePortMove { + self.recent_port_moves.lock().unwrap_or_else(|e| e.into_inner()).push(DevicePortMove { previous_port, port: key.clone(), serial_number, @@ -386,17 +386,17 @@ impl DeviceManager { /// Get all devices. pub fn get_all_devices(&self) -> HashMap { - self.devices.lock().unwrap().clone() + self.devices.lock().unwrap_or_else(|e| e.into_inner()).clone() } pub fn take_recent_port_moves(&self) -> Vec { - let mut moves = self.recent_port_moves.lock().unwrap(); + let mut moves = self.recent_port_moves.lock().unwrap_or_else(|e| e.into_inner()); std::mem::take(&mut *moves) } /// Get status for a specific device (by port name). pub fn get_device_status(&self, port: &str) -> Option { - self.devices.lock().unwrap().get(port).cloned() + self.devices.lock().unwrap_or_else(|e| e.into_inner()).get(port).cloned() } /// Acquire an exclusive lease on a device. @@ -407,7 +407,7 @@ impl DeviceManager { description: &str, track_serial: bool, ) -> Result { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); let state = devices .get_mut(port) .ok_or_else(|| DeviceLeaseError::NotFound { @@ -455,7 +455,7 @@ impl DeviceManager { description: &str, track_serial: bool, ) -> Result { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); let state = devices .get_mut(port) .ok_or_else(|| DeviceLeaseError::NotFound { @@ -490,7 +490,7 @@ impl DeviceManager { /// Release a lease by lease_id (searches all devices). pub fn release_lease(&self, lease_id: &str) -> Result<(), String> { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); for state in devices.values_mut() { // Check exclusive if let Some(ref exc) = state.exclusive_lease { @@ -512,7 +512,7 @@ impl DeviceManager { /// Release all leases for a device (by port). pub fn release_device_leases(&self, port: &str) -> Result { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); let state = devices .get_mut(port) .ok_or_else(|| format!("device '{}' not found", port))?; @@ -541,7 +541,7 @@ impl DeviceManager { }); } - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); let state = devices .get_mut(port) .ok_or_else(|| DeviceLeaseError::NotFound { @@ -598,7 +598,7 @@ impl DeviceManager { /// Returns `None` in every other case, so the deploy handler falls /// back to the regular `verify-flash` path on any doubt. pub fn trusted_firmware_hash(&self, port: &str) -> Option<[u8; 32]> { - let devices = self.devices.lock().unwrap(); + let devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); let state = devices.get(port)?; if !state.is_connected { return None; @@ -621,7 +621,7 @@ impl DeviceManager { /// yet: the hash will be recorded on the next deploy once /// `refresh_devices` has picked the port up. pub fn set_trusted_firmware_hash(&self, port: &str, hash: [u8; 32]) { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); if let Some(state) = devices.get_mut(port) { state.trusted_firmware = Some(TrustedFirmwareHash { hash, @@ -634,7 +634,7 @@ impl DeviceManager { /// fails part-way through — partial writes mean we can't say /// what's on the chip anymore. pub fn clear_trusted_firmware_hash(&self, port: &str) { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); if let Some(state) = devices.get_mut(port) { state.trusted_firmware = None; } @@ -642,7 +642,7 @@ impl DeviceManager { /// Remove stale disconnected devices that have no leases. pub fn cleanup_stale_devices(&self) -> usize { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); let stale: Vec = devices .iter() .filter(|(_, s)| { @@ -659,7 +659,7 @@ impl DeviceManager { #[cfg(test)] pub(crate) fn insert_test_device(&self, port: &str) { - let mut devices = self.devices.lock().unwrap(); + let mut devices = self.devices.lock().unwrap_or_else(|e| e.into_inner()); devices.insert( port.to_string(), DeviceState { diff --git a/crates/fbuild-daemon/src/handlers/emulator/avr8js_headless.rs b/crates/fbuild-daemon/src/handlers/emulator/avr8js_headless.rs index 4cdfcb7ca..08b30c5ca 100644 --- a/crates/fbuild-daemon/src/handlers/emulator/avr8js_headless.rs +++ b/crates/fbuild-daemon/src/handlers/emulator/avr8js_headless.rs @@ -5,6 +5,7 @@ use super::avr8js_npm::{avr8js_cache_is_intact, REFRESH_EMU_CACHE_ENV}; use super::shared::{spawn_line_reader, ProcessEvent}; use crate::handlers::operations::{MonitorOutcome, MonitorState}; +use fbuild_core::channel::unbounded; use std::path::Path; use std::process::Stdio; @@ -99,7 +100,7 @@ pub(crate) async fn run_avr8js_headless( fbuild_core::FbuildError::DeployFailed("failed to capture avr8js stderr".to_string()) })?; - let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel::(); + let (tx, mut rx) = unbounded::(); let stdout_task = tokio::spawn(spawn_line_reader(stdout, false, tx.clone())); let stderr_task = tokio::spawn(spawn_line_reader(stderr, true, tx)); diff --git a/crates/fbuild-daemon/src/handlers/emulator/avr8js_npm.rs b/crates/fbuild-daemon/src/handlers/emulator/avr8js_npm.rs index e5999a983..b637e3177 100644 --- a/crates/fbuild-daemon/src/handlers/emulator/avr8js_npm.rs +++ b/crates/fbuild-daemon/src/handlers/emulator/avr8js_npm.rs @@ -11,7 +11,20 @@ pub(crate) async fn find_node() -> fbuild_core::Result { // captured by the daemon's containment group (issue #32). The probe // is short-lived (`node --version`) but a missing binary should // still bubble up the same way. - match fbuild_core::subprocess::run_command(&[node, "--version"], None, None, None).await { + // + // FastLED/fbuild#844: explicit 5 s timeout. `node --version` is a + // sub-100ms operation on every supported host — anything longer is a + // wedged process node and we want to fall through to the + // "not found" error path quickly so the user sees the install + // hint instead of a stalled emulator launch. + match fbuild_core::subprocess::run_command( + &[node, "--version"], + None, + None, + Some(std::time::Duration::from_secs(5)), + ) + .await + { Ok(output) if output.success() => Ok(PathBuf::from(node)), _ => Err(fbuild_core::FbuildError::DeployFailed( "Node.js is required for headless avr8js emulation but 'node' was not found on PATH. \ @@ -153,8 +166,14 @@ pub(crate) async fn ensure_avr8js_npm_in( // Route through `run_command` (which spawns via the daemon's // containment group) so an `npm install` killed mid-flight doesn't // leak node processes after the daemon dies. See FastLED/fbuild#32. + // + // FastLED/fbuild#844: explicit no-timeout. A cold `npm install` of + // avr8js@0.21.0 can take 30 s on a fast box and several minutes on + // a corporate-proxy + slow-disk Windows host; the 15 min default + // subprocess cap would mask that as a generic timeout. We rely on + // the daemon's containment + parent-shutdown signal instead. let cache_dir_str = cache_dir.to_string_lossy().to_string(); - let output = fbuild_core::subprocess::run_command( + let output = fbuild_core::subprocess::run_command_no_timeout( &[ npm, "install", @@ -165,7 +184,6 @@ pub(crate) async fn ensure_avr8js_npm_in( ], None, None, - None, ) .await .map_err(|e| { diff --git a/crates/fbuild-daemon/src/handlers/emulator/runners.rs b/crates/fbuild-daemon/src/handlers/emulator/runners.rs index b671f6e46..428b28053 100644 --- a/crates/fbuild-daemon/src/handlers/emulator/runners.rs +++ b/crates/fbuild-daemon/src/handlers/emulator/runners.rs @@ -83,7 +83,7 @@ impl EmulatorRunner for QemuRunner { let qemu = resolve_esp_qemu_for_mcu(&self.project_dir, &self.board.mcu).await?; let session_dir = qemu_session_dir(&self.project_dir, &self.env_name); - std::fs::create_dir_all(&session_dir)?; + fbuild_core::fs::create_dir_all(&session_dir).await?; let flash_image = session_dir.join("qemu_flash.bin"); @@ -185,7 +185,7 @@ impl EmulatorRunner for Avr8jsRunner { // (`include_str!`), so concurrent writes are idempotent. // See FastLED/fbuild#291. let script_path = avr8js_cache.join("headless.mjs"); - std::fs::write(&script_path, AVR8JS_HEADLESS_MJS)?; + fbuild_core::fs::write(&script_path, AVR8JS_HEADLESS_MJS).await?; let f_cpu_hz: u32 = self .board @@ -247,7 +247,19 @@ async fn find_simavr() -> fbuild_core::Result { // Try running simavr to verify it exists; route through containment // (issue #32). This is a short-lived probe so the containment // difference is purely consistency. - match fbuild_core::subprocess::run_command(&[simavr, "--help"], None, None, None).await { + // + // FastLED/fbuild#844: explicit 5 s timeout. `simavr --help` prints a + // static usage string in well under 100 ms on every supported host — + // anything longer is a wedged binary and we want to fall through to + // the "not found" install hint quickly. + match fbuild_core::subprocess::run_command( + &[simavr, "--help"], + None, + None, + Some(std::time::Duration::from_secs(5)), + ) + .await + { Ok(_) => Ok(PathBuf::from(simavr)), Err(_) => { let install_hint = if cfg!(target_os = "linux") { diff --git a/crates/fbuild-daemon/src/handlers/emulator/shared.rs b/crates/fbuild-daemon/src/handlers/emulator/shared.rs index 849cf4f92..fb728b65c 100644 --- a/crates/fbuild-daemon/src/handlers/emulator/shared.rs +++ b/crates/fbuild-daemon/src/handlers/emulator/shared.rs @@ -7,6 +7,7 @@ //! flags, `monitor_outcome_to_emulator`). use crate::handlers::operations::{MonitorOutcome, MonitorState}; +use fbuild_core::channel::{unbounded, UnboundedSender}; use fbuild_core::emulator::EmulatorOutcome; use fbuild_packages::{Package, Toolchain}; use std::path::{Path, PathBuf}; @@ -167,7 +168,7 @@ fn apply_windows_process_flags(_cmd: &mut tokio::process::Command, _exe_path: &P pub(crate) async fn spawn_line_reader( stream: impl tokio::io::AsyncRead + Unpin + Send + 'static, is_stderr: bool, - tx: tokio::sync::mpsc::UnboundedSender, + tx: UnboundedSender, ) { let mut lines = BufReader::new(stream).lines(); while let Ok(Some(line)) = lines.next_line().await { @@ -214,7 +215,7 @@ pub(crate) async fn run_qemu_process( fbuild_core::FbuildError::DeployFailed(format!("failed to capture {} stderr", label)) })?; - let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel::(); + let (tx, mut rx) = unbounded::(); let stdout_task = tokio::spawn(spawn_line_reader(stdout, false, tx.clone())); let stderr_task = tokio::spawn(spawn_line_reader(stderr, true, tx)); diff --git a/crates/fbuild-daemon/src/handlers/operations/build.rs b/crates/fbuild-daemon/src/handlers/operations/build.rs index 8d9a6c0ba..f06d22a67 100644 --- a/crates/fbuild-daemon/src/handlers/operations/build.rs +++ b/crates/fbuild-daemon/src/handlers/operations/build.rs @@ -8,6 +8,7 @@ use crate::models::{BuildRequest, OperationResponse}; use axum::extract::State; use axum::http::StatusCode; use axum::Json; +use fbuild_core::channel::{unbounded, UnboundedSender}; use std::path::PathBuf; use std::sync::atomic::Ordering; use std::sync::Arc; @@ -18,13 +19,13 @@ use std::sync::Arc; /// `stream error: error decoding response body` from reqwest with no clue /// what went wrong. See fbuild#401. struct StreamTerminationGuard { - tx: tokio::sync::mpsc::UnboundedSender, + tx: UnboundedSender, request_id: String, completed: bool, } impl StreamTerminationGuard { - fn new(tx: tokio::sync::mpsc::UnboundedSender, request_id: String) -> Self { + fn new(tx: UnboundedSender, request_id: String) -> Self { Self { tx, request_id, @@ -58,7 +59,7 @@ impl Drop for StreamTerminationGuard { } fn send_stream_status_event( - tx: &tokio::sync::mpsc::UnboundedSender, + tx: &UnboundedSender, ctx: &DaemonContext, request_id: &str, message: impl Into, @@ -207,8 +208,8 @@ pub async fn build( // been removed — `UnboundedSender::send` is sync and callable from // any context, and `UnboundedReceiver::recv` is awaited directly // from the async forwarder task. - let (log_tx, mut log_rx) = tokio::sync::mpsc::unbounded_channel::(); - let (async_tx, async_rx) = tokio::sync::mpsc::unbounded_channel::(); + let (log_tx, mut log_rx) = unbounded::(); + let (async_tx, async_rx) = unbounded::(); let params = fbuild_build::BuildParams { project_dir: project_dir.clone(), diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index 8ab472125..979a14f8f 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -6,11 +6,12 @@ use crate::context::DaemonContext; use axum::extract::ws::{Message, WebSocket}; use axum::extract::{Path, State, WebSocketUpgrade}; use axum::response::IntoResponse; +use fbuild_core::channel as mpsc; use fbuild_serial::{SerialClientMessage, SerialServerMessage, SerialStreamEvent}; use futures::{SinkExt, StreamExt}; use std::sync::Arc; use std::time::Duration; -use tokio::sync::{mpsc, oneshot}; +use tokio::sync::oneshot; /// Serialize a `SerialServerMessage` (or any `serde::Serialize` value) /// to JSON, falling back to a hardcoded JSON error frame if serialization @@ -268,7 +269,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { // mismatch, which is exactly what the device-burst case needs. // See FastLED/fbuild#749. - let (out_tx, mut out_rx) = mpsc::unbounded_channel::(); + let (out_tx, mut out_rx) = mpsc::unbounded::(); let (mut ws_sink, mut ws_stream) = socket.split(); // Inbound -> reader control channel (#756). Inbound issues Drain / // GetDepth requests on this; reader handles them inline alongside @@ -276,7 +277,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { // the inbound task's ClearBuffer / GetInWaiting handlers, which // emit at most one message per client RPC -- bounded capacity // would only add deadlock corner cases for no real win. - let (control_tx, mut control_rx) = mpsc::unbounded_channel::(); + let (control_tx, mut control_rx) = mpsc::unbounded::(); // READER task -- broadcast -> mpsc queue. let reader_handle = { diff --git a/crates/fbuild-daemon/src/handlers/websockets_tests.rs b/crates/fbuild-daemon/src/handlers/websockets_tests.rs index 654d7af6e..20ae3b7d3 100644 --- a/crates/fbuild-daemon/src/handlers/websockets_tests.rs +++ b/crates/fbuild-daemon/src/handlers/websockets_tests.rs @@ -83,7 +83,7 @@ async fn run_toy_reader( #[tokio::test] async fn reader_control_drain_reports_drop_count() { let (bcast_tx, bcast_rx) = broadcast::channel::(16); - let (ctl_tx, ctl_rx) = mpsc::unbounded_channel(); + let (ctl_tx, ctl_rx) = mpsc::unbounded(); let reader = tokio::spawn(run_toy_reader(bcast_rx, ctl_rx)); // Push 5 events, do NOT let the reader drain them via its @@ -128,7 +128,7 @@ async fn reader_control_drain_reports_drop_count() { #[tokio::test] async fn reader_control_get_depth_reports_broadcast_length() { let (bcast_tx, bcast_rx) = broadcast::channel::(16); - let (ctl_tx, ctl_rx) = mpsc::unbounded_channel(); + let (ctl_tx, ctl_rx) = mpsc::unbounded(); let reader = tokio::spawn(run_toy_reader(bcast_rx, ctl_rx)); for i in 0..3u32 { diff --git a/crates/fbuild-daemon/src/status_manager.rs b/crates/fbuild-daemon/src/status_manager.rs index f9a2b0a4c..1e7b7cf11 100644 --- a/crates/fbuild-daemon/src/status_manager.rs +++ b/crates/fbuild-daemon/src/status_manager.rs @@ -191,7 +191,15 @@ impl StatusManager { } } - /// Atomic write: write to temp file then rename (prevents partial reads). + /// Atomic write: route through the canonical async helper + /// `fbuild_core::fs::write_atomic` so the daemon-side state file + /// uses the same write-tempfile / fsync / rename path as every + /// other state-file writer (FastLED/fbuild#844 bridge pair 6). + /// + /// Called from synchronous code paths inside the tokio runtime, so + /// we bridge to the async helper via `block_in_place` + + /// `Handle::block_on`. Matches the pattern in + /// `fbuild_packages::toolchain::esp32_metadata::resolve_toolchain_url_sync`. fn write_atomic(&self, status: &DaemonStatus) { let json = match serde_json::to_string_pretty(status) { Ok(j) => j, @@ -201,27 +209,36 @@ impl StatusManager { } }; - // Ensure parent directory exists + // Ensure parent directory exists. `fbuild_core::fs::write_atomic` + // probes for it and errors if missing — surfacing that as a + // simple `create_dir_all` keeps the daemon's bootstrap behaviour. if let Some(parent) = self.path.parent() { let _ = std::fs::create_dir_all(parent); } - // Write to temp file then rename for atomicity - let tmp_path = self.path.with_extension("json.tmp"); - if let Err(e) = std::fs::write(&tmp_path, json) { - tracing::warn!("failed to write daemon status temp file: {}", e); - return; - } - if let Err(e) = std::fs::rename(&tmp_path, &self.path) { - // Fallback: on some Windows configurations rename across volumes may fail + let path = self.path.clone(); + let result = if let Ok(handle) = tokio::runtime::Handle::try_current() { + tokio::task::block_in_place(|| { + handle.block_on(fbuild_core::fs::write_atomic(&path, json.as_bytes())) + }) + } else { + // No tokio runtime — spin up a one-shot. Path hit only by + // unit tests and the (rare) standalone bootstrap before the + // daemon's runtime exists. + match tokio::runtime::Runtime::new() { + Ok(rt) => rt.block_on(fbuild_core::fs::write_atomic(&path, json.as_bytes())), + Err(e) => { + tracing::warn!("failed to create tokio runtime for status write: {}", e); + return; + } + } + }; + if let Err(e) = result { tracing::warn!( - "failed to rename daemon status file, falling back to direct write: {}", + "failed to atomically write daemon status to {}: {}", + self.path.display(), e ); - if let Ok(contents) = std::fs::read_to_string(&tmp_path) { - let _ = std::fs::write(&self.path, contents); - } - let _ = std::fs::remove_file(&tmp_path); } } } diff --git a/crates/fbuild-daemon/tests/build_streaming.rs b/crates/fbuild-daemon/tests/build_streaming.rs index df504ec53..60a96596e 100644 --- a/crates/fbuild-daemon/tests/build_streaming.rs +++ b/crates/fbuild-daemon/tests/build_streaming.rs @@ -56,7 +56,7 @@ async fn streaming_build_failure_emits_log_before_result() { "verbose": false, }); - let resp = reqwest::Client::new() + let resp = fbuild_core::http::client_with_timeout(Duration::from_secs(15)) .post(format!("http://{}/api/build", addr)) .json(&body) .timeout(Duration::from_secs(10)) diff --git a/crates/fbuild-daemon/tests/test_emu_endpoint.rs b/crates/fbuild-daemon/tests/test_emu_endpoint.rs index 10665fcb6..21b62dc84 100644 --- a/crates/fbuild-daemon/tests/test_emu_endpoint.rs +++ b/crates/fbuild-daemon/tests/test_emu_endpoint.rs @@ -72,7 +72,7 @@ async fn test_emu_endpoint_returns_structured_error_for_missing_project() { "verbose": false, }); - let resp = reqwest::Client::new() + let resp = fbuild_core::http::client_with_timeout(Duration::from_secs(10)) .post(format!("http://{}/api/test-emu", addr)) .json(&body) .timeout(Duration::from_secs(5)) diff --git a/crates/fbuild-deploy/src/lib.rs b/crates/fbuild-deploy/src/lib.rs index b34d2cfbd..cf7396c69 100644 --- a/crates/fbuild-deploy/src/lib.rs +++ b/crates/fbuild-deploy/src/lib.rs @@ -128,7 +128,7 @@ pub trait Deployer: Send + Sync { /// // TODO(#605 Phase 1): LPC + CMSIS-DAP wedge-recovery override. async fn post_deploy_recovery(&self, port: &str) -> Result<()> { - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(3); + let deadline = std::time::Instant::now() + fbuild_core::time::POST_DEPLOY_RECOVERY_DEADLINE; let port = port.to_string(); while std::time::Instant::now() < deadline { // serialport::new(...).open() is blocking; offload it. Each @@ -146,7 +146,7 @@ pub trait Deployer: Send + Sync { if opened { return Ok(()); } - tokio::time::sleep(std::time::Duration::from_millis(100)).await; + fbuild_core::time::sleep(fbuild_core::time::POLL_100MS).await; } tracing::warn!("USB re-enumeration: port {} not available after 3s", port); Ok(()) diff --git a/crates/fbuild-header-scan/src/walker.rs b/crates/fbuild-header-scan/src/walker.rs index aae2c35b1..9933fbfa9 100644 --- a/crates/fbuild-header-scan/src/walker.rs +++ b/crates/fbuild-header-scan/src/walker.rs @@ -198,6 +198,12 @@ fn resolve_include(inc: &IncludeRef, from: &Path, search_paths: &[PathBuf]) -> O } fn canon(p: &Path) -> PathBuf { + // FastLED/fbuild#844 sync-context allowlist: this helper runs inside + // a rayon-parallel BFS (`walk_with_state`). Using + // `fbuild_core::path::canonicalize_existing(...).await` would force + // the walker async, which would cascade through every downstream LDF + // caller. File is allowlisted in + // `dylints/ban_std_fs_canonicalize/src/allowlist.txt`. std::fs::canonicalize(p).unwrap_or_else(|_| p.to_path_buf()) } diff --git a/crates/fbuild-library-select/src/lib.rs b/crates/fbuild-library-select/src/lib.rs index 91e56773f..c72a467ed 100644 --- a/crates/fbuild-library-select/src/lib.rs +++ b/crates/fbuild-library-select/src/lib.rs @@ -204,6 +204,12 @@ pub fn resolve_with_stats( } fn canon(p: &Path) -> PathBuf { + // FastLED/fbuild#844 sync-context allowlist: `resolve_with_stats` + // is sync (called from the daemon's `BuildOrchestrator` chain and + // from the diagnostic `fbuild lib-select` CLI). Making it async to + // adopt `fbuild_core::path::canonicalize_existing` would cascade + // through every caller. File is allowlisted in + // `dylints/ban_std_fs_canonicalize/src/allowlist.txt`. match std::fs::canonicalize(p) { Ok(c) => c, Err(err) => { diff --git a/crates/fbuild-packages/src/disk_cache/index/mod.rs b/crates/fbuild-packages/src/disk_cache/index/mod.rs index 86432f30c..a1e031788 100644 --- a/crates/fbuild-packages/src/disk_cache/index/mod.rs +++ b/crates/fbuild-packages/src/disk_cache/index/mod.rs @@ -125,7 +125,7 @@ impl CacheIndex { } fn migrate(&self) -> rusqlite::Result<()> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); Self::run_migrations(&conn) } @@ -178,7 +178,7 @@ impl CacheIndex { /// Get the current schema version. pub fn schema_version(&self) -> rusqlite::Result { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.query_row( "SELECT value FROM cache_meta WHERE key = 'schema_version'", [], diff --git a/crates/fbuild-packages/src/disk_cache/index/queries.rs b/crates/fbuild-packages/src/disk_cache/index/queries.rs index aabd582cf..ec4117c78 100644 --- a/crates/fbuild-packages/src/disk_cache/index/queries.rs +++ b/crates/fbuild-packages/src/disk_cache/index/queries.rs @@ -20,7 +20,7 @@ impl CacheIndex { ) -> rusqlite::Result> { use rusqlite::OptionalExtension; let (_, hash) = paths::stem_and_hash(url); - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.query_row( "SELECT id, kind, url, stem, hash, version, archive_path, archive_bytes, archive_sha256, @@ -46,7 +46,7 @@ impl CacheIndex { ) -> rusqlite::Result { let (stem, hash) = paths::stem_and_hash(url); let now = Self::now_epoch(); - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.execute( "INSERT INTO entries (kind, url, stem, hash, version, archive_path, archive_bytes, archive_sha256, @@ -88,7 +88,7 @@ impl CacheIndex { ) -> rusqlite::Result { let (stem, hash) = paths::stem_and_hash(url); let now = Self::now_epoch(); - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.execute( "INSERT INTO entries (kind, url, stem, hash, version, installed_path, installed_bytes, installed_at, @@ -120,7 +120,7 @@ impl CacheIndex { /// Bump LRU timestamp and use count for an entry. pub fn touch(&self, entry_id: i64) -> rusqlite::Result<()> { let now = Self::now_epoch(); - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.execute( "UPDATE entries SET last_used_at = ?1, use_count = use_count + 1 WHERE id = ?2", params![now, entry_id], @@ -133,7 +133,7 @@ impl CacheIndex { /// process correctly track independent pins. pub fn pin(&self, entry_id: i64, holder_pid: u32, holder_nonce: u64) -> rusqlite::Result<()> { let now = Self::now_epoch(); - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); let updated = conn.execute( "UPDATE leases SET refcount = refcount + 1 WHERE entry_id = ?1 AND holder_pid = ?2 AND holder_nonce = ?3", @@ -156,7 +156,7 @@ impl CacheIndex { /// Decrement the pinned count for an entry (lease released). /// Decrements refcount; removes the row when it reaches zero. pub fn unpin(&self, entry_id: i64, holder_pid: u32, holder_nonce: u64) -> rusqlite::Result<()> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.execute( "UPDATE leases SET refcount = refcount - 1 WHERE entry_id = ?1 AND holder_pid = ?2 AND holder_nonce = ?3", @@ -176,7 +176,7 @@ impl CacheIndex { /// Get total bytes for all archives. pub fn total_archive_bytes(&self) -> rusqlite::Result { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.query_row( "SELECT COALESCE(SUM(archive_bytes), 0) FROM entries WHERE archive_path IS NOT NULL", [], @@ -186,7 +186,7 @@ impl CacheIndex { /// Get total bytes for all installed entries. pub fn total_installed_bytes(&self) -> rusqlite::Result { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.query_row( "SELECT COALESCE(SUM(installed_bytes), 0) FROM entries WHERE installed_path IS NOT NULL", [], @@ -196,7 +196,7 @@ impl CacheIndex { /// Get LRU installed entries (oldest first), skipping pinned. pub fn lru_installed_entries(&self, limit: usize) -> rusqlite::Result> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); let mut stmt = conn.prepare( "SELECT id, kind, url, stem, hash, version, archive_path, archive_bytes, archive_sha256, @@ -216,7 +216,7 @@ impl CacheIndex { /// Get LRU archive entries (oldest first), skipping pinned. pub fn lru_archive_entries(&self, limit: usize) -> rusqlite::Result> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); let mut stmt = conn.prepare( "SELECT id, kind, url, stem, hash, version, archive_path, archive_bytes, archive_sha256, @@ -236,7 +236,7 @@ impl CacheIndex { /// Null out the installed_path for an entry (after evicting its directory). pub fn clear_installed(&self, entry_id: i64) -> rusqlite::Result<()> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.execute( "UPDATE entries SET installed_path = NULL, installed_bytes = NULL, installed_at = NULL WHERE id = ?1", params![entry_id], @@ -246,7 +246,7 @@ impl CacheIndex { /// Null out the archive_path for an entry (after evicting its archive). pub fn clear_archive(&self, entry_id: i64) -> rusqlite::Result<()> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.execute( "UPDATE entries SET archive_path = NULL, archive_bytes = NULL, archive_sha256 = NULL, archived_at = NULL WHERE id = ?1", params![entry_id], @@ -256,14 +256,14 @@ impl CacheIndex { /// Delete an entry entirely (when both archive and installed are gone). pub fn delete_entry(&self, entry_id: i64) -> rusqlite::Result<()> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.execute("DELETE FROM entries WHERE id = ?1", params![entry_id])?; Ok(()) } /// Reap leases for dead PIDs. pub fn reap_dead_leases(&self) -> rusqlite::Result { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); let mut stmt = conn.prepare("SELECT DISTINCT holder_pid FROM leases")?; let pids: Vec = stmt .query_map([], |row| row.get::<_, i64>(0))? @@ -291,7 +291,7 @@ impl CacheIndex { /// Get all entries (for reconciliation). pub fn all_entries(&self) -> rusqlite::Result> { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); let mut stmt = conn.prepare( "SELECT id, kind, url, stem, hash, version, archive_path, archive_bytes, archive_sha256, @@ -311,7 +311,7 @@ impl CacheIndex { pub fn lookup_latest(&self, kind: Kind, url: &str) -> rusqlite::Result> { use rusqlite::OptionalExtension; let (_, hash) = paths::stem_and_hash(url); - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.query_row( "SELECT id, kind, url, stem, hash, version, archive_path, archive_bytes, archive_sha256, @@ -329,7 +329,7 @@ impl CacheIndex { /// Get total entry count. pub fn entry_count(&self) -> rusqlite::Result { - let conn = self.conn.lock().unwrap(); + let conn = self.conn.lock().unwrap_or_else(|e| e.into_inner()); conn.query_row("SELECT COUNT(*) FROM entries", [], |row| { row.get::<_, i64>(0) }) diff --git a/crates/fbuild-packages/src/http.rs b/crates/fbuild-packages/src/http.rs index 6512d1cad..d76eda707 100644 --- a/crates/fbuild-packages/src/http.rs +++ b/crates/fbuild-packages/src/http.rs @@ -1,42 +1,14 @@ -//! Shared async `reqwest::Client` with configured timeouts. +//! Forward-compat alias for the HTTP client bridge. //! -//! FastLED/fbuild#813 (async migration) + #805 (timeout audit): -//! every HTTP call in fbuild-packages goes through this client. -//! Tokio-console sees the I/O because the underlying tokio -//! reactor is the daemon's. Timeouts default to safe values; per- -//! call overrides via `client_with_timeout(...)`. - -use std::sync::OnceLock; -use std::time::Duration; - -use reqwest::Client; - -/// Default per-request timeout. Long enough for slow CDN downloads -/// of multi-MB toolchain archives, short enough that a wedged -/// server doesn't pin a fbuild-daemon worker indefinitely. -const DEFAULT_TIMEOUT: Duration = Duration::from_secs(300); // 5 min -const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(30); - -static CLIENT: OnceLock = OnceLock::new(); - -/// Get the shared client. Lazily built on first call. -pub fn client() -> &'static Client { - CLIENT.get_or_init(|| { - Client::builder() - .timeout(DEFAULT_TIMEOUT) - .connect_timeout(DEFAULT_CONNECT_TIMEOUT) - .build() - .expect("reqwest client builder should never fail with these settings") - }) -} +//! FastLED/fbuild#844 hoisted the shared `reqwest::Client` to +//! `fbuild_core::http`. This module is a re-export so existing +//! `use fbuild_packages::http::client;` call sites compile until the +//! Phase 2 migration sweep moves them to `fbuild_core::http::client()`. +//! +//! New code should import directly from `fbuild_core::http`. The +//! `ban_bare_reqwest` dylint forbids `reqwest::Client::new()` outside +//! the bridge. -/// Build a client with a custom total timeout (for callers that -/// need a tighter deadline, e.g. a registry ping that should -/// fail fast). -pub fn client_with_timeout(total: Duration) -> Client { - Client::builder() - .timeout(total) - .connect_timeout(DEFAULT_CONNECT_TIMEOUT) - .build() - .expect("reqwest client builder should never fail with valid settings") -} +pub use fbuild_core::http::{ + blocking_client, client, client_with_timeout, DEFAULT_CONNECT_TIMEOUT, DEFAULT_TIMEOUT, +}; diff --git a/crates/fbuild-packages/src/lib.rs b/crates/fbuild-packages/src/lib.rs index 1dea4eee9..0bfd2a8c7 100644 --- a/crates/fbuild-packages/src/lib.rs +++ b/crates/fbuild-packages/src/lib.rs @@ -470,7 +470,7 @@ fn package_touch_key( fn mark_package_touch_needed(key: String) -> bool { let touched = PACKAGE_TOUCHES.get_or_init(|| Mutex::new(HashSet::new())); - let mut touched = touched.lock().unwrap(); + let mut touched = touched.lock().unwrap_or_else(|e| e.into_inner()); touched.insert(key) } diff --git a/crates/fbuild-packages/src/library/arduino_api.rs b/crates/fbuild-packages/src/library/arduino_api.rs index c82e28f72..0d589c562 100644 --- a/crates/fbuild-packages/src/library/arduino_api.rs +++ b/crates/fbuild-packages/src/library/arduino_api.rs @@ -44,10 +44,13 @@ pub async fn ensure_arduino_api(core_dir: &Path) -> Result<()> { core_dir.display() ); - // Download to a temp directory - let tmp_dir = tempfile::TempDir::new().map_err(|e| { - fbuild_core::FbuildError::PackageError(format!("failed to create temp dir: {}", e)) - })?; + // Download to a temp directory rooted under + // `~/.fbuild/{dev|prod}/tmp/arduino-api/` — FastLED/fbuild#844 + // bridge pair 10. + let tmp_dir = + tempfile::TempDir::new_in(fbuild_paths::temp_subdir("arduino-api")).map_err(|e| { + fbuild_core::FbuildError::PackageError(format!("failed to create temp dir: {}", e)) + })?; // Async HTTP via the shared client (FastLED/fbuild#813). let response = http::client() diff --git a/crates/fbuild-packages/src/library/esp32_framework/libs.rs b/crates/fbuild-packages/src/library/esp32_framework/libs.rs index db62ef8d9..80f9ca43a 100644 --- a/crates/fbuild-packages/src/library/esp32_framework/libs.rs +++ b/crates/fbuild-packages/src/library/esp32_framework/libs.rs @@ -123,7 +123,12 @@ impl Esp32Framework { } // Extract to a short temp path to avoid Windows MAX_PATH (260 char) limit. - let temp_dir = tempfile::Builder::new().prefix("fbuild_sdk_").tempdir()?; + // Rooted under `~/.fbuild/{dev|prod}/tmp/esp32-framework/` so the + // extract scratch dir is reachable from a single user-visible + // location — FastLED/fbuild#844 bridge pair 10. + let temp_dir = tempfile::Builder::new() + .prefix("fbuild_sdk_") + .tempdir_in(fbuild_paths::temp_subdir("esp32-framework"))?; tracing::info!( "extracting ESP32 SDK libs ({} MB)", @@ -171,7 +176,11 @@ impl Esp32Framework { crate::downloader::download_file(libs_url, &tools_dir).await?; } - let temp_dir = tempfile::Builder::new().prefix("fbuild_skel_").tempdir()?; + // Rooted under `~/.fbuild/{dev|prod}/tmp/esp32-framework/` — + // FastLED/fbuild#844 bridge pair 10. + let temp_dir = tempfile::Builder::new() + .prefix("fbuild_skel_") + .tempdir_in(fbuild_paths::temp_subdir("esp32-framework"))?; tracing::info!("extracting {} skeleton libs", mcu); crate::extractor::extract(&archive_path, temp_dir.path())?; diff --git a/crates/fbuild-packages/src/library/library_compiler.rs b/crates/fbuild-packages/src/library/library_compiler.rs index 39438520a..20f34324d 100644 --- a/crates/fbuild-packages/src/library/library_compiler.rs +++ b/crates/fbuild-packages/src/library/library_compiler.rs @@ -370,7 +370,17 @@ async fn compile_one_source( }; let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - let result = run_command(&args_ref, None, None, None).await?; + // FastLED/fbuild#844: explicit 15 min cap. A single-file compile + // (cold-cache, full templated FastLED translation unit) is the + // worst-case path that still finishes inside REAL_BUILD_TIMEOUT; + // anything beyond that is wedged toolchain rather than a slow build. + let result = run_command( + &args_ref, + None, + None, + Some(fbuild_core::time::REAL_BUILD_TIMEOUT), + ) + .await?; if !result.success() { return Err(FbuildError::BuildFailed(format!( @@ -549,7 +559,19 @@ async fn archive_objects(ar_path: &Path, objects: &[PathBuf], output: &Path) -> } let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - let result = run_command(&args_ref, None, None, None).await?; + // FastLED/fbuild#844: explicit 60 s cap. `ar rcs` over a large + // FastLED object set (hundreds of .o files) finishes in <10 s on + // every host fbuild supports; anything past a minute is a disk / + // filesystem fault rather than legitimately slow work, so we cap + // tightly to surface those failures fast instead of waiting out + // the 15 min default. + let result = run_command( + &args_ref, + None, + None, + Some(std::time::Duration::from_secs(60)), + ) + .await?; if !result.success() { return Err(FbuildError::BuildFailed(format!( diff --git a/crates/fbuild-packages/src/library/library_manager.rs b/crates/fbuild-packages/src/library/library_manager.rs index 152a8cd82..261e3ad50 100644 --- a/crates/fbuild-packages/src/library/library_manager.rs +++ b/crates/fbuild-packages/src/library/library_manager.rs @@ -3,14 +3,35 @@ //! Coordinates: spec parsing → download → include discovery → compile → archive. use std::path::{Path, PathBuf}; +use std::sync::OnceLock; -use fbuild_core::Result; +use fbuild_core::{FbuildError, Result}; +use tokio::runtime::Runtime; use super::library_compiler; use super::library_downloader; use super::library_info::InstalledLibrary; use super::library_spec::LibrarySpec; +/// Module-level fallback runtime for sync bridge entry points. +/// +/// Constructed lazily on first sync invocation that occurs outside an +/// existing Tokio runtime. Reusing one runtime across calls is dramatically +/// cheaper than building/tearing one down per invocation (each construction +/// spawns worker threads + an I/O reactor). `OnceLock` makes this thread-safe +/// for free. +fn fallback_runtime() -> Result<&'static Runtime> { + static RT: OnceLock = OnceLock::new(); + if let Some(rt) = RT.get() { + return Ok(rt); + } + let rt = Runtime::new().map_err(|e| { + FbuildError::PackageError(format!("failed to create tokio runtime: {}", e)) + })?; + // If another thread won the race, our `rt` is dropped and we use theirs. + Ok(RT.get_or_init(|| rt)) +} + /// Result of library resolution and compilation. pub struct LibraryResult { /// All include directories from all libraries (for compiler `-I` flags). @@ -311,10 +332,7 @@ pub fn ensure_libraries_sync( if let Ok(handle) = tokio::runtime::Handle::try_current() { tokio::task::block_in_place(|| handle.block_on(fut)) } else { - let rt = tokio::runtime::Runtime::new().map_err(|e| { - fbuild_core::FbuildError::PackageError(format!("failed to create tokio runtime: {}", e)) - })?; - rt.block_on(fut) + fallback_runtime()?.block_on(fut) } } diff --git a/crates/fbuild-packages/src/lnk/resolver.rs b/crates/fbuild-packages/src/lnk/resolver.rs index 46b8d760f..339554705 100644 --- a/crates/fbuild-packages/src/lnk/resolver.rs +++ b/crates/fbuild-packages/src/lnk/resolver.rs @@ -15,9 +15,11 @@ //! and return the lease + path. use std::path::{Path, PathBuf}; +use std::sync::OnceLock; use fbuild_core::{FbuildError, Result}; use sha2::{Digest, Sha256}; +use tokio::runtime::Runtime; use tracing::{debug, info}; use super::format::LnkFile; @@ -25,6 +27,22 @@ use crate::disk_cache::{CacheEntry, Kind, Lease}; use crate::downloader::download_file; use crate::DiskCache; +/// Module-level fallback runtime for the sync `.lnk` resolver bridge. +/// +/// See [`crate::library::library_manager::fallback_runtime`] for the same +/// pattern — building a Tokio runtime per call is expensive and pointless +/// when this module gets hit repeatedly during a build. +fn fallback_runtime() -> Result<&'static Runtime> { + static RT: OnceLock = OnceLock::new(); + if let Some(rt) = RT.get() { + return Ok(rt); + } + let rt = Runtime::new().map_err(|e| { + FbuildError::PackageError(format!("failed to create tokio runtime: {}", e)) + })?; + Ok(RT.get_or_init(|| rt)) +} + /// A successfully resolved `.lnk` blob. Holds a `Lease` that keeps the /// blob pinned in the cache; the lease drops when this struct does. /// @@ -117,10 +135,7 @@ pub fn resolve(lnk: &LnkFile, cache: &DiskCache) -> Result { if let Ok(handle) = tokio::runtime::Handle::try_current() { tokio::task::block_in_place(|| handle.block_on(fut))? } else { - let rt = tokio::runtime::Runtime::new().map_err(|e| { - FbuildError::PackageError(format!("failed to create tokio runtime: {}", e)) - })?; - rt.block_on(fut)? + fallback_runtime()?.block_on(fut)? } }; diff --git a/crates/fbuild-packages/src/toolchain/avr.rs b/crates/fbuild-packages/src/toolchain/avr.rs index fd4d2e305..b26210e14 100644 --- a/crates/fbuild-packages/src/toolchain/avr.rs +++ b/crates/fbuild-packages/src/toolchain/avr.rs @@ -348,7 +348,9 @@ mod tests { let out_path = tmp.path().join("avr-gcc.archive"); // Blocking download (test uses standalone runtime since it's a sync #[test]). - let rt = tokio::runtime::Runtime::new().unwrap(); + // `Runtime::new()` can fail (e.g. fd exhaustion); use `expect` so the + // assertion shows up cleanly instead of a bare `unwrap` panic message. + let rt = tokio::runtime::Runtime::new().expect("tokio runtime should construct"); rt.block_on(async { let resp = crate::http::client() .get(&url) diff --git a/crates/fbuild-packages/src/toolchain/esp32_metadata.rs b/crates/fbuild-packages/src/toolchain/esp32_metadata.rs index 8047db194..13453c3f9 100644 --- a/crates/fbuild-packages/src/toolchain/esp32_metadata.rs +++ b/crates/fbuild-packages/src/toolchain/esp32_metadata.rs @@ -6,8 +6,26 @@ //! 3. Download actual toolchain from the resolved URL use std::path::{Path, PathBuf}; +use std::sync::OnceLock; use fbuild_core::{FbuildError, Result}; +use tokio::runtime::Runtime; + +/// Module-level fallback runtime for the sync `resolve_toolchain_url_sync` +/// bridge. Same rationale as +/// [`crate::library::library_manager::fallback_runtime`]: one process-wide +/// runtime is cheaper than a new one per call when legacy orchestrators +/// hit this many times during a single build. +fn fallback_runtime() -> Result<&'static Runtime> { + static RT: OnceLock = OnceLock::new(); + if let Some(rt) = RT.get() { + return Ok(rt); + } + let rt = Runtime::new().map_err(|e| { + FbuildError::PackageError(format!("failed to create tokio runtime: {}", e)) + })?; + Ok(RT.get_or_init(|| rt)) +} /// Resolved toolchain download info from tools.json. #[derive(Debug)] @@ -84,10 +102,7 @@ pub fn resolve_toolchain_url_sync( if let Ok(handle) = tokio::runtime::Handle::try_current() { tokio::task::block_in_place(|| handle.block_on(fut)) } else { - let rt = tokio::runtime::Runtime::new().map_err(|e| { - FbuildError::PackageError(format!("failed to create tokio runtime: {}", e)) - })?; - rt.block_on(fut) + fallback_runtime()?.block_on(fut) } } diff --git a/crates/fbuild-paths/src/lib.rs b/crates/fbuild-paths/src/lib.rs index 0e48bacbc..ec1c2d557 100644 --- a/crates/fbuild-paths/src/lib.rs +++ b/crates/fbuild-paths/src/lib.rs @@ -66,6 +66,33 @@ pub fn get_cache_root() -> PathBuf { get_fbuild_root().join("cache") } +/// Root directory for short-lived working / scratch dirs. +/// +/// FastLED/fbuild#844 ("Bridge pair 10"). Returns `~/.fbuild/{dev|prod}/tmp`. +/// This is the rooted alternative to `std::env::temp_dir()` and +/// `tempfile::tempdir()`; per-platform package extractors, framework +/// hydration, linker scratch dirs, etc. all live under this root so +/// every byte fbuild writes is reachable from a single user-visible +/// directory. +/// +/// Note: this does NOT create the directory — pair with +/// [`temp_subdir`] for the create-on-use pattern. +pub fn dev_or_prod_temp_root() -> PathBuf { + get_fbuild_root().join("tmp") +} + +/// Get (and create) a named subdirectory under [`dev_or_prod_temp_root`]. +/// +/// Best-effort `create_dir_all`: if creation fails the returned path +/// still points where the caller asked, and the next filesystem op +/// will surface the real error. This matches the convention every +/// other "ensure dir" helper in fbuild-paths uses. +pub fn temp_subdir(name: &str) -> PathBuf { + let dir = dev_or_prod_temp_root().join(name); + let _ = std::fs::create_dir_all(&dir); + dir +} + /// Project-local `.fbuild` directory. pub fn get_project_fbuild_dir(project_dir: &Path) -> PathBuf { project_dir.join(".fbuild") @@ -545,6 +572,27 @@ mod tests { assert_eq!(resolved, PathBuf::from("/tmp/root/esp32dev/release")); } + #[test] + fn dev_or_prod_temp_root_lives_under_fbuild_root() { + let temp_root = dev_or_prod_temp_root(); + let fbuild_root = get_fbuild_root(); + assert!( + temp_root.starts_with(&fbuild_root), + "temp root {} must live under fbuild root {}", + temp_root.display(), + fbuild_root.display() + ); + assert!(temp_root.ends_with("tmp")); + } + + #[test] + fn temp_subdir_creates_and_returns_path() { + let dir = temp_subdir("__fbuild_test_temp_subdir__"); + assert!(dir.exists() || dir.parent().map(|p| !p.exists()).unwrap_or(true)); + // Cleanup best-effort. + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn build_layout_profile_dir_name_matches_buildprofile() { let project = PathBuf::from("/p"); diff --git a/crates/fbuild-python/src/async_serial_monitor.rs b/crates/fbuild-python/src/async_serial_monitor.rs index 01bed4aeb..f0e5d8edb 100644 --- a/crates/fbuild-python/src/async_serial_monitor.rs +++ b/crates/fbuild-python/src/async_serial_monitor.rs @@ -345,7 +345,7 @@ pub(crate) async fn post_reset_request_async( let url = format!("{}/api/reset", fbuild_paths::get_daemon_url()); let payload = ResetPayload { port, board }; - let resp = reqwest::Client::new() + let resp = fbuild_core::http::client() .post(&url) .json(&payload) .timeout(std::time::Duration::from_secs(10)) diff --git a/crates/fbuild-python/src/daemon.rs b/crates/fbuild-python/src/daemon.rs index c06dd1a42..f48dc34f5 100644 --- a/crates/fbuild-python/src/daemon.rs +++ b/crates/fbuild-python/src/daemon.rs @@ -147,7 +147,7 @@ fn one_shot_runtime() -> Result { } async fn verify_broker_daemon_cache_identity_async() -> Result<(), String> { - let info: serde_json::Value = reqwest::Client::new() + let info: serde_json::Value = fbuild_core::http::client() .get(direct_info_url()) .timeout(std::time::Duration::from_secs(5)) .send() @@ -181,7 +181,7 @@ async fn ensure_running_via_broker_async(url: &str) -> Result { ); match AsyncBrokerSession::adopt(request).await { Ok(_session) => { - let client = reqwest::Client::new(); + let client = fbuild_core::http::client(); for _ in 0..100 { if let Ok(resp) = client .get(url) @@ -235,7 +235,7 @@ async fn ensure_running_async_impl(url: &str, spawn_target: Option, dev Err(_) => return false, } - let client = reqwest::Client::new(); + let client = fbuild_core::http::client(); // Fast path: daemon is already up. if let Ok(resp) = client @@ -297,7 +297,7 @@ async fn ensure_running_async_impl(url: &str, spawn_target: Option, dev /// responded 2xx to the shutdown POST. FastLED/fbuild#817. async fn stop_async_impl() -> bool { let url = format!("{}/api/daemon/shutdown", fbuild_paths::get_daemon_url()); - reqwest::Client::new() + fbuild_core::http::client() .post(&url) .headers(shutdown_caller_headers()) .timeout(std::time::Duration::from_secs(10)) @@ -311,7 +311,7 @@ async fn stop_async_impl() -> bool { /// (the caller decodes JSON with the GIL held). FastLED/fbuild#817. async fn status_async_impl() -> PyResult { let url = format!("{}/api/daemon/info", fbuild_paths::get_daemon_url()); - let resp = reqwest::Client::new() + let resp = fbuild_core::http::client() .get(&url) .timeout(std::time::Duration::from_secs(10)) .send() diff --git a/crates/fbuild-python/src/outcome.rs b/crates/fbuild-python/src/outcome.rs index 25788e54f..7c09781b7 100644 --- a/crates/fbuild-python/src/outcome.rs +++ b/crates/fbuild-python/src/outcome.rs @@ -148,7 +148,7 @@ pub(crate) fn send_op(url: &str, req: &OpRequest, timeout: f64) -> OperationOutc /// Returns the same `OperationOutcome` so the sync and async surfaces share /// `parse_outcome` and `outcome_to_pydict`. See FastLED/fbuild#65. pub(crate) async fn send_op_async(url: String, req: OpRequest, timeout: f64) -> OperationOutcome { - let client = reqwest::Client::new(); + let client = fbuild_core::http::client(); let request = client .post(&url) .json(&req) diff --git a/crates/fbuild-python/src/serial_monitor.rs b/crates/fbuild-python/src/serial_monitor.rs index ec74154d3..76007c529 100644 --- a/crates/fbuild-python/src/serial_monitor.rs +++ b/crates/fbuild-python/src/serial_monitor.rs @@ -29,7 +29,13 @@ pub(crate) struct SerialMonitor { auto_reconnect: bool, verbose: bool, hooks: Vec, - runtime: Option, + // FastLED/fbuild#844: avoid `Runtime::new()` outside main/tests by + // borrowing the process-shared `pyo3_async_runtimes::tokio` runtime. + // Stored as `Option<&'static Runtime>` so the existing + // `Some(rt)` session-active guards keep their semantics — `None` + // means "before __enter__ / after __exit__", `Some(_)` means "WS + // session is live". + runtime: Option<&'static Runtime>, ws_write: Option>, ws_read: Option>, client_id: String, @@ -147,7 +153,7 @@ impl SerialMonitor { } fn close_ws(&mut self) { - if let (Some(ref rt), Some(ref ws_write)) = (&self.runtime, &self.ws_write) { + if let (Some(rt), Some(ws_write)) = (&self.runtime, &self.ws_write) { let detach = serde_json::to_string(&ClientMessage::Detach).unwrap(); if let Ok(mut write) = ws_write.lock() { let _ = rt.block_on(write.send(tungstenite::Message::Text(detach))); @@ -196,11 +202,17 @@ impl SerialMonitor { } fn __enter__(mut slf: PyRefMut<'_, Self>) -> PyResult> { - let rt = Runtime::new().map_err(|e| { - pyo3::exceptions::PyRuntimeError::new_err(format!("failed to create runtime: {}", e)) - })?; - - let (write, read) = slf.connect_ws(&rt)?; + // FastLED/fbuild#844: borrow the process-shared async runtime + // instead of constructing a fresh `tokio::runtime::Runtime` per + // monitor session. The pyo3-async-runtimes runtime is the same + // one the AsyncSerialMonitor binding already uses, so we don't + // pay the overhead of spinning up a dedicated multi-threaded + // pool every time FastLED enters a `with SerialMonitor(...)` + // block, and the new lint ban on bare `Runtime::new()` outside + // main/tests is satisfied. + let rt: &'static Runtime = pyo3_async_runtimes::tokio::get_runtime(); + + let (write, read) = slf.connect_ws(rt)?; slf.ws_write = Some(Mutex::new(write)); slf.ws_read = Some(Mutex::new(read)); slf.runtime = Some(rt); @@ -230,7 +242,7 @@ impl SerialMonitor { /// Returns a list of lines received within the timeout period. #[pyo3(signature = (timeout=30.0))] fn read_lines(&mut self, py: Python<'_>, timeout: f64) -> Vec { - let (Some(ref rt), Some(ref ws_read)) = (&self.runtime, &self.ws_read) else { + let (Some(rt), Some(ws_read)) = (&self.runtime, &self.ws_read) else { return vec![]; }; @@ -307,7 +319,7 @@ impl SerialMonitor { /// Write data to the serial port. fn write(&self, data: &str) -> usize { - let (Some(ref rt), Some(ref ws_write), Some(ref ws_read)) = + let (Some(rt), Some(ws_write), Some(ws_read)) = (&self.runtime, &self.ws_write, &self.ws_read) else { return 0; @@ -416,7 +428,7 @@ impl SerialMonitor { /// pyserial use by fbuild clients. #[getter] fn in_waiting(&self) -> usize { - let (Some(ref rt), Some(ref ws_write), Some(ref ws_read)) = + let (Some(rt), Some(ws_write), Some(ws_read)) = (&self.runtime, &self.ws_write, &self.ws_read) else { return 0; @@ -466,7 +478,7 @@ impl SerialMonitor { /// FastLED/fbuild#605 — added as part of the deprecation of direct /// pyserial use by fbuild clients. fn reset_input_buffer(&self) { - let (Some(ref rt), Some(ref ws_write)) = (&self.runtime, &self.ws_write) else { + let (Some(rt), Some(ws_write)) = (&self.runtime, &self.ws_write) else { return; }; let msg = serde_json::to_string(&ClientMessage::ClearBuffer).unwrap(); @@ -576,7 +588,7 @@ impl SerialMonitor { impl SerialMonitor { /// Internal read_lines without hook dispatch (for write_json_rpc which has &self). fn read_lines_inner(&self, timeout: f64) -> Vec { - let (Some(ref rt), Some(ref ws_read)) = (&self.runtime, &self.ws_read) else { + let (Some(rt), Some(ws_read)) = (&self.runtime, &self.ws_read) else { return vec![]; }; diff --git a/dylints/README.md b/dylints/README.md index f375f9720..918b9abc6 100644 --- a/dylints/README.md +++ b/dylints/README.md @@ -64,6 +64,62 @@ itself stays on stable 1.94.1). lock. Existing synchronous uses exempted via `src/allowlist.txt`. See #826. +### FastLED/fbuild#844 bridge sweep (Phase 1 — APIs + lints land together) + +- **`ban_bare_reqwest/`** — forbids `reqwest::Client::new()` / + `reqwest::ClientBuilder::new()` / `reqwest::get` / + `reqwest::blocking::get` outside the bridge module + (`crates/fbuild-core/src/http.rs`). Use + `fbuild_core::http::client()` / + `fbuild_core::http::client_with_timeout(...)` / + `fbuild_core::http::blocking_client(...)`. Zero allowlist. +- **`ban_std_fs_in_async/`** — forbids `std::fs::*` in + `crates/fbuild-daemon/src/**` (Phase 1 scope; Phase 2 will widen + via HIR async-fn detection). Use `fbuild_core::fs::*` (re-exports + `tokio::fs`) or wrap in `tokio::task::spawn_blocking`. Zero + allowlist. +- **`ban_tokio_fs_direct_import/`** — forbids `use tokio::fs[…]` + outside `crates/fbuild-core/src/fs.rs`. Use `use fbuild_core::fs` + instead so the workspace has one source of truth for the async + filesystem surface. Zero allowlist. +- **`ban_std_thread_sleep/`** — forbids `std::thread::sleep` / + `sleep_ms` in fbuild production code. Use + `fbuild_core::time::sleep(...).await` (with the named + `POLL_*` / `*_TIMEOUT` constants where applicable). Zero + allowlist. +- **`ban_std_mpsc_in_async_reachable/`** — forbids any `std::sync::mpsc` + item (channel, Sender, Receiver, SyncSender, sync_channel) in + fbuild production code. Use `fbuild_core::channel::{bounded, + unbounded}` (tokio mpsc) so `.recv().await` yields to the + runtime. Zero allowlist. +- **`ban_tokio_mpsc_direct_import/`** — forbids `use tokio::sync::mpsc[…]` + outside `crates/fbuild-core/src/channel.rs`. Use `use + fbuild_core::channel` instead. Zero allowlist. +- **`ban_std_fs_canonicalize/`** — forbids `std::fs::canonicalize` + and `tokio::fs::canonicalize` in fbuild production code. Use + `fbuild_core::path::canonicalize_existing(...).await` — strips + the Windows `\\?\` UNC prefix and returns a `NormalizedPath` with + platform-stable Hash/Eq/Ord. Zero allowlist. +- **`ban_runtime_new_outside_main/`** — forbids + `tokio::runtime::Runtime::new` and `Builder::new_*` in production + code outside `main.rs`, `src/bin/*.rs`, `#[cfg(test)]` modules, + and `tests/**`. Restructure to `async fn`, accept a + `tokio::runtime::Handle`, or use `tokio::task::spawn_blocking`. + Zero allowlist. +- **`ban_poison_panic/`** — flags `.unwrap()` / `.expect(...)` on + `LockResult` returned by `std::sync::Mutex::lock` / + `RwLock::read` / `RwLock::write`. Either switch to + `tokio::sync::Mutex` / `RwLock` (no poison concept) or handle + the poison: `.unwrap_or_else(|e| e.into_inner())`. Zero + allowlist. +- **`ban_print_in_production/`** — forbids `println!` / `eprintln!` + / `print!` / `eprint!` in `crates/fbuild-cli/src/**` and + `crates/fbuild-build/src/**`, except `crates/fbuild-cli/src/output.rs` + (the bridge itself). Use `fbuild_cli::output::{progress, result, + warn, error, debug}` (tracing-backed) so `--quiet`, + `--verbose`, and `--color={auto,always,never}` flow through one + level filter. Bridge-only allowlist. + ## Running locally ```bash diff --git a/dylints/ban_bare_reqwest/Cargo.toml b/dylints/ban_bare_reqwest/Cargo.toml new file mode 100644 index 000000000..138e1f274 --- /dev/null +++ b/dylints/ban_bare_reqwest/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_bare_reqwest" +version = "0.1.0" +description = "Ban direct reqwest::Client construction outside the fbuild HTTP bridge" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_bare_reqwest/README.md b/dylints/ban_bare_reqwest/README.md new file mode 100644 index 000000000..65a08ca3a --- /dev/null +++ b/dylints/ban_bare_reqwest/README.md @@ -0,0 +1,42 @@ +# `ban_bare_reqwest` + +Custom [dylint](https://github.com/trailofbits/dylint) that forbids direct +construction of `reqwest::Client` / `reqwest::ClientBuilder` and direct calls +to `reqwest::get` / `reqwest::blocking::get` in fbuild production code +(anything under `crates/*/src/`), except inside the bridge modules +themselves. + +## Why + +All HTTP traffic in fbuild flows through `fbuild_core::http::client()` so +the workspace shares one configured timeout matrix, one TLS configuration, +and one reqwest dependency surface. Bypassing the bridge silently regresses +those invariants. + +See FastLED/fbuild#844 (bridge sweep, "Bridge pair 1") for context. + +## Replacement + +```rust,ignore +// Shared async client (300s total, 30s connect): +let client = fbuild_core::http::client(); + +// Per-call timeout override: +let client = fbuild_core::http::client_with_timeout(Duration::from_secs(10)); + +// Rare OS-thread case (e.g. port_scan): +let client = fbuild_core::http::blocking_client(Duration::from_secs(10)); +``` + +## Scope + +Production code only: files whose path contains both `crates/` and a +subsequent `/src/` segment. Tests, examples, and benches are out of scope +by design. + +## Allowlist + +Empty by design — Phase 2 of #844 migrates every direct construction site. +The bridge modules (`crates/fbuild-core/src/http.rs` and the forward-compat +re-export at `crates/fbuild-packages/src/http.rs`) are exempted by file +path in `lib.rs`. diff --git a/dylints/ban_bare_reqwest/rust-toolchain.toml b/dylints/ban_bare_reqwest/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_bare_reqwest/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_bare_reqwest/src/README.md b/dylints/ban_bare_reqwest/src/README.md new file mode 100644 index 000000000..3a9227cb5 --- /dev/null +++ b/dylints/ban_bare_reqwest/src/README.md @@ -0,0 +1,13 @@ +# `ban_bare_reqwest` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_bare_reqwest/src/allowlist.txt b/dylints/ban_bare_reqwest/src/allowlist.txt new file mode 100644 index 000000000..aff543fb2 --- /dev/null +++ b/dylints/ban_bare_reqwest/src/allowlist.txt @@ -0,0 +1,8 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every reqwest call site migrates to fbuild_core::http +# in Phase 2. The bridge module itself is exempted by file path in +# `lib.rs` (`is_bridge_module`) rather than via this file. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). Add an entry ONLY for +# files that legitimately need to construct a bare reqwest client. diff --git a/dylints/ban_bare_reqwest/src/lib.rs b/dylints/ban_bare_reqwest/src/lib.rs new file mode 100644 index 000000000..5853d14e3 --- /dev/null +++ b/dylints/ban_bare_reqwest/src/lib.rs @@ -0,0 +1,196 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans direct `reqwest::Client::new`, `reqwest::ClientBuilder`, + /// `reqwest::get`, and `reqwest::blocking::get` outside the fbuild + /// HTTP bridge (`crates/fbuild-core/src/http.rs` and the + /// forward-compat re-export at `crates/fbuild-packages/src/http.rs`). + /// + /// ### Why is this bad? + /// + /// Every HTTP call in fbuild must go through + /// `fbuild_core::http::client()` so the workspace shares one + /// configured timeout matrix, one TLS configuration, and one + /// reqwest dependency surface. Direct construction sites silently + /// regress those invariants. + /// + /// FastLED/fbuild#844 (bridge sweep, "Bridge pair 1"). + /// + /// ### Example + /// + /// ```rust,ignore + /// let client = reqwest::Client::new(); // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// let client = fbuild_core::http::client(); + /// ``` + pub BAN_BARE_REQWEST, + Deny, + "ban direct reqwest::Client construction outside the fbuild HTTP bridge" +} + +/// Fully-qualified paths to banned items. +const BANNED_PATHS: &[&[&str]] = &[ + &["reqwest", "Client", "new"], + &["reqwest", "ClientBuilder", "new"], + &["reqwest", "get"], + &["reqwest", "blocking", "Client", "new"], + &["reqwest", "blocking", "ClientBuilder", "new"], + &["reqwest", "blocking", "get"], +]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +/// Files that ARE the bridge — exempted by path so they can construct +/// the underlying reqwest client. Slash-normalized suffix match. +const BRIDGE_MODULES: &[&str] = &[ + "crates/fbuild-core/src/http.rs", + "crates/fbuild-packages/src/http.rs", +]; + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanBareReqwest { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_bridge_module(&normalized) || is_allowlisted(&normalized) { + return; + } + + match expr.kind { + ExprKind::MethodCall(_, _, _, _) => { + if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + ExprKind::Path(ref qpath) => { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + _ => {} + } + } +} + +fn check_def_id(cx: &LateContext<'_>, span: rustc_span::Span, def_id: rustc_hir::def_id::DefId) { + for banned in BANNED_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, span, banned); + return; + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, banned: &[&str]) { + let joined = banned.join("::"); + cx.opt_span_lint( + BAN_BARE_REQWEST, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`{joined}` bypasses fbuild's HTTP bridge. Use \ + `fbuild_core::http::client()` (shared async client), \ + `fbuild_core::http::client_with_timeout(...)` (per-call \ + timeout override), or `fbuild_core::http::blocking_client(...)` \ + (for the rare OS-thread case). See FastLED/fbuild#844." + )); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_bridge_module(normalized: &str) -> bool { + BRIDGE_MODULES + .iter() + .any(|bridge| normalized.ends_with(bridge)) +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn bridge_modules_recognized() { + assert!(is_bridge_module("crates/fbuild-core/src/http.rs")); + assert!(is_bridge_module("crates/fbuild-packages/src/http.rs")); + assert!(!is_bridge_module("crates/fbuild-cli/src/cli/build.rs")); + } + + #[test] + fn production_scope_matches() { + assert!(in_production_scope("crates/fbuild-cli/src/cli/port_scan.rs")); + assert!(!in_production_scope("crates/fbuild-daemon/tests/test_emu_endpoint.rs")); + } +} diff --git a/dylints/ban_poison_panic/Cargo.toml b/dylints/ban_poison_panic/Cargo.toml new file mode 100644 index 000000000..042d1e2b3 --- /dev/null +++ b/dylints/ban_poison_panic/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_poison_panic" +version = "0.1.0" +description = "Ban .unwrap()/.expect() on std::sync::Mutex/RwLock lock guards" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_poison_panic/README.md b/dylints/ban_poison_panic/README.md new file mode 100644 index 000000000..78ecd354d --- /dev/null +++ b/dylints/ban_poison_panic/README.md @@ -0,0 +1,26 @@ +# `ban_poison_panic` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +.unwrap()/.expect() on LockResult returned by std::sync::Mutex::lock / RwLock::read / RwLock::write cascades the original panic across every other lock holder. Switch to tokio::sync::Mutex/RwLock or handle the poison via unwrap_or_else(|e| e.into_inner()). + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_poison_panic/rust-toolchain.toml b/dylints/ban_poison_panic/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_poison_panic/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_poison_panic/src/README.md b/dylints/ban_poison_panic/src/README.md new file mode 100644 index 000000000..20e485f07 --- /dev/null +++ b/dylints/ban_poison_panic/src/README.md @@ -0,0 +1,13 @@ +# `ban_poison_panic` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_poison_panic/src/allowlist.txt b/dylints/ban_poison_panic/src/allowlist.txt new file mode 100644 index 000000000..db0cc6dd7 --- /dev/null +++ b/dylints/ban_poison_panic/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every call site migrates in Phase 2. Bridge / scope +# exemptions live in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_poison_panic/src/lib.rs b/dylints/ban_poison_panic/src/lib.rs new file mode 100644 index 000000000..e1f30793d --- /dev/null +++ b/dylints/ban_poison_panic/src/lib.rs @@ -0,0 +1,250 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind, HirId}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::ty; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Flags `.unwrap()` / `.expect(...)` on the `LockResult` / + /// `TryLockResult` returned by `std::sync::Mutex::lock`, + /// `std::sync::RwLock::read`, and `std::sync::RwLock::write`. + /// + /// ### Why is this bad? + /// + /// `LockResult::Err(_)` indicates the lock was poisoned because + /// a previous holder panicked. `.unwrap()` then panics again — + /// cascading the original panic across every other lock holder. + /// In a daemon process that's an outage. Either: + /// + /// 1. switch to `tokio::sync::Mutex` / `RwLock` (they have no + /// poison concept and integrate with the reactor), or + /// 2. handle the poison: `.unwrap_or_else(|e| e.into_inner())`. + /// + /// FastLED/fbuild#844 (bridge sweep, lint 9). Per user directive + /// this ships with **zero allowlist**. + /// + /// ### Example + /// + /// ```rust,ignore + /// let guard = my_mutex.lock().unwrap(); // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// // Preferred: switch the type: + /// let guard = my_mutex.lock().await; // tokio::sync::Mutex + /// + /// // Or handle poison explicitly: + /// let guard = my_mutex.lock().unwrap_or_else(|e| e.into_inner()); + /// ``` + pub BAN_POISON_PANIC, + Deny, + "ban .unwrap()/.expect() on std::sync::Mutex/RwLock lock guards" +} + +/// `.unwrap()` / `.expect()` on `Result`. +const UNWRAP_PATHS: &[&[&str]] = &[ + &["core", "result", "Result", "unwrap"], + &["std", "result", "Result", "unwrap"], + &["core", "result", "Result", "expect"], + &["std", "result", "Result", "expect"], +]; + +/// Lock-returning methods we treat as "poison-prone receivers". +const LOCK_METHODS: &[&[&str]] = &[ + &["std", "sync", "poison", "mutex", "Mutex", "lock"], + &["std", "sync", "mutex", "Mutex", "lock"], + &["std", "sync", "poison", "rwlock", "RwLock", "read"], + &["std", "sync", "poison", "rwlock", "RwLock", "write"], + &["std", "sync", "rwlock", "RwLock", "read"], + &["std", "sync", "rwlock", "RwLock", "write"], + // try_* variants return TryLockResult which has the same poison surface. + &["std", "sync", "poison", "mutex", "Mutex", "try_lock"], + &["std", "sync", "mutex", "Mutex", "try_lock"], +]; + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanPoisonPanic { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) || is_test_file(&normalized) { + return; + } + if owned_by_cfg_test_module(cx, expr.hir_id) { + return; + } + + // We're looking for `.unwrap()` / `.expect(...)` where + // `` is a `LockResult` / `TryLockResult` produced by + // one of `LOCK_METHODS`. + let ExprKind::MethodCall(_seg, recv, _args, _) = expr.kind else { + return; + }; + let Some(unwrap_def) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else { + return; + }; + if !UNWRAP_PATHS.iter().any(|p| def_path_equals(cx, unwrap_def, p)) { + return; + } + + // The receiver must itself be a method-call to one of LOCK_METHODS. + if !receiver_is_lock_call(cx, recv) { + return; + } + + emit_lint(cx, expr.span); + } +} + +fn receiver_is_lock_call(cx: &LateContext<'_>, recv: &Expr<'_>) -> bool { + match recv.kind { + ExprKind::MethodCall(_, _, _, _) => { + let Some(def_id) = cx.typeck_results().type_dependent_def_id(recv.hir_id) else { + return false; + }; + LOCK_METHODS.iter().any(|p| def_path_equals(cx, def_id, p)) + } + // `Mutex::lock(&m).unwrap()` — qualified-path call. + ExprKind::Call(func, _) => { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + return LOCK_METHODS.iter().any(|p| def_path_equals(cx, def_id, p)); + } + } + false + } + _ => { + // Fallback by *type*: if the receiver's type resolves to + // `LockResult<...>` we still flag, even if the syntactic + // shape is unusual (e.g. assigned via a `let`). + let ty = cx.typeck_results().expr_ty_adjusted(recv); + ty_is_lock_result(ty) + } + } +} + +fn ty_is_lock_result(ty: ty::Ty<'_>) -> bool { + let s = format!("{ty:?}"); + s.contains("LockResult") || s.contains("TryLockResult") +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_POISON_PANIC, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "`.unwrap()`/`.expect()` on a `LockResult` cascades the \ + original lock-holder panic across every other lock holder. \ + Either switch to `tokio::sync::Mutex` / `RwLock` (no poison \ + concept, integrates with the reactor) or handle the poison \ + with `.unwrap_or_else(|e| e.into_inner())`. See \ + FastLED/fbuild#844.", + ); + }), + ); +} + +fn owned_by_cfg_test_module(cx: &LateContext<'_>, hir_id: HirId) -> bool { + let mut current = cx.tcx.hir_get_parent_item(hir_id); + loop { + let attrs = cx.tcx.hir_attrs(current.into()); + for attr in attrs { + if attr_is_cfg_test(attr) { + return true; + } + } + let parent = cx.tcx.hir_get_parent_item(current.into()); + if parent == current { + return false; + } + current = parent; + } +} + +fn attr_is_cfg_test(attr: &rustc_hir::Attribute) -> bool { + let Some(meta) = attr.meta() else { + return false; + }; + let path = meta.path(); + if path.segments.len() != 1 || path.segments[0].ident.as_str() != "cfg" { + return false; + } + let Some(list) = meta.meta_item_list() else { + return false; + }; + list.iter().any(|nested| { + nested + .ident() + .map(|id| id.as_str() == "test") + .unwrap_or(false) + }) +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_test_file(normalized: &str) -> bool { + if normalized.contains("/tests/") { + return true; + } + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + name.contains("tests") && name.ends_with(".rs") +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} diff --git a/dylints/ban_print_in_production/Cargo.toml b/dylints/ban_print_in_production/Cargo.toml new file mode 100644 index 000000000..fa28bbc6a --- /dev/null +++ b/dylints/ban_print_in_production/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_print_in_production" +version = "0.1.0" +description = "Ban println!/eprintln! in CLI + build production code — use fbuild_cli::output::*" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_print_in_production/README.md b/dylints/ban_print_in_production/README.md new file mode 100644 index 000000000..c1304a004 --- /dev/null +++ b/dylints/ban_print_in_production/README.md @@ -0,0 +1,26 @@ +# `ban_print_in_production` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans println!/eprintln!/print!/eprint! in crates/fbuild-cli/src/** and crates/fbuild-build/src/** (except crates/fbuild-cli/src/output.rs which IS the bridge). Replacement: fbuild_cli::output::{progress, result, warn, error, debug} or tracing::*. + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_print_in_production/rust-toolchain.toml b/dylints/ban_print_in_production/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_print_in_production/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_print_in_production/src/README.md b/dylints/ban_print_in_production/src/README.md new file mode 100644 index 000000000..d7fd60889 --- /dev/null +++ b/dylints/ban_print_in_production/src/README.md @@ -0,0 +1,13 @@ +# `ban_print_in_production` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_print_in_production/src/allowlist.txt b/dylints/ban_print_in_production/src/allowlist.txt new file mode 100644 index 000000000..db0cc6dd7 --- /dev/null +++ b/dylints/ban_print_in_production/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every call site migrates in Phase 2. Bridge / scope +# exemptions live in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_print_in_production/src/lib.rs b/dylints/ban_print_in_production/src/lib.rs new file mode 100644 index 000000000..f90965c09 --- /dev/null +++ b/dylints/ban_print_in_production/src/lib.rs @@ -0,0 +1,218 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, ExpnKind, FileName, MacroKind, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `println!`, `eprintln!`, `print!`, `eprint!` invocations + /// inside `crates/fbuild-cli/src/**` and `crates/fbuild-build/src/**`, + /// EXCEPT in `crates/fbuild-cli/src/output.rs` (which IS the bridge). + /// + /// ### Why is this bad? + /// + /// `println!`/`eprintln!` bypass the workspace's verbosity and + /// color discipline. The bridge `fbuild_cli::output::*` is + /// `tracing`-backed so `--quiet`, `--verbose`, and + /// `--color={auto,always,never}` all flow through one level + /// filter. + /// + /// `result()` (the one exception) keeps the final-answer line on + /// `println!` so pipe redirection still works — but it's gated + /// behind the bridge. + /// + /// FastLED/fbuild#844 (bridge sweep, lint 12). Per user directive + /// the allowlist is empty except for the bridge module itself. + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-cli/src/cli/build.rs + /// println!("Building env {env}"); // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// use crate::output; + /// output::progress(format!("Building env {env}")); + /// // for the final answer: + /// output::result(format!("OK")); + /// ``` + pub BAN_PRINT_IN_PRODUCTION, + Deny, + "ban println!/eprintln!/print!/eprint! in CLI + build production code" +} + +/// Macro expansions we ban. We detect by the macro's def path. +const BANNED_MACRO_NAMES: &[&str] = &["println", "eprintln", "print", "eprint"]; + +/// Underlying `_print` / `_eprint` calls (the macros expand to these). +const PRINT_DEF_PATHS: &[&[&str]] = &[ + &["std", "io", "stdio", "_print"], + &["std", "io", "stdio", "_eprint"], +]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +/// File-path scope. ONLY these two prefixes are linted. +const SCOPES: &[&str] = &[ + "crates/fbuild-cli/src/", + "crates/fbuild-build/src/", +]; + +/// The bridge IS exempt — it's the module everything else is migrating to. +const BRIDGE_MODULES: &[&str] = &["crates/fbuild-cli/src/output.rs"]; + +impl<'tcx> LateLintPass<'tcx> for BanPrintInProduction { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_scope(&normalized) { + return; + } + if is_bridge_module(&normalized) || is_allowlisted(&normalized) { + return; + } + if is_test_file(&normalized) { + return; + } + + // Two detection paths: + // 1. Span's macro expansion is one of `println!`/`eprintln!`/… + // 2. The call expression resolves to `std::io::_print` / `_eprint`. + if let Some(name) = macro_name(expr.span) { + if BANNED_MACRO_NAMES.contains(&name.as_str()) { + emit_lint(cx, expr.span, &name); + return; + } + } + + if let ExprKind::Call(ref func, _) = expr.kind { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + for banned in PRINT_DEF_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, expr.span, "println-family"); + return; + } + } + } + } + } + } +} + +fn macro_name(span: rustc_span::Span) -> Option { + let expn = span.ctxt().outer_expn_data(); + match expn.kind { + ExpnKind::Macro(MacroKind::Bang, name) => Some(name.to_string()), + _ => None, + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, what: &str) { + let what = what.to_string(); + cx.opt_span_lint( + BAN_PRINT_IN_PRODUCTION, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`{what}!` bypasses fbuild's verbosity + color discipline. \ + Use `crate::output::{{progress, result, warn, error, debug}}` \ + (in fbuild-cli) or `tracing::{{info, debug, warn, error}}` \ + (in fbuild-build) instead. See FastLED/fbuild#844." + )); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_scope(normalized: &str) -> bool { + SCOPES.iter().any(|s| normalized.contains(s)) +} + +fn is_bridge_module(normalized: &str) -> bool { + BRIDGE_MODULES + .iter() + .any(|bridge| normalized.ends_with(bridge)) +} + +fn is_test_file(normalized: &str) -> bool { + if normalized.contains("/tests/") { + return true; + } + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + name.contains("tests") && name.ends_with(".rs") +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn scope_matches_cli_and_build() { + assert!(in_scope("crates/fbuild-cli/src/cli/build.rs")); + assert!(in_scope("crates/fbuild-build/src/orchestrator.rs")); + assert!(!in_scope("crates/fbuild-daemon/src/context.rs")); + } + + #[test] + fn bridge_module_is_exempt() { + assert!(is_bridge_module("crates/fbuild-cli/src/output.rs")); + assert!(!is_bridge_module("crates/fbuild-cli/src/cli/build.rs")); + } +} diff --git a/dylints/ban_runtime_new_outside_main/Cargo.toml b/dylints/ban_runtime_new_outside_main/Cargo.toml new file mode 100644 index 000000000..553f62da1 --- /dev/null +++ b/dylints/ban_runtime_new_outside_main/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_runtime_new_outside_main" +version = "0.1.0" +description = "Ban tokio::runtime::Runtime construction outside binary entry points" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_runtime_new_outside_main/README.md b/dylints/ban_runtime_new_outside_main/README.md new file mode 100644 index 000000000..db979f0eb --- /dev/null +++ b/dylints/ban_runtime_new_outside_main/README.md @@ -0,0 +1,26 @@ +# `ban_runtime_new_outside_main` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans tokio::runtime::Runtime::new and Builder::new_* in production code outside main.rs, src/bin/*, #[cfg(test)] modules, and tests/**. Restructure to async fn, accept a Runtime::Handle, or use spawn_blocking. + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_runtime_new_outside_main/rust-toolchain.toml b/dylints/ban_runtime_new_outside_main/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_runtime_new_outside_main/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_runtime_new_outside_main/src/README.md b/dylints/ban_runtime_new_outside_main/src/README.md new file mode 100644 index 000000000..0f266da30 --- /dev/null +++ b/dylints/ban_runtime_new_outside_main/src/README.md @@ -0,0 +1,13 @@ +# `ban_runtime_new_outside_main` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_runtime_new_outside_main/src/allowlist.txt b/dylints/ban_runtime_new_outside_main/src/allowlist.txt new file mode 100644 index 000000000..db0cc6dd7 --- /dev/null +++ b/dylints/ban_runtime_new_outside_main/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every call site migrates in Phase 2. Bridge / scope +# exemptions live in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_runtime_new_outside_main/src/lib.rs b/dylints/ban_runtime_new_outside_main/src/lib.rs new file mode 100644 index 000000000..3cd5fe66d --- /dev/null +++ b/dylints/ban_runtime_new_outside_main/src/lib.rs @@ -0,0 +1,245 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind, HirId}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `tokio::runtime::Runtime::new`, + /// `tokio::runtime::Builder::*`, and friends in production code + /// EXCEPT in: + /// + /// - `**/main.rs` (binary entry points) + /// - `**/src/bin/**` (alternate binary entry points) + /// - `**/tests/**` (integration tests) + /// - any module annotated `#[cfg(test)]` + /// - the workspace root `examples/` / `benches/` + /// + /// ### Why is this bad? + /// + /// Spinning up a fresh runtime mid-program is almost always a + /// shape mismatch — the calling site is already inside the + /// daemon's tokio runtime, so the new runtime fights for the same + /// OS-thread pool and the `block_on` call panics if the outer + /// future is on a current-thread executor. The library audit in + /// #844 found 8 such sites; all 8 should restructure to either + /// (a) be async themselves, (b) take a runtime handle from their + /// caller, or (c) explicitly use `tokio::task::spawn_blocking`. + /// + /// FastLED/fbuild#844 (bridge sweep, lint 8). Per user directive + /// this ships with **zero allowlist**. + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-packages/src/library/library_manager.rs + /// fn install_library(...) -> Result<()> { + /// let rt = tokio::runtime::Runtime::new()?; // banned + /// rt.block_on(async { ... }) + /// } + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// pub async fn install_library(...) -> Result<()> { + /// // ... call .await directly + /// } + /// ``` + pub BAN_RUNTIME_NEW_OUTSIDE_MAIN, + Deny, + "ban tokio::runtime::Runtime construction outside binary entry points" +} + +const BANNED_PATHS: &[&[&str]] = &[ + &["tokio", "runtime", "Runtime", "new"], + &["tokio", "runtime", "Builder", "new_current_thread"], + &["tokio", "runtime", "Builder", "new_multi_thread"], +]; + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanRuntimeNewOutsideMain { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_entry_point(&normalized) || is_test_file(&normalized) { + return; + } + // `#[cfg(test)]` module walk: a runtime built in a unit-test + // module is fine even when the surrounding file is production. + if owned_by_cfg_test_module(cx, expr.hir_id) { + return; + } + + match expr.kind { + ExprKind::Path(ref qpath) => { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + ExprKind::Call(ref func, _) => { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + } + _ => {} + } + } +} + +fn check_def_id(cx: &LateContext<'_>, span: rustc_span::Span, def_id: rustc_hir::def_id::DefId) { + for banned in BANNED_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, span, banned); + return; + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, banned: &[&str]) { + let joined = banned.join("::"); + cx.opt_span_lint( + BAN_RUNTIME_NEW_OUTSIDE_MAIN, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`{joined}` is only allowed in `main.rs`, `src/bin/*.rs`, \ + `#[cfg(test)]` modules, or `tests/**`. Restructure the \ + calling function to be `async fn` (and await directly), \ + accept a `tokio::runtime::Handle` from its caller, or use \ + `tokio::task::spawn_blocking`. See FastLED/fbuild#844." + )); + }), + ); +} + +fn owned_by_cfg_test_module(cx: &LateContext<'_>, hir_id: HirId) -> bool { + let mut current = cx.tcx.hir_get_parent_item(hir_id); + loop { + let attrs = cx.tcx.hir_attrs(current.into()); + for attr in attrs { + if attr_is_cfg_test(attr) { + return true; + } + } + let parent = cx.tcx.hir_get_parent_item(current.into()); + if parent == current { + return false; + } + current = parent; + } +} + +fn attr_is_cfg_test(attr: &rustc_hir::Attribute) -> bool { + let Some(meta) = attr.meta() else { + return false; + }; + let path = meta.path(); + if path.segments.len() != 1 || path.segments[0].ident.as_str() != "cfg" { + return false; + } + let Some(list) = meta.meta_item_list() else { + return false; + }; + list.iter().any(|nested| { + nested + .ident() + .map(|id| id.as_str() == "test") + .unwrap_or(false) + }) +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_entry_point(normalized: &str) -> bool { + // `**/main.rs` or `**/src/bin/**`. + normalized.ends_with("/main.rs") + || normalized.contains("/src/bin/") +} + +fn is_test_file(normalized: &str) -> bool { + if normalized.contains("/tests/") { + return true; + } + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + name.contains("tests") && name.ends_with(".rs") +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn entry_points_are_exempt() { + assert!(is_entry_point("crates/fbuild-cli/src/main.rs")); + assert!(is_entry_point("crates/fbuild-daemon/src/bin/containment_harness.rs")); + assert!(!is_entry_point("crates/fbuild-packages/src/library/library_manager.rs")); + } + + #[test] + fn test_files_are_exempt() { + assert!(is_test_file("crates/fbuild-cli/tests/integration.rs")); + assert!(is_test_file("crates/fbuild-core/src/subprocess_tests.rs")); + assert!(!is_test_file("crates/fbuild-cli/src/main.rs")); + } +} diff --git a/dylints/ban_std_fs_canonicalize/Cargo.toml b/dylints/ban_std_fs_canonicalize/Cargo.toml new file mode 100644 index 000000000..9ae404e24 --- /dev/null +++ b/dylints/ban_std_fs_canonicalize/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_std_fs_canonicalize" +version = "0.1.0" +description = "Ban std::fs::canonicalize — use fbuild_core::path::canonicalize_existing" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_std_fs_canonicalize/README.md b/dylints/ban_std_fs_canonicalize/README.md new file mode 100644 index 000000000..6d761423a --- /dev/null +++ b/dylints/ban_std_fs_canonicalize/README.md @@ -0,0 +1,26 @@ +# `ban_std_fs_canonicalize` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans std::fs::canonicalize and tokio::fs::canonicalize in fbuild production code. Replacement: fbuild_core::path::canonicalize_existing(...).await (strips Windows UNC prefix, returns NormalizedPath). + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_std_fs_canonicalize/rust-toolchain.toml b/dylints/ban_std_fs_canonicalize/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_std_fs_canonicalize/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_std_fs_canonicalize/src/README.md b/dylints/ban_std_fs_canonicalize/src/README.md new file mode 100644 index 000000000..418c35b2e --- /dev/null +++ b/dylints/ban_std_fs_canonicalize/src/README.md @@ -0,0 +1,13 @@ +# `ban_std_fs_canonicalize` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_std_fs_canonicalize/src/allowlist.txt b/dylints/ban_std_fs_canonicalize/src/allowlist.txt new file mode 100644 index 000000000..c6e0f3bb7 --- /dev/null +++ b/dylints/ban_std_fs_canonicalize/src/allowlist.txt @@ -0,0 +1,18 @@ +# FastLED/fbuild#844 — Phase 2 migration. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). Bridge / scope exemptions +# live in `lib.rs` by file path, not here. +# +# The entries below are sync `canon()` helpers (and their colocated +# `#[cfg(test)]` blocks) that live on hot LDF paths. Converting them to +# async would cascade through `fbuild_header_scan::walk{,_with_state}` → +# `fbuild_library_select::resolve{,_with_stats}` → 9 downstream call +# sites in `fbuild-daemon`, `fbuild-build`, `fbuild-test-support`, and +# `fbuild-cli`, none of which are in this PR's scope. The `canon` helper +# is called from within a rayon-parallel walker where a blocking +# `tokio::fs` call would deadlock the reactor; the sync `std::fs` +# variant is the deliberate choice. See FastLED/fbuild#844 Phase 2. +crates/fbuild-header-scan/src/walker.rs +crates/fbuild-library-select/src/lib.rs +crates/fbuild-cli/src/lib_select.rs diff --git a/dylints/ban_std_fs_canonicalize/src/lib.rs b/dylints/ban_std_fs_canonicalize/src/lib.rs new file mode 100644 index 000000000..a9b44207b --- /dev/null +++ b/dylints/ban_std_fs_canonicalize/src/lib.rs @@ -0,0 +1,169 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::fs::canonicalize` and `tokio::fs::canonicalize` in + /// production code, except inside the bridge module + /// (`crates/fbuild-core/src/path.rs`). + /// + /// ### Why is this bad? + /// + /// Raw `canonicalize` on Windows injects the `\\?\` extended- + /// length prefix, which then leaks into cache keys, log lines, + /// and JSON dumps where the un-prefixed form is canonical. The + /// bridge (`fbuild_core::path::canonicalize_existing`) strips + /// the prefix and returns a `NormalizedPath` whose `Hash` / + /// `Eq` / `Ord` are platform-stable. + /// + /// FastLED/fbuild#844 (bridge sweep, "Bridge pair 5"). + /// + /// ### Example + /// + /// ```rust,ignore + /// let canon = std::fs::canonicalize(&path)?; // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// let canon = fbuild_core::path::canonicalize_existing(&path).await?; + /// ``` + pub BAN_STD_FS_CANONICALIZE, + Deny, + "ban std::fs::canonicalize and tokio::fs::canonicalize outside the path bridge" +} + +const BANNED_PATHS: &[&[&str]] = &[ + &["std", "fs", "canonicalize"], + &["tokio", "fs", "canonicalize"], +]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +const BRIDGE_MODULES: &[&str] = &["crates/fbuild-core/src/path.rs", "crates/fbuild-core/src/fs.rs"]; + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanStdFsCanonicalize { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_bridge_module(&normalized) || is_allowlisted(&normalized) { + return; + } + + match expr.kind { + ExprKind::Path(ref qpath) => { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + ExprKind::Call(ref func, _) => { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + } + _ => {} + } + } +} + +fn check_def_id(cx: &LateContext<'_>, span: rustc_span::Span, def_id: rustc_hir::def_id::DefId) { + for banned in BANNED_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, span); + return; + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_STD_FS_CANONICALIZE, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "raw `canonicalize` leaks the Windows `\\\\?\\` extended-length \ + prefix into cache keys and logs. Use \ + `fbuild_core::path::canonicalize_existing(...).await` — it \ + strips the prefix and returns a `NormalizedPath` whose \ + Hash/Eq/Ord are platform-stable. See FastLED/fbuild#844.", + ); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_bridge_module(normalized: &str) -> bool { + BRIDGE_MODULES + .iter() + .any(|bridge| normalized.ends_with(bridge)) +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} diff --git a/dylints/ban_std_fs_in_async/Cargo.toml b/dylints/ban_std_fs_in_async/Cargo.toml new file mode 100644 index 000000000..376792d62 --- /dev/null +++ b/dylints/ban_std_fs_in_async/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_std_fs_in_async" +version = "0.1.0" +description = "Ban std::fs::* in scopes where async is the convention (fbuild-daemon production code)" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_std_fs_in_async/README.md b/dylints/ban_std_fs_in_async/README.md new file mode 100644 index 000000000..751e89199 --- /dev/null +++ b/dylints/ban_std_fs_in_async/README.md @@ -0,0 +1,26 @@ +# `ban_std_fs_in_async` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans std::fs::* in crates/fbuild-daemon/src/** (Phase 1 scope; Phase 2 widens via HIR async-fn detection). Replacement: fbuild_core::fs::* (re-exports tokio::fs) or tokio::task::spawn_blocking. + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_std_fs_in_async/rust-toolchain.toml b/dylints/ban_std_fs_in_async/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_std_fs_in_async/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_std_fs_in_async/src/README.md b/dylints/ban_std_fs_in_async/src/README.md new file mode 100644 index 000000000..4770d182d --- /dev/null +++ b/dylints/ban_std_fs_in_async/src/README.md @@ -0,0 +1,13 @@ +# `ban_std_fs_in_async` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_std_fs_in_async/src/allowlist.txt b/dylints/ban_std_fs_in_async/src/allowlist.txt new file mode 100644 index 000000000..0ab04468f --- /dev/null +++ b/dylints/ban_std_fs_in_async/src/allowlist.txt @@ -0,0 +1,15 @@ +# FastLED/fbuild#844 — Phase 2 migration. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). Bridge / scope exemptions +# live in `lib.rs` by file path, not here. +# +# The entries below are sync helpers inside the daemon crate that are +# deliberately blocking: each call site below is invoked through +# `tokio::task::spawn_blocking` by its async caller (see +# `handlers/operations/build.rs:446`, `deploy.rs:319`, and +# `avr8js_npm.rs:130`). Converting them to async would force the +# spawn_blocking wrappers to be removed and pull the I/O back onto +# axum workers — the opposite of what this lint exists to encourage. +crates/fbuild-daemon/src/handlers/operations/common.rs +crates/fbuild-daemon/src/handlers/emulator/avr8js_npm.rs diff --git a/dylints/ban_std_fs_in_async/src/lib.rs b/dylints/ban_std_fs_in_async/src/lib.rs new file mode 100644 index 000000000..0d97890b3 --- /dev/null +++ b/dylints/ban_std_fs_in_async/src/lib.rs @@ -0,0 +1,216 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::fs::*` calls in scopes where async is the rule — + /// currently `crates/fbuild-daemon/src/**` production code. The + /// matching bridge is `fbuild_core::fs::*` (re-export of + /// `tokio::fs`) plus the `spawn_blocking` escape hatch for the + /// rare case where a synchronous filesystem call from inside + /// async is genuinely correct. + /// + /// ### Why is this bad? + /// + /// `std::fs::*` blocks the OS thread. Inside the tokio runtime + /// that's a worker — every other task waiting on that worker + /// stalls until the I/O completes. Multi-MB reads of toolchain + /// caches can pin a worker for seconds. + /// + /// FastLED/fbuild#844 (bridge sweep, "Bridge pair 2"). + /// + /// ### Known problems + /// + /// Detecting "is this expression reachable from an `async fn`" + /// from inside dylint is non-trivial (a free function called + /// from both sync and async contexts looks the same to the lint). + /// Phase 1 ships a file-path-scoped variant that bans `std::fs::*` + /// in `crates/fbuild-daemon/src/**` (where async is the + /// convention). A follow-up will broaden detection workspace-wide + /// via HIR analysis (TODO in this file). + /// + /// ### Example + /// + /// ```rust,ignore + /// // crates/fbuild-daemon/src/handlers/build.rs + /// async fn handle_build() { + /// let data = std::fs::read(path).unwrap(); // banned + /// } + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// let data = fbuild_core::fs::read(path).await.unwrap(); + /// ``` + pub BAN_STD_FS_IN_ASYNC, + Deny, + "ban std::fs::* in scopes where async is the convention" +} + +/// `std::fs` items that block the calling thread. Matched on the +/// canonical DefId path so `std::fs::*` resolves regardless of +/// `use std::fs` aliasing. +const BANNED_PATHS: &[&[&str]] = &[ + &["std", "fs", "canonicalize"], + &["std", "fs", "copy"], + &["std", "fs", "create_dir"], + &["std", "fs", "create_dir_all"], + &["std", "fs", "hard_link"], + &["std", "fs", "metadata"], + &["std", "fs", "read"], + &["std", "fs", "read_dir"], + &["std", "fs", "read_link"], + &["std", "fs", "read_to_string"], + &["std", "fs", "remove_dir"], + &["std", "fs", "remove_dir_all"], + &["std", "fs", "remove_file"], + &["std", "fs", "rename"], + &["std", "fs", "set_permissions"], + &["std", "fs", "symlink_metadata"], + &["std", "fs", "write"], +]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +/// File-path scope. Phase 1 covers fbuild-daemon production code, +/// where async is the rule. Phase 2 (TODO via HIR analysis) widens +/// to "anywhere inside an `async fn`". +const SCOPE_PREFIX: &str = "crates/fbuild-daemon/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanStdFsInAsync { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_scope(&normalized) { + return; + } + if is_test_file(&normalized) || is_allowlisted(&normalized) { + return; + } + + match expr.kind { + ExprKind::Path(ref qpath) => { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + ExprKind::Call(ref func, _) => { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + } + _ => {} + } + } +} + +fn check_def_id(cx: &LateContext<'_>, span: rustc_span::Span, def_id: rustc_hir::def_id::DefId) { + for banned in BANNED_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, span, banned); + return; + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span, banned: &[&str]) { + let joined = banned.join("::"); + cx.opt_span_lint( + BAN_STD_FS_IN_ASYNC, + Some(span), + DiagDecorator(move |diag| { + diag.primary_message(format!( + "`{joined}` blocks the tokio worker. Use `fbuild_core::fs::*` \ + (async) or wrap in `tokio::task::spawn_blocking` if a \ + synchronous filesystem call is genuinely required. See \ + FastLED/fbuild#844." + )); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_scope(normalized: &str) -> bool { + normalized.contains(SCOPE_PREFIX) +} + +fn is_test_file(normalized: &str) -> bool { + let Some(name) = normalized.rsplit('/').next() else { + return false; + }; + name.contains("tests") && name.ends_with(".rs") +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn daemon_paths_are_in_scope() { + assert!(in_scope("crates/fbuild-daemon/src/context.rs")); + assert!(in_scope("crates/fbuild-daemon/src/handlers/build.rs")); + } + + #[test] + fn other_crates_are_out_of_scope_for_now() { + // Phase 1: only fbuild-daemon. Phase 2 widens via HIR analysis. + assert!(!in_scope("crates/fbuild-cli/src/cli/build.rs")); + assert!(!in_scope("crates/fbuild-build/src/orchestrator.rs")); + } +} diff --git a/dylints/ban_std_mpsc_in_async_reachable/Cargo.toml b/dylints/ban_std_mpsc_in_async_reachable/Cargo.toml new file mode 100644 index 000000000..9569e1e32 --- /dev/null +++ b/dylints/ban_std_mpsc_in_async_reachable/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_std_mpsc_in_async_reachable" +version = "0.1.0" +description = "Ban std::sync::mpsc in fbuild production code — use fbuild_core::channel" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_std_mpsc_in_async_reachable/README.md b/dylints/ban_std_mpsc_in_async_reachable/README.md new file mode 100644 index 000000000..dcfe3b8a8 --- /dev/null +++ b/dylints/ban_std_mpsc_in_async_reachable/README.md @@ -0,0 +1,26 @@ +# `ban_std_mpsc_in_async_reachable` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans std::sync::mpsc::* in fbuild production code. Replacement: fbuild_core::channel::{bounded, unbounded} (tokio mpsc wrappers). + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_std_mpsc_in_async_reachable/rust-toolchain.toml b/dylints/ban_std_mpsc_in_async_reachable/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_std_mpsc_in_async_reachable/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_std_mpsc_in_async_reachable/src/README.md b/dylints/ban_std_mpsc_in_async_reachable/src/README.md new file mode 100644 index 000000000..8c23a37bd --- /dev/null +++ b/dylints/ban_std_mpsc_in_async_reachable/src/README.md @@ -0,0 +1,13 @@ +# `ban_std_mpsc_in_async_reachable` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_std_mpsc_in_async_reachable/src/allowlist.txt b/dylints/ban_std_mpsc_in_async_reachable/src/allowlist.txt new file mode 100644 index 000000000..db0cc6dd7 --- /dev/null +++ b/dylints/ban_std_mpsc_in_async_reachable/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every call site migrates in Phase 2. Bridge / scope +# exemptions live in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_std_mpsc_in_async_reachable/src/lib.rs b/dylints/ban_std_mpsc_in_async_reachable/src/lib.rs new file mode 100644 index 000000000..d10b4422a --- /dev/null +++ b/dylints/ban_std_mpsc_in_async_reachable/src/lib.rs @@ -0,0 +1,156 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, AmbigArg, Expr, ExprKind, Ty, TyKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::sync::mpsc::*` (channel, Sender, Receiver, + /// SyncSender, sync_channel) in fbuild production code. + /// + /// ### Why is this bad? + /// + /// `std::sync::mpsc::Receiver::recv` blocks the calling OS + /// thread. From inside async that's a worker — every other task + /// on that worker stalls until a message arrives. The tokio + /// equivalent (`tokio::sync::mpsc`) integrates with the reactor + /// and is awaitable. + /// + /// FastLED/fbuild#844 (bridge sweep, "Bridge pair 4"). Per user + /// directive this ships with **zero allowlist**. + /// + /// ### Example + /// + /// ```rust,ignore + /// let (tx, rx) = std::sync::mpsc::channel(); // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// let (tx, rx) = fbuild_core::channel::bounded(64); + /// // or unbounded: + /// let (tx, rx) = fbuild_core::channel::unbounded(); + /// ``` + pub BAN_STD_MPSC_IN_ASYNC_REACHABLE, + Deny, + "ban std::sync::mpsc::* in fbuild production code" +} + +/// Module prefix we match on (any item under `std::sync::mpsc::`). +const MPSC_PREFIX: &[&str] = &["std", "sync", "mpsc"]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanStdMpscInAsyncReachable { + fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx, AmbigArg>) { + let filename = source_filename(cx, ty.span); + let normalized = normalize_slashes(&filename); + if !in_production_scope(&normalized) || is_allowlisted(&normalized) { + return; + } + if let TyKind::Path(qpath) = ty.kind { + let res = cx.qpath_res(&qpath, ty.hir_id); + if res_is_under_mpsc(cx, res) { + emit_lint(cx, ty.span); + } + } + } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + if !in_production_scope(&normalized) || is_allowlisted(&normalized) { + return; + } + if let ExprKind::Path(qpath) = expr.kind { + let res = cx.qpath_res(&qpath, expr.hir_id); + if res_is_under_mpsc(cx, res) { + emit_lint(cx, expr.span); + } + } + } +} + +fn res_is_under_mpsc(cx: &LateContext<'_>, res: Res) -> bool { + match res { + Res::Def(_, def_id) => def_path_starts_with(cx, def_id, MPSC_PREFIX), + _ => false, + } +} + +fn def_path_starts_with( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + prefix: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + def_path.len() >= prefix.len() + && def_path + .iter() + .take(prefix.len()) + .zip(prefix.iter()) + .all(|(actual, expected)| *actual == Symbol::intern(expected)) +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_STD_MPSC_IN_ASYNC_REACHABLE, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "`std::sync::mpsc::*` blocks the OS thread on `.recv()`. \ + Use `fbuild_core::channel::{bounded, unbounded}` (tokio mpsc) \ + so the channel integrates with the reactor and `.recv().await` \ + yields to the runtime. See FastLED/fbuild#844.", + ); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} diff --git a/dylints/ban_std_thread_sleep/Cargo.toml b/dylints/ban_std_thread_sleep/Cargo.toml new file mode 100644 index 000000000..050f284c3 --- /dev/null +++ b/dylints/ban_std_thread_sleep/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_std_thread_sleep" +version = "0.1.0" +description = "Ban std::thread::sleep — use fbuild_core::time::sleep instead" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_std_thread_sleep/README.md b/dylints/ban_std_thread_sleep/README.md new file mode 100644 index 000000000..69ee635bc --- /dev/null +++ b/dylints/ban_std_thread_sleep/README.md @@ -0,0 +1,26 @@ +# `ban_std_thread_sleep` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans std::thread::sleep in fbuild production code. Replacement: fbuild_core::time::sleep(...).await. + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_std_thread_sleep/rust-toolchain.toml b/dylints/ban_std_thread_sleep/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_std_thread_sleep/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_std_thread_sleep/src/README.md b/dylints/ban_std_thread_sleep/src/README.md new file mode 100644 index 000000000..6ff20f401 --- /dev/null +++ b/dylints/ban_std_thread_sleep/src/README.md @@ -0,0 +1,13 @@ +# `ban_std_thread_sleep` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_std_thread_sleep/src/allowlist.txt b/dylints/ban_std_thread_sleep/src/allowlist.txt new file mode 100644 index 000000000..db0cc6dd7 --- /dev/null +++ b/dylints/ban_std_thread_sleep/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every call site migrates in Phase 2. Bridge / scope +# exemptions live in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_std_thread_sleep/src/lib.rs b/dylints/ban_std_thread_sleep/src/lib.rs new file mode 100644 index 000000000..f03d2801f --- /dev/null +++ b/dylints/ban_std_thread_sleep/src/lib.rs @@ -0,0 +1,171 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{def::Res, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{symbol::Symbol, FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `std::thread::sleep` in fbuild production code. + /// + /// ### Why is this bad? + /// + /// `std::thread::sleep` blocks the OS thread. Inside the tokio + /// runtime that's a worker — every other task on that worker + /// stalls until the sleep returns. Even outside async contexts, + /// the consistent convention is `tokio::time::sleep` so + /// switching a function from sync to async doesn't silently + /// introduce a worker-blocking call. + /// + /// FastLED/fbuild#844 (bridge sweep, "Bridge pair 3"). Per user + /// directive this ships with **zero allowlist** — every + /// `std::thread::sleep` migrates in Phase 2. + /// + /// ### Example + /// + /// ```rust,ignore + /// std::thread::sleep(Duration::from_millis(100)); // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// fbuild_core::time::sleep(Duration::from_millis(100)).await; + /// // or, with a named constant: + /// fbuild_core::time::sleep(fbuild_core::time::POLL_100MS).await; + /// ``` + pub BAN_STD_THREAD_SLEEP, + Deny, + "ban std::thread::sleep — use fbuild_core::time::sleep instead" +} + +const BANNED_PATHS: &[&[&str]] = &[&["std", "thread", "sleep"], &["std", "thread", "sleep_ms"]]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanStdThreadSleep { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + let filename = source_filename(cx, expr.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_allowlisted(&normalized) { + return; + } + + match expr.kind { + ExprKind::Path(ref qpath) => { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, expr.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + ExprKind::Call(ref func, _) => { + if let ExprKind::Path(ref qpath) = func.kind { + if let Res::Def(_, def_id) = cx.qpath_res(qpath, func.hir_id) { + check_def_id(cx, expr.span, def_id); + } + } + } + _ => {} + } + } +} + +fn check_def_id(cx: &LateContext<'_>, span: rustc_span::Span, def_id: rustc_hir::def_id::DefId) { + for banned in BANNED_PATHS { + if def_path_equals(cx, def_id, banned) { + emit_lint(cx, span); + return; + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_STD_THREAD_SLEEP, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "`std::thread::sleep` blocks the OS thread (or a tokio worker). \ + Use `fbuild_core::time::sleep(...).await` so the workspace has \ + one consistent sleep surface — and so switching the calling \ + function from sync to async doesn't silently introduce a \ + worker-blocking call. See FastLED/fbuild#844.", + ); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +fn def_path_equals( + cx: &LateContext<'_>, + def_id: rustc_hir::def_id::DefId, + expected: &[&str], +) -> bool { + let def_path = cx.get_def_path(def_id); + if def_path.len() != expected.len() { + return false; + } + def_path + .iter() + .zip(expected.iter()) + .all(|(actual, expected_segment)| *actual == Symbol::intern(expected_segment)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn production_scope_matches() { + assert!(in_production_scope("crates/fbuild-cli/src/cli/build.rs")); + assert!(!in_production_scope("crates/fbuild-cli/tests/integration.rs")); + } +} diff --git a/dylints/ban_tokio_fs_direct_import/Cargo.toml b/dylints/ban_tokio_fs_direct_import/Cargo.toml new file mode 100644 index 000000000..dda0075a1 --- /dev/null +++ b/dylints/ban_tokio_fs_direct_import/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_tokio_fs_direct_import" +version = "0.1.0" +description = "Ban direct `use tokio::fs` imports outside the fbuild fs bridge" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_tokio_fs_direct_import/README.md b/dylints/ban_tokio_fs_direct_import/README.md new file mode 100644 index 000000000..6668b354b --- /dev/null +++ b/dylints/ban_tokio_fs_direct_import/README.md @@ -0,0 +1,26 @@ +# `ban_tokio_fs_direct_import` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans direct use tokio::fs imports outside crates/fbuild-core/src/fs.rs. Replacement: use fbuild_core::fs. + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_tokio_fs_direct_import/rust-toolchain.toml b/dylints/ban_tokio_fs_direct_import/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_tokio_fs_direct_import/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_tokio_fs_direct_import/src/README.md b/dylints/ban_tokio_fs_direct_import/src/README.md new file mode 100644 index 000000000..5a31b9dbe --- /dev/null +++ b/dylints/ban_tokio_fs_direct_import/src/README.md @@ -0,0 +1,13 @@ +# `ban_tokio_fs_direct_import` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_tokio_fs_direct_import/src/allowlist.txt b/dylints/ban_tokio_fs_direct_import/src/allowlist.txt new file mode 100644 index 000000000..db0cc6dd7 --- /dev/null +++ b/dylints/ban_tokio_fs_direct_import/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every call site migrates in Phase 2. Bridge / scope +# exemptions live in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_tokio_fs_direct_import/src/lib.rs b/dylints/ban_tokio_fs_direct_import/src/lib.rs new file mode 100644 index 000000000..b26078c02 --- /dev/null +++ b/dylints/ban_tokio_fs_direct_import/src/lib.rs @@ -0,0 +1,149 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{Item, ItemKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `use tokio::fs::*` (and any subpath thereof) in + /// production code, except inside the bridge module + /// (`crates/fbuild-core/src/fs.rs`). + /// + /// ### Why is this bad? + /// + /// All async filesystem access in fbuild flows through + /// `fbuild_core::fs::*` so the workspace has one source of truth + /// for the tokio fs surface. Direct imports bypass the bridge and + /// the cohesive surface starts to fragment. + /// + /// FastLED/fbuild#844 (bridge sweep, "Bridge pair 2"). + /// + /// ### Example + /// + /// ```rust,ignore + /// use tokio::fs; // banned + /// use tokio::fs::File; // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// use fbuild_core::fs; + /// use fbuild_core::fs::File; + /// ``` + pub BAN_TOKIO_FS_DIRECT_IMPORT, + Deny, + "ban direct `use tokio::fs` imports outside the fbuild fs bridge" +} + +const BRIDGE_MODULES: &[&str] = &["crates/fbuild-core/src/fs.rs"]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanTokioFsDirectImport { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + let filename = source_filename(cx, item.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_bridge_module(&normalized) || is_allowlisted(&normalized) { + return; + } + + if let ItemKind::Use(path, _kind) = &item.kind { + let segs = path + .segments + .iter() + .map(|s| s.ident.as_str().to_string()) + .collect::>(); + // `use tokio::fs[::...]`. The resolver expands list-imports + // (`use tokio::{fs, sync}`) into individual `UseKind::Single` + // items in HIR — each one shows up here with its own + // resolved path, so this single check covers all forms. + if segs.len() >= 2 && segs[0] == "tokio" && segs[1] == "fs" { + emit_lint(cx, item.span); + } + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_TOKIO_FS_DIRECT_IMPORT, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "direct `use tokio::fs` bypasses the fbuild fs bridge. Import \ + from `fbuild_core::fs` instead so the workspace has one \ + source of truth for the async filesystem surface. See \ + FastLED/fbuild#844.", + ); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_bridge_module(normalized: &str) -> bool { + BRIDGE_MODULES + .iter() + .any(|bridge| normalized.ends_with(bridge)) +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn bridge_module_recognized() { + assert!(is_bridge_module("crates/fbuild-core/src/fs.rs")); + assert!(!is_bridge_module("crates/fbuild-cli/src/cli/build.rs")); + } +} diff --git a/dylints/ban_tokio_mpsc_direct_import/Cargo.toml b/dylints/ban_tokio_mpsc_direct_import/Cargo.toml new file mode 100644 index 000000000..fc2513c24 --- /dev/null +++ b/dylints/ban_tokio_mpsc_direct_import/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "ban_tokio_mpsc_direct_import" +version = "0.1.0" +description = "Ban direct `use tokio::sync::mpsc` imports outside the fbuild channel bridge" +edition = "2021" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[dev-dependencies] +dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/dylints/ban_tokio_mpsc_direct_import/README.md b/dylints/ban_tokio_mpsc_direct_import/README.md new file mode 100644 index 000000000..53f327061 --- /dev/null +++ b/dylints/ban_tokio_mpsc_direct_import/README.md @@ -0,0 +1,26 @@ +# `ban_tokio_mpsc_direct_import` + +Custom [dylint](https://github.com/trailofbits/dylint) for the fbuild +internal bridge sweep (FastLED/fbuild#844). + +## What + +Bans direct use tokio::sync::mpsc imports outside crates/fbuild-core/src/channel.rs. Replacement: use fbuild_core::channel. + +## Why + +See FastLED/fbuild#844. fbuild standardizes on internal bridge APIs +(`fbuild_core::http`, `fbuild_core::fs`, `fbuild_core::time`, +`fbuild_core::channel`, `fbuild_core::path`, `fbuild_cli::output`) +so the workspace has one source of truth for each external primitive. + +## Allowlist + +Empty by design. Bridge / scope exemptions live in `lib.rs` by file +path, not in `src/allowlist.txt`. + +## Toolchain + +Pinned to `nightly-2026-03-26` to match every other dylint in this +repo. See the top-level `dylints/README.md` for the full setup +instructions and the rationale for `build_dylint_driver.py`. diff --git a/dylints/ban_tokio_mpsc_direct_import/rust-toolchain.toml b/dylints/ban_tokio_mpsc_direct_import/rust-toolchain.toml new file mode 100644 index 000000000..3b6ccbe8a --- /dev/null +++ b/dylints/ban_tokio_mpsc_direct_import/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2026-03-26" +components = ["llvm-tools-preview", "rust-src", "rustc-dev"] diff --git a/dylints/ban_tokio_mpsc_direct_import/src/README.md b/dylints/ban_tokio_mpsc_direct_import/src/README.md new file mode 100644 index 000000000..a3363f7f1 --- /dev/null +++ b/dylints/ban_tokio_mpsc_direct_import/src/README.md @@ -0,0 +1,13 @@ +# `ban_tokio_mpsc_direct_import` — sources + +See the top-level [`../README.md`](../README.md) for the lint contract, +allowlist policy, and replacement API. This directory contains: + +- **`lib.rs`** — the late-pass `LintContext` visitor. +- **`allowlist.txt`** — empty by design for the #844 sweep. Bridge / + scope exemptions live in `lib.rs` by file path. + +Both files are loaded at lint-compile time via `include_str!` in +`lib.rs`. To regenerate after editing the allowlist bump the version +in this lint's `Cargo.toml` (the dylint .so cache is keyed off the +manifest, not the allowlist). diff --git a/dylints/ban_tokio_mpsc_direct_import/src/allowlist.txt b/dylints/ban_tokio_mpsc_direct_import/src/allowlist.txt new file mode 100644 index 000000000..db0cc6dd7 --- /dev/null +++ b/dylints/ban_tokio_mpsc_direct_import/src/allowlist.txt @@ -0,0 +1,6 @@ +# FastLED/fbuild#844 ships this lint with ZERO allowlist entries — per +# user directive every call site migrates in Phase 2. Bridge / scope +# exemptions live in `lib.rs` by file path, not here. +# +# Each non-empty, non-comment line is matched against the tail of each +# source file path (slashes normalized to /). diff --git a/dylints/ban_tokio_mpsc_direct_import/src/lib.rs b/dylints/ban_tokio_mpsc_direct_import/src/lib.rs new file mode 100644 index 000000000..19bf65208 --- /dev/null +++ b/dylints/ban_tokio_mpsc_direct_import/src/lib.rs @@ -0,0 +1,146 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_errors::DiagDecorator; +use rustc_hir::{Item, ItemKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{FileName, RemapPathScopeComponents}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Bans `use tokio::sync::mpsc::*` (and any subpath thereof) in + /// production code, except inside the bridge module + /// (`crates/fbuild-core/src/channel.rs`). + /// + /// ### Why is this bad? + /// + /// All async channels in fbuild flow through + /// `fbuild_core::channel::{bounded, unbounded}` so the workspace + /// has one source of truth for the channel surface. Direct + /// imports bypass the bridge and the cohesive surface starts to + /// fragment. + /// + /// FastLED/fbuild#844 (bridge sweep, "Bridge pair 4"). + /// + /// ### Example + /// + /// ```rust,ignore + /// use tokio::sync::mpsc; // banned + /// use tokio::sync::mpsc::{channel, Sender}; // banned + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// use fbuild_core::channel::{bounded, Sender}; + /// ``` + pub BAN_TOKIO_MPSC_DIRECT_IMPORT, + Deny, + "ban direct `use tokio::sync::mpsc` imports outside the fbuild channel bridge" +} + +const BRIDGE_MODULES: &[&str] = &["crates/fbuild-core/src/channel.rs"]; + +const ALLOWLIST: &str = include_str!("allowlist.txt"); + +const CRATES_PREFIX: &str = "crates/"; +const SRC_SEGMENT: &str = "/src/"; + +impl<'tcx> LateLintPass<'tcx> for BanTokioMpscDirectImport { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + let filename = source_filename(cx, item.span); + let normalized = normalize_slashes(&filename); + + if !in_production_scope(&normalized) { + return; + } + if is_bridge_module(&normalized) || is_allowlisted(&normalized) { + return; + } + + if let ItemKind::Use(path, _) = &item.kind { + let segs = path + .segments + .iter() + .map(|s| s.ident.as_str().to_string()) + .collect::>(); + // `use tokio::sync::mpsc[::...]` + if segs.len() >= 3 && segs[0] == "tokio" && segs[1] == "sync" && segs[2] == "mpsc" { + emit_lint(cx, item.span); + } + } + } +} + +fn emit_lint(cx: &LateContext<'_>, span: rustc_span::Span) { + cx.opt_span_lint( + BAN_TOKIO_MPSC_DIRECT_IMPORT, + Some(span), + DiagDecorator(|diag| { + diag.primary_message( + "direct `use tokio::sync::mpsc` bypasses the fbuild channel \ + bridge. Import from `fbuild_core::channel` instead so the \ + workspace has one source of truth for the async channel \ + surface. See FastLED/fbuild#844.", + ); + }), + ); +} + +fn source_filename(cx: &LateContext<'_>, span: rustc_span::Span) -> String { + match cx.sess().source_map().span_to_filename(span) { + FileName::Real(real_filename) => real_filename + .local_path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| { + real_filename + .path(RemapPathScopeComponents::DIAGNOSTICS) + .to_string_lossy() + .into_owned() + }), + filename => filename + .display(RemapPathScopeComponents::DIAGNOSTICS) + .to_string(), + } +} + +fn in_production_scope(normalized: &str) -> bool { + let Some(crates_at) = normalized.find(CRATES_PREFIX) else { + return false; + }; + let after_crates = &normalized[crates_at + CRATES_PREFIX.len()..]; + after_crates.contains(SRC_SEGMENT) +} + +fn is_bridge_module(normalized: &str) -> bool { + BRIDGE_MODULES + .iter() + .any(|bridge| normalized.ends_with(bridge)) +} + +fn is_allowlisted(normalized: &str) -> bool { + ALLOWLIST + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .any(|allowed| normalized.ends_with(allowed)) +} + +fn normalize_slashes(path: &str) -> String { + path.replace('\\', "/") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn bridge_module_recognized() { + assert!(is_bridge_module("crates/fbuild-core/src/channel.rs")); + assert!(!is_bridge_module("crates/fbuild-cli/src/cli/build.rs")); + } +} diff --git a/dylints/ban_unrooted_tempdir/src/allowlist.txt b/dylints/ban_unrooted_tempdir/src/allowlist.txt index c0459d557..a17a80099 100644 --- a/dylints/ban_unrooted_tempdir/src/allowlist.txt +++ b/dylints/ban_unrooted_tempdir/src/allowlist.txt @@ -46,7 +46,6 @@ crates/fbuild-build/src/renesas/renesas_compiler.rs crates/fbuild-build/src/resolution.rs crates/fbuild-build/src/rp2040/orchestrator.rs crates/fbuild-build/src/sam/orchestrator.rs -crates/fbuild-build/src/script_runtime.rs crates/fbuild-build/src/script_runtime_tests.rs crates/fbuild-build/src/stm32/orchestrator/mod.rs crates/fbuild-build/src/symbol_analyzer/tests.rs