From 7ab8c117f38aff9b831d0e44e0b90a0fb7bb296d Mon Sep 17 00:00:00 2001 From: zackees Date: Sun, 23 Aug 2026 10:29:46 -0700 Subject: [PATCH] chore(paths): route fbuild-deploy tool paths through fbuild-paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five tool resolvers in `fbuild-deploy` each hand-rolled `~/.fbuild/{dev|prod}/tools`, and all five disagreed with `fbuild-paths` about what dev mode is: they tested `var_os("FBUILD_DEV_MODE").is_some()`, while `fbuild_paths::is_dev_mode()` requires the literal `1`. So `FBUILD_DEV_MODE=0` — an explicit opt *out* — sent lpc21isp, probe-rs, wchisp, wlink and the LPC-Link2 debugger assets to the dev tree while every other fbuild path stayed in prod. A tool installed where fbuild said to install it would then go on being not-found. The new test in `lpc.rs` fails on the old code with exactly that split. That is the drift FastLED/fbuild#1349 predicted, found by removing the duplication rather than by hitting it in the field. `fbuild-paths` grows the accessors the call sites needed and did not have: - `try_get_fbuild_root` / `try_get_tools_dir` — the `Option`-returning forms. The resolvers report a missing home directory rather than panicking, and that is why they re-derived the path instead of calling `get_fbuild_root`, which panics. - `get_tools_dir` / `TOOLS_DIR_NAME` — the managed-tools location itself. - `tools_dir_label` — the `~/.fbuild/{dev|prod}/tools` spelling for diagnostics, so an install hint names the tree fbuild will actually search instead of hardcoding `prod`. Allowlist: 48 -> 43. Lint crate version bumped so the cached `.so` is rebuilt against the shrunk list. Co-Authored-By: Claude Opus 5 (1M context) --- crates/fbuild-deploy/src/lpc.rs | 100 ++++++++++++++---- .../fbuild-deploy/src/lpc_debugger_reflash.rs | 17 +-- crates/fbuild-deploy/src/probe_rs.rs | 13 +-- crates/fbuild-deploy/src/wchisp.rs | 20 +--- crates/fbuild-deploy/src/wlink.rs | 20 +--- crates/fbuild-paths/src/lib.rs | 47 +++++++- dylints/ban_raw_fbuild_path/Cargo.toml | 2 +- dylints/ban_raw_fbuild_path/src/allowlist.txt | 5 - 8 files changed, 138 insertions(+), 86 deletions(-) diff --git a/crates/fbuild-deploy/src/lpc.rs b/crates/fbuild-deploy/src/lpc.rs index 0843a9e9..17a9256d 100644 --- a/crates/fbuild-deploy/src/lpc.rs +++ b/crates/fbuild-deploy/src/lpc.rs @@ -20,13 +20,6 @@ use crate::{DeployOutcome, Deployer, DeploymentResult}; /// lpc21isp itself. pub const LPC21ISP_PATH_ENV_VAR: &str = "FBUILD_LPC21ISP_PATH"; -/// Resolve the user's home directory through the neutral host facade -/// (`%USERPROFILE%` with a `%HOME%` fallback on Windows, `$HOME` -/// elsewhere) so this module carries no per-OS env logic. -fn home_dir() -> Option { - fbuild_core::platform::host::home_dir().map(|home| home.into_path_buf()) -} - /// The one canonical location fbuild manages lpc21isp at. FastLED/fbuild#921 /// treats lpc21isp as a fbuild-owned dependency — auto-install will drop /// the binary here in a follow-up PR, and the deployer reads it back from @@ -38,13 +31,7 @@ fn home_dir() -> Option { /// isolation the rest of `fbuild-paths` applies. pub fn managed_lpc21isp_path() -> Option { let exe = fbuild_core::platform::executable::native_name("lpc21isp"); - let home = home_dir()?; - let mode = if std::env::var_os("FBUILD_DEV_MODE").is_some() { - "dev" - } else { - "prod" - }; - Some(home.join(".fbuild").join(mode).join("tools").join(exe)) + Some(fbuild_paths::try_get_tools_dir()?.join(exe)) } /// Resolve where `lpc21isp` lives on this system. @@ -89,7 +76,14 @@ pub fn find_lpc21isp() -> Option { /// deploy path. Kept as a standalone function so the test module can /// assert the exact URLs / paths without shelling out. pub(crate) fn lpc21isp_install_hint() -> String { - let tools_dir = "~/.fbuild/prod/tools/"; + // Name the directory fbuild will actually look in — under + // `FBUILD_DEV_MODE=1` that is the dev tree, and telling a dev-mode user + // to install into `prod` is how a "correctly installed" tool goes on + // being not-found (FastLED/fbuild#1349). + let tools_dir = fbuild_paths::try_get_tools_dir() + .map(|p| p.display().to_string()) + .unwrap_or_else(fbuild_paths::tools_dir_label); + let tools_dir = format!("{tools_dir}{}", std::path::MAIN_SEPARATOR); let exe = fbuild_core::platform::executable::native_name("lpc21isp"); format!( "lpc21isp not found on PATH or in any fbuild-managed tools dir.\n\ @@ -714,12 +708,39 @@ fn port_names_match(sys: &str, user: &str) -> bool { mod tests { use super::*; - // The resolver tests below temporarily mutate the same process-wide - // override. Cargo runs unit tests in parallel, so serialize that narrow - // shared state rather than letting one test restore the other test's - // value (which was visible as a Windows-only intermittent miss). + // The resolver tests below temporarily mutate process-wide env + // overrides (`FBUILD_LPC21ISP_PATH`, `FBUILD_DEV_MODE`). Cargo runs unit + // tests in parallel, so serialize that narrow shared state rather than + // letting one test restore the other test's value (which was visible as a + // Windows-only intermittent miss). Tests that only *read* a path derived + // from those vars take the lock too — otherwise they sample the value + // mid-flip. static LPC21ISP_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Restores an env var when dropped, so a failing assertion cannot leave + /// the flipped value behind for whichever test runs next. + struct EnvVarGuard { + key: &'static str, + saved: Option, + } + + impl EnvVarGuard { + fn set(key: &'static str, value: &str) -> Self { + let saved = std::env::var_os(key); + std::env::set_var(key, value); + Self { key, saved } + } + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + match self.saved.take() { + Some(v) => std::env::set_var(self.key, v), + None => std::env::remove_var(self.key), + } + } + } + #[test] fn test_lpc_deployer_creation() { let deployer = LpcDeployer::new("115200", 12_000, 60, None, false); @@ -905,6 +926,9 @@ mod tests { #[test] fn install_hint_mentions_env_var_and_download_source() { + // Reads a `FBUILD_DEV_MODE`-derived path twice; without the lock it + // can sample prod on one read and dev on the other. + let _lock = LPC21ISP_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let hint = lpc21isp_install_hint(); assert!( hint.contains(LPC21ISP_PATH_ENV_VAR), @@ -914,14 +938,48 @@ mod tests { hint.contains("sourceforge.net"), "hint must point at a download" ); + let tools_dir = fbuild_paths::try_get_tools_dir() + .map(|p| p.display().to_string()) + .unwrap_or_else(fbuild_paths::tools_dir_label); assert!( - hint.contains("~/.fbuild/prod/tools/"), + hint.contains(&tools_dir), "hint must direct the user at the fbuild-managed tools dir \ - (never an out-of-tree C:\\tools\\ path or PATH-walk)" + (never an out-of-tree C:\\tools\\ path or PATH-walk): {hint}" ); assert!(hint.contains("#921"), "hint must cite the tracking issue"); } + /// FastLED/fbuild#1349: the managed-tools path must be the one + /// `fbuild-paths` defines, not a private re-derivation of it. + /// + /// The re-derivations tested `FBUILD_DEV_MODE` with `var_os(..).is_some()`, + /// so `FBUILD_DEV_MODE=0` — an explicit *opt out* — selected the dev tree + /// here while `fbuild_paths::is_dev_mode()` (which requires the literal + /// `1`) kept every other path in prod. A deploy would then look for its + /// tools in a directory nothing installs into. + #[test] + fn managed_tools_path_agrees_with_fbuild_paths_on_dev_mode() { + let _lock = LPC21ISP_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + + for (value, expected_dev) in [("0", false), ("1", true), ("true", false)] { + let _dev_mode = EnvVarGuard::set("FBUILD_DEV_MODE", value); + let Some(expected) = fbuild_paths::try_get_tools_dir() else { + continue; // no home dir on this host; nothing to compare against + }; + let got = managed_lpc21isp_path().expect("home dir was resolvable just above"); + assert_eq!( + got.parent(), + Some(expected.as_path()), + "FBUILD_DEV_MODE={value:?} must resolve the same tools dir as fbuild-paths" + ); + assert_eq!( + fbuild_paths::is_dev_mode(), + expected_dev, + "only the literal `1` enables dev mode" + ); + } + } + // ---------- FastLED/fbuild#927 baud + hex-flag fixes ---------- #[test] diff --git a/crates/fbuild-deploy/src/lpc_debugger_reflash.rs b/crates/fbuild-deploy/src/lpc_debugger_reflash.rs index bbf2b674..05bed7bd 100644 --- a/crates/fbuild-deploy/src/lpc_debugger_reflash.rs +++ b/crates/fbuild-deploy/src/lpc_debugger_reflash.rs @@ -75,18 +75,7 @@ pub const LPC_LINK2_FIRMWARE_ENV_VAR: &str = "FBUILD_LPC_LINK2_FIRMWARE"; /// tools under. Honors `FBUILD_DEV_MODE=1` for `~/.fbuild/dev/…` /// isolation, same as `find_lpc21isp` and the rest of `fbuild-paths`. pub fn managed_tools_dir() -> Option { - let home = fbuild_core::platform::host::home_dir()?; - let mode = if std::env::var_os("FBUILD_DEV_MODE").is_some() { - "dev" - } else { - "prod" - }; - Some( - home.join(".fbuild") - .join(mode) - .join("tools") - .join("lpc-link2-debugger"), - ) + Some(NormalizedPath::from(fbuild_paths::try_get_tools_dir()?).join("lpc-link2-debugger")) } /// Absolute URL for one of the vendored assets under the framework @@ -189,7 +178,9 @@ pub fn install_hint() -> String { Tracked under FastLED/fbuild#921.", tools = managed_tools_dir() .map(|p| p.display().to_string()) - .unwrap_or_else(|| "~/.fbuild/prod/tools/lpc-link2-debugger/".to_string()), + .unwrap_or_else(|| { + format!("{}/lpc-link2-debugger", fbuild_paths::tools_dir_label()) + }), v2_name = CMSIS_DAP_V2_HEX_NAME, dfu_env = DFU_UTIL_PATH_ENV_VAR, fw_env = LPC_LINK2_FIRMWARE_ENV_VAR, diff --git a/crates/fbuild-deploy/src/probe_rs.rs b/crates/fbuild-deploy/src/probe_rs.rs index ce1cedb7..fa4623e0 100644 --- a/crates/fbuild-deploy/src/probe_rs.rs +++ b/crates/fbuild-deploy/src/probe_rs.rs @@ -117,18 +117,7 @@ pub fn probe_rs_release_asset_for_host() -> Result { /// Honors `FBUILD_DEV_MODE=1` → `~/.fbuild/dev/tools/probe-rs/` to /// match the isolation the rest of `fbuild-paths` applies. pub fn managed_probe_rs_dir() -> Option { - let home = fbuild_core::platform::host::home_dir()?; - let mode = if std::env::var_os("FBUILD_DEV_MODE").is_some() { - "dev" - } else { - "prod" - }; - Some( - home.join(".fbuild") - .join(mode) - .join("tools") - .join("probe-rs"), - ) + Some(NormalizedPath::from(fbuild_paths::try_get_tools_dir()?).join("probe-rs")) } pub fn managed_probe_rs_path() -> Option { diff --git a/crates/fbuild-deploy/src/wchisp.rs b/crates/fbuild-deploy/src/wchisp.rs index 63f1b1c1..9c4eb29f 100644 --- a/crates/fbuild-deploy/src/wchisp.rs +++ b/crates/fbuild-deploy/src/wchisp.rs @@ -48,22 +48,10 @@ fn release_asset() -> Result { } fn managed_wchisp_path() -> Result { - let home = std::env::var_os(if fbuild_core::platform::host::is_windows() { - "USERPROFILE" - } else { - "HOME" - }) - .map(PathBuf::from) - .ok_or_else(|| FbuildError::PackageError("could not determine home directory".to_string()))?; - let mode = if std::env::var_os("FBUILD_DEV_MODE").is_some() { - "dev" - } else { - "prod" - }; - Ok(home - .join(".fbuild") - .join(mode) - .join("tools") + let tools = fbuild_paths::try_get_tools_dir().ok_or_else(|| { + FbuildError::PackageError("could not determine home directory".to_string()) + })?; + Ok(tools .join("wchisp") .join(fbuild_core::platform::executable::native_name("wchisp"))) } diff --git a/crates/fbuild-deploy/src/wlink.rs b/crates/fbuild-deploy/src/wlink.rs index 97d3ef02..e17c83cf 100644 --- a/crates/fbuild-deploy/src/wlink.rs +++ b/crates/fbuild-deploy/src/wlink.rs @@ -45,22 +45,10 @@ fn release_asset() -> Result { } fn managed_wlink_path() -> Result { - let home = std::env::var_os(if fbuild_core::platform::host::is_windows() { - "USERPROFILE" - } else { - "HOME" - }) - .map(PathBuf::from) - .ok_or_else(|| FbuildError::PackageError("could not determine home directory".to_string()))?; - let mode = if std::env::var_os("FBUILD_DEV_MODE").is_some() { - "dev" - } else { - "prod" - }; - Ok(home - .join(".fbuild") - .join(mode) - .join("tools") + let tools = fbuild_paths::try_get_tools_dir().ok_or_else(|| { + FbuildError::PackageError("could not determine home directory".to_string()) + })?; + Ok(tools .join("wlink") .join(fbuild_core::platform::executable::native_name("wlink"))) } diff --git a/crates/fbuild-paths/src/lib.rs b/crates/fbuild-paths/src/lib.rs index cb910bc0..c9d7fa97 100644 --- a/crates/fbuild-paths/src/lib.rs +++ b/crates/fbuild-paths/src/lib.rs @@ -34,10 +34,53 @@ pub fn is_dev_mode() -> bool { } /// Root fbuild directory: `~/.fbuild/{dev|prod}` +/// +/// Panics when the home directory cannot be determined. Callers that must +/// degrade instead of panicking want [`try_get_fbuild_root`]. pub fn get_fbuild_root() -> PathBuf { - let home = dirs_next().expect("could not determine home directory"); + try_get_fbuild_root().expect("could not determine home directory") +} + +/// [`get_fbuild_root`] for callers that report a missing home directory +/// rather than panicking on it. +/// +/// Exists because several tool resolvers return `Option`/`Result` precisely +/// so a home-less environment is a diagnosable failure and not a crash; they +/// had each hand-rolled this path to keep that property (FastLED/fbuild#1349). +pub fn try_get_fbuild_root() -> Option { let mode = if is_dev_mode() { "dev" } else { "prod" }; - home.join(FBUILD_DIR_NAME).join(mode) + Some(dirs_next()?.join(FBUILD_DIR_NAME).join(mode)) +} + +/// The segment holding fbuild-managed external tools, under +/// [`get_fbuild_root`]: `tools`. +pub const TOOLS_DIR_NAME: &str = "tools"; + +/// Where fbuild installs and looks for managed external tools: +/// `~/.fbuild/{dev|prod}/tools`. +/// +/// Panics when the home directory cannot be determined; see +/// [`try_get_tools_dir`]. +pub fn get_tools_dir() -> PathBuf { + get_fbuild_root().join(TOOLS_DIR_NAME) +} + +/// [`get_tools_dir`] for callers that report a missing home directory rather +/// than panicking on it. +pub fn try_get_tools_dir() -> Option { + Some(try_get_fbuild_root()?.join(TOOLS_DIR_NAME)) +} + +/// Human-facing label for [`get_tools_dir`] when the real path cannot be +/// resolved — `~/.fbuild/{dev|prod}/tools`. +/// +/// Diagnostics that tell a user where to install a managed tool need +/// *something* to print even on a host with no discoverable home directory. +/// Producing it here keeps those messages honest about the current mode, and +/// keeps the `.fbuild` spelling from being re-typed at each call site. +pub fn tools_dir_label() -> String { + let mode = if is_dev_mode() { "dev" } else { "prod" }; + format!("~/{FBUILD_DIR_NAME}/{mode}/{TOOLS_DIR_NAME}") } /// Root fbuild directory for the OTHER mode (cross-mode fallback). diff --git a/dylints/ban_raw_fbuild_path/Cargo.toml b/dylints/ban_raw_fbuild_path/Cargo.toml index 5012b1f0..cdd9e7ec 100644 --- a/dylints/ban_raw_fbuild_path/Cargo.toml +++ b/dylints/ban_raw_fbuild_path/Cargo.toml @@ -3,7 +3,7 @@ name = "ban_raw_fbuild_path" # Bump the version to bust the dylint .so cache when allowlist.txt # changes (setup-soldr's dylint-cache key hashes the manifest but not # src/allowlist.txt). Same convention ban_manual_slash_normalize follows. -version = "0.1.0" +version = "0.1.1" description = "Ban raw '.fbuild' path literals outside fbuild-paths" edition = "2021" publish = false diff --git a/dylints/ban_raw_fbuild_path/src/allowlist.txt b/dylints/ban_raw_fbuild_path/src/allowlist.txt index 716ffe69..685d7f5b 100644 --- a/dylints/ban_raw_fbuild_path/src/allowlist.txt +++ b/dylints/ban_raw_fbuild_path/src/allowlist.txt @@ -62,10 +62,5 @@ crates/fbuild-daemon/src/handlers/operations/build.rs crates/fbuild-daemon/src/main.rs crates/fbuild-daemon/src/models.rs crates/fbuild-daemon/tests/legacy_daemon_transition.rs -crates/fbuild-deploy/src/lpc.rs -crates/fbuild-deploy/src/lpc_debugger_reflash.rs -crates/fbuild-deploy/src/probe_rs.rs -crates/fbuild-deploy/src/wchisp.rs -crates/fbuild-deploy/src/wlink.rs crates/fbuild-library-select/tests/teensy41_ldf_diag.rs crates/fbuild-library/src/library/library_compiler.rs