From 0d75f597b3375dfcc866c4ae999f2f5d3702ec19 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 12:32:44 -0500 Subject: [PATCH 01/10] Harden the shared library converter Split out of #18662: the converter/manifest hardening without the download parallelization, which stays there. The PIO extra-script shim moves from espidf/ to platformio/ and gains type checks for every manifest field, capture of the Append variants and CPPPATH, warn-once diagnostics for unmodelled env reads, and uniform shlex quoting so captured values survive the lex_build_flags round-trip. The dependency walk normalizes every spelling PIO accepts, splits the routine cross-platform skip (IncompatiblePlatform) from real manifest problems, validates manifest shape once, and the shared flag lexer warns and drops trailing or empty glued arguments. --- esphome/espidf/component.py | 69 +-- esphome/espidf/extra_script.py | 161 ------ esphome/platformio/extra_script.py | 299 +++++++++++ esphome/platformio/library.py | 301 ++++++++--- esphome/platformio/toolchain.py | 3 + script/determine-jobs.py | 1 + tests/script/test_determine_jobs.py | 1 + tests/unit_tests/test_espidf_component.py | 173 ++----- .../test_platformio_extra_script.py | 478 ++++++++++++++++++ tests/unit_tests/test_platformio_library.py | 212 +++++++- tests/unit_tests/test_platformio_toolchain.py | 7 + 11 files changed, 1294 insertions(+), 411 deletions(-) delete mode 100644 esphome/espidf/extra_script.py create mode 100644 esphome/platformio/extra_script.py create mode 100644 tests/unit_tests/test_platformio_extra_script.py diff --git a/esphome/espidf/component.py b/esphome/espidf/component.py index aa6f10c261..4eeaa30e7f 100644 --- a/esphome/espidf/component.py +++ b/esphome/espidf/component.py @@ -27,6 +27,7 @@ from esphome.platformio.library import ( collect_filtered_files, convert_libraries, ensure_list, + lex_build_flags, split_list_by_condition, ) @@ -40,37 +41,6 @@ def _idf_framework() -> str: return "arduino" if CORE.using_arduino else "espidf" -def _apply_extra_script(component: IDFComponent) -> None: - """Run a PIO ``extraScript`` and fold its captured env vars into - ``component.data["build"]["flags"]`` so the existing -L/-l/-D - extraction in ``generate_cmakelists_txt`` picks them up.""" - extra_script = component.data.get("build", {}).get("extraScript") - if not extra_script: - return - # Resolve and confine to the library's source dir so a malicious - # library.json can't escape (e.g. ``"extraScript": "../../etc/passwd"``). - source_path = component.source_dir - library_root = source_path.resolve() - script_path = (source_path / extra_script).resolve() - if not script_path.is_relative_to(library_root) or not script_path.is_file(): - return - from esphome.components.esp32 import get_esp32_variant - from esphome.espidf.extra_script import captured_as_build_flags, run_extra_script - - idf_target = variant_to_idf_target(get_esp32_variant()) - result = run_extra_script( - script_path, library_dir=source_path, idf_target=idf_target - ) - extra_flags = captured_as_build_flags(result, library_dir=source_path) - if not extra_flags: - return - flags = component.data.setdefault("build", {}).setdefault("flags", []) - if isinstance(flags, str): - flags = [flags] - flags.extend(extra_flags) - component.data["build"]["flags"] = flags - - def generate_cmakelists_txt(component: IDFComponent) -> str: """ Generate a CMakeLists.txt file for an ESP-IDF component. @@ -85,10 +55,6 @@ def generate_cmakelists_txt(component: IDFComponent) -> str: Returns: str: The complete CMakeLists.txt content as a string """ - # Late import: this module loads with the esp32 platform on every - # validate/compile, but shlex is only needed when generating component - # CMakeLists. - import shlex def escape_entry(p: PathType) -> str: # In CMakeLists.txt, backslashes need to be escaped @@ -122,26 +88,12 @@ def generate_cmakelists_txt(component: IDFComponent) -> str: build_src_filter = ensure_list( component.data.get("build", {}).get("srcFilter", DEFAULT_BUILD_SRC_FILTER) ) - build_flags = ensure_list( - component.data.get("build", {}).get("flags", DEFAULT_BUILD_FLAGS) + # PlatformIO shell-lexes each build.flags entry; bare -I/-L/-l/-D tokens + # re-glue to their argument so the prefix classifiers below route them. + build_flags = lex_build_flags( + component.data.get("build", {}).get("flags", DEFAULT_BUILD_FLAGS), + f"library {component.name}", ) - # PlatformIO shell-lexes each build.flags entry, so one entry can carry a - # flag and its argument (e.g. "-include cp_custom_alloc.h"). Split the - # same way; emitting such an entry as a single quoted compile option - # hands the compiler one argv with an embedded space. - build_flags = [token for entry in build_flags for token in shlex.split(entry)] - # Re-glue bare -I/-L/-l tokens to their argument ("-I foo" -> "-Ifoo") so - # the prefix classifiers below still route them to INCLUDE_DIRS and the - # link handling. - tokens, build_flags = build_flags, [] - i = 0 - while i < len(tokens): - if tokens[i] in ("-I", "-L", "-l") and i + 1 < len(tokens): - build_flags.append(tokens[i] + tokens[i + 1]) - i += 2 - else: - build_flags.append(tokens[i]) - i += 1 # List all sources files build_src_files = collect_filtered_files( @@ -299,7 +251,14 @@ def generate_idf_component_yml(component: IDFComponent) -> str: def _emit_idf_component(component: IDFComponent) -> None: """Write the ESP-IDF build files for a resolved library into its cache dir.""" - _apply_extra_script(component) + from esphome.components.esp32 import get_esp32_variant + from esphome.platformio.extra_script import apply_extra_script + + apply_extra_script( + component, + board_mcu=lambda: variant_to_idf_target(get_esp32_variant()), + pio_platform="espressif32", + ) write_file_if_changed( component.path / "CMakeLists.txt", generate_cmakelists_txt(component), diff --git a/esphome/espidf/extra_script.py b/esphome/espidf/extra_script.py deleted file mode 100644 index 487fef7cc1..0000000000 --- a/esphome/espidf/extra_script.py +++ /dev/null @@ -1,161 +0,0 @@ -"""Run a PlatformIO ``extraScript`` against a captured SCons-env stand-in. - -PlatformIO libraries occasionally configure per-target link/build state -via a Python ``extraScript`` declared in ``library.json``'s ``build`` -section instead of static fields. The script runs under SCons during -PIO's build and mutates the active ``Environment`` (``env.Append``, -``env.Replace``, …) — chiefly to set ``LIBPATH``/``LIBS`` per chip MCU. - -ESPHome's PIO→IDF converter doesn't run SCons, so these scripts were -previously ignored and any library -relying on them failed to link under ``toolchain: esp-idf``. This -module provides a small shim that ``exec``s an extra-script with a -fake ``env`` object, captures the common ``env.Append(...)`` calls, -and returns the captured vars so the caller can fold them back into -the library's generated CMakeLists. - -Caveats -------- -* Only the ``env.Append`` API is captured. ``env.Replace``, - ``env.Prepend``, ``env.AddPreAction``, SCons file generators, and any - arbitrary I/O are silently no-ops. Scripts that depend on those will - produce incomplete output. -* Running arbitrary Python from third-party libraries is a non-trivial - trust decision. The shim does no sandboxing — anything in the - script's process can run. Use only with libraries whose source you - trust. -""" - -from __future__ import annotations - -from dataclasses import dataclass, field -import logging -import os -from pathlib import Path - -_LOGGER = logging.getLogger(__name__) - -# Keys we know how to translate back into ESPHome's build-flag pipeline. -# Other env.Append kwargs are recorded but ignored downstream. -_CAPTURED_KEYS = frozenset({"LIBPATH", "LIBS", "CPPDEFINES", "LINKFLAGS", "CPPFLAGS"}) - - -@dataclass -class ExtraScriptResult: - """Build-var deltas captured from a PIO extra-script ``env.Append`` call.""" - - libpath: list[str] = field(default_factory=list) - libs: list[str] = field(default_factory=list) - cppdefines: list[str | tuple[str, str]] = field(default_factory=list) - linkflags: list[str] = field(default_factory=list) - cppflags: list[str] = field(default_factory=list) - - -class _FakeSConsEnv: - """Minimal stand-in for SCons ``Environment`` exposed to extra-scripts. - - Implements just enough surface area to let scripts query ``BOARD_MCU`` - / ``PIOENV`` and call ``env.Append(LIBPATH=…, LIBS=…, …)``. Every - other env method swallows silently so unrelated calls don't raise - ``AttributeError`` and abort the script. - """ - - def __init__(self, *, board_mcu: str, pio_env: str) -> None: - self._vars: dict[str, str] = { - "BOARD_MCU": board_mcu, - "PIOPLATFORM": "espressif32", - "PIOENV": pio_env, - } - self.result = ExtraScriptResult() - - # ----- SCons env API the common scripts use ----- - - def get(self, key: str, default: str | None = None) -> str | None: - return self._vars.get(key, default) - - def Append(self, **kwargs) -> None: # noqa: N802 (SCons API name) - for key, value in kwargs.items(): - if key not in _CAPTURED_KEYS: - continue - items = list(value) if isinstance(value, (list, tuple)) else [value] - bucket = getattr(self.result, key.lower()) - bucket.extend(items) - - # ----- Everything else is a no-op so unsupported scripts don't crash ----- - - def __getattr__(self, name: str): - def _noop(*args, **kwargs): - return None - - return _noop - - -def run_extra_script( - script_path: Path, *, library_dir: Path, idf_target: str -) -> ExtraScriptResult: - """Execute ``script_path`` with a fake SCons env and return captured vars. - - ``idf_target`` is the active ESP-IDF target name (e.g. ``esp32``, - ``esp32s3``); it's exposed to the script as PlatformIO's - ``BOARD_MCU`` so chip-conditional logic resolves the same way it - would under PIO. The script runs with ``library_dir`` as the - process CWD so relative-path lookups (``join``, ``realpath``, - ``open``) resolve against the library tree. - - On any exception inside the script we log at debug level and return - an empty result — extra-scripts are best-effort, and an unsupported - script shouldn't block the build. - """ - env = _FakeSConsEnv(board_mcu=idf_target, pio_env=f"esphome_{idf_target}") - code = compile(script_path.read_text(encoding="utf-8"), str(script_path), "exec") - old_cwd = Path.cwd() - try: - os.chdir(library_dir) - exec( # noqa: S102 pylint: disable=exec-used - code, - { - "Import": lambda *_args: None, # SCons-side import; harmless here - "env": env, - "__file__": str(script_path), - "__name__": "__pio_extra_script__", - }, - ) - except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught - _LOGGER.warning("PIO extra-script %s raised %s; skipping", script_path, e) - return ExtraScriptResult() - finally: - os.chdir(old_cwd) - return env.result - - -def captured_as_build_flags( - result: ExtraScriptResult, *, library_dir: Path -) -> list[str]: - """Translate captured env vars into the ``-L`` / ``-l`` / ``-D`` / - raw-flag form ``_generate_cmakelists_txt`` already knows how to consume. - - ``LIBPATH`` entries are made relative to ``library_dir`` so the - generated CMakeLists is portable; absolute paths outside the library - tree are kept as-is (CMake handles absolute paths in - ``target_link_directories`` fine). - """ - flags: list[str] = [] - library_root = library_dir.resolve() - for path in result.libpath: - # Anchor relative paths to library_dir (not the current CWD, which - # has been restored by the time we get here). Joining an absolute - # path against library_dir returns the absolute path unchanged. - resolved = (library_dir / path).resolve() - try: - flags.append(f"-L{resolved.relative_to(library_root)}") - except ValueError: - flags.append(f"-L{resolved}") - flags.extend(f"-l{lib}" for lib in result.libs) - for define in result.cppdefines: - if isinstance(define, tuple) and len(define) == 2: - flags.append(f"-D{define[0]}={define[1]}") - else: - flags.append(f"-D{define}") - flags.extend(result.linkflags) - flags.extend(result.cppflags) - return flags diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py new file mode 100644 index 0000000000..363c40561f --- /dev/null +++ b/esphome/platformio/extra_script.py @@ -0,0 +1,299 @@ +"""Run a PlatformIO library ``extraScript`` against a fake SCons env. + +The shim execs the script with a stand-in ``env``, captures ``env.Append`` +calls (everything else is a logged no-op), and folds the result into the +library's build flags. No sandboxing: the script runs with full process +access, so it carries the same trust as the library's own source. +""" + +from __future__ import annotations + +from collections.abc import Callable +from dataclasses import dataclass, field +import logging +import os +from pathlib import Path +import shlex +from typing import TYPE_CHECKING + +from esphome.core import EsphomeError + +if TYPE_CHECKING: + from esphome.platformio.library import ConvertedLibrary + +_LOGGER = logging.getLogger(__name__) + + +def apply_extra_script( + component: ConvertedLibrary, + board_mcu: Callable[[], str], + pio_platform: str, +) -> None: + """Run a library's ``extraScript`` and fold its captured env vars into + ``component.data["build"]["flags"]``. + + ``board_mcu`` is a callable so its lookup runs only when a script will. + """ + extra_script = component.data.get("build", {}).get("extraScript") + if not extra_script: + return + if not isinstance(extra_script, str): + # A list/dict value would raise an opaque TypeError on the join below + raise EsphomeError( + f"extraScript of library {component.name} must be a string, " + f"got {type(extra_script).__name__}" + ) + # Resolve and confine to the library's source dir so a malicious + # library.json can't escape (e.g. ``"extraScript": "../../etc/passwd"``). + source_path = component.source_dir + library_root = source_path.resolve() + script_path = (source_path / extra_script).resolve() + if not script_path.is_relative_to(library_root): + # More hostile than a missing script; must not be quieter than it + raise EsphomeError( + f"extraScript {extra_script} of library {component.name} escapes " + "the library directory" + ) + if not script_path.is_file(): + # A declared-but-absent script is a broken or half-downloaded + # package, not an unsupported script; PlatformIO fails on it too + raise EsphomeError( + f"extraScript {extra_script} of library {component.name} not found" + ) + result = run_extra_script( + script_path, + library_dir=source_path, + board_mcu=board_mcu(), + pio_platform=pio_platform, + ) + extra_flags = captured_as_build_flags(result, library_dir=source_path) + if not extra_flags: + return + flags = component.data.setdefault("build", {}).setdefault("flags", []) + if isinstance(flags, str): + flags = [flags] + elif not isinstance(flags, list): + # A null/dict value coerced through a list wrapper would inject a + # non-string into the compiler command line; fail naming the library + raise EsphomeError( + f"Library {component.name} has a malformed build.flags " + f"({type(flags).__name__}); expected a string or list" + ) + component.data["build"]["flags"] = [*flags, *extra_flags] + + +# Keys we know how to translate back into ESPHome's build-flag pipeline. +# Other env.Append kwargs are recorded but ignored downstream. +_CAPTURED_KEYS = frozenset( + {"CPPPATH", "LIBPATH", "LIBS", "CPPDEFINES", "LINKFLAGS", "CPPFLAGS"} +) + + +@dataclass +class ExtraScriptResult: + """Build-var deltas captured from a PIO extra-script ``env.Append`` call.""" + + cpppath: list[str] = field(default_factory=list) + libpath: list[str] = field(default_factory=list) + libs: list[str] = field(default_factory=list) + cppdefines: list[str | tuple[str, str]] = field(default_factory=list) + linkflags: list[str] = field(default_factory=list) + cppflags: list[str] = field(default_factory=list) + + +class _FakeSConsEnv: + """Minimal SCons ``Environment`` stand-in: ``get`` and ``Append`` work; + every other method is a swallowed no-op so scripts don't abort.""" + + def __init__(self, *, board_mcu: str, pio_env: str, pio_platform: str) -> None: + self._vars: dict[str, str] = { + "BOARD_MCU": board_mcu, + "PIOPLATFORM": pio_platform, + "PIOENV": pio_env, + } + self.result = ExtraScriptResult() + self._warned_methods: set[str] = set() + self._warned_keys: set[str] = set() + self._warned_gets: set[str] = set() + + # ----- SCons env API the common scripts use ----- + + def get(self, key: str, default: str | None = None) -> str | None: + if key not in self._vars and key not in self._warned_gets: + # A script branching on an unmodelled var silently takes the + # default branch; make that diagnosable from a normal build log + self._warned_gets.add(key) + _LOGGER.warning( + "PIO extra-script env.get(%r) is not modelled; returning the default", + key, + ) + return self._vars.get(key, default) + + def __getitem__(self, key: str) -> str: + # Scripts also read env["BOARD_MCU"]; without this the broad + # handler would discard every flag the script captured + return self._vars[key] + + def Append(self, **kwargs) -> None: # noqa: N802 (SCons API name) + for key, value in kwargs.items(): + if key not in _CAPTURED_KEYS: + # Warn once per key so a loop of Appends cannot spam + if key not in self._warned_keys: + self._warned_keys.add(key) + _LOGGER.warning( + "PIO extra-script env.Append(%s=...) is not captured; ignoring", + key, + ) + continue + items = list(value) if isinstance(value, (list, tuple)) else [value] + bucket = getattr(self.result, key.lower()) + bucket.extend(items) + + # Same keys, same flattened capture; ordering/dedup don't matter since + # the consumer re-orders anyway + Prepend = Append + AppendUnique = Append + PrependUnique = Append + + # ----- Everything else is a no-op so unsupported scripts don't crash ----- + + def __getattr__(self, name: str): + def _noop(*args, **kwargs): + # Once per method: a script whose whole effect is env.Replace() + # must be diagnosable from a normal build log + if name not in self._warned_methods: + self._warned_methods.add(name) + _LOGGER.warning( + "PIO extra-script env.%s(...) is not supported; ignoring", name + ) + + return _noop + + +def run_extra_script( + script_path: Path, + *, + library_dir: Path, + board_mcu: str, + pio_platform: str, +) -> ExtraScriptResult: + """Execute ``script_path`` with a fake SCons env and return captured vars. + + Runs with ``library_dir`` as CWD so relative lookups resolve against + the library tree. A crashed script warns and returns an empty result, + never a partial capture. + """ + env = _FakeSConsEnv( + board_mcu=board_mcu, + pio_env=f"esphome_{board_mcu}", + pio_platform=pio_platform, + ) + try: + source = script_path.read_text(encoding="utf-8") + except OSError as err: + # An unreadable declared script is a broken package, exactly like a + # missing one; must not be quieter than that case + raise EsphomeError(f"extraScript {script_path} is unreadable: {err}") from err + except UnicodeDecodeError as e: + # A content problem, best-effort like a SyntaxError below + _LOGGER.warning( + "PIO extra-script %s (in %s) is not UTF-8 (%r); ignoring its output", + script_path, + library_dir.name, + e, + ) + return ExtraScriptResult() + old_cwd = Path.cwd() + try: + # Inside the try: a SyntaxError in a vendored script is just as + # best-effort as a runtime failure + code = compile(source, str(script_path), "exec") + os.chdir(library_dir) + exec( # noqa: S102 pylint: disable=exec-used + code, + { + "Import": lambda *_args: None, # SCons-side import; harmless here + "env": env, + "__file__": str(script_path), + "__name__": "__pio_extra_script__", + }, + ) + except SystemExit as e: + if not e.code: + # sys.exit() / sys.exit(0) is a normal PlatformIO script ending; + # the capture is complete + return env.result + _LOGGER.warning( + "PIO extra-script %s (in %s) exited with status %r; ignoring its output", + script_path, + library_dir.name, + e.code, + ) + return ExtraScriptResult() + except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught + # Discard any partial capture: half-applied flags could build wrong + # firmware that links cleanly. + _LOGGER.warning( + "PIO extra-script %s (in %s) raised %r; ignoring its output", + script_path, + library_dir.name, + e, + ) + return ExtraScriptResult() + finally: + os.chdir(old_cwd) + return env.result + + +def captured_as_build_flags( + result: ExtraScriptResult, *, library_dir: Path +) -> list[str]: + """Translate captured env vars into -L/-l/-D/raw build flags. + + ``LIBPATH`` entries are made relative to ``library_dir`` so the + generated build files stay portable. + """ + flags: list[str] = [] + + def _strs(bucket: list, kind: str) -> list[str]: + # Third-party scripts legally append SCons nodes, ints, or dicts; + # stringifying those into flags would hand the compiler garbage + good = [entry for entry in bucket if isinstance(entry, str)] + for entry in bucket: + if not isinstance(entry, str): + _LOGGER.warning("Ignoring unsupported %s entry %r", kind, entry) + return good + + library_root = library_dir.resolve() + + def _anchored(path: str) -> str: + # Anchor relative paths to library_dir; the script's CWD has been + # restored by now + resolved = (library_dir / path).resolve() + try: + return str(resolved.relative_to(library_root)) + except ValueError: + return str(resolved) + + # shlex.quote so a spaced path survives lex_build_flags as one token + flags.extend( + f"-I{shlex.quote(_anchored(path))}" for path in _strs(result.cpppath, "CPPPATH") + ) + flags.extend( + f"-L{shlex.quote(_anchored(path))}" for path in _strs(result.libpath, "LIBPATH") + ) + flags.extend(f"-l{shlex.quote(lib)}" for lib in _strs(result.libs, "LIBS")) + for define in result.cppdefines: + # SCons also accepts dict/list CPPDEFINES; formatting those blind + # would hand the compiler garbage like -D{'FOO': '1'} + if isinstance(define, (tuple, list)) and len(define) == 2: + flags.append(f"-D{define[0]}={define[1]}") + elif isinstance(define, str): + flags.append(f"-D{define}") + else: + _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", define) + # Each captured entry is one argv token in SCons; quote so the + # lex_build_flags round-trip cannot split a spaced value into two + flags.extend(shlex.quote(f) for f in _strs(result.linkflags, "LINKFLAGS")) + flags.extend(shlex.quote(f) for f in _strs(result.cppflags, "CPPFLAGS")) + return flags diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index ee0a758a31..af3b05c9a0 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -13,7 +13,7 @@ regardless of which toolchain consumes the result. """ from collections import deque -from collections.abc import Callable +from collections.abc import Callable, Iterable from dataclasses import dataclass, field import glob import hashlib @@ -47,20 +47,29 @@ DEFAULT_BUILD_SRC_FILTER = ( DEFAULT_BUILD_SRC_DIRS = "src" DEFAULT_BUILD_INCLUDE_DIR = "include" DEFAULT_BUILD_FLAGS = [] -SRC_FILE_EXTENSIONS = [ - ".c", - ".cpp", - ".cc", - ".cxx", - ".c++", - ".S", - ".spp", - ".SPP", - ".sx", - ".s", - ".asm", - ".ASM", -] +# Suffix -> compiler kind (PlatformIO's CSUFFIXES/CXXSUFFIXES/ASSUFFIXES). +# "asm" merges SCons's AS and ASPP sets: all compile as assembler-with-cpp. +# The kind values drive the ESP8266 native ninja rules (later in this +# chain); existing backends consume only the keys. Note .C/.C++ join the +# suffix set here per CXXSUFFIXES; SCons demotes .C to C on +# case-insensitive filesystems, we always treat it as C++. +SOURCE_KIND_FOR_SUFFIX: dict[str, str] = { + ".c": "c", + ".cpp": "cxx", + ".cc": "cxx", + ".cxx": "cxx", + ".c++": "cxx", + ".C": "cxx", + ".C++": "cxx", + ".S": "asm", + ".spp": "asm", + ".SPP": "asm", + ".sx": "asm", + ".s": "asm", + ".asm": "asm", + ".ASM": "asm", +} +SRC_FILE_EXTENSIONS = list(SOURCE_KIND_FOR_SUFFIX) DOMAIN = "pio_components" @@ -203,6 +212,14 @@ class InvalidLibrary(Exception): pass +class IncompatiblePlatform(InvalidLibrary): + """The manifest's platform filter rejected the target platform. + + A distinct type so callers can treat the routine cross-platform skip + differently from other manifest problems without matching message text. + """ + + class ConvertedLibrary: """A resolved PlatformIO library plus its parsed manifest and on-disk path. @@ -432,7 +449,7 @@ def check_library_data(data: dict, platform: str | None, framework: str): valid_platforms = platform is None or "*" in platforms or platform in platforms if not valid_platforms: - raise InvalidLibrary(f"Unsupported library platforms: {platforms}") + raise IncompatiblePlatform(f"Unsupported library platforms: {platforms}") frameworks = data.get("frameworks", "*") if isinstance(frameworks, str): @@ -455,7 +472,7 @@ def check_library_data(data: dict, platform: str | None, framework: str): ) -def _parse_library_json(library_json_path: PathType): +def parse_library_json(library_json_path: PathType): """ Load and parse a JSON file describing a library. @@ -469,7 +486,7 @@ def _parse_library_json(library_json_path: PathType): return json.load(fp) -def _parse_library_properties(library_properties_path: PathType): +def parse_library_properties(library_properties_path: PathType): """ Parse a key-value platformio .properties style file into a dictionary. @@ -553,19 +570,139 @@ def _resolve_registry_version( return owner, name, best["name"], pkgfile["download_url"] -def _normalize_dependencies(dependencies: Any) -> list[dict]: +def split_flag_entry(entry: Any, owner: str) -> list[str]: + """``shlex.split`` with a clean error naming the offending flags entry.""" + # Late import: shlex is only needed when actually lexing flags + import shlex + + try: + return shlex.split(entry) + except (ValueError, AttributeError, TypeError) as err: + # AttributeError/TypeError: a dict or number from a third-party + # manifest; name the entry instead of an opaque shlex traceback + raise EsphomeError(f"Malformed build flag {entry!r} in {owner}: {err}") from err + + +def lex_build_flags(entries: str | list[str], owner: str) -> list[str]: + """Shell-lex ``build.flags`` entries the way PlatformIO's ParseFlags + does; bare -I/-L/-l/-D tokens re-glue to their argument.""" + # Join per entry, as SCons's ParseFlags lexes each string independently: + # a dangling -I ending one entry must warn, not absorb the next entry's + # first token. + return [ + token + for entry in ensure_list(entries) + for token in join_flag_args(split_flag_entry(entry, owner), owner) + ] + + +# Flags whose argument may follow as a separate token; ParseFlags glues them +BARE_ARG_FLAGS = frozenset({"-I", "-L", "-l", "-D"}) + + +def join_flag_args(tokens: Iterable[str], owner: str) -> list[str]: + """Join a bare ``-I``/``-L``/``-l``/``-D`` with its following token, + the way PlatformIO's ParseFlags lexes them. + + A trailing or empty argument (``-D ""``) is warned and dropped: the + bare flag would make gcc eat the next flag as its argument (or add + the CWD for ``-L``); always a typo. + """ + out: list[str] = [] + it = iter(tokens) + for tok in it: + if tok in BARE_ARG_FLAGS: + arg = next(it, None) + if arg is None: + _LOGGER.warning("Ignoring trailing '%s' in %s build flags", tok, owner) + break + if not arg: + _LOGGER.warning( + "Ignoring '%s' with empty argument in %s build flags", tok, owner + ) + continue + tok += arg + out.append(tok) + return out + + +def warn_properties_depends(name: str, data: object) -> None: + """Warn when a manifest declares dependencies only as ``depends=``. + + The dependency walk reads the JSON ``dependencies`` key; the raw + ``library.properties`` spelling would otherwise drop silently. + """ + if isinstance(data, dict) and not data.get("dependencies") and data.get("depends"): + # INFO: common and unactionable for transitive libraries; a WARNING + # on every build would train users to ignore the stream + _LOGGER.info( + "Library %s declares dependencies via library.properties " + "depends=, which are not resolved automatically; add them with " + "add_library() if needed", + name, + ) + + +def dependency_is_usable( + dep: dict, platform: str | None, framework: str, requester: str +) -> bool: + """Compatibility filter for a manifest dependency: platform mismatches + skip at debug, any other ``InvalidLibrary`` warns naming the requester.""" + try: + check_library_data(dep, platform, framework) + except IncompatiblePlatform as e: + _LOGGER.debug("Skip dependency %s of %s: %s", dep.get("name"), requester, e) + return False + except InvalidLibrary as e: + _LOGGER.warning( + "Skipping dependency %s of %s: %s", dep.get("name"), requester, e + ) + return False + return True + + +def _valid_dependency_entry(entry: dict, manifest_name: str) -> bool: + """Whether a normalized entry carries a usable name and version. + + The name must be a non-empty string (every consumer indexes or joins + it); a present version must be a string (a container would raise from + ``set.add()``, an int fails opaquely inside the registry resolution). + Invalid entries warn naming the manifest. + """ + name = entry.get("name") + if ( + isinstance(name, str) + and name + and ("version" not in entry or isinstance(entry["version"], str)) + ): + return True + _LOGGER.warning( + "Ignoring unrecognized dependency entry %r of %s", entry, manifest_name + ) + return False + + +def normalize_dependencies( + dependencies: Any, manifest_name: str = "manifest" +) -> list[dict]: """Normalize a library manifest's ``dependencies`` to a list of dicts. - PIO's library.json accepts both the list-of-dicts form and the shorthand - dict form (``{"owner/Name": "version_spec"}``); normalize the latter so - callers see a uniform list. + PIO's library.json accepts the list-of-dicts form, the shorthand dict + form (``{"owner/Name": "version_spec"}``), bare name strings inside the + list, and a plain (possibly comma-separated) string; normalize them all + so callers see a uniform list. ``manifest_name`` names the manifest in the + warning for entries that cannot be normalized. """ if not dependencies: return [] + if isinstance(dependencies, str): + # A plain string is one or more comma-separated names; iterating it + # as a list would shred it into one-character "libraries" + return [{"name": n.strip()} for n in dependencies.split(",") if n.strip()] if isinstance(dependencies, dict): normalized = [] for raw_name, spec in dependencies.items(): - if "/" in raw_name: + if isinstance(raw_name, str) and "/" in raw_name: owner, pkgname = raw_name.split("/", 1) else: owner, pkgname = None, raw_name @@ -574,9 +711,31 @@ def _normalize_dependencies(dependencies: Any) -> list[dict]: entry.update(spec) else: entry["version"] = spec - normalized.append(entry) + if _valid_dependency_entry(entry, manifest_name): + normalized.append(entry) return normalized - return [d for d in dependencies if isinstance(d, dict)] + if not isinstance(dependencies, (list, tuple)): + _LOGGER.warning( + "Ignoring unrecognized dependencies %r of %s", + dependencies, + manifest_name, + ) + return [] + normalized = [] + for entry in dependencies: + if isinstance(entry, dict): + if _valid_dependency_entry(entry, manifest_name): + normalized.append(entry) + elif isinstance(entry, str) and entry: + # PIO also accepts a bare list of names ("dependencies": ["Wire"]) + normalized.append({"name": entry}) + else: + _LOGGER.warning( + "Ignoring unrecognized dependency entry %r of %s", + entry, + manifest_name, + ) + return normalized @dataclass @@ -688,6 +847,24 @@ def _node_key( return name, "registry", (owner, pkgname) +def lib_ignore_set() -> set[str]: + """The ``lib_ignore`` names from ``esphome->platformio_options``, + normalized to lowercase short names (the part after the ``/``).""" + return { + name.split("/")[-1].lower() + for name in CORE.platformio_options.get("lib_ignore", []) + } + + +def is_lib_ignored(name: str | None, lib_ignore: set[str]) -> bool: + """Whether ``name`` matches the normalized ``lib_ignore`` set.""" + return ( + bool(lib_ignore) + and name is not None + and (name.split("/")[-1].lower() in lib_ignore) + ) + + def convert_libraries( libraries: list[Library], backend: LibraryBackend ) -> list[ConvertedLibrary]: @@ -713,10 +890,7 @@ def convert_libraries( """ nodes: dict[str, _LibNode] = {} - lib_ignore = { - name.split("/")[-1].lower() - for name in CORE.platformio_options.get("lib_ignore", []) - } + lib_ignore = lib_ignore_set() # The generated build files inside the shared cache bake in the dependency # wiring, which lib_ignore changes; salt the cache path so configs with @@ -728,11 +902,6 @@ def convert_libraries( else "" ) - def is_ignored(name: str | None) -> bool: - if not lib_ignore or name is None: - return False - return name.split("/")[-1].lower() in lib_ignore - def add_spec(name: str | None, version: str | None, repository: str | None) -> str: key, kind, locator = _node_key(name, version, repository) node = nodes.get(key) or _LibNode(key=key, is_git=kind == "git") @@ -781,7 +950,7 @@ def convert_libraries( top_level = [ add_spec(library.name, library.version, library.repository) for library in libraries - if not is_ignored(library.name) + if not is_lib_ignored(library.name, lib_ignore) ] # Collect + resolve to a fixpoint: a node is (re)resolved whenever its @@ -795,10 +964,8 @@ def convert_libraries( key = worklist.popleft() node = nodes[key] - # A node is queued once per referring edge; skip the (uncached) registry - # lookup + download + dependency walk unless its requirement set grew - # since the last resolve. Requirements only ever grow, so this still - # converges the fixpoint and terminates dependency cycles. + # Re-resolve only when the requirement set grew; requirements + # only ever grow, so the fixpoint converges and cycles terminate requirements = frozenset(node.requirements) if resolved_requirements.get(key) == requirements: continue @@ -823,11 +990,8 @@ def convert_libraries( has_json = library_json_path.is_file() has_properties = library_properties_path.is_file() if not has_json and not has_properties and not node.is_local: - # The shared cache can hold a broken copy (e.g. a clone or an - # extraction interrupted by a killed process). Force one - # re-download so a bad cache entry self-heals instead of failing - # every build until the user runs a full clean. A local source is - # read in place, so there is nothing to re-download. + # An interrupted clone/extraction self-heals with one forced + # re-download; a local source has nothing to re-download _LOGGER.warning( "Library %s at %s is missing library.json and library.properties; " "re-downloading", @@ -838,49 +1002,66 @@ def convert_libraries( has_json = library_json_path.is_file() has_properties = library_properties_path.is_file() if has_json: - component.data = _parse_library_json(library_json_path) + component.data = parse_library_json(library_json_path) elif has_properties: - component.data = _parse_library_properties(library_properties_path) + component.data = parse_library_properties(library_properties_path) else: - # For a local library a missing manifest is user input, so raise - # EsphomeError (clean CLI message) like the missing-directory case; - # for registry/git a missing manifest means a corrupt cache, which - # is not user error, so keep RuntimeError. + # Local sources are user input (EsphomeError); a registry/git + # miss means a corrupt cache (RuntimeError) error_cls = EsphomeError if node.is_local else RuntimeError raise error_cls( f"Invalid PIO library {key}: missing library.json and " f"library.properties in {source_dir}" ) + if not isinstance(component.data, dict) or not isinstance( + component.data.get("build", {}), dict + ): + # A bare json.load imposes no shape; every backend dereferences + # data/build, so validate once here and name the library + raise EsphomeError(f"Library {key} has a malformed manifest") + warn_properties_depends(component.name, component.data) + try: check_library_data(component.data, backend.platform, backend.framework) except InvalidLibrary as e: - # Skip an incompatible transitive dependency, but fail fast if a - # top-level library the build explicitly requested is incompatible. + # An explicitly requested library fails fast; the routine + # cross-platform skip stays at debug, other causes warn if key in top_level_keys: raise RuntimeError( f"Requested library {key} is not compatible with " f"{backend.framework}: {e}" ) from e - _LOGGER.debug("Skip incompatible dependency %s: %s", key, str(e)) + if isinstance(e, IncompatiblePlatform): + _LOGGER.debug("Skip incompatible dependency %s: %s", key, str(e)) + else: + _LOGGER.warning("Skipping dependency %s: %s", key, str(e)) continue components[key] = component # Requirements changed (we got past the short-circuit above), so # (re)walk this component's dependencies. node.edges = set() - for dependency in _normalize_dependencies(component.data.get("dependencies")): - if "name" not in dependency or "version" not in dependency: + for dependency in normalize_dependencies( + component.data.get("dependencies"), component.name + ): + if "version" not in dependency: + # Cannot resolve from the registry; common for bundled + # names (Wire, SPI) -- add_library() is the fix if real + _LOGGER.info( + "Skip version-less dependency %r of %s", + dependency.get("name"), + component.name, + ) continue - try: - check_library_data(dependency, backend.platform, backend.framework) - except InvalidLibrary as e: - _LOGGER.debug("Skip dependency %s: %s", dependency.get("name"), str(e)) + if not dependency_is_usable( + dependency, backend.platform, backend.framework, component.name + ): continue dep_name = _owner_pkgname_to_name( dependency.get("owner"), dependency.get("name") ) - if is_ignored(dep_name): + if is_lib_ignored(dep_name, lib_ignore): _LOGGER.debug("Skip ignored dependency %s", dep_name) continue # The version field may actually be a URL (git/archive dependency). diff --git a/esphome/platformio/toolchain.py b/esphome/platformio/toolchain.py index d76581d032..a98ef3e9fe 100644 --- a/esphome/platformio/toolchain.py +++ b/esphome/platformio/toolchain.py @@ -254,6 +254,9 @@ def _ccache_runs(ccache: str) -> bool: stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=15, + # Repo-wide convention (posix_spawn fast path); see the + # close_fds=False call sites across esphome/ and script/helpers.py + close_fds=False, ) except (OSError, subprocess.SubprocessError): _LOGGER.warning( diff --git a/script/determine-jobs.py b/script/determine-jobs.py index 722e2370fe..3e11deeb9a 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -537,6 +537,7 @@ ESP_IDF_INFRA_TRIGGER_FILES = frozenset( "esphome/build_gen/espidf.py", "esphome/framework_helpers.py", "esphome/platformio/library.py", + "esphome/platformio/extra_script.py", } ) diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index 1a568ca6c6..b42c33de96 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -1124,6 +1124,7 @@ def test_should_run_esp32_platformio_with_branch() -> None: (["esphome/build_helpers/idedata.py"], True), (["esphome/platformio/library.py"], True), (["esphome/framework_helpers.py"], True), + (["esphome/platformio/extra_script.py"], True), # PlatformIO build gen, its toolchain, and the esp32 component are # NOT IDF-infra triggers (["esphome/platformio/toolchain.py"], False), diff --git a/tests/unit_tests/test_espidf_component.py b/tests/unit_tests/test_espidf_component.py index f9e048f6f4..e2884454e5 100644 --- a/tests/unit_tests/test_espidf_component.py +++ b/tests/unit_tests/test_espidf_component.py @@ -1,12 +1,12 @@ import glob import hashlib import json -import os from pathlib import Path from unittest.mock import MagicMock import pytest +from esphome.components import esp32 as esp32_module from esphome.const import ( KEY_CORE, KEY_TARGET_FRAMEWORK, @@ -16,6 +16,7 @@ from esphome.const import ( ) from esphome.core import CORE, Library from esphome.espidf.component import ( + _emit_idf_component, generate_cmakelists_txt, generate_idf_component_yml, generate_idf_components, @@ -26,11 +27,11 @@ from esphome.platformio.library import ( GitSource, URLSource, _node_key, - _normalize_dependencies, - _parse_library_json, - _parse_library_properties, _resolve_registry_version, collect_filtered_files, + normalize_dependencies, + parse_library_json, + parse_library_properties, split_list_by_condition, ) @@ -369,133 +370,11 @@ def test_generate_idf_component_yml_missing_path_raises(tmp_component): generate_idf_component_yml(tmp_component) -def test_extra_script_captures_libpath_libs_and_defines(tmp_path): - from esphome.espidf.extra_script import captured_as_build_flags, run_extra_script - - (tmp_path / "src" / "esp32").mkdir(parents=True) - script = tmp_path / "extra_script.py" - script.write_text( - "Import('env')\n" - "mcu = env.get('BOARD_MCU')\n" - "env.Append(\n" - " LIBPATH=[join('src', mcu)],\n" - " LIBS=['algobsec'],\n" - " CPPDEFINES=['FOO', ('BAR', '1')],\n" - " LINKFLAGS=['-Wl,--gc-sections'],\n" - ")\n" - ) - # The script uses bare ``join`` (PIO's extra-scripts run inside SCons - # where this is in scope). Inject it via the script header so the - # shim's exec namespace can resolve it. - script.write_text("from os.path import join\n" + script.read_text()) - - result = run_extra_script(script, library_dir=tmp_path, idf_target="esp32") - - assert result.libpath == [str(Path("src") / "esp32")] - assert result.libs == ["algobsec"] - assert ("BAR", "1") in result.cppdefines - assert "FOO" in result.cppdefines - assert result.linkflags == ["-Wl,--gc-sections"] - - flags = captured_as_build_flags(result, library_dir=tmp_path) - sep = os.sep - assert f"-Lsrc{sep}esp32" in flags - assert "-lalgobsec" in flags - assert "-DFOO" in flags - assert "-DBAR=1" in flags - assert "-Wl,--gc-sections" in flags - - -def test_extra_script_libpath_relative_resolves_against_library_dir( - tmp_path, monkeypatch -): - """Relative LIBPATH entries must resolve against ``library_dir``, not the - caller's CWD (the shim restores CWD before ``captured_as_build_flags`` - runs).""" - from esphome.espidf.extra_script import ExtraScriptResult, captured_as_build_flags - - (tmp_path / "lib" / "esp32").mkdir(parents=True) - elsewhere = tmp_path.parent / "not_the_library_dir" - elsewhere.mkdir(exist_ok=True) - monkeypatch.chdir(elsewhere) - - result = ExtraScriptResult(libpath=["lib/esp32"]) - flags = captured_as_build_flags(result, library_dir=tmp_path) - - sep = os.sep - assert flags == [f"-Llib{sep}esp32"] - - -def test_extra_script_libpath_absolute_outside_library_dir(tmp_path): - from esphome.espidf.extra_script import ExtraScriptResult, captured_as_build_flags - - outside = tmp_path.parent / "system_lib" - outside.mkdir(exist_ok=True) - result = ExtraScriptResult(libpath=[str(outside)]) - - flags = captured_as_build_flags(result, library_dir=tmp_path) - assert flags == [f"-L{outside.resolve()}"] - - -def test_extra_script_failure_returns_empty_result(tmp_path, caplog): - from esphome.espidf.extra_script import run_extra_script - - script = tmp_path / "broken.py" - script.write_text("raise RuntimeError('boom')\n") - - with caplog.at_level("WARNING"): - result = run_extra_script(script, library_dir=tmp_path, idf_target="esp32") - - assert result.libpath == [] - assert result.libs == [] - assert "broken.py" in caplog.text - - -def test_apply_extra_script_path_traversal_is_rejected(tmp_path): - from esphome.espidf.component import _apply_extra_script - - library_dir = tmp_path / "lib" - library_dir.mkdir() - outside = tmp_path / "evil.py" - outside.write_text("env.Append(LIBS=['pwned'])\n") - - c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) - c.path = library_dir - c.data = {"build": {"extraScript": "../evil.py"}} - - _apply_extra_script(c) - - # Nothing was folded into flags: the traversal was rejected before - # the script could run. - assert "flags" not in c.data["build"] - - -def test_apply_extra_script_merges_into_existing_flags(tmp_path, monkeypatch): - from esphome.components import esp32 as esp32_module - - monkeypatch.setattr(esp32_module, "get_esp32_variant", lambda: "ESP32") - - from esphome.espidf.component import _apply_extra_script - - (tmp_path / "src").mkdir() - script = tmp_path / "extra.py" - script.write_text("env.Append(LIBS=['algobsec'])\n") - - c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) - c.path = tmp_path - c.data = {"build": {"extraScript": "extra.py", "flags": ["-DEXISTING"]}} - - _apply_extra_script(c) - - assert "-DEXISTING" in c.data["build"]["flags"] - assert "-lalgobsec" in c.data["build"]["flags"] - - def test_parse_library_json(tmp_path): f = tmp_path / "library.json" f.write_text(json.dumps({"name": "test"})) - result = _parse_library_json(f) + result = parse_library_json(f) assert result["name"] == "test" @@ -510,7 +389,7 @@ empty= """ ) - result = _parse_library_properties(f) + result = parse_library_properties(f) assert result["name"] == "Test" assert result["version"] == "1.0" @@ -680,22 +559,22 @@ def test_node_key_registry_bare_name(): def test_normalize_dependencies_none(): - assert _normalize_dependencies(None) == [] + assert normalize_dependencies(None) == [] def test_normalize_dependencies_list_form(): deps = [{"name": "foo", "version": "1.0"}] - assert _normalize_dependencies(deps) == [{"name": "foo", "version": "1.0"}] + assert normalize_dependencies(deps) == [{"name": "foo", "version": "1.0"}] def test_normalize_dependencies_dict_form(): - out = _normalize_dependencies({"nanopb/Nanopb": "^0.4.91", "BareName": "1.2.3"}) + out = normalize_dependencies({"nanopb/Nanopb": "^0.4.91", "BareName": "1.2.3"}) assert {"name": "Nanopb", "owner": "nanopb", "version": "^0.4.91"} in out assert {"name": "BareName", "owner": None, "version": "1.2.3"} in out def test_normalize_dependencies_dict_form_nested_spec(): - out = _normalize_dependencies( + out = normalize_dependencies( {"nanopb/Nanopb": {"version": "^0.4.91", "platforms": "espidf"}} ) assert out == [ @@ -1190,3 +1069,33 @@ def test_idf_component_download_passes_salt() -> None: "owner/name", force=True, salt="abcd1234", namespace="idf" ) assert c.path == Path("/converted/owner/name") + + +def test_emit_idf_component_wires_esp32_target(tmp_path, monkeypatch): + """Emitting a component resolves the esp32 variant into the shared + extraScript helper.""" + + monkeypatch.setattr(esp32_module, "get_esp32_variant", lambda: "ESP32") + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text("env.Append(LIBS=[env.get('BOARD_MCU')])\n") + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + _emit_idf_component(c) + assert c.data["build"]["flags"] == ["-lesp32"] + + +def test_build_flags_dangling_flag_does_not_cross_entries( + tmp_path, caplog: pytest.LogCaptureFixture +) -> None: + """Each entry is lexed independently, as ParseFlags does: a dangling -I ending one + entry warns instead of absorbing the next entry's first token.""" + (tmp_path / "src").mkdir() + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"flags": ["-Wall -I", "-DFOO=1"]}} + content = generate_cmakelists_txt(c) + assert "FOO=1" in content + assert "-I-DFOO" not in content + assert "Ignoring trailing '-I'" in caplog.text diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py new file mode 100644 index 0000000000..a4eeb5bf41 --- /dev/null +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -0,0 +1,478 @@ +"""Tests for the shared extraScript machinery (platformio.extra_script).""" + +from __future__ import annotations + +import logging +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +from esphome.core import EsphomeError +from esphome.platformio.extra_script import ( + ExtraScriptResult, + _FakeSConsEnv, + apply_extra_script, + captured_as_build_flags, + run_extra_script, +) +from esphome.platformio.library import ( + ConvertedLibrary as IDFComponent, + URLSource, + lex_build_flags, +) + + +def test_extra_script_captures_libpath_libs_and_defines(tmp_path): + + (tmp_path / "src" / "esp32").mkdir(parents=True) + script = tmp_path / "extra_script.py" + script.write_text( + "Import('env')\n" + "mcu = env.get('BOARD_MCU')\n" + "env.Append(\n" + " LIBPATH=[join('src', mcu)],\n" + " LIBS=['algobsec'],\n" + " CPPDEFINES=['FOO', ('BAR', '1')],\n" + " LINKFLAGS=['-Wl,--gc-sections'],\n" + ")\n" + ) + # The script uses bare ``join`` (PIO's extra-scripts run inside SCons + # where this is in scope). Inject it via the script header so the + # shim's exec namespace can resolve it. + script.write_text("from os.path import join\n" + script.read_text()) + + result = run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + + assert result.libpath == [str(Path("src") / "esp32")] + assert result.libs == ["algobsec"] + assert ("BAR", "1") in result.cppdefines + assert "FOO" in result.cppdefines + assert result.linkflags == ["-Wl,--gc-sections"] + + # Lex like the consumer does: quoting makes raw strings platform-varying + tokens = lex_build_flags( + captured_as_build_flags(result, library_dir=tmp_path), "test" + ) + sep = os.sep + assert f"-Lsrc{sep}esp32" in tokens + assert "-lalgobsec" in tokens + assert "-DFOO" in tokens + assert "-DBAR=1" in tokens + assert "-Wl,--gc-sections" in tokens + + +def test_extra_script_libpath_relative_resolves_against_library_dir( + tmp_path, monkeypatch +): + """Relative LIBPATH entries must resolve against ``library_dir``, not the + caller's CWD (the shim restores CWD before ``captured_as_build_flags`` + runs).""" + + (tmp_path / "lib" / "esp32").mkdir(parents=True) + elsewhere = tmp_path.parent / "not_the_library_dir" + elsewhere.mkdir(exist_ok=True) + monkeypatch.chdir(elsewhere) + + result = ExtraScriptResult(libpath=["lib/esp32"]) + flags = captured_as_build_flags(result, library_dir=tmp_path) + + sep = os.sep + assert lex_build_flags(flags, "test") == [f"-Llib{sep}esp32"] + + +def test_extra_script_libpath_absolute_outside_library_dir(tmp_path): + + outside = tmp_path.parent / "system_lib" + outside.mkdir(exist_ok=True) + result = ExtraScriptResult(libpath=[str(outside)]) + + flags = captured_as_build_flags(result, library_dir=tmp_path) + assert lex_build_flags(flags, "test") == [f"-L{outside.resolve()}"] + + +def test_extra_script_failure_returns_empty_result(tmp_path, caplog): + + script = tmp_path / "broken.py" + script.write_text("raise RuntimeError('boom')\n") + + with caplog.at_level("WARNING"): + result = run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + + assert result.libpath == [] + assert result.libs == [] + assert "broken.py" in caplog.text + + +def test_apply_extra_script_path_traversal_is_rejected(tmp_path): + + library_dir = tmp_path / "lib" + library_dir.mkdir() + outside = tmp_path / "evil.py" + outside.write_text("env.Append(LIBS=['pwned'])\n") + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = library_dir + c.data = {"build": {"extraScript": "../evil.py"}} + + with pytest.raises(EsphomeError, match="escapes the library directory"): + apply_extra_script(c, board_mcu=lambda: "esp32", pio_platform="espressif32") + # Nothing was folded into flags: the traversal was rejected before + # the script could run. + assert "flags" not in c.data["build"] + + +def test_apply_extra_script_merges_into_existing_flags(tmp_path): + + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text("env.Append(LIBS=['algobsec'])\n") + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py", "flags": ["-DEXISTING"]}} + + apply_extra_script(c, board_mcu=lambda: "esp32", pio_platform="espressif32") + + assert "-DEXISTING" in c.data["build"]["flags"] + assert "-lalgobsec" in c.data["build"]["flags"] + + +def test_apply_extra_script_malformed_flags_raises(tmp_path) -> None: + """A null/dict build.flags fails naming the library instead of injecting + a non-string into the compiler command line.""" + + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text("env.Append(LIBS=['algobsec'])\n") + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py", "flags": None}} + + with pytest.raises(EsphomeError, match="malformed build.flags"): + apply_extra_script(c, board_mcu=lambda: "esp32", pio_platform="espressif32") + + +def test_apply_extra_script_callable_target_and_str_flags(tmp_path) -> None: + """The shared helper resolves the board_mcu callable lazily and normalizes + a string ``build.flags`` value into a list before extending it.""" + + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text("env.Append(LIBS=[env.get('BOARD_MCU')])\n") + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py", "flags": "-DBASE=1"}} + + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + assert c.data["build"]["flags"] == ["-DBASE=1", "-lesp8266"] + + +def test_captured_nonstring_buckets_warn_and_skip(tmp_path, caplog) -> None: + """Non-string LIBS/LINKFLAGS/CPPFLAGS/LIBPATH entries (legal SCons + nodes) are skipped by name instead of stringified into garbage flags.""" + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text( + "env.Append(LIBS=['m', 42], LINKFLAGS=['-Wl,-x', {'no': 1}], " + "CPPFLAGS=['-Os', 3.5], LIBPATH=['libs', 7])\n" + ) + (tmp_path / "libs").mkdir() + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + flags = c.data["build"]["flags"] + assert "-lm" in flags and "-Wl,-x" in flags and "-Os" in flags + assert not any("42" in f or "no" in f or "3.5" in f for f in flags) + assert "Ignoring unsupported LIBS entry 42" in caplog.text + assert "Ignoring unsupported LIBPATH entry 7" in caplog.text + + +def test_captured_dict_cppdefines_warn_and_skip(tmp_path, caplog) -> None: + """A dict CPPDEFINES entry (legal SCons) must warn and skip; formatting + it blind would hand the compiler -D{'FOO': '1'} garbage.""" + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text( + "env.Append(CPPDEFINES=[{'FOO': '1'}, ('BAR', 2), ['BAZ', 3], 'PLAIN'])\n" + ) + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + assert c.data["build"]["flags"] == ["-DBAR=2", "-DBAZ=3", "-DPLAIN"] + assert "Ignoring unsupported CPPDEFINES entry" in caplog.text + + +def test_apply_extra_script_subscript_env_read(tmp_path) -> None: + """Scripts also read env["BOARD_MCU"]; the subscript form must work or + the broad handler discards every flag the script captured.""" + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text("env.Append(LIBS=[env['BOARD_MCU']])\n") + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + assert c.data["build"]["flags"] == ["-lesp8266"] + + +def test_apply_extra_script_no_script_and_no_flags(tmp_path) -> None: + + # No extraScript declared: nothing happens, the target is never resolved + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {}} + apply_extra_script( + c, + board_mcu=lambda: pytest.fail("target resolved without a script"), + pio_platform="espressif8266", + ) + + # A script that captures nothing leaves the flags untouched + script = tmp_path / "noop.py" + script.write_text("pass\n") + c.data = {"build": {"extraScript": "noop.py"}} + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + assert "flags" not in c.data["build"] + + +def test_apply_extra_script_ignores_uncaptured_env_calls(tmp_path, caplog) -> None: + """Un-captured env vars and unsupported env methods are skipped but + diagnosable from the build log.""" + + caplog.set_level(logging.DEBUG) + script = tmp_path / "extra.py" + script.write_text( + "env.Replace(CC='clang')\nenv.Append(UNCAPTURED=['x'], LIBS='single')\n" + ) + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + assert c.data["build"]["flags"] == ["-lsingle"] + assert "env.Append(UNCAPTURED=...) is not captured" in caplog.text + assert "env.Replace(...) is not supported" in caplog.text + + +def test_apply_extra_script_swallows_script_errors(tmp_path, caplog) -> None: + """A raising extra-script is best-effort: logged and skipped.""" + + script = tmp_path / "extra.py" + script.write_text("raise RuntimeError('boom')\n") + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + assert "flags" not in c.data["build"] + assert "ignoring its output" in caplog.text + + +def test_apply_extra_script_pio_platform(tmp_path) -> None: + """The backend's platform token is exposed to the script as PIOPLATFORM.""" + + script = tmp_path / "extra.py" + script.write_text("env.Append(LIBS=[env.get('PIOPLATFORM')])\n") + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + assert c.data["build"]["flags"] == ["-lespressif8266"] + + +def test_apply_extra_script_missing_script_raises(tmp_path) -> None: + """A declared but absent extraScript is a broken package and fails by + name, as it would under PlatformIO.""" + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "nope.py"}} + with pytest.raises(EsphomeError, match="nope.py of library owner/name not found"): + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + +@pytest.mark.parametrize("bad", (["a.py"], {"esp32": "a.py"}), ids=("list", "dict")) +def test_apply_extra_script_non_string_raises(tmp_path, bad) -> None: + """A non-string extraScript fails naming the library, not with a TypeError.""" + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": bad}} + with pytest.raises(EsphomeError, match="of library owner/name must be a string"): + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + +def test_extra_script_cpppath_captured_as_include_flags(tmp_path, monkeypatch): + """CPPPATH entries translate to -I flags anchored like LIBPATH.""" + + (tmp_path / "include").mkdir() + outside = tmp_path.parent / "system_inc" + outside.mkdir(exist_ok=True) + elsewhere = tmp_path.parent / "not_the_library_dir" + elsewhere.mkdir(exist_ok=True) + monkeypatch.chdir(elsewhere) + + result = ExtraScriptResult(cpppath=["include", str(outside), 7]) + flags = captured_as_build_flags(result, library_dir=tmp_path) + + assert lex_build_flags(flags, "test") == ["-Iinclude", f"-I{outside.resolve()}"] + + +def test_extra_script_spaced_paths_survive_relexing(tmp_path): + """-I/-L paths with spaces round-trip through lex_build_flags as one token.""" + (tmp_path / "my libs").mkdir() + result = ExtraScriptResult(cpppath=["my libs"], libpath=["my libs"]) + flags = captured_as_build_flags(result, library_dir=tmp_path) + assert lex_build_flags(flags, "test") == ["-Imy libs", "-Lmy libs"] + + +def test_run_extra_script_failure_discards_partial_capture(tmp_path, caplog) -> None: + """A crashed script yields an empty result: half-applied flags could + build wrong-output firmware that links cleanly.""" + + script = tmp_path / "extra.py" + script.write_text("env.Append(LIBS=['algobsec'])\nraise RuntimeError('boom')\n") + result = run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + assert result.libs == [] + assert "ignoring its output" in caplog.text + + +def test_run_extra_script_syntax_error_is_best_effort(tmp_path, caplog) -> None: + """A vendored script that does not even compile warns and skips instead + of aborting the build.""" + + script = tmp_path / "extra.py" + script.write_text("def broken(:\n") + result = run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + assert result.libs == [] + assert "ignoring its output" in caplog.text + + +def test_unsupported_env_method_warns_once(caplog) -> None: + """Repeated calls to the same unsupported method warn only once.""" + + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + env.Replace(CC="clang") + env.Replace(CC="gcc") + assert caplog.text.count("env.Replace(...) is not supported") == 1 + + +def test_run_extra_script_sys_exit_is_best_effort(tmp_path, caplog) -> None: + """A nonzero sys.exit() in a vendored script must not kill the esphome + run, and its output is discarded.""" + + script = tmp_path / "extra.py" + script.write_text("import sys\nenv.Append(LIBS=['x'])\nsys.exit(3)\n") + result = run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + assert result.libs == [] + assert "exited with status 3" in caplog.text + + +def test_run_extra_script_sys_exit_zero_is_success(tmp_path, caplog) -> None: + """sys.exit(0) is a normal PlatformIO script ending: the capture is kept.""" + + script = tmp_path / "extra.py" + script.write_text("import sys\nenv.Append(LIBS=['algobsec'])\nsys.exit(0)\n") + result = run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + assert result.libs == ["algobsec"] + assert "ignoring its output" not in caplog.text + + +def test_run_extra_script_unreadable_raises(tmp_path) -> None: + """An unreadable declared script is a broken package, like a missing one.""" + + script = tmp_path / "extra.py" + script.write_text("") + with ( + patch("pathlib.Path.read_text", side_effect=OSError("denied")), + pytest.raises(EsphomeError, match="is unreadable"), + ): + run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + + +def test_run_extra_script_bad_encoding_is_best_effort(tmp_path, caplog) -> None: + """Undecodable content warns and skips, like a SyntaxError.""" + + script = tmp_path / "extra.py" + script.write_bytes(b"\xff\xfe\x00bad") + result = run_extra_script( + script, library_dir=tmp_path, board_mcu="esp32", pio_platform="espressif32" + ) + assert result.libs == [] + assert "is not UTF-8" in caplog.text + + +@pytest.mark.parametrize("method", ("Prepend", "AppendUnique", "PrependUnique")) +def test_append_variants_capture_like_append(method: str) -> None: + """Prepend/AppendUnique/PrependUnique write the captured keys too.""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + getattr(env, method)(LIBS=["algobsec"], LIBPATH=["lib"]) + assert env.result.libs == ["algobsec"] + assert env.result.libpath == ["lib"] + + +def test_env_get_unknown_key_warns_once(caplog) -> None: + """A script branching on an unmodelled env var is diagnosable.""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + assert env.get("BOARD") is None + assert env.get("BOARD", "d1") == "d1" + assert env.get("BOARD_MCU") == "esp8266" + assert caplog.text.count("env.get('BOARD') is not modelled") == 1 + assert "BOARD_MCU" not in caplog.text + + +def test_spaced_linkflag_survives_relexing(tmp_path) -> None: + """A captured argv token with a space stays one token after lexing.""" + result = ExtraScriptResult( + linkflags=["-Wl,-T my linker.ld"], cppflags=["-include my hdr.h"] + ) + flags = captured_as_build_flags(result, library_dir=tmp_path) + assert lex_build_flags(flags, "test") == [ + "-Wl,-T my linker.ld", + "-include my hdr.h", + ] + + +def test_uncaptured_append_key_warns_once(caplog) -> None: + """A loop of Appends to the same uncaptured key warns once.""" + + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + env.Append(RANLIBFLAGS=["a"]) + env.Append(RANLIBFLAGS=["b"]) + assert caplog.text.count("env.Append(RANLIBFLAGS=...) is not captured") == 1 diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 0eede78656..3eb4f2aa75 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -4,6 +4,7 @@ Covers the shared download/parse/resolve/dependency-walk paths in ``esphome.platformio.library`` directly (the ESP-IDF and Zephyr backends are exercised in their own test modules).""" +from contextlib import contextmanager import json import logging from pathlib import Path @@ -13,6 +14,7 @@ import pytest from esphome.core import EsphomeError, Library import esphome.platformio.library as lib from esphome.platformio.library import ( + SOURCE_KIND_FOR_SUFFIX, ConvertedLibrary, GitSource, InvalidLibrary, @@ -23,6 +25,8 @@ from esphome.platformio.library import ( _resolve_registry_version, check_library_data, convert_libraries, + join_flag_args, + split_flag_entry, ) @@ -150,11 +154,30 @@ def test_localsource_download_returns_empty_build_dir(setup_core: Path) -> None: assert plain != out +@contextmanager +def caplog_at_info(): + records: list[logging.LogRecord] = [] + handler = logging.Handler() + handler.emit = records.append + logger = logging.getLogger("esphome.platformio.library") + logger.addHandler(handler) + # The level must actually admit INFO or the no-INFO assertions are vacuous + old_level = logger.level + logger.setLevel(logging.INFO) + try: + yield records + finally: + logger.setLevel(old_level) + logger.removeHandler(handler) + + def test_urlsource_download_extracts_then_reuses_marker(setup_core, monkeypatch): monkeypatch.setattr(lib, "rmdir", lambda path, msg="": None) dl_calls: list[list[str]] = [] monkeypatch.setattr( - lib, "download_from_mirrors", lambda urls, headers, f: dl_calls.append(urls) + lib, + "download_from_mirrors", + lambda urls, headers, f: dl_calls.append(urls), ) def fake_extract(fileobj, path): @@ -213,7 +236,7 @@ def _patch_registry_resolve(monkeypatch: pytest.MonkeyPatch) -> None: def _patch_download_with_manifests(monkeypatch, tmp_path, manifests, *, properties=()): """Fake ConvertedLibrary.download to materialize canned manifests on disk.""" - def fake_download(self, force=False, salt="", namespace=""): + def fake_download(self, force=False, salt="", namespace="", progress=None): self.path = tmp_path / self.get_require_name() self.path.mkdir(parents=True, exist_ok=True) if self.name in properties: @@ -292,7 +315,11 @@ def _patch_download_without_manifest( calls: list[bool] = [] def fake_download( - self: ConvertedLibrary, force: bool = False, salt: str = "", namespace: str = "" + self: ConvertedLibrary, + force: bool = False, + salt: str = "", + namespace: str = "", + progress=None, ) -> None: calls.append(force) self.path = tmp_path / self.get_require_name() @@ -531,3 +558,182 @@ def test_convert_libraries_skips_incompatible_dependency(tmp_path, monkeypatch): top = convert_libraries([Library("esphome/A", "1.0.0", None)], _backend()) assert top[0].dependencies == [] + + +def test_split_flag_entry_unbalanced_quote_is_clean() -> None: + """A malformed flags entry raises EsphomeError, not a raw ValueError.""" + + assert split_flag_entry('-DX="a b"', "library x") == ["-DX=a b"] + with pytest.raises(EsphomeError, match=r"Malformed build flag.*library x"): + split_flag_entry('-DX="unclosed', "library x") + + +def test_join_flag_args_reglues_spaced_define() -> None: + """A spaced -D re-glues to its argument, as ParseFlags does.""" + + assert join_flag_args(["-D", "FOO=1", "-Os"], "x") == ["-DFOO=1", "-Os"] + + +def test_join_flag_args_trailing_bare_flag_warns( + caplog: pytest.LogCaptureFixture, +) -> None: + + assert join_flag_args(["-Os", "-l"], "library x") == ["-Os"] + assert "Ignoring trailing '-l'" in caplog.text + + +def test_lex_build_flags_dangling_flag_does_not_cross_entries( + caplog: pytest.LogCaptureFixture, +) -> None: + """Each entry is lexed independently, as ParseFlags does: a dangling -I + ending one entry warns instead of absorbing the next entry's first token.""" + from esphome.platformio.library import lex_build_flags + + assert lex_build_flags(["-Wall -I", "-DFOO=1"], "lib x") == ["-Wall", "-DFOO=1"] + assert "Ignoring trailing '-I'" in caplog.text + + +def test_join_flag_args_empty_argument_warns_and_drops( + caplog: pytest.LogCaptureFixture, +) -> None: + """An empty glued argument is dropped: a bare -D would eat the next flag.""" + assert lib.lex_build_flags('-D "" -DFOO', "build_flags") == ["-DFOO"] + assert "Ignoring '-D' with empty argument in build_flags" in caplog.text + + +def test_normalize_dependencies_forms(caplog) -> None: + """Every PIO-legal spelling normalizes; unrecognizable entries warn.""" + from esphome.platformio.library import normalize_dependencies + + assert normalize_dependencies( + ["Wire", {"name": "SPI"}, 5, "", {"version": "1.0"}], "libx" + ) == [ + {"name": "Wire"}, + {"name": "SPI"}, + ] + # The int, the empty string, and the nameless dict all warn + assert caplog.text.count("unrecognized dependency entry") == 3 + # A plain string is names, never iterated into characters + assert normalize_dependencies("Wire, SPI") == [ + {"name": "Wire"}, + {"name": "SPI"}, + ] + assert normalize_dependencies("Wire") == [{"name": "Wire"}] + # A non-iterable value fails by manifest name, never a bare TypeError + assert normalize_dependencies(5, "libx") == [] + assert "Ignoring unrecognized dependencies 5 of libx" in caplog.text + # The dict-shorthand form validates names like the list form: an empty + # key and a spec overriding name with a non-string both warn and drop + assert normalize_dependencies( + {"": "1.0", "Wire": {"name": 123, "version": "1.0"}, "SPI": "*"}, "libx" + ) == [{"name": "SPI", "owner": None, "version": "*"}] + assert caplog.text.count("unrecognized dependency entry") == 5 + # A container or numeric version would raise from set.add() or fail + # opaquely in the registry; both spellings warn and drop + assert normalize_dependencies({"Foo": ["1.0", "2.0"]}, "libx") == [] + assert normalize_dependencies([{"name": "Foo", "version": 1}], "libx") == [] + assert caplog.text.count("unrecognized dependency entry") == 7 + + +@pytest.mark.parametrize( + "manifest", [["not", "a", "manifest"], {"name": "A", "build": "src"}] +) +def test_convert_libraries_malformed_manifest_raises( + tmp_path, monkeypatch, manifest +) -> None: + """A manifest without the expected dict shape fails by library name + before any backend dereferences data/build.""" + _patch_download_with_manifests(monkeypatch, tmp_path, {"esphome/A": manifest}) + with pytest.raises(EsphomeError, match="has a malformed manifest"): + convert_libraries([Library("esphome/A", None, None)], _backend()) + + +def test_walk_warns_for_properties_only_depends( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A manifest declaring dependencies only as library.properties depends= + warns in the shared walk, so every backend reports the drop.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": "name=A\nversion=1.0\ndepends=Wire, SPI\n"}, + properties=("esphome/A",), + ) + caplog.set_level("INFO") + convert_libraries([Library("esphome/A", "1.0.0", None)], _backend()) + assert "declares dependencies via library.properties" in caplog.text + + +def test_walk_warns_for_nonplatform_invalid_library( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A dependency dropped for any cause other than the routine platform + filter is visible in every backend.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": {"name": "A", "dependencies": [{"name": "B", "version": "1.0"}]}}, + ) + calls = {"n": 0} + real = lib.check_library_data + + def flaky(data, platform, framework): + calls["n"] += 1 + if calls["n"] > 1: + raise InvalidLibrary("manifest is corrupt") + return real(data, platform, framework) + + monkeypatch.setattr(lib, "check_library_data", flaky) + convert_libraries([Library("esphome/A", None, None)], _backend()) + assert "Skipping dependency B of esphome/A: manifest is corrupt" in caplog.text + + +def test_convert_libraries_warns_for_nonplatform_invalid_dependency_component( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A dependency component dropped for any cause other than the platform + filter warns; only the routine cross-platform skip stays at debug.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "C", "owner": "esphome", "version": "1.0"}], + }, + "esphome/C": {"name": "C"}, + }, + ) + real = lib.check_library_data + + def flaky(data, platform, framework): + # Fail only on C's resolved manifest, not on A's dependency entry + if data.get("name") == "C" and "version" not in data: + raise InvalidLibrary("manifest is corrupt") + return real(data, platform, framework) + + monkeypatch.setattr(lib, "check_library_data", flaky) + convert_libraries([Library("esphome/A", "1.0.0", None)], _backend()) + assert "manifest is corrupt" in caplog.text + assert "Skipping dependency" in caplog.text + + +def test_split_flag_entry_non_string_is_clean() -> None: + """A dict or number from a third-party manifest fails naming the entry, + not with an opaque shlex traceback.""" + + with pytest.raises(EsphomeError, match="Malformed build flag"): + split_flag_entry({"esp32": ["-DX"]}, "lib x") + with pytest.raises(EsphomeError, match="Malformed build flag 5"): + split_flag_entry(5, "lib x") + + +def test_source_kind_map_shape() -> None: + """The kind values the native compile rules key on, and the deliberate + AS/ASPP merge (.s and .S both map to asm).""" + + assert set(SOURCE_KIND_FOR_SUFFIX.values()) == {"c", "cxx", "asm"} + assert SOURCE_KIND_FOR_SUFFIX[".s"] == "asm" + assert SOURCE_KIND_FOR_SUFFIX[".S"] == "asm" + assert SOURCE_KIND_FOR_SUFFIX[".c"] == "c" + assert SOURCE_KIND_FOR_SUFFIX[".cpp"] == "cxx" diff --git a/tests/unit_tests/test_platformio_toolchain.py b/tests/unit_tests/test_platformio_toolchain.py index 172b288c25..28304270a4 100644 --- a/tests/unit_tests/test_platformio_toolchain.py +++ b/tests/unit_tests/test_platformio_toolchain.py @@ -1977,3 +1977,10 @@ def test_run_platformio_cli_invokes_heal( with patch.object(toolchain, "heal_platformio_python_env") as mock_heal: toolchain.run_platformio_cli("test") mock_heal.assert_called_once() + + +def test_ccache_probe_spawns_with_close_fds_false() -> None: + """The probe follows the repo-wide posix_spawn convention.""" + with patch("subprocess.run") as mock_run: + assert toolchain._ccache_runs("/usr/bin/ccache") is True + assert mock_run.call_args.kwargs["close_fds"] is False From df76fc6fba5e102fd9d9327cb22eabda69e7fa5d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 12:55:52 -0500 Subject: [PATCH 02/10] Trim comments and docstrings --- esphome/platformio/extra_script.py | 20 ++++--------- esphome/platformio/library.py | 48 ++++++++++-------------------- 2 files changed, 21 insertions(+), 47 deletions(-) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index 363c40561f..f29cf32951 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -30,10 +30,7 @@ def apply_extra_script( pio_platform: str, ) -> None: """Run a library's ``extraScript`` and fold its captured env vars into - ``component.data["build"]["flags"]``. - - ``board_mcu`` is a callable so its lookup runs only when a script will. - """ + ``build.flags``; ``board_mcu`` is a callable so it resolves lazily.""" extra_script = component.data.get("build", {}).get("extraScript") if not extra_script: return @@ -177,12 +174,10 @@ def run_extra_script( board_mcu: str, pio_platform: str, ) -> ExtraScriptResult: - """Execute ``script_path`` with a fake SCons env and return captured vars. + """Execute ``script_path`` with a fake SCons env, ``library_dir`` as CWD. - Runs with ``library_dir`` as CWD so relative lookups resolve against - the library tree. A crashed script warns and returns an empty result, - never a partial capture. - """ + A crashed script warns and returns an empty result, never a partial + capture.""" env = _FakeSConsEnv( board_mcu=board_mcu, pio_env=f"esphome_{board_mcu}", @@ -248,11 +243,8 @@ def run_extra_script( def captured_as_build_flags( result: ExtraScriptResult, *, library_dir: Path ) -> list[str]: - """Translate captured env vars into -L/-l/-D/raw build flags. - - ``LIBPATH`` entries are made relative to ``library_dir`` so the - generated build files stay portable. - """ + """Translate captured env vars into -L/-l/-D/raw build flags; path + entries anchor to ``library_dir`` so the build files stay portable.""" flags: list[str] = [] def _strs(bucket: list, kind: str) -> list[str]: diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index af3b05c9a0..f2a9971814 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -47,12 +47,9 @@ DEFAULT_BUILD_SRC_FILTER = ( DEFAULT_BUILD_SRC_DIRS = "src" DEFAULT_BUILD_INCLUDE_DIR = "include" DEFAULT_BUILD_FLAGS = [] -# Suffix -> compiler kind (PlatformIO's CSUFFIXES/CXXSUFFIXES/ASSUFFIXES). -# "asm" merges SCons's AS and ASPP sets: all compile as assembler-with-cpp. -# The kind values drive the ESP8266 native ninja rules (later in this -# chain); existing backends consume only the keys. Note .C/.C++ join the -# suffix set here per CXXSUFFIXES; SCons demotes .C to C on -# case-insensitive filesystems, we always treat it as C++. +# Suffix -> compiler kind (PlatformIO's CSUFFIXES/CXXSUFFIXES/ASSUFFIXES); +# "asm" merges SCons's AS and ASPP sets. Per CXXSUFFIXES .C/.C++ are C++ +# here, even where SCons demotes .C on case-insensitive filesystems. SOURCE_KIND_FOR_SUFFIX: dict[str, str] = { ".c": "c", ".cpp": "cxx", @@ -213,11 +210,8 @@ class InvalidLibrary(Exception): class IncompatiblePlatform(InvalidLibrary): - """The manifest's platform filter rejected the target platform. - - A distinct type so callers can treat the routine cross-platform skip - differently from other manifest problems without matching message text. - """ + """The routine cross-platform skip, typed so callers need not match + message text.""" class ConvertedLibrary: @@ -586,9 +580,8 @@ def split_flag_entry(entry: Any, owner: str) -> list[str]: def lex_build_flags(entries: str | list[str], owner: str) -> list[str]: """Shell-lex ``build.flags`` entries the way PlatformIO's ParseFlags does; bare -I/-L/-l/-D tokens re-glue to their argument.""" - # Join per entry, as SCons's ParseFlags lexes each string independently: - # a dangling -I ending one entry must warn, not absorb the next entry's - # first token. + # Lex per entry as ParseFlags does: a dangling -I must warn, not absorb + # the next entry's first token return [ token for entry in ensure_list(entries) @@ -601,13 +594,9 @@ BARE_ARG_FLAGS = frozenset({"-I", "-L", "-l", "-D"}) def join_flag_args(tokens: Iterable[str], owner: str) -> list[str]: - """Join a bare ``-I``/``-L``/``-l``/``-D`` with its following token, - the way PlatformIO's ParseFlags lexes them. - - A trailing or empty argument (``-D ""``) is warned and dropped: the - bare flag would make gcc eat the next flag as its argument (or add - the CWD for ``-L``); always a typo. - """ + """Join a bare ``-I``/``-L``/``-l``/``-D`` with its following token, as + PlatformIO's ParseFlags does. A trailing or empty argument is warned and + dropped: the bare flag would make gcc eat the next flag.""" out: list[str] = [] it = iter(tokens) for tok in it: @@ -627,11 +616,8 @@ def join_flag_args(tokens: Iterable[str], owner: str) -> list[str]: def warn_properties_depends(name: str, data: object) -> None: - """Warn when a manifest declares dependencies only as ``depends=``. - - The dependency walk reads the JSON ``dependencies`` key; the raw - ``library.properties`` spelling would otherwise drop silently. - """ + """Warn for ``depends=``-only manifests; the walk reads only the JSON + ``dependencies`` key, so they would otherwise drop silently.""" if isinstance(data, dict) and not data.get("dependencies") and data.get("depends"): # INFO: common and unactionable for transitive libraries; a WARNING # on every build would train users to ignore the stream @@ -662,13 +648,9 @@ def dependency_is_usable( def _valid_dependency_entry(entry: dict, manifest_name: str) -> bool: - """Whether a normalized entry carries a usable name and version. - - The name must be a non-empty string (every consumer indexes or joins - it); a present version must be a string (a container would raise from - ``set.add()``, an int fails opaquely inside the registry resolution). - Invalid entries warn naming the manifest. - """ + """Whether a normalized entry carries a usable name (non-empty string) + and version (string, if present); invalid entries warn naming the + manifest.""" name = entry.get("name") if ( isinstance(name, str) From 4da886851006573e63e0f94af12801a5346a4664 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 13:34:19 -0500 Subject: [PATCH 03/10] Quote CPPDEFINES, warn on env access, degrade unmodelled subscripts, realify InvalidLibrary CPPDEFINES joins the quoted buckets (a spaced define no longer splits across tokens) and its tuple branch validates the pair elements. The fake env warns on attribute access rather than call, so hasattr/ truthiness branches are diagnosable, with dunder probes excluded; an unmodelled subscript warns and returns '' instead of a KeyError discarding the whole capture. Malformed platforms/frameworks values raise plain InvalidLibrary, giving the non-platform warning branches a real producer, and their tests use real manifests instead of monkeypatched raisers. The version-less dependency drop moves to debug: bundled names (Wire, SPI) made it per-build noise nobody can act on. --- esphome/platformio/extra_script.py | 39 ++++++++++++------- esphome/platformio/library.py | 9 ++++- .../test_platformio_extra_script.py | 10 +++-- tests/unit_tests/test_platformio_library.py | 32 +++++---------- 4 files changed, 49 insertions(+), 41 deletions(-) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index f29cf32951..bdef357671 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -127,9 +127,14 @@ class _FakeSConsEnv: return self._vars.get(key, default) def __getitem__(self, key: str) -> str: - # Scripts also read env["BOARD_MCU"]; without this the broad - # handler would discard every flag the script captured - return self._vars[key] + # Scripts also read env["BOARD_MCU"]; an unmodelled subscript + # degrades one branch instead of discarding the whole capture + if key not in self._vars and key not in self._warned_gets: + self._warned_gets.add(key) + _LOGGER.warning( + "PIO extra-script env[%r] is not modelled; returning ''", key + ) + return self._vars.get(key, "") def Append(self, **kwargs) -> None: # noqa: N802 (SCons API name) for key, value in kwargs.items(): @@ -155,14 +160,18 @@ class _FakeSConsEnv: # ----- Everything else is a no-op so unsupported scripts don't crash ----- def __getattr__(self, name: str): + if name.startswith("__") and name.endswith("__"): + # Protocol probes (copy, pickle, iteration) are not script calls + raise AttributeError(name) + if name not in self._warned_methods: + # Warn on access, not call: hasattr()/truthiness branches would + # otherwise silently take the wrong path; a script whose whole + # effect is env.Replace() stays diagnosable either way + self._warned_methods.add(name) + _LOGGER.warning("PIO extra-script env.%s is not supported; ignoring", name) + def _noop(*args, **kwargs): - # Once per method: a script whose whole effect is env.Replace() - # must be diagnosable from a normal build log - if name not in self._warned_methods: - self._warned_methods.add(name) - _LOGGER.warning( - "PIO extra-script env.%s(...) is not supported; ignoring", name - ) + return None return _noop @@ -278,10 +287,14 @@ def captured_as_build_flags( for define in result.cppdefines: # SCons also accepts dict/list CPPDEFINES; formatting those blind # would hand the compiler garbage like -D{'FOO': '1'} - if isinstance(define, (tuple, list)) and len(define) == 2: - flags.append(f"-D{define[0]}={define[1]}") + if ( + isinstance(define, (tuple, list)) + and len(define) == 2 + and all(isinstance(part, (str, int)) for part in define) + ): + flags.append(shlex.quote(f"-D{define[0]}={define[1]}")) elif isinstance(define, str): - flags.append(f"-D{define}") + flags.append(shlex.quote(f"-D{define}")) else: _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", define) # Each captured entry is one argv token in SCons; quote so the diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index f2a9971814..963e10c255 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -438,6 +438,9 @@ def check_library_data(data: dict, platform: str | None, framework: str): if isinstance(platforms, str): platforms = [a.strip() for a in platforms.split(",")] platforms = ensure_list(platforms) + if not all(isinstance(pf, str) for pf in platforms): + # A real (non-platform) manifest problem; callers warn, not skip + raise InvalidLibrary(f"Malformed platforms value: {platforms!r}") # Check if library supports the target platform valid_platforms = platform is None or "*" in platforms or platform in platforms @@ -449,6 +452,8 @@ def check_library_data(data: dict, platform: str | None, framework: str): if isinstance(frameworks, str): frameworks = [a.strip() for a in frameworks.split(",")] frameworks = ensure_list(frameworks) + if not all(isinstance(fw, str) for fw in frameworks): + raise InvalidLibrary(f"Malformed frameworks value: {frameworks!r}") # Check if library declares the active framework. PIO library manifests # often list only "arduino" even when the library actually compiles fine @@ -1029,8 +1034,8 @@ def convert_libraries( ): if "version" not in dependency: # Cannot resolve from the registry; common for bundled - # names (Wire, SPI) -- add_library() is the fix if real - _LOGGER.info( + # names (Wire, SPI) -- unactionable noise above debug + _LOGGER.debug( "Skip version-less dependency %r of %s", dependency.get("name"), component.name, diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index a4eeb5bf41..e2484408eb 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -270,7 +270,7 @@ def test_apply_extra_script_ignores_uncaptured_env_calls(tmp_path, caplog) -> No apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") assert c.data["build"]["flags"] == ["-lsingle"] assert "env.Append(UNCAPTURED=...) is not captured" in caplog.text - assert "env.Replace(...) is not supported" in caplog.text + assert "env.Replace is not supported" in caplog.text def test_apply_extra_script_swallows_script_errors(tmp_path, caplog) -> None: @@ -378,7 +378,7 @@ def test_unsupported_env_method_warns_once(caplog) -> None: ) env.Replace(CC="clang") env.Replace(CC="gcc") - assert caplog.text.count("env.Replace(...) is not supported") == 1 + assert caplog.text.count("env.Replace is not supported") == 1 def test_run_extra_script_sys_exit_is_best_effort(tmp_path, caplog) -> None: @@ -458,10 +458,14 @@ def test_env_get_unknown_key_warns_once(caplog) -> None: def test_spaced_linkflag_survives_relexing(tmp_path) -> None: """A captured argv token with a space stays one token after lexing.""" result = ExtraScriptResult( - linkflags=["-Wl,-T my linker.ld"], cppflags=["-include my hdr.h"] + linkflags=["-Wl,-T my linker.ld"], + cppflags=["-include my hdr.h"], + cppdefines=[("MSG", '"hello world"'), "PLAIN"], ) flags = captured_as_build_flags(result, library_dir=tmp_path) assert lex_build_flags(flags, "test") == [ + '-DMSG="hello world"', + "-DPLAIN", "-Wl,-T my linker.ld", "-include my hdr.h", ] diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 3eb4f2aa75..da4dba842f 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -672,20 +672,15 @@ def test_walk_warns_for_nonplatform_invalid_library( _patch_download_with_manifests( monkeypatch, tmp_path, - {"esphome/A": {"name": "A", "dependencies": [{"name": "B", "version": "1.0"}]}}, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "B", "version": "1.0", "platforms": [123]}], + } + }, ) - calls = {"n": 0} - real = lib.check_library_data - - def flaky(data, platform, framework): - calls["n"] += 1 - if calls["n"] > 1: - raise InvalidLibrary("manifest is corrupt") - return real(data, platform, framework) - - monkeypatch.setattr(lib, "check_library_data", flaky) convert_libraries([Library("esphome/A", None, None)], _backend()) - assert "Skipping dependency B of esphome/A: manifest is corrupt" in caplog.text + assert "Skipping dependency B of esphome/A: Malformed platforms" in caplog.text def test_convert_libraries_warns_for_nonplatform_invalid_dependency_component( @@ -701,20 +696,11 @@ def test_convert_libraries_warns_for_nonplatform_invalid_dependency_component( "name": "A", "dependencies": [{"name": "C", "owner": "esphome", "version": "1.0"}], }, - "esphome/C": {"name": "C"}, + "esphome/C": {"name": "C", "frameworks": [None]}, }, ) - real = lib.check_library_data - - def flaky(data, platform, framework): - # Fail only on C's resolved manifest, not on A's dependency entry - if data.get("name") == "C" and "version" not in data: - raise InvalidLibrary("manifest is corrupt") - return real(data, platform, framework) - - monkeypatch.setattr(lib, "check_library_data", flaky) convert_libraries([Library("esphome/A", "1.0.0", None)], _backend()) - assert "manifest is corrupt" in caplog.text + assert "Malformed frameworks" in caplog.text assert "Skipping dependency" in caplog.text From 879cbf6264216d7fa1a613fae49191e8c4373bd3 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 13:49:03 -0500 Subject: [PATCH 04/10] Cover env access warnings and the unmodelled-subscript degrade --- .../test_platformio_extra_script.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index e2484408eb..4b839e30e8 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -471,6 +471,32 @@ def test_spaced_linkflag_survives_relexing(tmp_path) -> None: ] +def test_env_attribute_access_warns_without_call(caplog) -> None: + """hasattr()/truthiness on an unsupported method is diagnosable; dunder + protocol probes stay silent.""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + assert env.GetProjectOption + assert caplog.text.count("env.GetProjectOption is not supported") == 1 + assert not hasattr(env, "__deepcopy__") + assert "__deepcopy__" not in caplog.text + + +def test_env_unmodelled_subscript_degrades_one_branch(caplog) -> None: + """env[...] on an unmodelled var returns '' instead of KeyError + discarding the whole capture.""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + assert env["PIOFRAMEWORK"] == "" + assert env["PIOFRAMEWORK"] == "" + assert caplog.text.count("env['PIOFRAMEWORK'] is not modelled") == 1 + assert env["BOARD_MCU"] == "esp8266" + env.Append(LIBS=["still_captured"]) + assert env.result.libs == ["still_captured"] + + def test_uncaptured_append_key_warns_once(caplog) -> None: """A loop of Appends to the same uncaptured key warns once.""" From aa3268a6d8237b9e15dee583f12e49997374fc47 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 15:05:43 -0500 Subject: [PATCH 05/10] Match SCons' remaining CPPDEFINES spellings, drop dead test scaffolding A bare 2-tuple is one name=value pair (not -DFOO -D1), a dict maps names to values, and a None value is a bare define, per processDefines. The unused caplog_at_info helper and the progress kwarg the download stubs declared ahead of their PR are gone. --- esphome/platformio/extra_script.py | 29 +++++++++++++++---- .../test_platformio_extra_script.py | 18 ++++++++++++ tests/unit_tests/test_platformio_library.py | 21 +------------- 3 files changed, 43 insertions(+), 25 deletions(-) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index bdef357671..9fa73bafac 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -14,7 +14,7 @@ import logging import os from pathlib import Path import shlex -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from esphome.core import EsphomeError @@ -98,6 +98,17 @@ class ExtraScriptResult: cppflags: list[str] = field(default_factory=list) +def _cppdefines_items(value: Any) -> list: + """Normalize SCons ``processDefines`` spellings: a bare 2-tuple is one + ``name=value`` pair, a dict maps names to values, a list is + element-wise.""" + if isinstance(value, tuple) and len(value) == 2: + return [value] + if isinstance(value, dict): + return list(value.items()) + return list(value) if isinstance(value, (list, tuple)) else [value] + + class _FakeSConsEnv: """Minimal SCons ``Environment`` stand-in: ``get`` and ``Append`` work; every other method is a swallowed no-op so scripts don't abort.""" @@ -147,7 +158,10 @@ class _FakeSConsEnv: key, ) continue - items = list(value) if isinstance(value, (list, tuple)) else [value] + if key == "CPPDEFINES": + items = _cppdefines_items(value) + else: + items = list(value) if isinstance(value, (list, tuple)) else [value] bucket = getattr(self.result, key.lower()) bucket.extend(items) @@ -285,14 +299,19 @@ def captured_as_build_flags( ) flags.extend(f"-l{shlex.quote(lib)}" for lib in _strs(result.libs, "LIBS")) for define in result.cppdefines: - # SCons also accepts dict/list CPPDEFINES; formatting those blind + # SCons also accepts nested containers; formatting those blind # would hand the compiler garbage like -D{'FOO': '1'} if ( isinstance(define, (tuple, list)) and len(define) == 2 - and all(isinstance(part, (str, int)) for part in define) + and isinstance(define[0], (str, int)) + and isinstance(define[1], (str, int, type(None))) ): - flags.append(shlex.quote(f"-D{define[0]}={define[1]}")) + if define[1] is None: + # {"FOO": None} / ("FOO", None) is a bare -DFOO in SCons + flags.append(shlex.quote(f"-D{define[0]}")) + else: + flags.append(shlex.quote(f"-D{define[0]}={define[1]}")) elif isinstance(define, str): flags.append(shlex.quote(f"-D{define}")) else: diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index 4b839e30e8..ebe61d0d1f 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -497,6 +497,24 @@ def test_env_unmodelled_subscript_degrades_one_branch(caplog) -> None: assert env.result.libs == ["still_captured"] +def test_cppdefines_scons_spellings(tmp_path) -> None: + """A bare 2-tuple is one name=value pair, a dict maps names to values, + and a None value is a bare define (SCons processDefines).""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + env.Append(CPPDEFINES=("FOO", "1")) + env.Append(CPPDEFINES={"BAR": "2", "BAZ": None}) + env.Append(CPPDEFINES=["PLAIN"]) + flags = captured_as_build_flags(env.result, library_dir=tmp_path) + assert lex_build_flags(flags, "test") == [ + "-DFOO=1", + "-DBAR=2", + "-DBAZ", + "-DPLAIN", + ] + + def test_uncaptured_append_key_warns_once(caplog) -> None: """A loop of Appends to the same uncaptured key warns once.""" diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index da4dba842f..0f90ad3e84 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -4,7 +4,6 @@ Covers the shared download/parse/resolve/dependency-walk paths in ``esphome.platformio.library`` directly (the ESP-IDF and Zephyr backends are exercised in their own test modules).""" -from contextlib import contextmanager import json import logging from pathlib import Path @@ -154,23 +153,6 @@ def test_localsource_download_returns_empty_build_dir(setup_core: Path) -> None: assert plain != out -@contextmanager -def caplog_at_info(): - records: list[logging.LogRecord] = [] - handler = logging.Handler() - handler.emit = records.append - logger = logging.getLogger("esphome.platformio.library") - logger.addHandler(handler) - # The level must actually admit INFO or the no-INFO assertions are vacuous - old_level = logger.level - logger.setLevel(logging.INFO) - try: - yield records - finally: - logger.setLevel(old_level) - logger.removeHandler(handler) - - def test_urlsource_download_extracts_then_reuses_marker(setup_core, monkeypatch): monkeypatch.setattr(lib, "rmdir", lambda path, msg="": None) dl_calls: list[list[str]] = [] @@ -236,7 +218,7 @@ def _patch_registry_resolve(monkeypatch: pytest.MonkeyPatch) -> None: def _patch_download_with_manifests(monkeypatch, tmp_path, manifests, *, properties=()): """Fake ConvertedLibrary.download to materialize canned manifests on disk.""" - def fake_download(self, force=False, salt="", namespace="", progress=None): + def fake_download(self, force=False, salt="", namespace=""): self.path = tmp_path / self.get_require_name() self.path.mkdir(parents=True, exist_ok=True) if self.name in properties: @@ -319,7 +301,6 @@ def _patch_download_without_manifest( force: bool = False, salt: str = "", namespace: str = "", - progress=None, ) -> None: calls.append(force) self.path = tmp_path / self.get_require_name() From c6f9153e638a08e2e1e62062ec16d4ee655bb2e5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 17:08:04 -0500 Subject: [PATCH 06/10] Route captured LINKFLAGS to the link line and make Prepend a real front-insert --- esphome/espidf/component.py | 11 ++++ esphome/platformio/extra_script.py | 60 ++++++++++++------- esphome/platformio/library.py | 3 + tests/unit_tests/test_espidf_component.py | 21 +++++++ .../test_platformio_extra_script.py | 25 ++++++-- 5 files changed, 94 insertions(+), 26 deletions(-) diff --git a/esphome/espidf/component.py b/esphome/espidf/component.py index 4eeaa30e7f..4655d1c54d 100644 --- a/esphome/espidf/component.py +++ b/esphome/espidf/component.py @@ -20,6 +20,7 @@ from esphome.platformio.library import ( DEFAULT_BUILD_SRC_FILTER, ESPHOME_DATA_EXTRA_CMAKE_KEY, ESPHOME_DATA_KEY, + ESPHOME_DATA_LINK_FLAGS_KEY, SRC_FILE_EXTENSIONS, ConvertedLibrary as IDFComponent, LibraryBackend, @@ -205,6 +206,16 @@ def generate_cmakelists_txt(component: IDFComponent) -> str: content += f" {str_build_flag}\n" content += ")\n" + # Extra-script LINKFLAGS: routed to the link line; in + # target_compile_options they would be silently ineffective + if link_flags := component.data.get(ESPHOME_DATA_KEY, {}).get( + ESPHOME_DATA_LINK_FLAGS_KEY, [] + ): + content += "target_link_options(${COMPONENT_LIB} INTERFACE\n" + for link_flag in link_flags: + content += f" {escape_entry(link_flag)}\n" + content += ")\n" + # Add custom CMake scripts content += "\n".join( component.data.get(ESPHOME_DATA_KEY, {}).get(ESPHOME_DATA_EXTRA_CMAKE_KEY, []) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index 9fa73bafac..8ffd6ee24b 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -17,6 +17,7 @@ import shlex from typing import TYPE_CHECKING, Any from esphome.core import EsphomeError +from esphome.platformio.library import ESPHOME_DATA_KEY, ESPHOME_DATA_LINK_FLAGS_KEY if TYPE_CHECKING: from esphome.platformio.library import ConvertedLibrary @@ -63,6 +64,11 @@ def apply_extra_script( board_mcu=board_mcu(), pio_platform=pio_platform, ) + if link_flags := _str_entries(result.linkflags, "LINKFLAGS"): + # Kept apart from build.flags: the CMake emitters route those to + # target_compile_options, where a link flag is silently ineffective + esphome_data = component.data.setdefault(ESPHOME_DATA_KEY, {}) + esphome_data.setdefault(ESPHOME_DATA_LINK_FLAGS_KEY, []).extend(link_flags) extra_flags = captured_as_build_flags(result, library_dir=source_path) if not extra_flags: return @@ -148,6 +154,12 @@ class _FakeSConsEnv: return self._vars.get(key, "") def Append(self, **kwargs) -> None: # noqa: N802 (SCons API name) + self._add(kwargs, prepend=False) + + def Prepend(self, **kwargs) -> None: # noqa: N802 (SCons API name) + self._add(kwargs, prepend=True) + + def _add(self, kwargs: dict[str, Any], *, prepend: bool) -> None: for key, value in kwargs.items(): if key not in _CAPTURED_KEYS: # Warn once per key so a loop of Appends cannot spam @@ -163,13 +175,16 @@ class _FakeSConsEnv: else: items = list(value) if isinstance(value, (list, tuple)) else [value] bucket = getattr(self.result, key.lower()) - bucket.extend(items) + if prepend: + # SCons order: new values ahead of what is already there + # (scripts prepend LIBS for static-link symbol resolution) + bucket[:0] = items + else: + bucket.extend(items) - # Same keys, same flattened capture; ordering/dedup don't matter since - # the consumer re-orders anyway - Prepend = Append + # Dedup is not modelled; a repeated flag is harmless on the command line AppendUnique = Append - PrependUnique = Append + PrependUnique = Prepend # ----- Everything else is a no-op so unsupported scripts don't crash ----- @@ -263,22 +278,22 @@ def run_extra_script( return env.result +def _str_entries(bucket: list, kind: str) -> list[str]: + # Third-party scripts legally append SCons nodes, ints, or dicts; + # stringifying those into flags would hand the compiler garbage + good = [entry for entry in bucket if isinstance(entry, str)] + for entry in bucket: + if not isinstance(entry, str): + _LOGGER.warning("Ignoring unsupported %s entry %r", kind, entry) + return good + + def captured_as_build_flags( result: ExtraScriptResult, *, library_dir: Path ) -> list[str]: """Translate captured env vars into -L/-l/-D/raw build flags; path entries anchor to ``library_dir`` so the build files stay portable.""" flags: list[str] = [] - - def _strs(bucket: list, kind: str) -> list[str]: - # Third-party scripts legally append SCons nodes, ints, or dicts; - # stringifying those into flags would hand the compiler garbage - good = [entry for entry in bucket if isinstance(entry, str)] - for entry in bucket: - if not isinstance(entry, str): - _LOGGER.warning("Ignoring unsupported %s entry %r", kind, entry) - return good - library_root = library_dir.resolve() def _anchored(path: str) -> str: @@ -292,12 +307,14 @@ def captured_as_build_flags( # shlex.quote so a spaced path survives lex_build_flags as one token flags.extend( - f"-I{shlex.quote(_anchored(path))}" for path in _strs(result.cpppath, "CPPPATH") + f"-I{shlex.quote(_anchored(path))}" + for path in _str_entries(result.cpppath, "CPPPATH") ) flags.extend( - f"-L{shlex.quote(_anchored(path))}" for path in _strs(result.libpath, "LIBPATH") + f"-L{shlex.quote(_anchored(path))}" + for path in _str_entries(result.libpath, "LIBPATH") ) - flags.extend(f"-l{shlex.quote(lib)}" for lib in _strs(result.libs, "LIBS")) + flags.extend(f"-l{shlex.quote(lib)}" for lib in _str_entries(result.libs, "LIBS")) for define in result.cppdefines: # SCons also accepts nested containers; formatting those blind # would hand the compiler garbage like -D{'FOO': '1'} @@ -317,7 +334,8 @@ def captured_as_build_flags( else: _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", define) # Each captured entry is one argv token in SCons; quote so the - # lex_build_flags round-trip cannot split a spaced value into two - flags.extend(shlex.quote(f) for f in _strs(result.linkflags, "LINKFLAGS")) - flags.extend(shlex.quote(f) for f in _strs(result.cppflags, "CPPFLAGS")) + # lex_build_flags round-trip cannot split a spaced value into two. + # LINKFLAGS are deliberately absent: they travel via + # ESPHOME_DATA_LINK_FLAGS_KEY straight to the link line. + flags.extend(shlex.quote(f) for f in _str_entries(result.cppflags, "CPPFLAGS")) return flags diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 963e10c255..6d26fea534 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -72,6 +72,9 @@ DOMAIN = "pio_components" ESPHOME_DATA_KEY = "ESPHOME" ESPHOME_DATA_EXTRA_CMAKE_KEY = "EXTRA_CMAKE" +# Captured extra-script LINKFLAGS; kept apart from build.flags so they reach +# the link line (target_link_options), not target_compile_options +ESPHOME_DATA_LINK_FLAGS_KEY = "LINK_FLAGS" class Source: diff --git a/tests/unit_tests/test_espidf_component.py b/tests/unit_tests/test_espidf_component.py index e2884454e5..5a273b987c 100644 --- a/tests/unit_tests/test_espidf_component.py +++ b/tests/unit_tests/test_espidf_component.py @@ -23,6 +23,8 @@ from esphome.espidf.component import ( ) import esphome.platformio.library from esphome.platformio.library import ( + ESPHOME_DATA_KEY, + ESPHOME_DATA_LINK_FLAGS_KEY, ConvertedLibrary as IDFComponent, GitSource, URLSource, @@ -292,6 +294,25 @@ def test_generate_cmakelists_txt_multi_token_flag(tmp_component): assert ' "-include"\n "cp_custom_alloc.h"\n' in content +def test_generate_cmakelists_txt_extra_script_link_flags(tmp_component): + """Captured extra-script LINKFLAGS come out as target_link_options, not + compile options where they would be silently ineffective.""" + src_dir = tmp_component.path / "src" + src_dir.mkdir() + (src_dir / "main.c").write_text("int main() {}") + + tmp_component.data = { + ESPHOME_DATA_KEY: {ESPHOME_DATA_LINK_FLAGS_KEY: ["-Wl,--gc-sections"]} + } + + content = generate_cmakelists_txt(tmp_component) + assert ( + 'target_link_options(${COMPONENT_LIB} INTERFACE\n "-Wl,--gc-sections"\n)' + in content + ) + assert "target_compile_options" not in content + + def test_generate_cmakelists_txt_space_separated_classified_flags(tmp_component): # Space-separated -I/-L/-l entries routed to INCLUDE_DIRS and the link # handling before the shlex split was added; splitting must not leak diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index ebe61d0d1f..90ae4aa43b 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -18,6 +18,8 @@ from esphome.platformio.extra_script import ( run_extra_script, ) from esphome.platformio.library import ( + ESPHOME_DATA_KEY, + ESPHOME_DATA_LINK_FLAGS_KEY, ConvertedLibrary as IDFComponent, URLSource, lex_build_flags, @@ -62,7 +64,8 @@ def test_extra_script_captures_libpath_libs_and_defines(tmp_path): assert "-lalgobsec" in tokens assert "-DFOO" in tokens assert "-DBAR=1" in tokens - assert "-Wl,--gc-sections" in tokens + # LINKFLAGS travel via the link-flags channel, never the compile flags + assert "-Wl,--gc-sections" not in tokens def test_extra_script_libpath_relative_resolves_against_library_dir( @@ -194,9 +197,11 @@ def test_captured_nonstring_buckets_warn_and_skip(tmp_path, caplog) -> None: apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") flags = c.data["build"]["flags"] - assert "-lm" in flags and "-Wl,-x" in flags and "-Os" in flags + assert "-lm" in flags and "-Os" in flags + assert c.data[ESPHOME_DATA_KEY][ESPHOME_DATA_LINK_FLAGS_KEY] == ["-Wl,-x"] assert not any("42" in f or "no" in f or "3.5" in f for f in flags) assert "Ignoring unsupported LIBS entry 42" in caplog.text + assert "Ignoring unsupported LINKFLAGS entry {'no': 1}" in caplog.text assert "Ignoring unsupported LIBPATH entry 7" in caplog.text @@ -443,6 +448,18 @@ def test_append_variants_capture_like_append(method: str) -> None: assert env.result.libpath == ["lib"] +@pytest.mark.parametrize("method", ("Prepend", "PrependUnique")) +def test_prepend_inserts_ahead_of_existing(method: str) -> None: + """Prepend keeps SCons order: new values land ahead of what is already + captured (scripts prepend LIBS for static-link symbol resolution).""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + env.Append(LIBS=["m"]) + getattr(env, method)(LIBS=["algobsec", "bsec"]) + assert env.result.libs == ["algobsec", "bsec", "m"] + + def test_env_get_unknown_key_warns_once(caplog) -> None: """A script branching on an unmodelled env var is diagnosable.""" env = _FakeSConsEnv( @@ -455,10 +472,9 @@ def test_env_get_unknown_key_warns_once(caplog) -> None: assert "BOARD_MCU" not in caplog.text -def test_spaced_linkflag_survives_relexing(tmp_path) -> None: +def test_spaced_cppflag_survives_relexing(tmp_path) -> None: """A captured argv token with a space stays one token after lexing.""" result = ExtraScriptResult( - linkflags=["-Wl,-T my linker.ld"], cppflags=["-include my hdr.h"], cppdefines=[("MSG", '"hello world"'), "PLAIN"], ) @@ -466,7 +482,6 @@ def test_spaced_linkflag_survives_relexing(tmp_path) -> None: assert lex_build_flags(flags, "test") == [ '-DMSG="hello world"', "-DPLAIN", - "-Wl,-T my linker.ld", "-include my hdr.h", ] From 89671f9e0de12cbf3f26dce9e46341609bd765c7 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 17:15:29 -0500 Subject: [PATCH 07/10] Normalize captured CPPDEFINES to a CppDefine named tuple at capture time --- esphome/platformio/extra_script.py | 67 ++++++++++++------- .../test_platformio_extra_script.py | 7 +- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index 8ffd6ee24b..e60a50d746 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -14,7 +14,7 @@ import logging import os from pathlib import Path import shlex -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, NamedTuple from esphome.core import EsphomeError from esphome.platformio.library import ESPHOME_DATA_KEY, ESPHOME_DATA_LINK_FLAGS_KEY @@ -99,20 +99,47 @@ class ExtraScriptResult: cpppath: list[str] = field(default_factory=list) libpath: list[str] = field(default_factory=list) libs: list[str] = field(default_factory=list) - cppdefines: list[str | tuple[str, str]] = field(default_factory=list) + cppdefines: list[CppDefine] = field(default_factory=list) linkflags: list[str] = field(default_factory=list) cppflags: list[str] = field(default_factory=list) -def _cppdefines_items(value: Any) -> list: - """Normalize SCons ``processDefines`` spellings: a bare 2-tuple is one - ``name=value`` pair, a dict maps names to values, a list is - element-wise.""" +class CppDefine(NamedTuple): + """One normalized CPPDEFINES entry; a ``value`` of None is a bare -DNAME.""" + + name: str + value: str | None = None + + +def _cppdefine(entry: Any) -> CppDefine | None: + """Normalize one CPPDEFINES element, or warn and drop an unsupported + shape; formatting those blind would hand the compiler garbage like + ``-D{'FOO': '1'}``.""" + if isinstance(entry, str): + return CppDefine(entry) + if ( + isinstance(entry, (tuple, list)) + and len(entry) == 2 + and isinstance(entry[0], (str, int)) + and isinstance(entry[1], (str, int, type(None))) + ): + value = entry[1] + return CppDefine(str(entry[0]), None if value is None else str(value)) + _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", entry) + return None + + +def _cppdefines_items(value: Any) -> list[CppDefine]: + """Normalize SCons ``processDefines`` spellings into ``CppDefine``s: a + bare 2-tuple is one ``name=value`` pair, a dict maps names to values, a + list is element-wise.""" if isinstance(value, tuple) and len(value) == 2: - return [value] - if isinstance(value, dict): - return list(value.items()) - return list(value) if isinstance(value, (list, tuple)) else [value] + elements: list[Any] = [value] + elif isinstance(value, dict): + elements = list(value.items()) + else: + elements = list(value) if isinstance(value, (list, tuple)) else [value] + return [d for e in elements if (d := _cppdefine(e)) is not None] class _FakeSConsEnv: @@ -316,23 +343,11 @@ def captured_as_build_flags( ) flags.extend(f"-l{shlex.quote(lib)}" for lib in _str_entries(result.libs, "LIBS")) for define in result.cppdefines: - # SCons also accepts nested containers; formatting those blind - # would hand the compiler garbage like -D{'FOO': '1'} - if ( - isinstance(define, (tuple, list)) - and len(define) == 2 - and isinstance(define[0], (str, int)) - and isinstance(define[1], (str, int, type(None))) - ): - if define[1] is None: - # {"FOO": None} / ("FOO", None) is a bare -DFOO in SCons - flags.append(shlex.quote(f"-D{define[0]}")) - else: - flags.append(shlex.quote(f"-D{define[0]}={define[1]}")) - elif isinstance(define, str): - flags.append(shlex.quote(f"-D{define}")) + if define.value is None: + # {"FOO": None} / ("FOO", None) is a bare -DFOO in SCons + flags.append(shlex.quote(f"-D{define.name}")) else: - _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", define) + flags.append(shlex.quote(f"-D{define.name}={define.value}")) # Each captured entry is one argv token in SCons; quote so the # lex_build_flags round-trip cannot split a spaced value into two. # LINKFLAGS are deliberately absent: they travel via diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index 90ae4aa43b..980ce29ccb 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -11,6 +11,7 @@ import pytest from esphome.core import EsphomeError from esphome.platformio.extra_script import ( + CppDefine, ExtraScriptResult, _FakeSConsEnv, apply_extra_script, @@ -51,8 +52,8 @@ def test_extra_script_captures_libpath_libs_and_defines(tmp_path): assert result.libpath == [str(Path("src") / "esp32")] assert result.libs == ["algobsec"] - assert ("BAR", "1") in result.cppdefines - assert "FOO" in result.cppdefines + assert CppDefine("BAR", "1") in result.cppdefines + assert CppDefine("FOO") in result.cppdefines assert result.linkflags == ["-Wl,--gc-sections"] # Lex like the consumer does: quoting makes raw strings platform-varying @@ -476,7 +477,7 @@ def test_spaced_cppflag_survives_relexing(tmp_path) -> None: """A captured argv token with a space stays one token after lexing.""" result = ExtraScriptResult( cppflags=["-include my hdr.h"], - cppdefines=[("MSG", '"hello world"'), "PLAIN"], + cppdefines=[CppDefine("MSG", '"hello world"'), CppDefine("PLAIN")], ) flags = captured_as_build_flags(result, library_dir=tmp_path) assert lex_build_flags(flags, "test") == [ From 1f5152892c73192f150c90d1e8527ca29179886f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 18:12:14 -0500 Subject: [PATCH 08/10] Give the fake env real membership, widen the manifest gate, route zephyr through the shared lexer --- esphome/components/zephyr/library.py | 7 ++- esphome/espidf/component.py | 6 ++- esphome/platformio/extra_script.py | 11 ++++- esphome/platformio/library.py | 46 +++++++++++-------- tests/unit_tests/test_espidf_component.py | 13 ++++++ .../test_platformio_extra_script.py | 21 +++++++++ tests/unit_tests/test_platformio_library.py | 20 +++++++- tests/unit_tests/test_zephyr_library.py | 16 +++++++ 8 files changed, 117 insertions(+), 23 deletions(-) diff --git a/esphome/components/zephyr/library.py b/esphome/components/zephyr/library.py index 0e6551ccf1..b339ae45b0 100644 --- a/esphome/components/zephyr/library.py +++ b/esphome/components/zephyr/library.py @@ -28,6 +28,7 @@ from esphome.platformio.library import ( collect_filtered_files, convert_libraries, ensure_list, + lex_build_flags, split_list_by_condition, ) @@ -80,7 +81,11 @@ def generate_cmakelists_txt(component: ConvertedLibrary) -> str: build_include_dir = build.get("includeDir", DEFAULT_BUILD_INCLUDE_DIR) build_src_filter = ensure_list(build.get("srcFilter", DEFAULT_BUILD_SRC_FILTER)) - build_flags = ensure_list(build.get("flags", DEFAULT_BUILD_FLAGS)) + # The shared lexer re-glues spaced entries and drops bare/empty + # arguments, same as the espidf emitter + build_flags = lex_build_flags( + build.get("flags", DEFAULT_BUILD_FLAGS), component.name + ) src_files = collect_filtered_files( read_path / Path(build_src_dir), build_src_filter diff --git a/esphome/espidf/component.py b/esphome/espidf/component.py index 4655d1c54d..105413cf44 100644 --- a/esphome/espidf/component.py +++ b/esphome/espidf/component.py @@ -58,8 +58,10 @@ def generate_cmakelists_txt(component: IDFComponent) -> str: """ def escape_entry(p: PathType) -> str: - # In CMakeLists.txt, backslashes need to be escaped - return f'"{str(p)}"'.replace("\\", "\\\\") + # In CMakeLists.txt, backslashes and embedded quotes need escaping + # (a quoted define value reaches here via the shlex round-trip) + escaped = str(p).replace("\\", "\\\\").replace('"', '\\"') + return f'"{escaped}"' def escape_path(p: PathType) -> str: # CMake uses forward slashes for paths on every platform and treats diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index e60a50d746..e04ccd1f55 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -33,7 +33,7 @@ def apply_extra_script( """Run a library's ``extraScript`` and fold its captured env vars into ``build.flags``; ``board_mcu`` is a callable so it resolves lazily.""" extra_script = component.data.get("build", {}).get("extraScript") - if not extra_script: + if extra_script is None or extra_script == "": return if not isinstance(extra_script, str): # A list/dict value would raise an opaque TypeError on the join below @@ -170,6 +170,15 @@ class _FakeSConsEnv: ) return self._vars.get(key, default) + def __contains__(self, key: object) -> bool: + # Without this, "KEY" in env falls back to the legacy sequence + # protocol: __getitem__(0), (1), ... never raises, so it loops + # forever flooding the log + return key in self._vars + + def __iter__(self): + return iter(self._vars) + def __getitem__(self, key: str) -> str: # Scripts also read env["BOARD_MCU"]; an unmodelled subscript # degrades one branch instead of discarding the whole capture diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 6d26fea534..df1d6aa07b 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -656,15 +656,15 @@ def dependency_is_usable( def _valid_dependency_entry(entry: dict, manifest_name: str) -> bool: - """Whether a normalized entry carries a usable name (non-empty string) - and version (string, if present); invalid entries warn naming the - manifest.""" + """Whether a normalized entry carries a usable name (non-empty string), + version (string, if present), and owner (string, if present); invalid + entries warn naming the manifest.""" name = entry.get("name") - if ( - isinstance(name, str) - and name - and ("version" not in entry or isinstance(entry["version"], str)) - ): + owner = entry.get("owner") + name_ok = isinstance(name, str) and name + version_ok = "version" not in entry or isinstance(entry["version"], str) + owner_ok = owner is None or isinstance(owner, str) + if name_ok and version_ok and owner_ok: return True _LOGGER.warning( "Ignoring unrecognized dependency entry %r of %s", entry, manifest_name @@ -683,7 +683,7 @@ def normalize_dependencies( so callers see a uniform list. ``manifest_name`` names the manifest in the warning for entries that cannot be normalized. """ - if not dependencies: + if dependencies is None: return [] if isinstance(dependencies, str): # A plain string is one or more comma-separated names; iterating it @@ -1004,11 +1004,19 @@ def convert_libraries( f"library.properties in {source_dir}" ) - if not isinstance(component.data, dict) or not isinstance( - component.data.get("build", {}), dict - ): - # A bare json.load imposes no shape; every backend dereferences - # data/build, so validate once here and name the library + # A bare json.load imposes no shape; every backend dereferences + # these fields, so validate once here and name the library + malformed = not isinstance(component.data, dict) + if not malformed: + build = component.data.get("build", {}) + malformed = ( + not isinstance(build, dict) + or not isinstance(component.data.get(ESPHOME_DATA_KEY, {}), dict) + or not isinstance(build.get("srcDir", ""), str) + or not isinstance(build.get("includeDir", ""), str) + or not isinstance(build.get("srcFilter", ""), (str, list)) + ) + if malformed: raise EsphomeError(f"Library {key} has a malformed manifest") warn_properties_depends(component.name, component.data) @@ -1018,10 +1026,12 @@ def convert_libraries( # An explicitly requested library fails fast; the routine # cross-platform skip stays at debug, other causes warn if key in top_level_keys: - raise RuntimeError( - f"Requested library {key} is not compatible with " - f"{backend.framework}: {e}" - ) from e + reason = ( + f"is not compatible with {backend.framework}" + if isinstance(e, IncompatiblePlatform) + else "has a malformed manifest" + ) + raise RuntimeError(f"Requested library {key} {reason}: {e}") from e if isinstance(e, IncompatiblePlatform): _LOGGER.debug("Skip incompatible dependency %s: %s", key, str(e)) else: diff --git a/tests/unit_tests/test_espidf_component.py b/tests/unit_tests/test_espidf_component.py index 5a273b987c..0caff8174e 100644 --- a/tests/unit_tests/test_espidf_component.py +++ b/tests/unit_tests/test_espidf_component.py @@ -294,6 +294,19 @@ def test_generate_cmakelists_txt_multi_token_flag(tmp_component): assert ' "-include"\n "cp_custom_alloc.h"\n' in content +def test_generate_cmakelists_txt_escapes_embedded_quotes(tmp_component): + """A define value carrying a literal quote survives into CMake as an + escaped quote, not a prematurely-terminated string.""" + src_dir = tmp_component.path / "src" + src_dir.mkdir() + (src_dir / "main.c").write_text("int main() {}") + # shlex keeps the backslash-escaped quotes as literal characters + tmp_component.data = {"build": {"flags": ['-DMSG=\\"hi\\"']}} + + content = generate_cmakelists_txt(tmp_component) + assert '"-DMSG=\\"hi\\""' in content + + def test_generate_cmakelists_txt_extra_script_link_flags(tmp_component): """Captured extra-script LINKFLAGS come out as target_link_options, not compile options where they would be silently ineffective.""" diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index 980ce29ccb..07f0e78500 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -461,6 +461,27 @@ def test_prepend_inserts_ahead_of_existing(method: str) -> None: assert env.result.libs == ["algobsec", "bsec", "m"] +def test_env_membership_and_iteration(tmp_path) -> None: + """Membership tests and for-loops must use the mapping protocol; the + legacy sequence fallback through __getitem__ would loop forever.""" + env = _FakeSConsEnv( + board_mcu="esp8266", pio_env="esphome_esp8266", pio_platform="espressif8266" + ) + assert "BOARD_MCU" in env + assert "NOPE" not in env + assert sorted(env) == ["BOARD_MCU", "PIOENV", "PIOPLATFORM"] + + +def test_apply_extra_script_non_string_falsey_raises(tmp_path) -> None: + """A falsey non-string extraScript (false, 0, []) is a malformed + manifest, not an absent script.""" + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": False}} + with pytest.raises(EsphomeError, match="must be a string"): + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + def test_env_get_unknown_key_warns_once(caplog) -> None: """A script branching on an unmodelled env var is diagnosable.""" env = _FakeSConsEnv( diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 0f90ad3e84..1f62513b7b 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -614,10 +614,28 @@ def test_normalize_dependencies_forms(caplog) -> None: assert normalize_dependencies({"Foo": ["1.0", "2.0"]}, "libx") == [] assert normalize_dependencies([{"name": "Foo", "version": 1}], "libx") == [] assert caplog.text.count("unrecognized dependency entry") == 7 + # A non-string owner would stringify into a malformed registry name + assert ( + normalize_dependencies( + [{"name": "Foo", "owner": {"bad": 1}, "version": "1.0"}], "libx" + ) + == [] + ) + # A falsey scalar (0, false) is malformed, not an empty list + assert normalize_dependencies(0, "libx") == [] + assert "Ignoring unrecognized dependencies 0 of libx" in caplog.text @pytest.mark.parametrize( - "manifest", [["not", "a", "manifest"], {"name": "A", "build": "src"}] + "manifest", + [ + ["not", "a", "manifest"], + {"name": "A", "build": "src"}, + {"name": "A", "ESPHOME": "yes"}, + {"name": "A", "build": {"srcDir": 123}}, + {"name": "A", "build": {"includeDir": ["inc"]}}, + {"name": "A", "build": {"srcFilter": {"+": "src"}}}, + ], ) def test_convert_libraries_malformed_manifest_raises( tmp_path, monkeypatch, manifest diff --git a/tests/unit_tests/test_zephyr_library.py b/tests/unit_tests/test_zephyr_library.py index b370fe0c47..0d899ec91d 100644 --- a/tests/unit_tests/test_zephyr_library.py +++ b/tests/unit_tests/test_zephyr_library.py @@ -66,6 +66,22 @@ def test_generate_cmakelists_txt_flags_and_includes(tmp_path): assert "-lm" in out +def test_generate_cmakelists_txt_lexes_spaced_flags(tmp_path): + """A spaced -I entry routes to include dirs instead of landing verbatim + in compile options; same shared lexer as the espidf emitter.""" + c = _make_component(tmp_path) + (tmp_path / "src").mkdir() + (tmp_path / "src" / "a.c").write_text("") + (tmp_path / "include").mkdir() + c.data = {"build": {"flags": "-I include -DBAR=1"}} + + out = generate_cmakelists_txt(c) + + assert str((tmp_path / "include").resolve()).replace("\\", "\\\\") in out + assert "-DBAR=1" in out + assert "-I include" not in out + + def test_generate_zephyr_modules_collects_all_dirs_and_writes(tmp_path, monkeypatch): # Two converted libraries: one top-level, one transitive dependency. The # converter calls backend.emit for both; generate_zephyr_modules must return From d80d2c7034b47f0433455d53decfcb2f93534ebc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 18:20:43 -0500 Subject: [PATCH 09/10] Tolerate malformed repository/description metadata instead of crashing --- esphome/espidf/component.py | 11 ++++++++--- tests/unit_tests/test_espidf_component.py | 12 ++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/esphome/espidf/component.py b/esphome/espidf/component.py index 105413cf44..bed999656b 100644 --- a/esphome/espidf/component.py +++ b/esphome/espidf/component.py @@ -240,12 +240,17 @@ def generate_idf_component_yml(component: IDFComponent) -> str: data = {} + # Metadata only: tolerate malformed shapes instead of crashing on a + # third-party manifest (repository may legally be {"url": ...} or a + # plain URL string) description = component.data.get("description") - if description: + if isinstance(description, str) and description: data["description"] = description - repository = component.data.get("repository", {}).get("url", None) - if repository: + repository = component.data.get("repository") + if isinstance(repository, dict): + repository = repository.get("url") + if isinstance(repository, str) and repository: data["repository"] = repository for dependency in component.dependencies: diff --git a/tests/unit_tests/test_espidf_component.py b/tests/unit_tests/test_espidf_component.py index 0caff8174e..7b4f848979 100644 --- a/tests/unit_tests/test_espidf_component.py +++ b/tests/unit_tests/test_espidf_component.py @@ -374,6 +374,18 @@ def test_generate_idf_component_yml_basic(tmp_component): assert result == "description: test\nrepository: http://aaa\n" +def test_generate_idf_component_yml_tolerates_malformed_metadata(tmp_component): + """A string repository is the URL itself; junk shapes drop instead of + crashing on a third-party manifest.""" + tmp_component.data = {"description": "test", "repository": "http://aaa"} + assert ( + generate_idf_component_yml(tmp_component) + == "description: test\nrepository: http://aaa\n" + ) + tmp_component.data = {"description": {"en": "x"}, "repository": 123} + assert generate_idf_component_yml(tmp_component) == "{}\n" + + def test_generate_idf_component_yml_with_dependencies(tmp_component, tmp_path): dep = IDFComponent("dep", "1.0", source=URLSource("http://dummy.com")) dep.path = tmp_path / "dep" From 5f2b8f72515fa35e584927bdb223a45bae695826 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 19:30:02 -0500 Subject: [PATCH 10/10] Type LINK_FLAGS in the gate, skip malformed transitive deps, escape quotes in zephyr CMake entries --- esphome/components/zephyr/library.py | 12 ++++++----- esphome/platformio/library.py | 17 +++++++++++++-- tests/unit_tests/test_platformio_library.py | 24 +++++++++++++++++++++ tests/unit_tests/test_zephyr_library.py | 12 +++++++++++ 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/esphome/components/zephyr/library.py b/esphome/components/zephyr/library.py index b339ae45b0..d09f0afedb 100644 --- a/esphome/components/zephyr/library.py +++ b/esphome/components/zephyr/library.py @@ -39,11 +39,13 @@ ZEPHYR_FRAMEWORK = "zephyr" def _escape(p: PathType) -> str: - # In CMakeLists.txt, backslashes need to be escaped (mirrors the ESP-IDF - # backend's escape_entry). Doubling -- rather than rewriting '\' -> '/' -- - # preserves content, so it's safe for arbitrary build flags (e.g. a -D value - # containing a backslash) as well as Windows paths. - return f'"{str(p)}"'.replace("\\", "\\\\") + # In CMakeLists.txt, backslashes and embedded quotes need escaping + # (mirrors the ESP-IDF backend's escape_entry; the lex round-trip makes + # a literal quote in a -D value reachable). Doubling backslashes -- + # rather than rewriting '\' -> '/' -- preserves content, so it's safe + # for arbitrary build flags as well as Windows paths. + escaped = str(p).replace("\\", "\\\\").replace('"', '\\"') + return f'"{escaped}"' def generate_module_yml(component: ConvertedLibrary) -> str: diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index df1d6aa07b..a3899fa860 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -682,6 +682,10 @@ def normalize_dependencies( list, and a plain (possibly comma-separated) string; normalize them all so callers see a uniform list. ``manifest_name`` names the manifest in the warning for entries that cannot be normalized. + + Bare-name spellings carry no version; the dependency walk later drops + version-less entries at DEBUG, since they are usually names of bundled + framework libraries (Wire, SPI) that need no registry install. """ if dependencies is None: return [] @@ -1009,15 +1013,24 @@ def convert_libraries( malformed = not isinstance(component.data, dict) if not malformed: build = component.data.get("build", {}) + esphome_data = component.data.get(ESPHOME_DATA_KEY, {}) malformed = ( not isinstance(build, dict) - or not isinstance(component.data.get(ESPHOME_DATA_KEY, {}), dict) + or not isinstance(esphome_data, dict) + or not isinstance( + esphome_data.get(ESPHOME_DATA_LINK_FLAGS_KEY, []), list + ) or not isinstance(build.get("srcDir", ""), str) or not isinstance(build.get("includeDir", ""), str) or not isinstance(build.get("srcFilter", ""), (str, list)) ) if malformed: - raise EsphomeError(f"Library {key} has a malformed manifest") + # Fail fast only for a library the user asked for; a defect in + # an unrequested corner of the graph must not block the build + if key in top_level_keys: + raise EsphomeError(f"Library {key} has a malformed manifest") + _LOGGER.warning("Skipping dependency %s: malformed manifest", key) + continue warn_properties_depends(component.name, component.data) try: diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 1f62513b7b..bf8340cea0 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -635,6 +635,7 @@ def test_normalize_dependencies_forms(caplog) -> None: {"name": "A", "build": {"srcDir": 123}}, {"name": "A", "build": {"includeDir": ["inc"]}}, {"name": "A", "build": {"srcFilter": {"+": "src"}}}, + {"name": "A", "ESPHOME": {"LINK_FLAGS": "-Wl,-x"}}, ], ) def test_convert_libraries_malformed_manifest_raises( @@ -647,6 +648,29 @@ def test_convert_libraries_malformed_manifest_raises( convert_libraries([Library("esphome/A", None, None)], _backend()) +def test_convert_libraries_malformed_transitive_dep_skips( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """A malformed manifest on a dependency the user never asked for warns + and skips; only a top-level library fails the build.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "B", "owner": "esphome", "version": "1.0"}], + }, + "esphome/B": {"name": "B", "build": {"srcDir": 123}}, + }, + ) + components = convert_libraries([Library("esphome/A", None, None)], _backend()) + names = [c.name for c in components] + assert "esphome/A" in names + assert "esphome/B" not in names + assert "Skipping dependency esphome/B: malformed manifest" in caplog.text + + def test_walk_warns_for_properties_only_depends( tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture ) -> None: diff --git a/tests/unit_tests/test_zephyr_library.py b/tests/unit_tests/test_zephyr_library.py index 0d899ec91d..765e505c38 100644 --- a/tests/unit_tests/test_zephyr_library.py +++ b/tests/unit_tests/test_zephyr_library.py @@ -66,6 +66,18 @@ def test_generate_cmakelists_txt_flags_and_includes(tmp_path): assert "-lm" in out +def test_generate_cmakelists_txt_escapes_embedded_quotes(tmp_path): + """A define value carrying a literal quote (reachable via the lex + round-trip) survives as an escaped quote, not a broken CMake string.""" + c = _make_component(tmp_path) + (tmp_path / "src").mkdir() + (tmp_path / "src" / "a.c").write_text("") + c.data = {"build": {"flags": ['-DMSG=\\"hi\\"']}} + + out = generate_cmakelists_txt(c) + assert '"-DMSG=\\"hi\\""' in out + + def test_generate_cmakelists_txt_lexes_spaced_flags(tmp_path): """A spaced -I entry routes to include dirs instead of landing verbatim in compile options; same shared lexer as the espidf emitter."""