From df4099d7972292cdfdd8d86573622e4747e4f3aa Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Fri, 2 Oct 2026 11:50:00 -0700 Subject: [PATCH 1/3] fix(deploy): ESP32 deploy spawns the build-resolved esptool Esp32Deployer hard-coded a bare `esptool` argv[0], so deploy depended on PATH even though the build provisions a pinned esptool for the same env. - fbuild-deploy: add Esp32Deployer::with_esptool_path(Option); find_esptool() returns the resolved path, else bare `esptool` (which keeps the #1234 caller-PATH overlay). - fbuild-build-esp: add pub resolve_deploy_esptool(project_dir, env), reusing resolve_esptool (FBUILD_ESPTOOL_PATH, then the provisioned tool-esptoolpy package). A v4 esptool.py source package yields None, since the deployer emits the v5 hyphenated CLI. - fbuild-daemon: the ESP32 deploy arm passes the resolved path to the deployer; a resolution error is logged and falls back to PATH. Closes #1616 --- .../src/esp32/orchestrator/mod.rs | 1 + .../src/esp32/orchestrator/packages.rs | 46 ++++++++++++++++ .../src/esp32/orchestrator/tests.rs | 19 +++++++ .../src/handlers/operations/deploy.rs | 27 ++++++++- crates/fbuild-deploy/src/esp32/deployer.rs | 38 +++++++++---- crates/fbuild-deploy/src/esp32/tests.rs | 55 ++++++++++++++++++- 6 files changed, 172 insertions(+), 14 deletions(-) diff --git a/crates/fbuild-build-esp/src/esp32/orchestrator/mod.rs b/crates/fbuild-build-esp/src/esp32/orchestrator/mod.rs index f271a0f3a..c06ac6266 100644 --- a/crates/fbuild-build-esp/src/esp32/orchestrator/mod.rs +++ b/crates/fbuild-build-esp/src/esp32/orchestrator/mod.rs @@ -36,6 +36,7 @@ mod job_pool; mod local_libs; mod packages; +pub use packages::resolve_deploy_esptool; pub(crate) use packages::{downloadable_lib_deps, provision_esp32}; #[cfg(test)] diff --git a/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs b/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs index 4ac125280..8e82286db 100644 --- a/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs +++ b/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs @@ -125,6 +125,52 @@ pub(super) async fn resolve_pioarduino_packages( Ok((toolchain, framework, esptool_py, platform_info)) } +/// Resolve the esptool executable a deploy should spawn: the same one the +/// build uses (`FBUILD_ESPTOOL_PATH` first, then the provisioned +/// `tool-esptoolpy` package from the env's `platform.json`), so deploy works +/// from a shell with no esptool on PATH and runs the build's esptool version +/// (FastLED/fbuild#1616). +/// +/// `Ok(None)` means no usable resolved esptool; the deployer then falls back +/// to a bare `esptool` PATH lookup. That includes the v4 `esptool.py` source +/// package older pioarduino pins provision: the deployer speaks v5's +/// hyphenated CLI (`write-flash`, `default-reset`), which v4 rejects. +pub async fn resolve_deploy_esptool( + project_dir: &Path, + env_config: Option<&HashMap>, +) -> Result> { + let resolved = match fbuild_packages::library::esptool_path_override()? { + Some(path) => Some(path), + None => { + let Some(platform) = pioarduino_platform(project_dir, env_config, true).await? else { + return Ok(None); + }; + fbuild_packages::Package::ensure_installed(&platform).await?; + resolve_esptool(&platform, project_dir).await? + } + }; + Ok(resolved.filter(|path| { + let usable = speaks_v5_cli(path); + if !usable { + tracing::info!( + "resolved esptool {} is the v4 source package; deploy uses an `esptool` on PATH", + path.display() + ); + } + usable + })) +} + +/// Whether `esptool` speaks the v5 hyphenated CLI the deployer emits. The v4 +/// source package installs an `esptool.py` entry point (the same signal +/// `esp32_linker` uses to pick underscore flags for `elf2image`). +pub(super) fn speaks_v5_cli(esptool: &Path) -> bool { + !esptool + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("esptool.py")) +} + /// Provision what [`resolve_pioarduino_packages`] installs — platform, /// MCU-primary toolchain, framework, SDK libs and esptool — one report row /// each, for `fbuild install` (FastLED/fbuild#1433). Check and dry-run modes diff --git a/crates/fbuild-build-esp/src/esp32/orchestrator/tests.rs b/crates/fbuild-build-esp/src/esp32/orchestrator/tests.rs index ae41586d7..1cdfe0783 100644 --- a/crates/fbuild-build-esp/src/esp32/orchestrator/tests.rs +++ b/crates/fbuild-build-esp/src/esp32/orchestrator/tests.rs @@ -301,3 +301,22 @@ fn effective_define_flags_match_compiler_overlay_order() { ); assert!(!defines.contains_key("VALUE")); } + +/// FastLED/fbuild#1616: deploy reuses the build's esptool only when it speaks +/// the v5 hyphenated CLI the deployer emits; the v4 `esptool.py` source +/// package falls back to an `esptool` on PATH. +#[test] +fn deploy_esptool_requires_v5_cli() { + use super::packages::speaks_v5_cli; + use std::path::Path; + assert!(speaks_v5_cli(Path::new( + "/cache/tasmota-esptool/5.1.0/esptool-linux-amd64/esptool" + ))); + assert!(speaks_v5_cli(Path::new("C:/cache/esptool/esptool.exe"))); + assert!(!speaks_v5_cli(Path::new( + "/cache/tool-esptoolpy/fbuild-tools/esptool/bin/esptool.py" + ))); + assert!(!speaks_v5_cli(Path::new( + "C:/cache/fbuild-tools/esptool/Scripts/esptool.py.exe" + ))); +} diff --git a/crates/fbuild-daemon/src/handlers/operations/deploy.rs b/crates/fbuild-daemon/src/handlers/operations/deploy.rs index 4d0b17c45..183f56bfd 100644 --- a/crates/fbuild-daemon/src/handlers/operations/deploy.rs +++ b/crates/fbuild-daemon/src/handlers/operations/deploy.rs @@ -611,8 +611,13 @@ pub async fn deploy( &esptool_params, false, ) - // esptool is spawned as a bare name; resolve it against - // the requesting CLI's PATH, not the daemon's (#1234). + // Spawn the esptool the build resolved (FBUILD_ESPTOOL_PATH, + // then the provisioned package), not whatever `esptool` is on + // PATH (#1616). + .with_esptool_path(resolve_deploy_esptool(&deploy_project, env_config).await) + // Without a resolved esptool it is spawned as a bare name; + // resolve that against the requesting CLI's PATH, not the + // daemon's (#1234). .with_caller_path(deploy_caller_path.clone()); let deployer = if let Some(baud) = baud_override { deployer.with_baud_rate(&baud.to_string()) @@ -1440,6 +1445,24 @@ fn deployer_caller_path(req: &DeployRequest) -> Option { req.caller_path.clone() } +/// The esptool the build resolved for this env (FastLED/fbuild#1616), or +/// `None` to keep the bare `esptool` PATH lookup. A resolution failure is +/// logged rather than failing the deploy: an `esptool` on PATH still works. +async fn resolve_deploy_esptool( + project_dir: &std::path::Path, + env_config: &std::collections::HashMap, +) -> Option { + match fbuild_build::esp32::orchestrator::resolve_deploy_esptool(project_dir, Some(env_config)) + .await + { + Ok(path) => path.map(fbuild_core::path::NormalizedPath::into_path_buf), + Err(e) => { + tracing::warn!("could not resolve the build's esptool ({e}); using `esptool` on PATH"); + None + } + } +} + fn deploy_error_response( request_id: String, error: &fbuild_core::FbuildError, diff --git a/crates/fbuild-deploy/src/esp32/deployer.rs b/crates/fbuild-deploy/src/esp32/deployer.rs index bd6048b27..0d0be447e 100644 --- a/crates/fbuild-deploy/src/esp32/deployer.rs +++ b/crates/fbuild-deploy/src/esp32/deployer.rs @@ -1,7 +1,7 @@ //! `Esp32Deployer` core: construction, args, verify and write paths, and //! the [`Deployer`] trait implementation. -use std::path::Path; +use std::path::{Path, PathBuf}; use fbuild_core::Result; use fbuild_core::subprocess::run_command; @@ -65,11 +65,16 @@ pub struct Esp32Deployer { /// Feature-gated — see `use_native_verify`. #[cfg(feature = "espflash-native")] pub(super) use_native_write: bool, - /// The requesting CLI's PATH. `esptool` is spawned as a bare name - /// (see [`Esp32Deployer::find_esptool`]), so without this it would - /// resolve against the long-lived daemon's boot-time PATH + /// The requesting CLI's PATH. Without a resolved `esptool_path`, + /// `esptool` is spawned as a bare name (see + /// [`Esp32Deployer::find_esptool`]), so without this it would resolve + /// against the long-lived daemon's boot-time PATH /// (FastLED/fbuild#1234). `None` keeps legacy daemon-env behavior. pub(super) caller_path: Option, + /// The esptool the build resolved (`FBUILD_ESPTOOL_PATH` or the + /// provisioned `tool-esptoolpy` package). `None` falls back to a bare + /// `esptool` PATH lookup (FastLED/fbuild#1616). + pub(super) esptool_path: Option, } #[cfg(feature = "espflash-native")] @@ -152,6 +157,7 @@ impl Esp32Deployer { #[cfg(feature = "espflash-native")] use_native_write: false, caller_path: None, + esptool_path: None, } } @@ -240,11 +246,21 @@ impl Esp32Deployer { self } - /// Find the esptool executable. - /// - /// Uses standalone `esptool` command (available when esptool is pip-installed). - pub(super) fn find_esptool() -> Vec { - vec!["esptool".to_string()] + /// Use the esptool the build resolved instead of a bare `esptool` + /// PATH lookup (FastLED/fbuild#1616). `None` keeps the PATH lookup. + #[must_use] + pub fn with_esptool_path(mut self, esptool_path: Option) -> Self { + self.esptool_path = esptool_path; + self + } + + /// The esptool program: the resolved path when one was supplied, + /// otherwise the bare `esptool` command found on PATH. + pub(super) fn find_esptool(&self) -> Vec { + match &self.esptool_path { + Some(path) => vec![path.to_string_lossy().into_owned()], + None => vec!["esptool".to_string()], + } } /// Build the `esptool verify-flash` command line that this deployer @@ -262,7 +278,7 @@ impl Esp32Deployer { let bootloader_path = build_dir.join("bootloader.bin"); let partitions_path = build_dir.join("partitions.bin"); - let mut args = Self::find_esptool(); + let mut args = self.find_esptool(); args.extend([ "--chip".to_string(), self.chip.clone(), @@ -666,7 +682,7 @@ impl Esp32Deployer { let bootloader_path = build_dir.join("bootloader.bin"); let partitions_path = build_dir.join("partitions.bin"); - let mut args = Self::find_esptool(); + let mut args = self.find_esptool(); args.extend([ "--chip".to_string(), self.chip.clone(), diff --git a/crates/fbuild-deploy/src/esp32/tests.rs b/crates/fbuild-deploy/src/esp32/tests.rs index 9210e226a..5571a00e5 100644 --- a/crates/fbuild-deploy/src/esp32/tests.rs +++ b/crates/fbuild-deploy/src/esp32/tests.rs @@ -65,7 +65,7 @@ fn with_caller_path_stores_the_requesting_cli_path() { // The esptool program is a bare name, so the deploy-time spawn gets // the [("PATH", ...)] overlay from the stored caller PATH. - let esptool = Esp32Deployer::find_esptool(); + let esptool = deployer.find_esptool(); assert_eq!( fbuild_core::subprocess::bare_name_path_overlay( &esptool[0], @@ -75,6 +75,59 @@ fn with_caller_path_stores_the_requesting_cli_path() { ); } +/// FastLED/fbuild#1616: a resolved esptool path (the one the build +/// provisioned, or `FBUILD_ESPTOOL_PATH`) becomes argv[0] of both the +/// verify-flash and write-flash invocations, and an absolute path does not +/// get the caller-PATH overlay meant for bare names. +#[test] +fn with_esptool_path_is_argv0_for_verify_and_write() { + let tmp = tempfile::TempDir::new().unwrap(); + let fw = tmp.path().join("firmware.bin"); + std::fs::write(&fw, b"FIRM").unwrap(); + let esptool = tmp.path().join("tools").join("esptool"); + + let params = test_esptool_params(); + let deployer = Esp32Deployer::new( + "esp32c6", "460800", "0x0", "0x8000", "0x10000", ¶ms, false, + ) + .with_caller_path(Some("/opt/tools/bin".to_string())) + .with_esptool_path(Some(esptool.clone())); + + let expected = esptool.to_string_lossy().to_string(); + let verify = deployer.build_verify_flash_args(&fw, "COM13"); + assert_eq!(verify[0], expected); + assert!(verify.contains(&"verify-flash".to_string())); + let write = deployer.build_write_flash_args(&fw, "COM13", None); + assert_eq!(write[0], expected); + assert!(write.contains(&"write-flash".to_string())); + + assert_eq!( + fbuild_core::subprocess::bare_name_path_overlay(&write[0], deployer.caller_path.as_deref()), + None + ); +} + +/// FastLED/fbuild#1616: with no resolved path the deployer keeps the bare +/// `esptool` PATH lookup (and so the #1234 caller-PATH overlay). +#[test] +fn without_esptool_path_falls_back_to_bare_esptool() { + let tmp = tempfile::TempDir::new().unwrap(); + let fw = tmp.path().join("firmware.bin"); + std::fs::write(&fw, b"FIRM").unwrap(); + + let params = test_esptool_params(); + let deployer = Esp32Deployer::new( + "esp32c6", "460800", "0x0", "0x8000", "0x10000", ¶ms, false, + ) + .with_esptool_path(None); + + assert_eq!(deployer.build_verify_flash_args(&fw, "COM13")[0], "esptool"); + assert_eq!( + deployer.build_write_flash_args(&fw, "COM13", None)[0], + "esptool" + ); +} + #[test] fn qemu_flash_size_resolution_accepts_supported_sizes() { let mut board = fbuild_test_support::board_for_test("esp32-s3-devkitc-1"); From ec6ad67e3065322114ea8b20923a125dacd0d9ab Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Fri, 2 Oct 2026 11:59:08 -0700 Subject: [PATCH 2/3] fix(deploy): carry the resolved esptool as NormalizedPath (dylint) --- crates/fbuild-daemon/src/handlers/operations/deploy.rs | 4 ++-- crates/fbuild-deploy/src/esp32/deployer.rs | 8 +++++--- crates/fbuild-deploy/src/esp32/tests.rs | 4 +++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/operations/deploy.rs b/crates/fbuild-daemon/src/handlers/operations/deploy.rs index 183f56bfd..a31462a91 100644 --- a/crates/fbuild-daemon/src/handlers/operations/deploy.rs +++ b/crates/fbuild-daemon/src/handlers/operations/deploy.rs @@ -1451,11 +1451,11 @@ fn deployer_caller_path(req: &DeployRequest) -> Option { async fn resolve_deploy_esptool( project_dir: &std::path::Path, env_config: &std::collections::HashMap, -) -> Option { +) -> Option { match fbuild_build::esp32::orchestrator::resolve_deploy_esptool(project_dir, Some(env_config)) .await { - Ok(path) => path.map(fbuild_core::path::NormalizedPath::into_path_buf), + Ok(path) => path, Err(e) => { tracing::warn!("could not resolve the build's esptool ({e}); using `esptool` on PATH"); None diff --git a/crates/fbuild-deploy/src/esp32/deployer.rs b/crates/fbuild-deploy/src/esp32/deployer.rs index 0d0be447e..082ce1818 100644 --- a/crates/fbuild-deploy/src/esp32/deployer.rs +++ b/crates/fbuild-deploy/src/esp32/deployer.rs @@ -1,7 +1,9 @@ //! `Esp32Deployer` core: construction, args, verify and write paths, and //! the [`Deployer`] trait implementation. -use std::path::{Path, PathBuf}; +use std::path::Path; + +use fbuild_core::path::NormalizedPath; use fbuild_core::Result; use fbuild_core::subprocess::run_command; @@ -74,7 +76,7 @@ pub struct Esp32Deployer { /// The esptool the build resolved (`FBUILD_ESPTOOL_PATH` or the /// provisioned `tool-esptoolpy` package). `None` falls back to a bare /// `esptool` PATH lookup (FastLED/fbuild#1616). - pub(super) esptool_path: Option, + pub(super) esptool_path: Option, } #[cfg(feature = "espflash-native")] @@ -249,7 +251,7 @@ impl Esp32Deployer { /// Use the esptool the build resolved instead of a bare `esptool` /// PATH lookup (FastLED/fbuild#1616). `None` keeps the PATH lookup. #[must_use] - pub fn with_esptool_path(mut self, esptool_path: Option) -> Self { + pub fn with_esptool_path(mut self, esptool_path: Option) -> Self { self.esptool_path = esptool_path; self } diff --git a/crates/fbuild-deploy/src/esp32/tests.rs b/crates/fbuild-deploy/src/esp32/tests.rs index 5571a00e5..4fd2b2277 100644 --- a/crates/fbuild-deploy/src/esp32/tests.rs +++ b/crates/fbuild-deploy/src/esp32/tests.rs @@ -91,7 +91,9 @@ fn with_esptool_path_is_argv0_for_verify_and_write() { "esp32c6", "460800", "0x0", "0x8000", "0x10000", ¶ms, false, ) .with_caller_path(Some("/opt/tools/bin".to_string())) - .with_esptool_path(Some(esptool.clone())); + .with_esptool_path(Some(fbuild_core::path::NormalizedPath::from( + esptool.clone(), + ))); let expected = esptool.to_string_lossy().to_string(); let verify = deployer.build_verify_flash_args(&fw, "COM13"); From d18aaaabdaec26f7c548e745211bbec5a11f3c17 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Fri, 2 Oct 2026 12:26:09 -0700 Subject: [PATCH 3/3] fix(deploy): trust an explicit FBUILD_ESPTOOL_PATH regardless of file name Review: v5 still ships esptool.py entry points, so the v4 file-name heuristic must not reject an explicit override. Apply it only to the package fbuild provisioned. --- .../src/esp32/orchestrator/packages.rs | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs b/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs index 8e82286db..5e1519125 100644 --- a/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs +++ b/crates/fbuild-build-esp/src/esp32/orchestrator/packages.rs @@ -139,17 +139,18 @@ pub async fn resolve_deploy_esptool( project_dir: &Path, env_config: Option<&HashMap>, ) -> Result> { - let resolved = match fbuild_packages::library::esptool_path_override()? { - Some(path) => Some(path), - None => { - let Some(platform) = pioarduino_platform(project_dir, env_config, true).await? else { - return Ok(None); - }; - fbuild_packages::Package::ensure_installed(&platform).await?; - resolve_esptool(&platform, project_dir).await? - } + // An explicit override is the user's choice and is used as-is, whatever + // its file name (v5 still ships `esptool.py` entry points). + if let Some(path) = fbuild_packages::library::esptool_path_override()? { + return Ok(Some(path)); + } + let Some(platform) = pioarduino_platform(project_dir, env_config, true).await? else { + return Ok(None); }; - Ok(resolved.filter(|path| { + fbuild_packages::Package::ensure_installed(&platform).await?; + // The file-name check only applies to the package fbuild provisioned, + // where `esptool.py` means the v4 source package. + Ok(resolve_esptool(&platform, project_dir).await?.filter(|path| { let usable = speaks_v5_cli(path); if !usable { tracing::info!(