From 6b4b5fc32a46dc75d255eddc14ce99f724bb31aa Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Mon, 28 Sep 2026 12:16:38 -0700 Subject: [PATCH] fix(esp32): pass PlatformIO's SDK defines on 2.x-layout frameworks arduino-esp32 2.x SDKs ship no flags/defines, so get_sdk_defines returned nothing and every compile lacked ESP_PLATFORM, IDF_VER, MBEDTLS_CONFIG_FILE, _GNU_SOURCE and friends -- FastLED failed to build (taskENTER_CRITICAL takes its argument only #ifdef ESP_PLATFORM). Fall back to the CPPDEFINES block of the framework's platformio-build-.py, as include dirs already do, keeping only literal SDK-level entries. The ESP32 fast-path fingerprint now includes the SDK defines; before, a change to them replayed the previous firmware until a source changed. Fixes #1549. --- .../src/esp32/orchestrator/build.rs | 4 +- .../src/esp32/orchestrator/fingerprint.rs | 15 ++++ .../src/library/esp32_framework/parsing.rs | 74 ++++++++++++++++++ .../src/library/esp32_framework/sdk_paths.rs | 16 +++- .../src/library/esp32_framework/tests.rs | 78 ++++++++++++++++++- 5 files changed, 179 insertions(+), 8 deletions(-) diff --git a/crates/fbuild-build-esp/src/esp32/orchestrator/build.rs b/crates/fbuild-build-esp/src/esp32/orchestrator/build.rs index d0ea14814..fb4be41da 100644 --- a/crates/fbuild-build-esp/src/esp32/orchestrator/build.rs +++ b/crates/fbuild-build-esp/src/esp32/orchestrator/build.rs @@ -124,9 +124,6 @@ impl BuildOrchestrator for Esp32Orchestrator { mcu_config.disable_lto(); } - let mut user_flags = sdk_defines; - let user_build_flags = ctx.config.get_build_flags(¶ms.env_name)?; - user_flags.extend(user_build_flags.clone()); let embed_files = ctx.config.get_embed_files(¶ms.env_name)?; let embed_txtfiles = ctx.config.get_embed_txtfiles(¶ms.env_name)?; @@ -163,6 +160,7 @@ impl BuildOrchestrator for Esp32Orchestrator { crate::eh_frame_policy::EhFramePolicy::Strip => "strip", crate::eh_frame_policy::EhFramePolicy::Preserve => "preserve", }, + sdk_defines, }, &ctx, )?; diff --git a/crates/fbuild-build-esp/src/esp32/orchestrator/fingerprint.rs b/crates/fbuild-build-esp/src/esp32/orchestrator/fingerprint.rs index 89825155c..be1010500 100644 --- a/crates/fbuild-build-esp/src/esp32/orchestrator/fingerprint.rs +++ b/crates/fbuild-build-esp/src/esp32/orchestrator/fingerprint.rs @@ -30,6 +30,9 @@ pub(super) struct Esp32FingerprintMetadata { pub max_flash: Option, pub max_ram: Option, pub eh_frame_policy: &'static str, + /// SDK `-D` flags reach every TU but come from the framework, not the + /// project config, so a change there must not replay a stale fast path. + pub sdk_defines: Vec, } #[cfg(test)] @@ -64,6 +67,7 @@ mod tests { eh_frame_policy: "preserve", toolchain_name: toolchain_name.into(), toolchain_version: toolchain_version.into(), + sdk_defines: vec!["-DESP_PLATFORM".into()], } } @@ -80,4 +84,15 @@ mod tests { assert_ne!(original, different_name); assert_ne!(original, different_version); } + + #[test] + fn sdk_defines_change_fast_path_metadata() { + let original = metadata("toolchain-xtensa-esp32s3", "8.4.0+2021r2-patch5"); + let mut without = metadata("toolchain-xtensa-esp32s3", "8.4.0+2021r2-patch5"); + without.sdk_defines.clear(); + assert_ne!( + serde_json::to_vec(&original).unwrap(), + serde_json::to_vec(&without).unwrap() + ); + } } diff --git a/crates/fbuild-library/src/library/esp32_framework/parsing.rs b/crates/fbuild-library/src/library/esp32_framework/parsing.rs index 81c025157..5caea8a2a 100644 --- a/crates/fbuild-library/src/library/esp32_framework/parsing.rs +++ b/crates/fbuild-library/src/library/esp32_framework/parsing.rs @@ -107,3 +107,77 @@ pub(crate) fn extract_framework_version(url: &str) -> String { // Fallback: hash crate::cache::hash_url(url) } + +/// SDK `-D` flags from the `CPPDEFINES` block of an arduino-esp32 2.x +/// `tools/platformio-build-.py`, in the raw form `flags/defines` uses. +/// +/// Those SDK layouts ship no `flags/defines`, so this block is the only place +/// PlatformIO's SDK defines (`ESP_PLATFORM`, `IDF_VER`, `MBEDTLS_CONFIG_FILE`, +/// ...) are written down (FastLED/fbuild#1549). Entries computed from the +/// SCons environment are skipped, as are the Arduino/board-level names +/// (`ARDUINO*`, `ESP32`, `F_CPU`) that fbuild derives from the board itself. +pub(crate) fn parse_pio_cppdefines(script: &str) -> Vec { + let mut defines = Vec::new(); + let mut in_block = false; + for line in script.lines() { + let entry = line.trim(); + if !in_block { + in_block = entry.starts_with("CPPDEFINES=[") || entry.starts_with("CPPDEFINES = ["); + continue; + } + if entry.starts_with(']') { + break; + } + let entry = entry.trim_end_matches(',').trim(); + if entry.contains('%') || entry.contains('$') || entry.contains("env.") { + continue; + } + let (name, value) = match entry.strip_prefix('(').and_then(|e| e.strip_suffix(')')) { + Some(tuple) => { + let Some((name, value)) = tuple.split_once(',') else { + continue; + }; + let Some(value) = python_literal(value.trim()) else { + continue; + }; + (name.trim(), Some(value)) + } + None => (entry, None), + }; + let Some(name) = python_literal(name).filter(|n| is_sdk_define_name(n)) else { + continue; + }; + defines.push(match value { + Some(value) => format!("-D{name}={value}"), + None => format!("-D{name}"), + }); + } + defines +} + +fn is_sdk_define_name(name: &str) -> bool { + !name.is_empty() + && name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') + && !name.starts_with("ARDUINO") + && !matches!(name, "ESP32" | "F_CPU") +} + +/// A Python string literal's contents (`\\` -> `\`, `\'` -> `'`, `\"` -> `"`), +/// or an integer literal as written. +fn python_literal(token: &str) -> Option { + if !token.is_empty() && token.chars().all(|c| c.is_ascii_digit()) { + return Some(token.to_string()); + } + let quote = token.chars().next().filter(|c| matches!(c, '"' | '\''))?; + let body = token.strip_prefix(quote)?.strip_suffix(quote)?; + let mut out = String::with_capacity(body.len()); + let mut chars = body.chars(); + while let Some(c) = chars.next() { + if c == '\\' { + out.push(chars.next()?); + } else { + out.push(c); + } + } + Some(out) +} diff --git a/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs b/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs index 8461f464a..08537f5d8 100644 --- a/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs +++ b/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs @@ -7,7 +7,7 @@ use std::path::{Path, PathBuf}; use super::Esp32Framework; use super::fs_utils::{collect_archive_files, scan_include_dirs_recursive}; -use super::parsing::{parse_include_flags, split_defines}; +use super::parsing::{parse_include_flags, parse_pio_cppdefines, split_defines}; /// A parsed builder script yielding fewer entries than this is treated as a /// failed parse. An upstream format change must degrade to the tree scan, @@ -326,8 +326,10 @@ impl Esp32Framework { /// Get the SDK compiler defines from `flags/defines`. /// /// Returns `-D` flags that must be passed to the compiler for SDK headers - /// to work correctly (e.g., `MBEDTLS_CONFIG_FILE`, `IDF_VER`). - /// Returns empty if the flags file doesn't exist. + /// to work correctly (e.g., `MBEDTLS_CONFIG_FILE`, `IDF_VER`). SDK layouts + /// without `flags/defines` (arduino-esp32 2.x) fall back to the + /// `CPPDEFINES` block of the framework's PlatformIO builder script; empty + /// when neither exists. /// /// Uses `split_defines` instead of `shell_split` because define values /// like `-DMBEDTLS_CONFIG_FILE=\"mbedtls/esp_config.h\"` contain escaped @@ -337,7 +339,13 @@ impl Esp32Framework { if let Ok(content) = std::fs::read_to_string(&defines_file) { return split_defines(&content); } - Vec::new() + let script = self + .resolved_dir() + .join("tools") + .join(format!("platformio-build-{mcu}.py")); + std::fs::read_to_string(script) + .map(|content| parse_pio_cppdefines(&content)) + .unwrap_or_default() } /// Get the ordered SDK linker flags from `flags/ld_flags`. diff --git a/crates/fbuild-library/src/library/esp32_framework/tests.rs b/crates/fbuild-library/src/library/esp32_framework/tests.rs index f7d372875..4bccd68ab 100644 --- a/crates/fbuild-library/src/library/esp32_framework/tests.rs +++ b/crates/fbuild-library/src/library/esp32_framework/tests.rs @@ -2,7 +2,7 @@ use std::path::Path; use super::Esp32Framework; use super::fs_utils::{collect_archive_files, find_framework_root}; -use super::parsing::{parse_include_flags, split_defines}; +use super::parsing::{parse_include_flags, parse_pio_cppdefines, split_defines}; use crate::{CacheSubdir, Package, PackageBase}; #[test] @@ -580,3 +580,79 @@ fn old_sdk_rejects_builder_script_padded_with_duplicate_entries() { "duplicate-padded script should have been rejected in favour of the tree scan" ); } + +/// The `CPPDEFINES` block of arduino-esp32 2.x `platformio-build-esp32s3.py`. +const PIO_BUILD_SCRIPT: &str = r#" +env.Append( + CPPDEFINES=[ + "HAVE_CONFIG_H", + ("MBEDTLS_CONFIG_FILE", '\\"mbedtls/esp_config.h\\"'), + "UNITY_INCLUDE_CONFIG_H", + "WITH_POSIX", + "_GNU_SOURCE", + ("IDF_VER", '\\"v4.4.7-dirty\\"'), + "ESP_PLATFORM", + "_POSIX_READER_WRITER_LOCKS", + "ARDUINO_ARCH_ESP32", + "ESP32", + ("F_CPU", "$BOARD_F_CPU"), + ("ARDUINO", 10812), + ("ARDUINO_VARIANT", '\\"%s\\"' % env.BoardConfig().get("build.variant").replace('"', "")), + "ARDUINO_PARTITION_%s" % basename(env.BoardConfig().get( + "build.partitions", "default.csv")).replace(".csv", "").replace("-", "_") + ] +) +"#; + +#[test] +fn pio_cppdefines_yield_the_sdk_defines_in_flags_defines_form() { + assert_eq!( + parse_pio_cppdefines(PIO_BUILD_SCRIPT), + [ + "-DHAVE_CONFIG_H", + r#"-DMBEDTLS_CONFIG_FILE=\"mbedtls/esp_config.h\""#, + "-DUNITY_INCLUDE_CONFIG_H", + "-DWITH_POSIX", + "-D_GNU_SOURCE", + r#"-DIDF_VER=\"v4.4.7-dirty\""#, + "-DESP_PLATFORM", + "-D_POSIX_READER_WRITER_LOCKS", + ] + ); +} + +#[test] +fn sdk_defines_fall_back_to_the_pio_build_script_without_flags_defines() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("tools/sdk/esp32s3")).unwrap(); + std::fs::write( + root.join("tools/platformio-build-esp32s3.py"), + PIO_BUILD_SCRIPT, + ) + .unwrap(); + let mut fw = Esp32Framework::new(root, "esp32s3"); + fw.install_dir = Some(root.to_path_buf()); + + let defines = fw.get_sdk_defines("esp32s3"); + + assert!(defines.contains(&"-DESP_PLATFORM".to_string())); + assert_eq!(defines.len(), 8); +} + +#[test] +fn sdk_defines_prefer_flags_defines_over_the_pio_build_script() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("tools/sdk/esp32s3/flags")).unwrap(); + std::fs::write(root.join("tools/sdk/esp32s3/flags/defines"), "-DFROM_FLAGS").unwrap(); + std::fs::write( + root.join("tools/platformio-build-esp32s3.py"), + PIO_BUILD_SCRIPT, + ) + .unwrap(); + let mut fw = Esp32Framework::new(root, "esp32s3"); + fw.install_dir = Some(root.to_path_buf()); + + assert_eq!(fw.get_sdk_defines("esp32s3"), ["-DFROM_FLAGS"]); +}