From ed0478feafba5bd7d486bfc4984df78f92a0c587 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Fri, 2 Oct 2026 01:50:35 -0700 Subject: [PATCH 1/2] refactor(esp8266): apply boards.txt menu props as pure config -> config rules `apply_esp8266_board_props(&props, &mut config)` rewrote defines and linker libs in place through four scattered blocks. Replace it with `for_board_props(config, &props) -> config`, composed from the named rules `with_sdk_define`, `with_define` (folded over `board_define_flags`), and `with_first_lib_replaced`, applied in one line where the MCU config loads. Behaviour is unchanged; adds tests for the previously untested path. Co-Authored-By: Claude --- .../src/esp8266/orchestrator.rs | 192 +++++++++++++----- 1 file changed, 136 insertions(+), 56 deletions(-) diff --git a/crates/fbuild-build-esp/src/esp8266/orchestrator.rs b/crates/fbuild-build-esp/src/esp8266/orchestrator.rs index b1de6e0a..0c780e6f 100644 --- a/crates/fbuild-build-esp/src/esp8266/orchestrator.rs +++ b/crates/fbuild-build-esp/src/esp8266/orchestrator.rs @@ -27,7 +27,8 @@ use crate::{BuildOrchestrator, BuildParams, BuildResult, SourceScanner}; use super::esp8266_compiler::Esp8266Compiler; use super::esp8266_linker::Esp8266Linker; -use super::mcu_config::get_esp8266_config; +use super::mcu_config::{Esp8266McuConfig, get_esp8266_config}; +use crate::esp32::mcu_config::DefineEntry; /// ESP8266 platform build orchestrator. pub struct Esp8266Orchestrator; @@ -254,8 +255,7 @@ impl BuildOrchestrator for Esp8266Orchestrator { let variant_dir = framework.get_variant_dir(&ctx.board.variant); // 5. Load MCU config - let mut mcu_config = get_esp8266_config()?; - apply_esp8266_board_props(&board_props, &mut mcu_config); + let mcu_config = for_board_props(get_esp8266_config()?, &board_props); // Compute flash_freq early for the fast-path fingerprint (also used by // the linker constructor below). @@ -591,66 +591,84 @@ fn apply_define_flags_from_props( } } -fn apply_esp8266_board_props( +/// The recipe adapted to the board's `boards.txt` menu selections: the SDK +/// define, `-D` overrides from the flag properties, and the lwIP and libstdc++ +/// variants. +fn for_board_props( + config: Esp8266McuConfig, board_props: &Option>, - mcu_config: &mut super::mcu_config::Esp8266McuConfig, -) { +) -> Esp8266McuConfig { let Some(props) = board_props.as_ref() else { - return; + return config; }; - - if let Some(sdk_name) = props.get("sdk") { - mcu_config - .defines - .retain(|entry| !matches!(entry, crate::esp32::mcu_config::DefineEntry::KeyValue(name, _) if name.starts_with("NONOSDK"))); - mcu_config - .defines - .push(crate::esp32::mcu_config::DefineEntry::KeyValue( - sdk_name.clone(), - "1".to_string(), - )); - } - - for key in ["flash_flags", "lwip_flags", "mmuflags", "vtable_flags"] { - if let Some(flags) = props.get(key) { - for token in fbuild_core::shell_split::split(flags) { - if let Some(def) = token.strip_prefix("-D") { - let (name, value) = def - .split_once('=') - .map(|(name, value)| (name.to_string(), value.to_string())) - .unwrap_or_else(|| (def.to_string(), "1".to_string())); - mcu_config.defines.retain(|entry| match entry { - crate::esp32::mcu_config::DefineEntry::Simple(existing) => { - existing != &name - } - crate::esp32::mcu_config::DefineEntry::KeyValue(existing, _) => { - existing != &name - } - }); - mcu_config - .defines - .push(crate::esp32::mcu_config::DefineEntry::KeyValue(name, value)); - } - } + let config = match props.get("sdk") { + Some(sdk_name) => with_sdk_define(config, sdk_name), + None => config, + }; + let config = board_define_flags(props) + .into_iter() + .fold(config, |config, (name, value)| { + with_define(config, name, value) + }); + let config = match props.get("lwip_lib") { + Some(lib) => with_first_lib_replaced(config, |l| l.starts_with("-llwip"), lib), + None => config, + }; + match props.get("stdcpp_lib") { + Some(lib) => { + with_first_lib_replaced(config, |l| l == "-lstdc++" || l == "-lstdc++-exc", lib) } + None => config, } +} - if let Some(lwip_lib) = props.get("lwip_lib") { - for lib in &mut mcu_config.linker_libs { - if lib.starts_with("-llwip") { - *lib = lwip_lib.clone(); - break; - } - } - } - if let Some(stdcpp_lib) = props.get("stdcpp_lib") { - for lib in &mut mcu_config.linker_libs { - if lib == "-lstdc++" || lib == "-lstdc++-exc" { - *lib = stdcpp_lib.clone(); - break; - } - } +/// Replace the recipe's `NONOSDK*` key/value define with the board's SDK. +fn with_sdk_define(mut config: Esp8266McuConfig, sdk_name: &str) -> Esp8266McuConfig { + config.defines.retain( + |entry| !matches!(entry, DefineEntry::KeyValue(name, _) if name.starts_with("NONOSDK")), + ); + config + .defines + .push(DefineEntry::KeyValue(sdk_name.to_string(), "1".to_string())); + config +} + +/// The `-D` defines in the board's flag properties, in application order. +fn board_define_flags(props: &HashMap) -> Vec<(String, String)> { + ["flash_flags", "lwip_flags", "mmuflags", "vtable_flags"] + .into_iter() + .filter_map(|key| props.get(key)) + .flat_map(|flags| fbuild_core::shell_split::split(flags)) + .filter_map(|token| { + let def = token.strip_prefix("-D")?; + Some( + def.split_once('=') + .map(|(name, value)| (name.to_string(), value.to_string())) + .unwrap_or_else(|| (def.to_string(), "1".to_string())), + ) + }) + .collect() +} + +/// Set `name` to `value`, replacing any existing define of that name. +fn with_define(mut config: Esp8266McuConfig, name: String, value: String) -> Esp8266McuConfig { + config.defines.retain(|entry| match entry { + DefineEntry::Simple(existing) | DefineEntry::KeyValue(existing, _) => existing != &name, + }); + config.defines.push(DefineEntry::KeyValue(name, value)); + config +} + +/// Replace the first linker lib that `matches` with `lib`. +fn with_first_lib_replaced( + mut config: Esp8266McuConfig, + matches: impl Fn(&str) -> bool, + lib: &str, +) -> Esp8266McuConfig { + if let Some(slot) = config.linker_libs.iter_mut().find(|l| matches(l)) { + *slot = lib.to_string(); } + config } fn apply_esp8266_board_identity( @@ -707,6 +725,68 @@ mod tests { use super::*; use fbuild_packages::Package as _; + fn define_names(config: &Esp8266McuConfig) -> Vec<(String, Option)> { + config + .defines + .iter() + .map(|entry| match entry { + DefineEntry::Simple(name) => (name.clone(), None), + DefineEntry::KeyValue(name, value) => (name.clone(), Some(value.clone())), + }) + .collect() + } + + #[test] + fn board_props_none_keeps_recipe() { + let base = get_esp8266_config().unwrap(); + let config = for_board_props(base.clone(), &None); + assert_eq!(define_names(&config), define_names(&base)); + assert_eq!(config.linker_libs, base.linker_libs); + } + + #[test] + fn board_props_rewrite_sdk_defines_and_libs() { + let base = get_esp8266_config().unwrap(); + let props = HashMap::from([ + ("sdk".to_string(), "NONOSDK3V0".to_string()), + ( + "mmuflags".to_string(), + "-DMMU_IRAM_SIZE=0xC000 -DMMU_ICACHE_SIZE=0x4000 -DFOO".to_string(), + ), + ( + "vtable_flags".to_string(), + "-DMMU_IRAM_SIZE=0x8000".to_string(), + ), + ("lwip_lib".to_string(), "-llwip2-1460-feat".to_string()), + ("stdcpp_lib".to_string(), "-lstdc++-exc".to_string()), + ]); + let config = for_board_props(base.clone(), &Some(props)); + let defines = define_names(&config); + + assert!(!defines.iter().any(|(name, _)| name == "NONOSDK22x_190703")); + assert!(defines.contains(&("NONOSDK3V0".into(), Some("1".into())))); + // Later properties override earlier ones; each name appears once. + let iram: Vec<_> = defines + .iter() + .filter(|(n, _)| n == "MMU_IRAM_SIZE") + .collect(); + assert_eq!( + iram, + [&("MMU_IRAM_SIZE".to_string(), Some("0x8000".to_string()))] + ); + assert!(defines.contains(&("FOO".into(), Some("1".into())))); + + assert!( + config + .linker_libs + .contains(&"-llwip2-1460-feat".to_string()) + ); + assert!(!config.linker_libs.contains(&"-llwip2-536-feat".to_string())); + assert!(config.linker_libs.contains(&"-lstdc++-exc".to_string())); + assert!(!config.linker_libs.contains(&"-lstdc++".to_string())); + assert_eq!(config.linker_libs.len(), base.linker_libs.len()); + } + #[test] fn explicit_toolchain_registry_pin_does_not_use_fixed_github_toolchain() { let temp = tempfile::tempdir().unwrap(); From 71c909f10f6d3bd557459cfa11aff9702d465c83 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Fri, 2 Oct 2026 01:57:43 -0700 Subject: [PATCH 2/2] refactor(esp8266): move board-prop rules into their own module Keeps orchestrator.rs under the 1000-line gate. Co-Authored-By: Claude --- .../src/esp8266/board_props.rs | 156 ++++++++++++++++++ crates/fbuild-build-esp/src/esp8266/mod.rs | 1 + .../src/esp8266/orchestrator.rs | 146 +--------------- 3 files changed, 159 insertions(+), 144 deletions(-) create mode 100644 crates/fbuild-build-esp/src/esp8266/board_props.rs diff --git a/crates/fbuild-build-esp/src/esp8266/board_props.rs b/crates/fbuild-build-esp/src/esp8266/board_props.rs new file mode 100644 index 00000000..a0a5ef2b --- /dev/null +++ b/crates/fbuild-build-esp/src/esp8266/board_props.rs @@ -0,0 +1,156 @@ +//! Pure rules that adapt the ESP8266 recipe to a board's `boards.txt` menu +//! selections. Each rule is a side-effect-free `config -> config` function; +//! the orchestrator applies [`for_board_props`] once, in one line. + +use std::collections::HashMap; + +use super::mcu_config::Esp8266McuConfig; +use crate::esp32::mcu_config::DefineEntry; + +/// The recipe adapted to the board's `boards.txt` menu selections: the SDK +/// define, `-D` overrides from the flag properties, and the lwIP and libstdc++ +/// variants. +pub(super) fn for_board_props( + config: Esp8266McuConfig, + board_props: &Option>, +) -> Esp8266McuConfig { + let Some(props) = board_props.as_ref() else { + return config; + }; + let config = match props.get("sdk") { + Some(sdk_name) => with_sdk_define(config, sdk_name), + None => config, + }; + let config = board_define_flags(props) + .into_iter() + .fold(config, |config, (name, value)| { + with_define(config, name, value) + }); + let config = match props.get("lwip_lib") { + Some(lib) => with_first_lib_replaced(config, |l| l.starts_with("-llwip"), lib), + None => config, + }; + match props.get("stdcpp_lib") { + Some(lib) => { + with_first_lib_replaced(config, |l| l == "-lstdc++" || l == "-lstdc++-exc", lib) + } + None => config, + } +} + +/// Replace the recipe's `NONOSDK*` key/value define with the board's SDK. +fn with_sdk_define(mut config: Esp8266McuConfig, sdk_name: &str) -> Esp8266McuConfig { + config.defines.retain( + |entry| !matches!(entry, DefineEntry::KeyValue(name, _) if name.starts_with("NONOSDK")), + ); + config + .defines + .push(DefineEntry::KeyValue(sdk_name.to_string(), "1".to_string())); + config +} + +/// The `-D` defines in the board's flag properties, in application order. +fn board_define_flags(props: &HashMap) -> Vec<(String, String)> { + ["flash_flags", "lwip_flags", "mmuflags", "vtable_flags"] + .into_iter() + .filter_map(|key| props.get(key)) + .flat_map(|flags| fbuild_core::shell_split::split(flags)) + .filter_map(|token| { + let def = token.strip_prefix("-D")?; + Some( + def.split_once('=') + .map(|(name, value)| (name.to_string(), value.to_string())) + .unwrap_or_else(|| (def.to_string(), "1".to_string())), + ) + }) + .collect() +} + +/// Set `name` to `value`, replacing any existing define of that name. +fn with_define(mut config: Esp8266McuConfig, name: String, value: String) -> Esp8266McuConfig { + config.defines.retain(|entry| match entry { + DefineEntry::Simple(existing) | DefineEntry::KeyValue(existing, _) => existing != &name, + }); + config.defines.push(DefineEntry::KeyValue(name, value)); + config +} + +/// Replace the first linker lib that `matches` with `lib`. +fn with_first_lib_replaced( + mut config: Esp8266McuConfig, + matches: impl Fn(&str) -> bool, + lib: &str, +) -> Esp8266McuConfig { + if let Some(slot) = config.linker_libs.iter_mut().find(|l| matches(l)) { + *slot = lib.to_string(); + } + config +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::esp8266::mcu_config::get_esp8266_config; + + fn define_names(config: &Esp8266McuConfig) -> Vec<(String, Option)> { + config + .defines + .iter() + .map(|entry| match entry { + DefineEntry::Simple(name) => (name.clone(), None), + DefineEntry::KeyValue(name, value) => (name.clone(), Some(value.clone())), + }) + .collect() + } + + #[test] + fn board_props_none_keeps_recipe() { + let base = get_esp8266_config().unwrap(); + let config = for_board_props(base.clone(), &None); + assert_eq!(define_names(&config), define_names(&base)); + assert_eq!(config.linker_libs, base.linker_libs); + } + + #[test] + fn board_props_rewrite_sdk_defines_and_libs() { + let base = get_esp8266_config().unwrap(); + let props = HashMap::from([ + ("sdk".to_string(), "NONOSDK3V0".to_string()), + ( + "mmuflags".to_string(), + "-DMMU_IRAM_SIZE=0xC000 -DMMU_ICACHE_SIZE=0x4000 -DFOO".to_string(), + ), + ( + "vtable_flags".to_string(), + "-DMMU_IRAM_SIZE=0x8000".to_string(), + ), + ("lwip_lib".to_string(), "-llwip2-1460-feat".to_string()), + ("stdcpp_lib".to_string(), "-lstdc++-exc".to_string()), + ]); + let config = for_board_props(base.clone(), &Some(props)); + let defines = define_names(&config); + + assert!(!defines.iter().any(|(name, _)| name == "NONOSDK22x_190703")); + assert!(defines.contains(&("NONOSDK3V0".into(), Some("1".into())))); + // Later properties override earlier ones; each name appears once. + let iram: Vec<_> = defines + .iter() + .filter(|(n, _)| n == "MMU_IRAM_SIZE") + .collect(); + assert_eq!( + iram, + [&("MMU_IRAM_SIZE".to_string(), Some("0x8000".to_string()))] + ); + assert!(defines.contains(&("FOO".into(), Some("1".into())))); + + assert!( + config + .linker_libs + .contains(&"-llwip2-1460-feat".to_string()) + ); + assert!(!config.linker_libs.contains(&"-llwip2-536-feat".to_string())); + assert!(config.linker_libs.contains(&"-lstdc++-exc".to_string())); + assert!(!config.linker_libs.contains(&"-lstdc++".to_string())); + assert_eq!(config.linker_libs.len(), base.linker_libs.len()); + } +} diff --git a/crates/fbuild-build-esp/src/esp8266/mod.rs b/crates/fbuild-build-esp/src/esp8266/mod.rs index 9de0167e..bcba9b2b 100644 --- a/crates/fbuild-build-esp/src/esp8266/mod.rs +++ b/crates/fbuild-build-esp/src/esp8266/mod.rs @@ -1,5 +1,6 @@ //! ESP8266 platform build support (NodeMCU, Wemos D1, etc.) +mod board_props; pub mod esp8266_compiler; pub mod esp8266_linker; pub mod mcu_config; diff --git a/crates/fbuild-build-esp/src/esp8266/orchestrator.rs b/crates/fbuild-build-esp/src/esp8266/orchestrator.rs index 0c780e6f..7192db5e 100644 --- a/crates/fbuild-build-esp/src/esp8266/orchestrator.rs +++ b/crates/fbuild-build-esp/src/esp8266/orchestrator.rs @@ -25,10 +25,10 @@ use crate::compile_database::TargetArchitecture; use crate::pipeline; use crate::{BuildOrchestrator, BuildParams, BuildResult, SourceScanner}; +use super::board_props::for_board_props; use super::esp8266_compiler::Esp8266Compiler; use super::esp8266_linker::Esp8266Linker; -use super::mcu_config::{Esp8266McuConfig, get_esp8266_config}; -use crate::esp32::mcu_config::DefineEntry; +use super::mcu_config::get_esp8266_config; /// ESP8266 platform build orchestrator. pub struct Esp8266Orchestrator; @@ -591,86 +591,6 @@ fn apply_define_flags_from_props( } } -/// The recipe adapted to the board's `boards.txt` menu selections: the SDK -/// define, `-D` overrides from the flag properties, and the lwIP and libstdc++ -/// variants. -fn for_board_props( - config: Esp8266McuConfig, - board_props: &Option>, -) -> Esp8266McuConfig { - let Some(props) = board_props.as_ref() else { - return config; - }; - let config = match props.get("sdk") { - Some(sdk_name) => with_sdk_define(config, sdk_name), - None => config, - }; - let config = board_define_flags(props) - .into_iter() - .fold(config, |config, (name, value)| { - with_define(config, name, value) - }); - let config = match props.get("lwip_lib") { - Some(lib) => with_first_lib_replaced(config, |l| l.starts_with("-llwip"), lib), - None => config, - }; - match props.get("stdcpp_lib") { - Some(lib) => { - with_first_lib_replaced(config, |l| l == "-lstdc++" || l == "-lstdc++-exc", lib) - } - None => config, - } -} - -/// Replace the recipe's `NONOSDK*` key/value define with the board's SDK. -fn with_sdk_define(mut config: Esp8266McuConfig, sdk_name: &str) -> Esp8266McuConfig { - config.defines.retain( - |entry| !matches!(entry, DefineEntry::KeyValue(name, _) if name.starts_with("NONOSDK")), - ); - config - .defines - .push(DefineEntry::KeyValue(sdk_name.to_string(), "1".to_string())); - config -} - -/// The `-D` defines in the board's flag properties, in application order. -fn board_define_flags(props: &HashMap) -> Vec<(String, String)> { - ["flash_flags", "lwip_flags", "mmuflags", "vtable_flags"] - .into_iter() - .filter_map(|key| props.get(key)) - .flat_map(|flags| fbuild_core::shell_split::split(flags)) - .filter_map(|token| { - let def = token.strip_prefix("-D")?; - Some( - def.split_once('=') - .map(|(name, value)| (name.to_string(), value.to_string())) - .unwrap_or_else(|| (def.to_string(), "1".to_string())), - ) - }) - .collect() -} - -/// Set `name` to `value`, replacing any existing define of that name. -fn with_define(mut config: Esp8266McuConfig, name: String, value: String) -> Esp8266McuConfig { - config.defines.retain(|entry| match entry { - DefineEntry::Simple(existing) | DefineEntry::KeyValue(existing, _) => existing != &name, - }); - config.defines.push(DefineEntry::KeyValue(name, value)); - config -} - -/// Replace the first linker lib that `matches` with `lib`. -fn with_first_lib_replaced( - mut config: Esp8266McuConfig, - matches: impl Fn(&str) -> bool, - lib: &str, -) -> Esp8266McuConfig { - if let Some(slot) = config.linker_libs.iter_mut().find(|l| matches(l)) { - *slot = lib.to_string(); - } - config -} - fn apply_esp8266_board_identity( board_props: &Option>, board_id: &str, @@ -725,68 +645,6 @@ mod tests { use super::*; use fbuild_packages::Package as _; - fn define_names(config: &Esp8266McuConfig) -> Vec<(String, Option)> { - config - .defines - .iter() - .map(|entry| match entry { - DefineEntry::Simple(name) => (name.clone(), None), - DefineEntry::KeyValue(name, value) => (name.clone(), Some(value.clone())), - }) - .collect() - } - - #[test] - fn board_props_none_keeps_recipe() { - let base = get_esp8266_config().unwrap(); - let config = for_board_props(base.clone(), &None); - assert_eq!(define_names(&config), define_names(&base)); - assert_eq!(config.linker_libs, base.linker_libs); - } - - #[test] - fn board_props_rewrite_sdk_defines_and_libs() { - let base = get_esp8266_config().unwrap(); - let props = HashMap::from([ - ("sdk".to_string(), "NONOSDK3V0".to_string()), - ( - "mmuflags".to_string(), - "-DMMU_IRAM_SIZE=0xC000 -DMMU_ICACHE_SIZE=0x4000 -DFOO".to_string(), - ), - ( - "vtable_flags".to_string(), - "-DMMU_IRAM_SIZE=0x8000".to_string(), - ), - ("lwip_lib".to_string(), "-llwip2-1460-feat".to_string()), - ("stdcpp_lib".to_string(), "-lstdc++-exc".to_string()), - ]); - let config = for_board_props(base.clone(), &Some(props)); - let defines = define_names(&config); - - assert!(!defines.iter().any(|(name, _)| name == "NONOSDK22x_190703")); - assert!(defines.contains(&("NONOSDK3V0".into(), Some("1".into())))); - // Later properties override earlier ones; each name appears once. - let iram: Vec<_> = defines - .iter() - .filter(|(n, _)| n == "MMU_IRAM_SIZE") - .collect(); - assert_eq!( - iram, - [&("MMU_IRAM_SIZE".to_string(), Some("0x8000".to_string()))] - ); - assert!(defines.contains(&("FOO".into(), Some("1".into())))); - - assert!( - config - .linker_libs - .contains(&"-llwip2-1460-feat".to_string()) - ); - assert!(!config.linker_libs.contains(&"-llwip2-536-feat".to_string())); - assert!(config.linker_libs.contains(&"-lstdc++-exc".to_string())); - assert!(!config.linker_libs.contains(&"-lstdc++".to_string())); - assert_eq!(config.linker_libs.len(), base.linker_libs.len()); - } - #[test] fn explicit_toolchain_registry_pin_does_not_use_fixed_github_toolchain() { let temp = tempfile::tempdir().unwrap();