From dada0f2c2b300d2226e4c970432be757f7cfce9e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 21:58:41 -0500 Subject: [PATCH 1/3] [core] Extract the shared idedata and size-summary helpers into build_helpers --- esphome/__main__.py | 15 +- esphome/build_helpers/__init__.py | 1 + esphome/{espidf => build_helpers}/idedata.py | 146 ++++- esphome/build_helpers/size_summary.py | 24 + esphome/espidf/clang_tidy.py | 8 +- esphome/espidf/size_summary.py | 37 +- esphome/espidf/toolchain.py | 33 +- script/determine-jobs.py | 16 +- tests/script/test_determine_jobs.py | 8 +- tests/unit_tests/__init__.py | 0 .../analyze_memory/test_build_artifacts.py | 2 +- tests/unit_tests/build_gen/__init__.py | 0 tests/unit_tests/build_helpers/__init__.py | 0 .../unit_tests/build_helpers/test_idedata.py | 549 ++++++++++++++++++ .../build_helpers/test_size_summary.py | 22 + tests/unit_tests/test_core.py | 2 +- tests/unit_tests/test_espidf_clang_tidy.py | 35 ++ tests/unit_tests/test_espidf_idedata.py | 264 --------- tests/unit_tests/test_espidf_toolchain.py | 112 +--- tests/unit_tests/test_main.py | 104 +++- tests/unit_tests/test_size_summary.py | 67 +++ 21 files changed, 973 insertions(+), 472 deletions(-) create mode 100644 esphome/build_helpers/__init__.py rename esphome/{espidf => build_helpers}/idedata.py (61%) create mode 100644 esphome/build_helpers/size_summary.py create mode 100644 tests/unit_tests/__init__.py create mode 100644 tests/unit_tests/build_gen/__init__.py create mode 100644 tests/unit_tests/build_helpers/__init__.py create mode 100644 tests/unit_tests/build_helpers/test_idedata.py create mode 100644 tests/unit_tests/build_helpers/test_size_summary.py delete mode 100644 tests/unit_tests/test_espidf_idedata.py diff --git a/esphome/__main__.py b/esphome/__main__.py index 769b66ecc8..0da86b3ec0 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -857,7 +857,20 @@ def compile_program(args: ArgsProtocol, config: ConfigType) -> int: toolchain.create_factory_bin() toolchain.create_ota_bin() toolchain.create_elf_copy() - toolchain.get_idedata() + from esphome.build_helpers.idedata import IDEDATA_BEST_EFFORT_ERRORS + + try: + if toolchain.get_idedata() is None: + _LOGGER.warning("No idedata was generated for this build") + except IDEDATA_BEST_EFFORT_ERRORS as err: + # The firmware already built; an idedata failure must not fail + # a successful build. + _LOGGER.warning( + "Could not generate idedata: %s (IDE, clang-tidy, and " + "memory-analysis data will be unavailable for this build)", + err, + ) + _LOGGER.debug("Idedata failure detail", exc_info=True) else: from esphome.platformio import toolchain diff --git a/esphome/build_helpers/__init__.py b/esphome/build_helpers/__init__.py new file mode 100644 index 0000000000..df956a2509 --- /dev/null +++ b/esphome/build_helpers/__init__.py @@ -0,0 +1 @@ +"""Build helpers shared by the native (non-PlatformIO) toolchains.""" diff --git a/esphome/espidf/idedata.py b/esphome/build_helpers/idedata.py similarity index 61% rename from esphome/espidf/idedata.py rename to esphome/build_helpers/idedata.py index 0047d568e2..efee461a0a 100644 --- a/esphome/espidf/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -1,10 +1,10 @@ -"""Derive idedata from an ESP-IDF native-toolchain ``compile_commands.json``. +"""Derive idedata from a native (non-PlatformIO) build's ``compile_commands.json``. -PlatformIO exposes a curated ``pio run -t idedata`` JSON; the native ESP-IDF -toolchain has no such command, but its CMake build emits -``build/compile_commands.json`` (CMAKE_EXPORT_COMPILE_COMMANDS). This module -turns that file into the same fields consumers (IDE integration, clang-tidy) -expect: +PlatformIO exposes a curated ``pio run -t idedata`` JSON; the native +toolchains have no such command, but each build produces a +``compile_commands.json`` (CMAKE_EXPORT_COMPILE_COMMANDS for ESP-IDF, ninja's +compdb tool otherwise). This module turns that file into the same fields +consumers (IDE integration, clang-tidy) expect: {cc_path, cxx_path, cxx_flags, defines, includes: {build, toolchain}} """ @@ -18,6 +18,20 @@ from pathlib import Path import shlex import subprocess +from esphome.core import EsphomeError +from esphome.helpers import write_file + +# Everything idedata generation may raise after a successful link. Broad on +# purpose, and shared by every consumer: idedata is a bonus artifact, so +# these must be caught and warned about, never allowed to fail the build. +IDEDATA_BEST_EFFORT_ERRORS = ( + EsphomeError, + LookupError, + OSError, + RuntimeError, + ValueError, +) + _LOGGER = logging.getLogger(__name__) # C++ translation-unit suffixes used to identify ESPHome source files. @@ -120,7 +134,18 @@ def _pick_entry(entries: list[dict]) -> dict: raise ValueError("no C++ translation unit found in compile_commands.json") -def _parse_entry(entry: dict) -> tuple[str, list[str], list[str], list[str]]: +# Compiler launchers that may prefix a compile command; a closed launcher +# denylist beats enumerating compiler names, an open set. +_LAUNCHER_STEMS = frozenset({"ccache", "sccache", "distcc", "icecc", "buildcache"}) + + +def _is_launcher(token: str) -> bool: + return Path(token).stem.lower() in _LAUNCHER_STEMS + + +def parse_entry( + entry: dict, launcher: str | None = None +) -> tuple[str, list[str], list[str], list[str]]: """Parse one compile_commands entry -> (cxx_path, defines, includes, cxx_flags).""" directory = Path(entry["directory"]) tokens = _expand_response_files(_split_command(entry["command"]), directory) @@ -136,6 +161,14 @@ def _parse_entry(entry: dict) -> tuple[str, list[str], list[str], list[str]]: raw = os.path.normpath(directory / raw) return raw.replace("\\", "/") + # A launcher-wrapped command ("ccache g++ ...") names the compiler second + if launcher is not None and tokens[0] == launcher: + tokens = tokens[1:] + if _is_launcher(tokens[0]) and len(tokens) > 1 and not tokens[1].startswith("-"): + # A stale compile DB built with a launcher the current run no longer + # configures: the real compiler is the next token. + _LOGGER.debug("Stripping unconfigured launcher %s", tokens[0]) + tokens = tokens[1:] # token0 is the compiler path; the rest of the command already uses forward # slashes on Windows, so normalize it too for a consistent idedata file. cxx_path = tokens[0].replace("\\", "/") @@ -168,7 +201,7 @@ def _parse_entry(entry: dict) -> tuple[str, list[str], list[str], list[str]]: return cxx_path, defines, includes, cxx_flags -def _get_toolchain_includes(cxx_path: str) -> list[str]: +def get_toolchain_includes(cxx_path: str) -> list[str]: """Query the compiler for its builtin ``#include <...>`` search dirs.""" result = subprocess.run( [cxx_path, "-E", "-x", "c++", "-", "-v"], @@ -219,26 +252,99 @@ def _cc_path_from_cxx(cxx_path: str) -> str: return f"{stem}{suffix}" -def idedata_from_build(compile_commands: Path) -> dict: +def load_or_build_idedata( + compile_commands: Path, + elf_path: Path, + cache: Path, + launcher: str | None = None, +) -> dict | None: + """Return idedata for a compile_commands.json build, cached on mtime. + + Shared by the native ESP-IDF and ESP8266 Arduino toolchains. Returns None + when the compile DB doesn't exist yet (nothing was built). ``launcher`` + is the compiler-launcher path (ccache) the build was generated with, if + any; commands in the compile DB are prefixed with it. + """ + if not compile_commands.is_file(): + _LOGGER.debug("No %s yet; skipping idedata generation", compile_commands) + return None + + if cache.is_file() and cache.stat().st_mtime >= compile_commands.stat().st_mtime: + try: + cached = json.loads(cache.read_text(encoding="utf-8")) + except (ValueError, OSError) as err: + # A recurring cause (interrupted write, disk full) would otherwise + # look like unexplained slow builds + _LOGGER.warning("Discarding unreadable idedata cache %s: %s", cache, err) + else: + # Rebuild pre-cc_path caches on the field, not the timestamp; + # the type check keeps "in" from substring-matching a string + if isinstance(cached, dict) and "cc_path" in cached: + return cached + + data = idedata_from_build(compile_commands, launcher) + data["prog_path"] = str(elf_path) + cache.parent.mkdir(parents=True, exist_ok=True) + # Atomic so a crash mid-write cannot leave a truncated cache + write_file(cache, json.dumps(data, indent=2) + "\n") + return data + + +def idedata_from_build(compile_commands: Path, launcher: str | None = None) -> dict: """Parse compile_commands.json into the idedata fields consumers expect. - A single ESP-IDF compile entry only carries its own component's REQUIRES - include set, but consumers (clang-tidy) analyze ESPHome headers that - transitively pull in other components. So take cxx_path / cxx_flags / - defines from a representative ESPHome TU, but union the include dirs across - all ESPHome TUs to get a project-wide superset (as PlatformIO's idedata - provides). + A single compile entry only carries the include set its own translation + unit was built with (per-component under ESP-IDF), but consumers + (clang-tidy) analyze ESPHome headers that transitively pull in other + components. So take cxx_path / cxx_flags / defines from a representative + ESPHome TU, but union the include dirs across all ESPHome TUs to get a + project-wide superset (as PlatformIO's idedata provides). """ entries = json.loads(Path(compile_commands).read_text(encoding="utf-8")) - cxx_path, defines, _, cxx_flags = _parse_entry(_pick_entry(entries)) - build_includes: dict[str, None] = {} + representative = _pick_entry(entries) + cxx_path, defines, rep_includes, cxx_flags = parse_entry(representative, launcher) + if _is_launcher(cxx_path): + # Reject before the toolchain probe, which would fail opaquely on + # a launcher; never cache the unusable compile DB + raise EsphomeError( + f"compile_commands.json names the launcher {cxx_path} as the " + "compiler; the compile database is unusable" + ) + + # Seed with the representative's includes so it is not parsed twice + build_includes: dict[str, None] = dict.fromkeys( + rep_includes if _is_esphome_src(representative["file"]) else () + ) + + def _shape(entry: dict) -> str: + # The command minus its TU-specific paths: entries sharing a shape + # carry identical include sets (one ninja rule), so tokenize once + # per shape instead of once per TU + return ( + entry["command"] + .replace(entry.get("file", ""), "") + .replace(entry.get("output", ""), "") + ) + + seen_shapes = {_shape(representative)} for entry in entries: - if not _is_esphome_src(entry["file"]): + if entry is representative or not _is_esphome_src(entry["file"]): continue - for inc in _parse_entry(entry)[2]: + if (shape := _shape(entry)) in seen_shapes: + continue + seen_shapes.add(shape) + for inc in parse_entry(entry, launcher)[2]: build_includes.setdefault(inc, None) + if not build_includes: + # No ESPHome translation unit contributed includes: idedata with an + # empty build include set breaks clang-tidy/IDE consumers silently + _LOGGER.warning( + "No ESPHome source includes found in %s; idedata will be incomplete", + compile_commands, + ) + return { "cc_path": _cc_path_from_cxx(cxx_path), "cxx_path": cxx_path, @@ -246,6 +352,6 @@ def idedata_from_build(compile_commands: Path) -> dict: "defines": defines, "includes": { "build": list(build_includes), - "toolchain": _get_toolchain_includes(cxx_path), + "toolchain": get_toolchain_includes(cxx_path), }, } diff --git a/esphome/build_helpers/size_summary.py b/esphome/build_helpers/size_summary.py new file mode 100644 index 0000000000..b888111044 --- /dev/null +++ b/esphome/build_helpers/size_summary.py @@ -0,0 +1,24 @@ +"""The PlatformIO-format size bar shared by the native toolchains.""" + +from __future__ import annotations + + +def format_bar(used: int, total: int) -> str: + """Match PlatformIO's ``_format_availale_bytes`` (sic, pioupload.py) exactly.""" + pct_raw = used / total if total else 0 + blocks = 10 + filled = min(int(round(blocks * pct_raw)), blocks) + progress = "=" * filled + return ( + f"[{progress:<{blocks}}] {pct_raw: 6.1%} " + f"(used {used:d} bytes from {total:d} bytes)" + ) + + +def print_size_line(label: str, used: int, total: int) -> None: + """One PlatformIO-format summary line (``RAM``/``Flash``). + + The label padding is part of the format: ``script/ci_memory_impact_extract.py`` + matches these lines verbatim. + """ + print(f"{label + ':':<7}{format_bar(used, total)}") diff --git a/esphome/espidf/clang_tidy.py b/esphome/espidf/clang_tidy.py index c91db775a3..61ce94fc9d 100644 --- a/esphome/espidf/clang_tidy.py +++ b/esphome/espidf/clang_tidy.py @@ -23,6 +23,8 @@ from dataclasses import dataclass import os from pathlib import Path +from esphome.build_helpers.idedata import get_toolchain_includes, parse_entry + TIDY_PROJECT_NAME = "esphome_tidy" # A do-nothing C++ app: just enough for IDF to configure a valid project. It's @@ -415,13 +417,11 @@ def _idedata_from_tidy_project(compile_commands: Path) -> dict: """ import json - from esphome.espidf.idedata import _get_toolchain_includes, _parse_entry - entries = json.loads(Path(compile_commands).read_text(encoding="utf-8")) entry = next((e for e in entries if e["file"].endswith("tidy.cpp")), None) if entry is None: raise RuntimeError(f"tidy.cpp not found in {compile_commands}") - cxx_path, defines, includes, cxx_flags = _parse_entry(entry) + cxx_path, defines, includes, cxx_flags = parse_entry(entry) return { "cxx_path": cxx_path, @@ -429,7 +429,7 @@ def _idedata_from_tidy_project(compile_commands: Path) -> dict: "defines": defines, "includes": { "build": includes, - "toolchain": _get_toolchain_includes(cxx_path), + "toolchain": get_toolchain_includes(cxx_path), }, } diff --git a/esphome/espidf/size_summary.py b/esphome/espidf/size_summary.py index 7a5305ff0c..83791ce797 100644 --- a/esphome/espidf/size_summary.py +++ b/esphome/espidf/size_summary.py @@ -28,6 +28,8 @@ import json import logging from pathlib import Path +from esphome.build_helpers.size_summary import print_size_line + _LOGGER = logging.getLogger(__name__) _SIZE_SUFFIXES = {"K": 1024, "M": 1024 * 1024} @@ -67,31 +69,20 @@ def _find_app_partition_size(partitions_csv: Path) -> int: raise ValueError(f"No app+factory or app+ota_0 partition in {partitions_csv}") -def _format_bar(used: int, total: int) -> str: - """Match PlatformIO's ``_format_availale_bytes`` (pioupload.py) exactly.""" - pct_raw = used / total if total else 0 - blocks = 10 - filled = min(int(round(blocks * pct_raw)), blocks) - progress = "=" * filled - return ( - f"[{progress:<{blocks}}] {pct_raw: 6.1%} " - f"(used {used:d} bytes from {total:d} bytes)" - ) - - def print_summary(size_json: Path, partitions_csv: Path | None) -> None: """Print PlatformIO-shaped RAM and Flash one-liners. Failures are non-fatal: the build has already succeeded, we just couldn't - summarize. Logs the cause at debug level. + summarize. Logs the cause at warning level, so a missing RAM/Flash line + (which CI's memory-impact extraction greps for) is diagnosable. """ if not size_json.is_file(): - _LOGGER.debug("Skipping size summary: %s not found", size_json) + _LOGGER.warning("Skipping size summary: %s not found", size_json) return try: data = json.loads(size_json.read_text(encoding="utf-8")) except (OSError, json.JSONDecodeError) as e: - _LOGGER.debug("Skipping size summary: %s", e) + _LOGGER.warning("Skipping size summary: %s", e) return memory_types = data.get("memory_types", {}) @@ -99,14 +90,22 @@ def print_summary(size_json: Path, partitions_csv: Path | None) -> None: ram_used = ram_region.get("used") ram_total = ram_region.get("size") if ram_total and ram_used is not None: - print(f"RAM: {_format_bar(ram_used, ram_total)}") + print_size_line("RAM", ram_used, ram_total) + else: + _LOGGER.warning( + "Skipping RAM summary: no usable DRAM/DIRAM region in %s", size_json + ) image_size = data.get("image_size") - if image_size is None or partitions_csv is None: + if image_size is None: + _LOGGER.warning("Skipping Flash summary: no image_size in %s", size_json) + return + if partitions_csv is None: + _LOGGER.warning("Skipping Flash summary: no partition table given") return try: app_size = _find_app_partition_size(partitions_csv) except ValueError as e: - _LOGGER.debug("Skipping Flash summary: %s", e) + _LOGGER.warning("Skipping Flash summary: %s", e) return - print(f"Flash: {_format_bar(image_size, app_size)}") + print_size_line("Flash", image_size, app_size) diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 07ba03e2cf..3c5c4803c2 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -526,32 +526,15 @@ def get_idedata() -> dict | None: idedata fields IDE integrations and clang-tidy expect, cached alongside the PlatformIO idedata path. Returns None if the compile DB doesn't exist yet. """ - from esphome.espidf.idedata import idedata_from_build + from esphome.build_helpers.idedata import load_or_build_idedata - compile_commands = CORE.relative_build_path("build", "compile_commands.json") - if not compile_commands.is_file(): - _LOGGER.debug("No %s yet; skipping idedata generation", compile_commands) - return None - - cache = CORE.relative_internal_path("idedata", f"{CORE.name}.json") - if cache.is_file() and cache.stat().st_mtime >= compile_commands.stat().st_mtime: - try: - cached = json.loads(cache.read_text(encoding="utf-8")) - except ValueError: - pass - else: - # Caches written before cc_path was emitted stay newer than - # compile_commands.json forever, so rebuild them on the field rather - # than on the timestamp. Check the type too: a corrupted cache can - # still be valid JSON, and "in" would match a substring of a string. - if isinstance(cached, dict) and "cc_path" in cached: - return cached - - data = idedata_from_build(compile_commands) - data["prog_path"] = str(get_elf_path()) - cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") - return data + # No launcher: CMake excludes CMAKE__COMPILER_LAUNCHER (ccache) + # from the exported compile database, unlike ninja's compdb dump. + return load_or_build_idedata( + CORE.relative_build_path("build", "compile_commands.json"), + get_elf_path(), + CORE.relative_internal_path("idedata", f"{CORE.name}.json"), + ) def create_factory_bin() -> bool: diff --git a/script/determine-jobs.py b/script/determine-jobs.py index e4d002975c..3e11deeb9a 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -525,13 +525,21 @@ def _esp32_platformio_path_or_file_trigger(files: list[str]) -> bool: return False -# ESP-IDF infra: changes under esphome/espidf/ or to the IDF build generator -# affect every esp32 IDF build (now the default toolchain) but aren't +# Native-build infra: changes under esphome/espidf/, the shared +# esphome/build_helpers/ package, or the modules the native ESP-IDF build +# imports affect every esp32 IDF build (now the default toolchain) but aren't # components, so the component matrix wouldn't otherwise force any esp32 # compile. When they change we fold the `esp32` component into the matrix so # the default native-IDF build path is still compiled on an infra-only PR. -ESP_IDF_INFRA_TRIGGER_PATH_PREFIXES = ("esphome/espidf/",) -ESP_IDF_INFRA_TRIGGER_FILES = frozenset({"esphome/build_gen/espidf.py"}) +ESP_IDF_INFRA_TRIGGER_PATH_PREFIXES = ("esphome/espidf/", "esphome/build_helpers/") +ESP_IDF_INFRA_TRIGGER_FILES = frozenset( + { + "esphome/build_gen/espidf.py", + "esphome/framework_helpers.py", + "esphome/platformio/library.py", + "esphome/platformio/extra_script.py", + } +) def _esp_idf_infra_changed(files: list[str]) -> bool: diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index 80f572d9fe..297752b3ac 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -1120,7 +1120,13 @@ def test_should_run_esp32_platformio_with_branch() -> None: (["esphome/espidf/runner.py"], True), (["esphome/espidf/framework.py"], True), (["esphome/build_gen/espidf.py"], True), - # PlatformIO build gen and esp32 component are NOT IDF-infra triggers + # Shared native-build modules the IDF build imports -> trigger + (["esphome/build_helpers/idedata.py"], True), + (["esphome/platformio/library.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), (["esphome/build_gen/platformio.py"], False), (["esphome/components/esp32/__init__.py"], False), (["README.md"], False), diff --git a/tests/unit_tests/__init__.py b/tests/unit_tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unit_tests/analyze_memory/test_build_artifacts.py b/tests/unit_tests/analyze_memory/test_build_artifacts.py index d97ee94e1c..ed37858b86 100644 --- a/tests/unit_tests/analyze_memory/test_build_artifacts.py +++ b/tests/unit_tests/analyze_memory/test_build_artifacts.py @@ -9,7 +9,7 @@ from esphome.analyze_memory.toolchain import ( find_idedata_path, idedata_candidates, ) -from esphome.espidf.idedata import _cc_path_from_cxx +from esphome.build_helpers.idedata import _cc_path_from_cxx from esphome.platformio.toolchain import IDEData diff --git a/tests/unit_tests/build_gen/__init__.py b/tests/unit_tests/build_gen/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unit_tests/build_helpers/__init__.py b/tests/unit_tests/build_helpers/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unit_tests/build_helpers/test_idedata.py b/tests/unit_tests/build_helpers/test_idedata.py new file mode 100644 index 0000000000..2998d46ec3 --- /dev/null +++ b/tests/unit_tests/build_helpers/test_idedata.py @@ -0,0 +1,549 @@ +"""Tests for esphome.build_helpers.idedata (compile_commands.json -> idedata).""" + +# pylint: disable=protected-access + +import json +import logging +import os +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from esphome.build_helpers import idedata +from esphome.core import EsphomeError + +# An absolute, forward-slash (shlex-safe) path prefix valid on the host OS, so +# tests exercise the same is-absolute / normalize behavior as a real compile DB +# (a drive-qualified path on Windows, a leading slash elsewhere). +ABS = "C:/" if os.name == "nt" else "/" + + +def _entry(directory: str, file: str, command: str) -> dict: + return {"directory": directory, "file": file, "command": command} + + +def test_parse_entry_extracts_fields() -> None: + """cxx_path, defines, includes and remaining flags are split apart.""" + entry = _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/core/application.cpp", + f"/tools/xtensa-esp32-elf-g++ -DUSE_ESP32 -DESPHOME_LOG_LEVEL=5 " + f"-I{ABS}inc/a -isystem {ABS}sys/b -std=gnu++20 -c app.cpp -o app.cpp.o", + ) + + cxx_path, defines, includes, cxx_flags = idedata.parse_entry(entry) + + assert cxx_path == "/tools/xtensa-esp32-elf-g++" + assert "USE_ESP32" in defines + assert "ESPHOME_LOG_LEVEL=5" in defines + assert f"{ABS}inc/a" in includes + assert f"{ABS}sys/b" in includes + assert "-std=gnu++20" in cxx_flags + # input/output files and their flags are not treated as flags + assert "-c" not in cxx_flags + assert "-o" not in cxx_flags + assert "app.cpp" not in cxx_flags + assert "app.cpp.o" not in cxx_flags + + +def test_parse_entry_space_separated_args() -> None: + """``-D X`` / ``-I path`` (separate arg) and ``-isystem`` (joined).""" + entry = _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/x.cpp", + f"g++ -D FOO=1 -I {ABS}inc/sep -isystem{ABS}sys/joined -c x.cpp", + ) + + _, defines, includes, _ = idedata.parse_entry(entry) + + assert "FOO=1" in defines + assert f"{ABS}inc/sep" in includes + assert f"{ABS}sys/joined" in includes + + +def test_parse_entry_resolves_relative_includes() -> None: + """Relative includes are resolved against the entry's ``directory``.""" + directory = f"{ABS}build/proj" + entry = _entry( + directory, + f"{directory}/src/esphome/x.cpp", + "g++ -Iconfig -I../shared -isystem rel/sys -c x.cpp", + ) + + _, _, includes, _ = idedata.parse_entry(entry) + + def resolved(rel: str) -> str: + # parse_entry emits forward slashes for consistency (normpath would + # yield backslashes on Windows). + return os.path.normpath(Path(directory) / rel).replace("\\", "/") + + assert resolved("config") in includes + assert resolved("../shared") in includes # ../ normalized away + assert resolved("rel/sys") in includes + # nothing is left relative + assert all(Path(inc).is_absolute() for inc in includes) + + +def test_parse_entry_skips_dependency_flags() -> None: + """Dependency-generation flags (and their args) are dropped.""" + entry = _entry( + "/build", + "/build/src/esphome/x.cpp", + "g++ -MD -MT x.cpp.o -MF x.cpp.o.d -c x.cpp -o x.cpp.o", + ) + + _, _, _, cxx_flags = idedata.parse_entry(entry) + + for tok in ("-MD", "-MT", "x.cpp.o", "-MF", "x.cpp.o.d", "-c", "-o", "x.cpp"): + assert tok not in cxx_flags + + +def test_expand_response_files(tmp_path: Path) -> None: + """``@file`` arguments are inlined relative to the directory.""" + rsp = tmp_path / "flags.rsp" + rsp.write_text("-DFROM_RSP -I/rsp/inc") + + tokens = idedata._expand_response_files( + ["g++", f"@{rsp.name}", "-c", "x.cpp"], tmp_path + ) + + assert "-DFROM_RSP" in tokens + assert "-I/rsp/inc" in tokens + assert not any(t.startswith("@") for t in tokens) + + +def test_expand_response_files_keeps_literal_when_missing(tmp_path: Path) -> None: + """An unreadable ``@file`` token is kept verbatim rather than dropped.""" + tokens = idedata._expand_response_files(["g++", "@nope.rsp"], tmp_path) + assert "@nope.rsp" in tokens + + +def test_pick_entry_prefers_esphome_tu() -> None: + """A ``/src/esphome/`` C++ TU is picked over other compile entries.""" + entries = [ + _entry("/b", "/b/managed_components/foo/foo.c", "gcc -c foo.c"), + _entry("/b", "/b/src/esphome/core/app.cpp", "g++ -c app.cpp"), + ] + assert idedata._pick_entry(entries)["file"].endswith("app.cpp") + + +def test_pick_entry_falls_back_to_any_cxx_tu() -> None: + """With no ``/src/esphome/`` TU present, the first C++ entry is the fallback.""" + entries = [ + _entry("/b", "/b/managed_components/foo/foo.c", "gcc -c foo.c"), + _entry("/b", "/b/components/x/x.cpp", "g++ -c x.cpp"), + ] + assert idedata._pick_entry(entries)["file"].endswith("x.cpp") + + +def test_is_esphome_src_handles_backslash_paths() -> None: + r"""The src marker must match Windows ``\src\esphome\`` paths too. + + compile_commands ``file`` entries use the OS-native separator; if the + marker only matched forward slashes no source would match on Windows and + the build-include union would be silently empty. + """ + assert idedata._is_esphome_src(r"C:\b\src\esphome\core\app.cpp") + assert idedata._is_esphome_src("/b/src/esphome/core/app.cpp") + # non-esphome and non-C++ still rejected regardless of separator + assert not idedata._is_esphome_src(r"C:\b\managed_components\x\x.cpp") + assert not idedata._is_esphome_src(r"C:\b\src\esphome\core\app.h") + + +def test_idedata_from_build_empty_includes_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A compile DB with no ESPHome TU yields no build includes; that is + never a usable idedata, so it must be diagnosable.""" + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text( + json.dumps( + [ + _entry( + f"{ABS}build", + f"{ABS}build/other/lib.cpp", + "/tools/g++ -c other/lib.cpp -o lib.o", + ) + ] + ) + ) + with patch.object(idedata, "get_toolchain_includes", return_value=[]): + data = idedata.idedata_from_build(compile_commands) + assert data["includes"]["build"] == [] + assert "idedata will be incomplete" in caplog.text + + +def test_idedata_from_build_dedupes_identical_command_shapes( + tmp_path: Path, +) -> None: + """Translation units sharing one ninja rule (same command modulo + file/output) carry + identical includes, so only one per shape is tokenized; a differing + shape still contributes its includes.""" + + def _tu(name: str, inc: str) -> dict: + # ninja's compdb embeds the file and output strings verbatim + file = f"{ABS}build/src/esphome/core/{name}.cpp" + return _entry( + f"{ABS}build", file, f"/tools/g++ -I{ABS}inc/{inc} -c {file} -o {name}.o" + ) | {"output": f"{name}.o"} + + entries = [_tu(name, "shared") for name in ("application", "component", "helpers")] + entries.append(_tu("extra", "extra")) + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text(json.dumps(entries)) + with ( + patch.object(idedata, "get_toolchain_includes", return_value=[]), + patch.object(idedata, "parse_entry", wraps=idedata.parse_entry) as spy, + ): + data = idedata.idedata_from_build(compile_commands) + includes = set(data["includes"]["build"]) + assert f"{ABS}inc/shared".replace("\\", "/") in { + i.replace("\\", "/") for i in includes + } + assert any("inc/extra" in i for i in includes) + # Representative + one distinct shape; the two same-shape duplicates + # are never tokenized + assert spy.call_count == 2 + + +def test_idedata_from_build(tmp_path: Path) -> None: + """Full transform: representative entry + include union + toolchain dirs.""" + compile_commands = tmp_path / "compile_commands.json" + entries = [ + _entry( + f"{ABS}b", + f"{ABS}b/src/esphome/core/app.cpp", + f"g++ -DUSE_ESP32 -I{ABS}inc/core -std=gnu++20 -c app.cpp -o app.cpp.o", + ), + _entry( + f"{ABS}b", + f"{ABS}b/src/esphome/sensor/s.cpp", + f"g++ -DUSE_ESP32 -I{ABS}inc/sensor -c s.cpp -o s.cpp.o", + ), + # non-esphome TU: its includes must not leak into the union + _entry( + f"{ABS}b", + f"{ABS}b/managed_components/x/x.c", + f"gcc -I{ABS}inc/managed -c x.c", + ), + ] + compile_commands.write_text(json.dumps(entries)) + + fake_proc = MagicMock( + returncode=0, + stderr=( + "ignored\n" + "#include <...> search starts here:\n" + " /tc/inc/c++\n" + " /tc/inc\n" + "End of search list.\n" + "more ignored\n" + ), + ) + with patch.object(idedata.subprocess, "run", return_value=fake_proc): + data = idedata.idedata_from_build(compile_commands) + + assert data["cxx_path"] == "g++" + assert "USE_ESP32" in data["defines"] + assert "-std=gnu++20" in data["cxx_flags"] + # include dirs unioned across all esphome TUs + assert f"{ABS}inc/core" in data["includes"]["build"] + assert f"{ABS}inc/sensor" in data["includes"]["build"] + # the non-esphome TU is excluded from the union + assert f"{ABS}inc/managed" not in data["includes"]["build"] + # toolchain search dirs parsed from the compiler's -v output + assert data["includes"]["toolchain"] == ["/tc/inc/c++", "/tc/inc"] + + +def test_get_toolchain_includes_raises_on_probe_failure() -> None: + """A failed compiler probe is a hard error, not a silent empty list.""" + fake_proc = MagicMock(returncode=1, stderr="xtensa-esp32-elf-g++: not found") + with ( + patch.object(idedata.subprocess, "run", return_value=fake_proc), + pytest.raises(RuntimeError, match="builtin include dirs"), + ): + idedata.get_toolchain_includes("/bad/compiler") + + +def test_get_toolchain_includes_raises_when_no_dirs_found() -> None: + """Markers present but no dirs (anomalous output) also raises.""" + fake_proc = MagicMock( + returncode=0, + stderr="#include <...> search starts here:\nEnd of search list.\n", + ) + with ( + patch.object(idedata.subprocess, "run", return_value=fake_proc), + pytest.raises(RuntimeError, match="builtin include dirs"), + ): + idedata.get_toolchain_includes("/some/compiler") + + +# ESP-IDF's compile_commands.json on Windows mixes literal backslash path +# separators in the compiler path with shell ``\"`` quote-escaping in defines, +# which only the real Windows argv parser handles. These exercise that path. +@pytest.mark.skipif(os.name != "nt", reason="Windows argv tokenization") +def test_split_command_preserves_paths_and_unescapes_quotes() -> None: + r"""Backslash paths survive while ``\"`` define-quoting is unescaped.""" + command = r"C:\esp\bin\riscv32-esp-elf-g++.exe -DVER=\"1.2.3\" -IC:/inc/a -c x.cpp" + + tokens = idedata._split_command(command) + + assert tokens[0] == r"C:\esp\bin\riscv32-esp-elf-g++.exe" + assert '-DVER="1.2.3"' in tokens + assert "-IC:/inc/a" in tokens + + +@pytest.mark.skipif(os.name != "nt", reason="Windows argv tokenization") +def test_split_command_empty_returns_empty() -> None: + """An empty or blank command tokenizes to ``[]`` (e.g. an empty response file). + + Guards against ``CommandLineToArgvW("")`` returning the current process name + instead of an empty list. + """ + assert idedata._split_command("") == [] + assert idedata._split_command(" ") == [] + + +@pytest.mark.skipif(os.name != "nt", reason="Windows argv tokenization") +def test_parse_entry_normalizes_windows_cxx_path() -> None: + """A backslash compiler path is emitted forward-slashed; define unescaped.""" + entry = _entry( + r"C:\b", + r"C:\b\src\esphome\x.cpp", + r"C:\esp\bin\g++.exe -DVER=\"1.2.3\" -IC:/inc/a -c x.cpp", + ) + + cxx_path, defines, includes, _ = idedata.parse_entry(entry) + + assert cxx_path == "C:/esp/bin/g++.exe" + assert "\\" not in cxx_path + assert 'VER="1.2.3"' in defines + assert "C:/inc/a" in includes + + +def test_parse_entry_strips_launcher_prefix() -> None: + """A launcher-wrapped compile names the compiler second; the exact + configured launcher is stripped, not anything ccache-shaped.""" + entry = _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/core/application.cpp", + "/opt/homebrew/bin/ccache /tools/xtensa-lx106-elf-g++ -DUSE_ESP8266 " + "-c app.cpp -o app.cpp.o", + ) + cxx_path, defines, _, _ = idedata.parse_entry( + entry, launcher="/opt/homebrew/bin/ccache" + ) + assert cxx_path == "/tools/xtensa-lx106-elf-g++" + assert defines == ["USE_ESP8266"] + + +def test_parse_entry_recovers_from_unconfigured_launcher( + caplog: pytest.LogCaptureFixture, +) -> None: + """A stale compile DB built with a launcher this run no longer configures + still yields the real compiler (the next token), not the launcher.""" + entry = _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/core/application.cpp", + "/opt/homebrew/bin/ccache /tools/xtensa-lx106-elf-g++ -c a.cpp -o a.o", + ) + caplog.set_level(logging.DEBUG) + cxx_path, _, _, _ = idedata.parse_entry(entry) + assert cxx_path == "/tools/xtensa-lx106-elf-g++" + assert "Stripping unconfigured launcher" in caplog.text + + +def test_parse_entry_keeps_launcher_without_program() -> None: + """A launcher followed only by flags (no program to recover) stays as + token zero; the cache layer refuses to persist it.""" + entry = _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/core/application.cpp", + "/opt/homebrew/bin/ccache -c a.cpp -o a.o", + ) + cxx_path, _, _, _ = idedata.parse_entry(entry) + assert cxx_path == "/opt/homebrew/bin/ccache" + + +def _write_compile_commands(tmp_path: Path) -> Path: + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text( + json.dumps( + [ + _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/core/application.cpp", + "/tools/g++ -DUSE_ESP8266 -c app.cpp -o app.cpp.o", + ) + ] + ) + ) + return compile_commands + + +def test_load_or_build_idedata_missing_compile_db(tmp_path: Path) -> None: + assert ( + idedata.load_or_build_idedata( + tmp_path / "compile_commands.json", tmp_path / "f.elf", tmp_path / "c.json" + ) + is None + ) + + +def test_load_or_build_idedata_builds_and_caches(tmp_path: Path) -> None: + compile_commands = _write_compile_commands(tmp_path) + cache = tmp_path / "cache" / "test.json" + with patch.object( + idedata, "get_toolchain_includes", return_value=["/toolchain/include"] + ): + data = idedata.load_or_build_idedata( + compile_commands, tmp_path / "firmware.elf", cache + ) + assert data["cc_path"] == "/tools/gcc" + assert data["prog_path"] == str(tmp_path / "firmware.elf") + assert json.loads(cache.read_text()) == data + + # A fresh cache is served without re-parsing the compile DB + os.utime(cache, (compile_commands.stat().st_mtime + 10,) * 2) + with patch.object(idedata, "idedata_from_build") as mock_build: + assert ( + idedata.load_or_build_idedata( + compile_commands, tmp_path / "firmware.elf", cache + ) + == data + ) + mock_build.assert_not_called() + + +def test_load_or_build_idedata_rebuilds_bad_cache(tmp_path: Path) -> None: + compile_commands = _write_compile_commands(tmp_path) + cache = tmp_path / "cache.json" + for bad in ("not json", json.dumps({"no_cc_path": True})): + cache.write_text(bad) + os.utime(cache, (compile_commands.stat().st_mtime + 10,) * 2) + with patch.object(idedata, "get_toolchain_includes", return_value=[]): + data = idedata.load_or_build_idedata( + compile_commands, tmp_path / "f.elf", cache + ) + assert "cc_path" in data + + +def test_load_or_build_idedata_rebuilds_when_compile_db_newer(tmp_path: Path) -> None: + """A compile DB newer than the cache forces regeneration.""" + compile_commands = _write_compile_commands(tmp_path) + cache = tmp_path / "cache.json" + cache.write_text(json.dumps({"cc_path": "stale"})) + os.utime(compile_commands, (cache.stat().st_mtime + 10,) * 2) + with patch.object(idedata, "get_toolchain_includes", return_value=[]): + data = idedata.load_or_build_idedata( + compile_commands, tmp_path / "f.elf", cache + ) + assert data["cc_path"] != "stale" + + +def test_load_or_build_idedata_rebuilds_non_dict_cache(tmp_path: Path) -> None: + """Valid JSON that is not an object is regenerated, never handed out. + + A bare string would otherwise pass the cc_path check by substring. + """ + compile_commands = _write_compile_commands(tmp_path) + cache = tmp_path / "cache.json" + for bad in ('"cc_path is a string"', "[]", "42"): + cache.write_text(bad) + os.utime(cache, (compile_commands.stat().st_mtime + 10,) * 2) + with patch.object(idedata, "get_toolchain_includes", return_value=[]): + data = idedata.load_or_build_idedata( + compile_commands, tmp_path / "f.elf", cache + ) + assert isinstance(data, dict) + assert "cc_path" in data + + +def test_is_launcher_matches_only_known_launchers() -> None: + """Compilers of any shape pass; only the closed launcher set matches.""" + for token in ("/t/g++-13", "gcc-8.4.0", "clang++-17", "armcc", "icx", "cc"): + assert not idedata._is_launcher(token) + for token in ("/opt/homebrew/bin/ccache", "CCACHE.EXE", "distcc", "sccache"): + assert idedata._is_launcher(token) + + +def test_load_or_build_idedata_corrupted_cache_is_logged( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A truncated cache is diagnosable, not a silent slow-build cause.""" + compile_commands = _write_compile_commands(tmp_path) + cache = tmp_path / "c.json" + cache.write_text('{"cc_path": trunc') + os.utime(cache, (compile_commands.stat().st_mtime + 5,) * 2) + with patch.object(idedata, "get_toolchain_includes", return_value=[]): + data = idedata.load_or_build_idedata( + compile_commands, tmp_path / "f.elf", cache + ) + assert data["cxx_path"] == "/tools/g++" + assert "Discarding unreadable idedata cache" in caplog.text + + +def test_load_or_build_idedata_discards_unreadable_cache_file( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """An OSError on the cache read (permissions, I/O) regenerates like a + parse failure instead of aborting the consumer.""" + compile_commands = _write_compile_commands(tmp_path) + cache = tmp_path / "c.json" + cache.write_text("{}") + os.utime(cache, (compile_commands.stat().st_mtime + 5,) * 2) + real_read_text = Path.read_text + + def fail_cache_read(self: Path, *args: object, **kwargs: object) -> str: + # chmod(0) cannot revoke read access on Windows, so fault the read + # itself for a platform-independent OSError + if self == cache: + raise OSError("permission denied") + return real_read_text(self, *args, **kwargs) + + with ( + patch.object(idedata, "get_toolchain_includes", return_value=[]), + patch.object(Path, "read_text", fail_cache_read), + ): + data = idedata.load_or_build_idedata( + compile_commands, tmp_path / "f.elf", cache + ) + assert data["cxx_path"] == "/tools/g++" + assert "Discarding unreadable idedata cache" in caplog.text + + +def test_load_or_build_idedata_never_caches_a_launcher(tmp_path: Path) -> None: + """A compile DB naming a launcher as the compiler is rejected, never cached.""" + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text( + json.dumps( + [ + _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/core/application.cpp", + "/opt/homebrew/bin/ccache -c app.cpp -o app.cpp.o", + ) + ] + ) + ) + cache = tmp_path / "c.json" + # No probe patch needed: the launcher is rejected before the probe runs + with pytest.raises(EsphomeError, match="compile database is unusable"): + idedata.load_or_build_idedata(compile_commands, tmp_path / "f.elf", cache) + assert not cache.exists() + + +def test_load_or_build_idedata_cache_hit_skips_rebuild(tmp_path: Path) -> None: + """A valid cache newer than the compile DB is served without re-parsing.""" + compile_commands = _write_compile_commands(tmp_path) + cache = tmp_path / "c.json" + cache.write_text(json.dumps({"cc_path": "/tools/gcc", "cached": True})) + os.utime(cache, (compile_commands.stat().st_mtime + 5,) * 2) + with patch.object(idedata, "idedata_from_build") as mock_build: + data = idedata.load_or_build_idedata( + compile_commands, tmp_path / "f.elf", cache + ) + mock_build.assert_not_called() + assert data["cached"] is True diff --git a/tests/unit_tests/build_helpers/test_size_summary.py b/tests/unit_tests/build_helpers/test_size_summary.py new file mode 100644 index 0000000000..231ae271b2 --- /dev/null +++ b/tests/unit_tests/build_helpers/test_size_summary.py @@ -0,0 +1,22 @@ +"""Tests for the shared PlatformIO-format size bar.""" + +from __future__ import annotations + +import pytest + +from esphome.build_helpers.size_summary import format_bar, print_size_line + + +def test_format_bar_zero_total() -> None: + """A zero total must not divide by zero.""" + assert format_bar(0, 0) == "[ ] 0.0% (used 0 bytes from 0 bytes)" + + +def test_print_size_line_label_padding(capsys: pytest.CaptureFixture[str]) -> None: + """The label column is exactly what ci_memory_impact_extract.py greps.""" + print_size_line("RAM", 47932, 180736) + print_size_line("Flash", 888511, 1835008) + out = capsys.readouterr().out.splitlines() + assert out[0].startswith("RAM: [") + assert out[1].startswith("Flash: [") + assert "26.5% (used 47932 bytes from 180736 bytes)" in out[0] diff --git a/tests/unit_tests/test_core.py b/tests/unit_tests/test_core.py index c373116106..7adf955217 100644 --- a/tests/unit_tests/test_core.py +++ b/tests/unit_tests/test_core.py @@ -6,9 +6,9 @@ from unittest.mock import patch from hypothesis import given import pytest -from strategies import mac_addr_strings from esphome import const, core +from tests.unit_tests.strategies import mac_addr_strings class TestHexInt: diff --git a/tests/unit_tests/test_espidf_clang_tidy.py b/tests/unit_tests/test_espidf_clang_tidy.py index cb25535d8d..4cc445e29b 100644 --- a/tests/unit_tests/test_espidf_clang_tidy.py +++ b/tests/unit_tests/test_espidf_clang_tidy.py @@ -1,10 +1,13 @@ """Tests for esphome.espidf.clang_tidy tidy-project generation.""" +import json import os from pathlib import Path +from unittest.mock import patch import pytest +from esphome.espidf import clang_tidy from esphome.espidf.clang_tidy import _Settings, _setup_core, _write_tidy_project REPO_ROOT = Path(__file__).resolve().parents[2] @@ -64,3 +67,35 @@ def test_setup_core_sets_arduino_env( _setup_core(tmp_path / "proj", _settings(target_framework=target_framework)) assert os.environ["ESPHOME_ARDUINO_COMPONENT"] == expected + + +def test_idedata_from_tidy_project(tmp_path) -> None: + """The tidy TU's compile entry is assembled into consumer-shaped idedata.""" + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text( + json.dumps( + [ + { + "directory": str(tmp_path), + "file": str(tmp_path / "main" / "tidy.cpp"), + "command": "/tc/xtensa-esp32-elf-g++ -DUSE_ESP32 " + f"-I{tmp_path}/inc -c main/tidy.cpp -o tidy.o", + } + ] + ) + ) + with patch( + "esphome.espidf.clang_tidy.get_toolchain_includes", return_value=["/tc/inc"] + ): + data = clang_tidy._idedata_from_tidy_project(compile_commands) + assert data["cxx_path"] == "/tc/xtensa-esp32-elf-g++" + assert data["defines"] == ["USE_ESP32"] + assert data["includes"]["toolchain"] == ["/tc/inc"] + assert any(inc.endswith("/inc") for inc in data["includes"]["build"]) + + +def test_idedata_from_tidy_project_missing_tu_raises(tmp_path) -> None: + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text(json.dumps([])) + with pytest.raises(RuntimeError, match="tidy.cpp not found"): + clang_tidy._idedata_from_tidy_project(compile_commands) diff --git a/tests/unit_tests/test_espidf_idedata.py b/tests/unit_tests/test_espidf_idedata.py deleted file mode 100644 index 1088517ed1..0000000000 --- a/tests/unit_tests/test_espidf_idedata.py +++ /dev/null @@ -1,264 +0,0 @@ -"""Tests for esphome.espidf.idedata (compile_commands.json -> idedata).""" - -# pylint: disable=protected-access - -import json -import os -from pathlib import Path -from unittest.mock import MagicMock, patch - -import pytest - -from esphome.espidf import idedata - -# An absolute, forward-slash (shlex-safe) path prefix valid on the host OS, so -# tests exercise the same is-absolute / normalize behavior as a real compile DB -# (a drive-qualified path on Windows, a leading slash elsewhere). -ABS = "C:/" if os.name == "nt" else "/" - - -def _entry(directory: str, file: str, command: str) -> dict: - return {"directory": directory, "file": file, "command": command} - - -def test_parse_entry_extracts_fields() -> None: - """cxx_path, defines, includes and remaining flags are split apart.""" - entry = _entry( - f"{ABS}build", - f"{ABS}build/src/esphome/core/application.cpp", - f"/tools/xtensa-esp32-elf-g++ -DUSE_ESP32 -DESPHOME_LOG_LEVEL=5 " - f"-I{ABS}inc/a -isystem {ABS}sys/b -std=gnu++20 -c app.cpp -o app.cpp.o", - ) - - cxx_path, defines, includes, cxx_flags = idedata._parse_entry(entry) - - assert cxx_path == "/tools/xtensa-esp32-elf-g++" - assert "USE_ESP32" in defines - assert "ESPHOME_LOG_LEVEL=5" in defines - assert f"{ABS}inc/a" in includes - assert f"{ABS}sys/b" in includes - assert "-std=gnu++20" in cxx_flags - # input/output files and their flags are not treated as flags - assert "-c" not in cxx_flags - assert "-o" not in cxx_flags - assert "app.cpp" not in cxx_flags - assert "app.cpp.o" not in cxx_flags - - -def test_parse_entry_space_separated_args() -> None: - """``-D X`` / ``-I path`` (separate arg) and ``-isystem`` (joined).""" - entry = _entry( - f"{ABS}build", - f"{ABS}build/src/esphome/x.cpp", - f"g++ -D FOO=1 -I {ABS}inc/sep -isystem{ABS}sys/joined -c x.cpp", - ) - - _, defines, includes, _ = idedata._parse_entry(entry) - - assert "FOO=1" in defines - assert f"{ABS}inc/sep" in includes - assert f"{ABS}sys/joined" in includes - - -def test_parse_entry_resolves_relative_includes() -> None: - """Relative includes are resolved against the entry's ``directory``.""" - directory = f"{ABS}build/proj" - entry = _entry( - directory, - f"{directory}/src/esphome/x.cpp", - "g++ -Iconfig -I../shared -isystem rel/sys -c x.cpp", - ) - - _, _, includes, _ = idedata._parse_entry(entry) - - def resolved(rel: str) -> str: - # _parse_entry emits forward slashes for consistency (normpath would - # yield backslashes on Windows). - return os.path.normpath(Path(directory) / rel).replace("\\", "/") - - assert resolved("config") in includes - assert resolved("../shared") in includes # ../ normalized away - assert resolved("rel/sys") in includes - # nothing is left relative - assert all(Path(inc).is_absolute() for inc in includes) - - -def test_parse_entry_skips_dependency_flags() -> None: - """Dependency-generation flags (and their args) are dropped.""" - entry = _entry( - "/build", - "/build/src/esphome/x.cpp", - "g++ -MD -MT x.cpp.o -MF x.cpp.o.d -c x.cpp -o x.cpp.o", - ) - - _, _, _, cxx_flags = idedata._parse_entry(entry) - - for tok in ("-MD", "-MT", "x.cpp.o", "-MF", "x.cpp.o.d", "-c", "-o", "x.cpp"): - assert tok not in cxx_flags - - -def test_expand_response_files(tmp_path: Path) -> None: - """``@file`` arguments are inlined relative to the directory.""" - rsp = tmp_path / "flags.rsp" - rsp.write_text("-DFROM_RSP -I/rsp/inc") - - tokens = idedata._expand_response_files( - ["g++", f"@{rsp.name}", "-c", "x.cpp"], tmp_path - ) - - assert "-DFROM_RSP" in tokens - assert "-I/rsp/inc" in tokens - assert not any(t.startswith("@") for t in tokens) - - -def test_expand_response_files_keeps_literal_when_missing(tmp_path: Path) -> None: - """An unreadable ``@file`` token is kept verbatim rather than dropped.""" - tokens = idedata._expand_response_files(["g++", "@nope.rsp"], tmp_path) - assert "@nope.rsp" in tokens - - -def test_pick_entry_prefers_esphome_tu() -> None: - """A ``/src/esphome/`` C++ TU is picked over other compile entries.""" - entries = [ - _entry("/b", "/b/managed_components/foo/foo.c", "gcc -c foo.c"), - _entry("/b", "/b/src/esphome/core/app.cpp", "g++ -c app.cpp"), - ] - assert idedata._pick_entry(entries)["file"].endswith("app.cpp") - - -def test_pick_entry_falls_back_to_any_cxx_tu() -> None: - """With no ``/src/esphome/`` TU present, the first C++ entry is the fallback.""" - entries = [ - _entry("/b", "/b/managed_components/foo/foo.c", "gcc -c foo.c"), - _entry("/b", "/b/components/x/x.cpp", "g++ -c x.cpp"), - ] - assert idedata._pick_entry(entries)["file"].endswith("x.cpp") - - -def test_is_esphome_src_handles_backslash_paths() -> None: - r"""The src marker must match Windows ``\src\esphome\`` paths too. - - compile_commands ``file`` entries use the OS-native separator; if the - marker only matched forward slashes no source would match on Windows and - the build-include union would be silently empty. - """ - assert idedata._is_esphome_src(r"C:\b\src\esphome\core\app.cpp") - assert idedata._is_esphome_src("/b/src/esphome/core/app.cpp") - # non-esphome and non-C++ still rejected regardless of separator - assert not idedata._is_esphome_src(r"C:\b\managed_components\x\x.cpp") - assert not idedata._is_esphome_src(r"C:\b\src\esphome\core\app.h") - - -def test_idedata_from_build(tmp_path: Path) -> None: - """Full transform: representative entry + include union + toolchain dirs.""" - compile_commands = tmp_path / "compile_commands.json" - entries = [ - _entry( - f"{ABS}b", - f"{ABS}b/src/esphome/core/app.cpp", - f"g++ -DUSE_ESP32 -I{ABS}inc/core -std=gnu++20 -c app.cpp -o app.cpp.o", - ), - _entry( - f"{ABS}b", - f"{ABS}b/src/esphome/sensor/s.cpp", - f"g++ -DUSE_ESP32 -I{ABS}inc/sensor -c s.cpp -o s.cpp.o", - ), - # non-esphome TU: its includes must not leak into the union - _entry( - f"{ABS}b", - f"{ABS}b/managed_components/x/x.c", - f"gcc -I{ABS}inc/managed -c x.c", - ), - ] - compile_commands.write_text(json.dumps(entries)) - - fake_proc = MagicMock( - returncode=0, - stderr=( - "ignored\n" - "#include <...> search starts here:\n" - " /tc/inc/c++\n" - " /tc/inc\n" - "End of search list.\n" - "more ignored\n" - ), - ) - with patch.object(idedata.subprocess, "run", return_value=fake_proc): - data = idedata.idedata_from_build(compile_commands) - - assert data["cxx_path"] == "g++" - assert "USE_ESP32" in data["defines"] - assert "-std=gnu++20" in data["cxx_flags"] - # include dirs unioned across all esphome TUs - assert f"{ABS}inc/core" in data["includes"]["build"] - assert f"{ABS}inc/sensor" in data["includes"]["build"] - # the non-esphome TU is excluded from the union - assert f"{ABS}inc/managed" not in data["includes"]["build"] - # toolchain search dirs parsed from the compiler's -v output - assert data["includes"]["toolchain"] == ["/tc/inc/c++", "/tc/inc"] - - -def test_get_toolchain_includes_raises_on_probe_failure() -> None: - """A failed compiler probe is a hard error, not a silent empty list.""" - fake_proc = MagicMock(returncode=1, stderr="xtensa-esp32-elf-g++: not found") - with ( - patch.object(idedata.subprocess, "run", return_value=fake_proc), - pytest.raises(RuntimeError, match="builtin include dirs"), - ): - idedata._get_toolchain_includes("/bad/compiler") - - -def test_get_toolchain_includes_raises_when_no_dirs_found() -> None: - """Markers present but no dirs (anomalous output) also raises.""" - fake_proc = MagicMock( - returncode=0, - stderr="#include <...> search starts here:\nEnd of search list.\n", - ) - with ( - patch.object(idedata.subprocess, "run", return_value=fake_proc), - pytest.raises(RuntimeError, match="builtin include dirs"), - ): - idedata._get_toolchain_includes("/some/compiler") - - -# ESP-IDF's compile_commands.json on Windows mixes literal backslash path -# separators in the compiler path with shell ``\"`` quote-escaping in defines, -# which only the real Windows argv parser handles. These exercise that path. -@pytest.mark.skipif(os.name != "nt", reason="Windows argv tokenization") -def test_split_command_preserves_paths_and_unescapes_quotes() -> None: - r"""Backslash paths survive while ``\"`` define-quoting is unescaped.""" - command = r"C:\esp\bin\riscv32-esp-elf-g++.exe -DVER=\"1.2.3\" -IC:/inc/a -c x.cpp" - - tokens = idedata._split_command(command) - - assert tokens[0] == r"C:\esp\bin\riscv32-esp-elf-g++.exe" - assert '-DVER="1.2.3"' in tokens - assert "-IC:/inc/a" in tokens - - -@pytest.mark.skipif(os.name != "nt", reason="Windows argv tokenization") -def test_split_command_empty_returns_empty() -> None: - """An empty or blank command tokenizes to ``[]`` (e.g. an empty response file). - - Guards against ``CommandLineToArgvW("")`` returning the current process name - instead of an empty list. - """ - assert idedata._split_command("") == [] - assert idedata._split_command(" ") == [] - - -@pytest.mark.skipif(os.name != "nt", reason="Windows argv tokenization") -def test_parse_entry_normalizes_windows_cxx_path() -> None: - """A backslash compiler path is emitted forward-slashed; define unescaped.""" - entry = _entry( - r"C:\b", - r"C:\b\src\esphome\x.cpp", - r"C:\esp\bin\g++.exe -DVER=\"1.2.3\" -IC:/inc/a -c x.cpp", - ) - - cxx_path, defines, includes, _ = idedata._parse_entry(entry) - - assert cxx_path == "C:/esp/bin/g++.exe" - assert "\\" not in cxx_path - assert 'VER="1.2.3"' in defines - assert "C:/inc/a" in includes diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index 2556397aef..6c11a74d48 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -140,7 +140,7 @@ def test_get_idedata_generates_and_caches(setup_core: Path) -> None: compile_commands.write_text("[]") with patch( - "esphome.espidf.idedata.idedata_from_build", + "esphome.build_helpers.idedata.idedata_from_build", return_value={"cxx_path": "g++"}, ) as mock_transform: result = toolchain.get_idedata() @@ -151,114 +151,6 @@ def test_get_idedata_generates_and_caches(setup_core: Path) -> None: assert json.loads(cache.read_text()) == {"cxx_path": "g++", "prog_path": prog_path} -def test_get_idedata_uses_cache_when_valid(setup_core: Path) -> None: - """A cache at least as new as the compile DB is reused without regenerating.""" - compile_commands, cache = _setup_build(setup_core) - compile_commands.parent.mkdir(parents=True, exist_ok=True) - compile_commands.write_text("[]") - cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_text('{"cc_path": "cached-gcc", "cxx_path": "cached"}') - cc_mtime = compile_commands.stat().st_mtime - os.utime(cache, (cc_mtime + 1, cc_mtime + 1)) - - with patch("esphome.espidf.idedata.idedata_from_build") as mock_transform: - result = toolchain.get_idedata() - - mock_transform.assert_not_called() - assert result == {"cc_path": "cached-gcc", "cxx_path": "cached"} - - -def test_get_idedata_regenerates_cache_without_cc_path(setup_core: Path) -> None: - """A cache predating cc_path is rebuilt even though it is newer. - - Such a cache stays newer than the compile DB forever, so consumers that - derive the binutils paths from cc_path would keep failing on it. - """ - compile_commands, cache = _setup_build(setup_core) - compile_commands.parent.mkdir(parents=True, exist_ok=True) - compile_commands.write_text("[]") - cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_text('{"cxx_path": "cached"}') - cc_mtime = compile_commands.stat().st_mtime - os.utime(cache, (cc_mtime + 1, cc_mtime + 1)) - - with patch( - "esphome.espidf.idedata.idedata_from_build", - return_value={"cc_path": "gcc", "cxx_path": "g++"}, - ) as mock_transform: - result = toolchain.get_idedata() - - mock_transform.assert_called_once() - assert result["cc_path"] == "gcc" - - -def test_get_idedata_regenerates_when_compile_commands_newer(setup_core: Path) -> None: - """A compile DB newer than the cache forces regeneration.""" - compile_commands, cache = _setup_build(setup_core) - cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_text('{"cxx_path": "stale"}') - compile_commands.parent.mkdir(parents=True, exist_ok=True) - compile_commands.write_text("[]") - cache_mtime = cache.stat().st_mtime - os.utime(compile_commands, (cache_mtime + 1, cache_mtime + 1)) - - with patch( - "esphome.espidf.idedata.idedata_from_build", - return_value={"cxx_path": "fresh"}, - ) as mock_transform: - result = toolchain.get_idedata() - - mock_transform.assert_called_once() - assert result == {"cxx_path": "fresh", "prog_path": str(toolchain.get_elf_path())} - - -@pytest.mark.parametrize("cached", ['"cc_path is a string"', "[]", "42"]) -def test_get_idedata_regenerates_on_non_dict_cache( - setup_core: Path, cached: str -) -> None: - """A newer cache holding valid JSON that is not an object is regenerated. - - A bare string would otherwise pass the cc_path check by substring and be - handed to consumers expecting a dict. - """ - compile_commands, cache = _setup_build(setup_core) - compile_commands.parent.mkdir(parents=True, exist_ok=True) - compile_commands.write_text("[]") - cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_text(cached) - cc_mtime = compile_commands.stat().st_mtime - os.utime(cache, (cc_mtime + 1, cc_mtime + 1)) - - with patch( - "esphome.espidf.idedata.idedata_from_build", - return_value={"cc_path": "gcc", "cxx_path": "g++"}, - ) as mock_transform: - result = toolchain.get_idedata() - - mock_transform.assert_called_once() - assert isinstance(result, dict) - - -def test_get_idedata_regenerates_on_corrupted_cache(setup_core: Path) -> None: - """An unparseable (but newer) cache falls back to regeneration.""" - compile_commands, cache = _setup_build(setup_core) - compile_commands.parent.mkdir(parents=True, exist_ok=True) - compile_commands.write_text("[]") - cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_text("{not json") - cc_mtime = compile_commands.stat().st_mtime - os.utime(cache, (cc_mtime + 1, cc_mtime + 1)) - - with patch( - "esphome.espidf.idedata.idedata_from_build", - return_value={"cxx_path": "regen"}, - ) as mock_transform: - result = toolchain.get_idedata() - - mock_transform.assert_called_once() - assert result == {"cxx_path": "regen", "prog_path": str(toolchain.get_elf_path())} - - def test_get_idedata_prog_path_points_at_firmware_elf(setup_core: Path) -> None: """The idedata exposes prog_path (the ELF) so consumers like build-action can locate firmware.factory.bin / firmware.ota.bin as its siblings.""" @@ -267,7 +159,7 @@ def test_get_idedata_prog_path_points_at_firmware_elf(setup_core: Path) -> None: compile_commands.write_text("[]") with patch( - "esphome.espidf.idedata.idedata_from_build", + "esphome.build_helpers.idedata.idedata_from_build", return_value={"cxx_path": "g++"}, ): result = toolchain.get_idedata() diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py index 1cb710ca58..c7a5c85638 100644 --- a/tests/unit_tests/test_main.py +++ b/tests/unit_tests/test_main.py @@ -17,6 +17,7 @@ from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest from pytest import CaptureFixture +import serial from zeroconf import ServiceStateChange from esphome import __main__ as main, yaml_util @@ -26,6 +27,7 @@ from esphome.__main__ import ( _make_crystal_freq_callback, _redact_with_legacy_fallback, _resolve_network_devices, + _should_subscribe_states, _split_network_devices, _unresolved_default_error, _validate_bootloader_binary, @@ -69,19 +71,21 @@ from esphome.__main__ import ( ) from esphome.address_cache import AddressCache from esphome.bundle import BUNDLE_EXTENSION, BundleFile, BundleResult -from esphome.components import esp32, esp8266 +from esphome.components import esp32, esp8266, mqtt from esphome.components.esp32 import ( KEY_ESP32, KEY_VARIANT, VARIANT_ESP32, get_esp32_variant, ) +from esphome.config import Config from esphome.const import ( CONF_API, CONF_AUTH, CONF_BAUD_RATE, CONF_BROKER, CONF_DISABLED, + CONF_DISCOVER_IP, CONF_ESPHOME, CONF_LEVEL, CONF_LOG, @@ -103,6 +107,7 @@ from esphome.const import ( CONF_WEB_SERVER, CONF_WIFI, KEY_CORE, + KEY_TARGET_FRAMEWORK, KEY_TARGET_PLATFORM, PLATFORM_BK72XX, PLATFORM_ESP32, @@ -567,8 +572,6 @@ def test_command_config__no_defaults_dumps_user_snapshot( ) -> None: """``--no-defaults`` dumps ``config.user_config`` instead of the validated config, so schema defaults don't leak into the output.""" - from esphome.config import Config - setup_core(tmp_path=tmp_path, config={"esphome": {"name": "test"}}) args = MockArgs() args.show_secrets = True @@ -621,8 +624,6 @@ def test_command_config__no_defaults_skips_strip_default_ids( ) -> None: """When ``--no-defaults`` is set, ``strip_default_ids`` isn't run -- the user snapshot is already free of schema-injected IDs.""" - from esphome.config import Config - setup_core(tmp_path=tmp_path, config={"esphome": {"name": "test"}}) args = MockArgs() args.show_secrets = True @@ -3440,9 +3441,6 @@ def test_get_port_type() -> None: def test_mqtt_reexports_discover_ip() -> None: """The old import path must keep working for external code.""" - from esphome.components import mqtt - from esphome.const import CONF_DISCOVER_IP - assert mqtt.CONF_DISCOVER_IP is CONF_DISCOVER_IP @@ -5909,8 +5907,6 @@ class MockSerial: chunk = self.chunks[self.chunk_index] if chunk is MOCK_SERIAL_END: # Sentinel means we're done - simulate port closed - import serial - raise serial.SerialException("Port closed") # Respect the requested size and keep any remaining bytes if size <= 0: @@ -5924,8 +5920,6 @@ class MockSerial: # Entire chunk consumed; advance to the next one self.chunk_index += 1 return data # type: ignore[return-value] - import serial - raise serial.SerialException("Port closed") @@ -6784,8 +6778,6 @@ def test_parse_args_argcomplete_only_runs_when_completing() -> None: def test_should_subscribe_states_default() -> None: """Test that states are shown by default when nothing is set.""" - from esphome.__main__ import _should_subscribe_states - args = parse_args(["esphome", "logs", "device.yaml"]) with patch.dict(os.environ, {}, clear=False): os.environ.pop("ESPHOME_LOG_STATES", None) @@ -6794,8 +6786,6 @@ def test_should_subscribe_states_default() -> None: def test_should_subscribe_states_env_suppresses() -> None: """Test that ESPHOME_LOG_STATES=false suppresses states by default.""" - from esphome.__main__ import _should_subscribe_states - args = parse_args(["esphome", "logs", "device.yaml"]) with patch.dict(os.environ, {"ESPHOME_LOG_STATES": "false"}): assert _should_subscribe_states(args) is False @@ -6803,8 +6793,6 @@ def test_should_subscribe_states_env_suppresses() -> None: def test_should_subscribe_states_env_enables() -> None: """Test that ESPHOME_LOG_STATES=true enables states by default.""" - from esphome.__main__ import _should_subscribe_states - args = parse_args(["esphome", "logs", "device.yaml"]) with patch.dict(os.environ, {"ESPHOME_LOG_STATES": "true"}): assert _should_subscribe_states(args) is True @@ -6812,8 +6800,6 @@ def test_should_subscribe_states_env_enables() -> None: def test_should_subscribe_states_flag_overrides_env() -> None: """Test that --states overrides ESPHOME_LOG_STATES=false.""" - from esphome.__main__ import _should_subscribe_states - args = parse_args(["esphome", "logs", "--states", "device.yaml"]) with patch.dict(os.environ, {"ESPHOME_LOG_STATES": "false"}): assert _should_subscribe_states(args) is True @@ -6821,8 +6807,6 @@ def test_should_subscribe_states_flag_overrides_env() -> None: def test_should_subscribe_states_no_flag_overrides_env() -> None: """Test that --no-states overrides ESPHOME_LOG_STATES=true.""" - from esphome.__main__ import _should_subscribe_states - args = parse_args(["esphome", "logs", "--no-states", "device.yaml"]) with patch.dict(os.environ, {"ESPHOME_LOG_STATES": "true"}): assert _should_subscribe_states(args) is False @@ -7135,6 +7119,82 @@ def test_warn_source_tree_mismatch_falls_back_when_stat_fails( assert not caplog.text +@pytest.mark.parametrize( + "error", + [ + FileNotFoundError("no such compiler"), + RuntimeError("Could not query builtin include dirs"), + ValueError("no C++ translation unit found"), + KeyError("command"), + None, # replaced with EsphomeError inside + ], +) +def test_compile_program_espidf_idedata_failure_does_not_fail_build( + error: Exception, + caplog: pytest.LogCaptureFixture, +) -> None: + """A post-compile idedata error is a warning: the firmware already built.""" + if error is None: + error = EsphomeError("compile database is unusable") + CORE.toolchain = Toolchain.ESP_IDF + CORE.data[KEY_CORE] = { + KEY_TARGET_PLATFORM: "esp32", + KEY_TARGET_FRAMEWORK: "esp-idf", + } + with ( + patch("esphome.espidf.toolchain.run_compile", return_value=0), + patch("esphome.espidf.toolchain.create_factory_bin"), + patch("esphome.espidf.toolchain.create_ota_bin"), + patch("esphome.espidf.toolchain.create_elf_copy"), + patch("esphome.espidf.toolchain.get_idedata", side_effect=error), + patch("esphome.__main__._check_and_emit_build_info"), + ): + assert compile_program(MagicMock(), {}) == 0 + assert "Could not generate idedata" in caplog.text + + +def test_compile_program_espidf_idedata_success_is_silent( + caplog: pytest.LogCaptureFixture, +) -> None: + """The healthy path: idedata generated, nothing to warn about.""" + CORE.toolchain = Toolchain.ESP_IDF + CORE.data[KEY_CORE] = { + KEY_TARGET_PLATFORM: "esp32", + KEY_TARGET_FRAMEWORK: "esp-idf", + } + with ( + patch("esphome.espidf.toolchain.run_compile", return_value=0), + patch("esphome.espidf.toolchain.create_factory_bin"), + patch("esphome.espidf.toolchain.create_ota_bin"), + patch("esphome.espidf.toolchain.create_elf_copy"), + patch("esphome.espidf.toolchain.get_idedata", return_value={"cc_path": "x"}), + patch("esphome.__main__._check_and_emit_build_info"), + ): + assert compile_program(MagicMock(), {}) == 0 + assert "idedata" not in caplog.text + + +def test_compile_program_espidf_idedata_none_warns( + caplog: pytest.LogCaptureFixture, +) -> None: + """A silent None from the post-compile idedata refresh is made visible.""" + CORE.toolchain = Toolchain.ESP_IDF + CORE.data[KEY_CORE] = { + KEY_TARGET_PLATFORM: "esp32", + KEY_TARGET_FRAMEWORK: "esp-idf", + } + with ( + patch("esphome.espidf.toolchain.run_compile", return_value=0), + patch("esphome.espidf.toolchain.create_factory_bin"), + patch("esphome.espidf.toolchain.create_ota_bin"), + patch("esphome.espidf.toolchain.create_elf_copy"), + patch("esphome.espidf.toolchain.get_idedata", return_value=None), + patch("esphome.__main__._check_and_emit_build_info"), + ): + assert compile_program(MagicMock(), {}) == 0 + assert "No idedata was generated" in caplog.text + + @pytest.mark.asyncio async def test_wrap_to_code_comment_is_insertion_order_independent() -> None: """The config comment dumps with sorted keys: voluptuous fills schema diff --git a/tests/unit_tests/test_size_summary.py b/tests/unit_tests/test_size_summary.py index 933be88476..7a536a6838 100644 --- a/tests/unit_tests/test_size_summary.py +++ b/tests/unit_tests/test_size_summary.py @@ -126,3 +126,70 @@ def test_print_summary_handles_no_memory_types( size_json = _write_size_json(tmp_path, {"image_size": 0}) print_summary(size_json, partitions_csv=None) assert capsys.readouterr().out == "" + + +def test_print_summary_flash_line( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """image_size + a factory app partition produce the Flash line.""" + size_json = tmp_path / "esp_idf_size.json" + size_json.write_text( + json.dumps( + { + "memory_types": {"DRAM": {"used": 100, "size": 200}}, + "image_size": 500, + } + ) + ) + partitions = tmp_path / "partitions.csv" + partitions.write_text( + "# name, type, subtype, offset, size\napp0, app, factory, 0x10000, 0x100000\n" + ) + print_summary(size_json, partitions) + out = capsys.readouterr().out + assert "RAM: [===== ] 50.0% (used 100 bytes from 200 bytes)" in out + assert "Flash: [ ] 0.0% (used 500 bytes from 1048576 bytes)" in out + + +def test_print_summary_missing_ram_region_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A missing RAM line is diagnosable, not a silently absent CI metric.""" + size_json = _write_size_json(tmp_path, {"memory_types": {}, "image_size": 100}) + print_summary(size_json, partitions_csv=None) + assert "Skipping RAM summary" in caplog.text + + +def test_print_summary_bad_partitions_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """An unparseable partition table skips the Flash line with a warning.""" + size_json = _write_size_json(tmp_path, _esp32_size_data()) + partitions = tmp_path / "partitions.csv" + partitions.write_text("not,a,valid,partition,table\n") + print_summary(size_json, partitions_csv=partitions) + assert "Skipping Flash summary" in caplog.text + + +def test_print_summary_corrupt_json_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + size_json = tmp_path / "size.json" + size_json.write_text("{not json") + print_summary(size_json, partitions_csv=None) + assert "Skipping size summary" in caplog.text + + +def test_print_summary_missing_flash_inputs_warn( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Both absent-input paths for the Flash line name their cause.""" + size_json = _write_size_json(tmp_path, _esp32_size_data()) + print_summary(size_json, partitions_csv=None) + assert "no partition table given" in caplog.text + caplog.clear() + data = _esp32_size_data() + data.pop("image_size", None) + size_json = _write_size_json(tmp_path, data) + print_summary(size_json, partitions_csv=tmp_path / "partitions.cssv") + assert "no image_size" in caplog.text From a1e6370e8bf0893ed7e35f06261c8d73ae2d2236 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 21:59:19 -0500 Subject: [PATCH 2/3] [core] Harden the shared library converter and download registry archives in parallel --- esphome/espidf/component.py | 69 +-- esphome/espidf/extra_script.py | 161 ----- esphome/espidf/framework.py | 74 ++- esphome/framework_helpers.py | 140 ++++- esphome/platformio/extra_script.py | 253 ++++++++ esphome/platformio/library.py | 582 ++++++++++++++---- esphome/platformio/toolchain.py | 3 + tests/unit_tests/test_espidf_component.py | 175 ++---- tests/unit_tests/test_espidf_framework.py | 154 ++++- tests/unit_tests/test_framework_helpers.py | 103 ++++ .../test_platformio_extra_script.py | 377 ++++++++++++ tests/unit_tests/test_platformio_library.py | 359 ++++++++++- tests/unit_tests/test_platformio_toolchain.py | 7 + 13 files changed, 1927 insertions(+), 530 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/espidf/framework.py b/esphome/espidf/framework.py index 0f6ef873b8..a31a339fee 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -15,6 +15,7 @@ import platformdirs from esphome.core import CORE, Version from esphome.framework_helpers import ( + BatchDownloadProgress, PathType, archive_extract_all, create_venv, @@ -23,6 +24,7 @@ from esphome.framework_helpers import ( get_python_env_executable_path, get_system_python_path, rmdir, + run_batch_downloads, run_command, run_command_ok, str_to_lst_of_str, @@ -702,10 +704,10 @@ def _prefetch_idf_tool_archives( which makes large archives effectively impossible to fetch on unstable connections (#17703). This asks the framework's idf_tools (via ``get_tool_downloads.py``) which archives the coming install needs, then - downloads each into ``/dist`` with - ``download_with_resume``. The installer then finds the verified archives - already in place ("file ... is already downloaded") and never touches the - network. + downloads them into ``/dist`` with + ``download_with_resume``, a few at a time under one combined progress + bar. The installer then finds the verified archives already in place + ("file ... is already downloaded") and never touches the network. Strictly best-effort: any failure here just logs and returns, leaving ``idf_tools.py install`` to download whatever is missing exactly as @@ -727,26 +729,52 @@ def _prefetch_idf_tool_archives( ) return dist_path = get_idf_tools_path() / "dist" - entries = [ - entry - for entry in json.loads(stdout) - if not (dist_path / entry["dest"]).is_file() - ] - for index, entry in enumerate(entries, start=1): - _LOGGER.info( - "Downloading %s (%d/%d) ...", entry["name"], index, len(entries) - ) - try: - download_with_resume( - entry["url"], - dist_path / entry["dest"], - sha256=entry["sha256"], - size=entry["size"], + entries = [] + for entry in json.loads(stdout): + if (dist_path / entry["dest"]).is_file(): + continue + # tools.json always carries sha256 and size; an entry missing + # either must not be downloaded unverified here, so leave it to + # the installer (which fails loudly on a bad archive). + if entry.get("sha256") and entry.get("size"): + entries.append(entry) + else: + _LOGGER.warning( + "Tool %s has no sha256/size in the download list; " + "leaving it to the installer", + entry["name"], ) - except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught - # Keep prefetching the remaining archives; the installer - # will retry this one itself (without resume). - _LOGGER.warning("Could not prefetch %s: %s", entry["name"], e) + if not entries: + return + _LOGGER.info( + "Downloading %d ESP-IDF tool archive(s): %s", + len(entries), + ", ".join(entry["name"] for entry in entries), + ) + + # Every entry carries a size (checked above), so the combined bar can + # be trusted. Unlike the library prefetch there is no sequential + # fallback: per-file bars from several threads would interleave, and + # skipping the prefetch would lose the resume workaround for #17703. + def _download(entry: dict): + return lambda tracker: download_with_resume( + entry["url"], + dist_path / entry["dest"], + sha256=entry["sha256"], + size=entry["size"], + progress=tracker, + ) + + # A failed archive is retried by the installer itself (without + # resume); keep prefetching the rest. + failures = run_batch_downloads( + BatchDownloadProgress( + "Downloading ESP-IDF tools", sum(entry["size"] for entry in entries) + ), + [(entry["name"], _download(entry)) for entry in entries], + ) + for name, e in failures: + _LOGGER.warning("Could not prefetch %s: %s", name, e) except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught # The installer downloads anything missing itself; never let the # prefetch become a new way for the install to fail. diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index b8a43220ff..4d0574372a 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -1,6 +1,7 @@ """Generic toolchain installation helpers shared across framework implementations.""" -from collections.abc import Iterable +from collections.abc import Callable, Iterable +from concurrent.futures import ThreadPoolExecutor from contextlib import ExitStack import hashlib import io @@ -10,6 +11,7 @@ import os from pathlib import Path import subprocess import sys +import threading import time from typing import IO, TYPE_CHECKING @@ -23,6 +25,7 @@ PathType = str | os.PathLike _LOGGER = logging.getLogger(__name__) + # Attempts per mirror URL before falling through to the next mirror; only # mid-stream drops retry (resuming when the server gave a validator), # connect errors move on to the next mirror immediately. @@ -697,7 +700,11 @@ def _response_validator(resp: "requests.Response") -> str | None: def _stream_response_to_file( - resp: "requests.Response", f: IO[bytes], offset: int, size: int | None = None + resp: "requests.Response", + f: IO[bytes], + offset: int, + size: int | None = None, + progress: Callable[[int], None] | None = None, ) -> None: """Stream an open ``_open_ranged`` response body into ``f`` at ``offset``. @@ -705,21 +712,112 @@ def _stream_response_to_file( (effective offset 0) discards the stale bytes. ``offset`` also seeds the progress bar so a resumed download shows overall progress. ``size`` is the known full file size; when None it is derived from the response's - content-length, and without either there is no progress bar. + content-length, and without either there is no progress bar. With + ``progress`` set, no bar is drawn here; the callback gets the absolute + byte count, seeded with ``offset`` and then after each chunk. """ f.seek(offset) f.truncate(offset) total_size = size or offset + _content_length(resp) downloaded = offset - progress = ProgressBar("Downloading") if total_size > 0 else None + own_bar: ProgressBar | None = None + if progress is None: + own_bar = ProgressBar("Downloading") if total_size > 0 else None + progress = ( + (lambda done: own_bar.update(done / total_size)) + if own_bar + else (lambda _: None) + ) + progress(downloaded) for chunk in resp.iter_content(chunk_size=256 * 1024): if chunk: f.write(chunk) downloaded += len(chunk) - if progress is not None: - progress.update(downloaded / total_size) - if progress is not None: - progress.update(1) + progress(downloaded) + if own_bar is not None: + own_bar.update(1) + + +# Concurrent downloads per batch; enough to hide latency without +# hammering the host or the mirrors. +BATCH_DOWNLOAD_WORKERS = 4 + + +def run_batch_downloads( + progress: "BatchDownloadProgress", + jobs: list[tuple[str, Callable[[Callable[[int], None]], None]]], + max_workers: int = BATCH_DOWNLOAD_WORKERS, +) -> list[tuple[str, Exception]]: + """Run download jobs concurrently, reporting into one combined bar. + + ``jobs`` holds ``(name, fetch)`` pairs where ``fetch(tracker)`` performs + one download reporting absolute byte counts to ``tracker``. Failures are + collected (list.append is atomic under the GIL) and returned after the + bar is done, so the caller's warnings never land on the bar's row; a + failed job credits its tracker 0 so the bar can still complete. Ctrl-C + drops queued jobs instead of downloading them all before the process + can exit; in-flight ones still finish. + """ + failures: list[tuple[str, Exception]] = [] + + def _run(name: str, fetch: Callable[[Callable[[int], None]], None]) -> None: + tracker = progress.tracker() + try: + fetch(tracker) + except Exception as err: # noqa: BLE001 # pylint: disable=broad-exception-caught + failures.append((name, err)) + tracker(0) + + ex = ThreadPoolExecutor(max_workers=min(max_workers, len(jobs))) + try: + for future in [ex.submit(_run, name, fetch) for name, fetch in jobs]: + future.result() + finally: + ex.shutdown(wait=True, cancel_futures=True) + progress.done() + return failures + + +class BatchDownloadProgress: + """One progress bar across several concurrent ``download_with_resume`` calls. + + Each ``tracker()`` is a ``progress`` callback for one download; it reports + that file's absolute byte count and the bar shows the sum over ``total``. + The lock also serialises the bar's stderr writes, so worker threads never + interleave frames. With an unknown ``total`` (0) nothing is drawn. Call + ``done()`` once every download has finished (or failed) so a bar that + never reached 100% still ends its line before the next log message. + """ + + def __init__(self, header: str, total: int) -> None: + self._bar = ProgressBar(header) if total > 0 else None + self._total = total + self._sum = 0 + self._lock = threading.Lock() + + def tracker(self) -> Callable[[int], None]: + last = 0 + + def update(done: int) -> None: + nonlocal last + if self._bar is None: + return + with self._lock: + self._sum += done - last + last = done + self._bar.update(min(self._sum / self._total, 1)) + + return update + + def done(self) -> None: + # Nothing to end unless a frame was drawn and it was not the final + # one (update(1) already emitted its own newline). + if ( + self._bar is not None + and self._bar.last_progress is not None + and self._bar.last_progress != 100 + ): + self._bar.done() def download_with_resume( @@ -732,6 +830,7 @@ def download_with_resume( attempts: int = 5, timeout: int = 30, retry_connect_errors: bool = True, + progress: Callable[[int], None] | None = None, ) -> None: """Download ``url`` to ``dest``, resuming partial downloads. @@ -754,6 +853,12 @@ def download_with_resume( of consuming attempts — for callers with their own fallback, like ``download_from_mirrors``. + ``progress``, when given, replaces the built-in progress bar: it is called + with the absolute number of bytes of ``dest`` obtained so far (including + a resumed prefix, and the final size once the file is verified), so a + caller running several downloads at once can draw one combined bar (see + ``BatchDownloadProgress``). + Raises EsphomeError when all attempts are exhausted. """ # Imported lazily: requests is a heavy import (~85ms) and is only needed @@ -777,6 +882,8 @@ def download_with_resume( if dest.is_file() and (sha256 is not None or size is not None): try: _verify_file(dest, sha256, size) + if progress is not None: + progress(size if size is not None else dest.stat().st_size) return except EsphomeError: dest.unlink() @@ -822,7 +929,7 @@ def download_with_resume( # Recorded so a later run can prove an If-Range # resume of this part file safe. _write_download_meta(meta, url, validator, expected_total) - _stream_response_to_file(resp, f, offset, size) + _stream_response_to_file(resp, f, offset, size, progress) # else: a previous run already wrote every byte (or more) but # was killed before the rename below. Skip the network entirely # — a Range request past EOF would draw HTTP 416 — and let @@ -831,6 +938,10 @@ def download_with_resume( expected_size = size if size is not None else expected_total _verify_file(part, sha256, expected_size or None) + if progress is not None: + # Also credits a part file an earlier run completed without + # streaming anything this time. + progress(expected_size or part.stat().st_size) if not expected_size and sha256 is None: # No sha, no size, and the server sent no usable # content-length: nothing can prove the download complete @@ -933,6 +1044,7 @@ def _try_mirrors_once( f: IO[bytes] | None, timeout: int, failures: list[tuple[str, Exception]], + progress: Callable[[int], None] | None = None, ) -> str | None: """Single pass over the resolved mirror ``urls``, one try per URL. @@ -961,6 +1073,7 @@ def _try_mirrors_once( # next mirror immediately; only mid-stream drops # retry-with-resume on the same URL. retry_connect_errors=False, + progress=progress, ) return url except (requests.RequestException, OSError, EsphomeError) as e: @@ -1002,7 +1115,7 @@ def _try_mirrors_once( if offset == 0: validator = _response_validator(resp) expected_total = _content_length(resp) - _stream_response_to_file(resp, f, offset) + _stream_response_to_file(resp, f, offset, progress=progress) if expected_total and f.tell() != expected_total: raise EsphomeError( @@ -1051,6 +1164,7 @@ def download_from_mirrors( substitutions: dict[str, str], target: io.RawIOBase | IO[bytes] | PathType, timeout: int = 30, + progress: Callable[[int], None] | None = None, ) -> str: """ Download file from multiple mirrors with substitution support. @@ -1060,6 +1174,8 @@ def download_from_mirrors( substitutions: Dictionary of substitutions to apply to URLs target: Target file path or file-like object timeout: Download timeout in seconds + progress: Passed through to the download (see ``download_with_resume``); + replaces the built-in per-file bar Returns: The source URL. @@ -1124,7 +1240,9 @@ def download_from_mirrors( for sweep in range(1, _MIRROR_SWEEP_ATTEMPTS + 1): sweep_failures: list[tuple[str, Exception]] = [] if ( - url := _try_mirrors_once(urls, path_target, f, timeout, sweep_failures) + url := _try_mirrors_once( + urls, path_target, f, timeout, sweep_failures, progress + ) ) is not None: return url failures.extend(sweep_failures) diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py new file mode 100644 index 0000000000..2d5667a3e5 --- /dev/null +++ b/esphome/platformio/extra_script.py @@ -0,0 +1,253 @@ +"""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 +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 + # 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({"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 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() + + # ----- 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 __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) + + # ----- 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] = [] + library_root = library_dir.resolve() + for path in result.libpath: + # Anchor relative paths to library_dir; the script's CWD has been + # restored by now + 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: + # 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) + flags.extend(result.linkflags) + flags.extend(result.cppflags) + return flags diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index ee0a758a31..24bd9db490 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -13,7 +13,8 @@ regardless of which toolchain consumes the result. """ from collections import deque -from collections.abc import Callable +from collections.abc import Callable, Iterable +from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass, field import glob import hashlib @@ -30,7 +31,14 @@ from urllib.request import url2pathname from esphome import git from esphome.core import CORE, EsphomeError, Library -from esphome.framework_helpers import archive_extract_all, download_from_mirrors, rmdir +from esphome.framework_helpers import ( + BATCH_DOWNLOAD_WORKERS, + BatchDownloadProgress, + archive_extract_all, + download_from_mirrors, + rmdir, + run_batch_downloads, +) _LOGGER = logging.getLogger(__name__) @@ -47,20 +55,25 @@ 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. +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" @@ -70,7 +83,12 @@ ESPHOME_DATA_EXTRA_CMAKE_KEY = "EXTRA_CMAKE" class Source: def download( - self, dir_suffix: str, force: bool = False, salt: str = "", namespace: str = "" + self, + dir_suffix: str, + force: bool = False, + salt: str = "", + namespace: str = "", + progress: Callable[[int], None] | None = None, ) -> Path: raise NotImplementedError @@ -87,9 +105,7 @@ class URLSource(Source): def __init__(self, url: str): self.url = url - def download( - self, dir_suffix: str, force: bool = False, salt: str = "", namespace: str = "" - ) -> Path: + def _cache_dir(self, dir_suffix: str, salt: str, namespace: str) -> Path: # Namespace the cache per backend (e.g. pio_components/idf, .../zephyr) so # the build files each backend writes into the library dir can't collide. base_dir = Path(CORE.data_dir) / DOMAIN @@ -99,7 +115,23 @@ class URLSource(Source): h.update(self.url.encode()) if salt: h.update(salt.encode()) - path = base_dir / h.hexdigest()[:8] / dir_suffix + return base_dir / h.hexdigest()[:8] / dir_suffix + + def is_cached(self, dir_suffix: str, salt: str = "", namespace: str = "") -> bool: + """Whether a completed extraction already exists for this source.""" + return ( + self._cache_dir(dir_suffix, salt, namespace) / ".esphome_extracted" + ).is_file() + + def download( + self, + dir_suffix: str, + force: bool = False, + salt: str = "", + namespace: str = "", + progress: Callable[[int], None] | None = None, + ) -> Path: + path = self._cache_dir(dir_suffix, salt, namespace) # Marker file written last to signal a complete extraction. Using a # marker (instead of just `path.is_dir()`) means an interrupted # extraction is correctly detected and re-run on the next invocation, @@ -111,10 +143,12 @@ class URLSource(Source): # Download in temporary file with tempfile.NamedTemporaryFile() as tmp: - _LOGGER.info("Downloading %s ...", self.url) + if progress is None: + # A batch caller draws one combined bar and logs the list + _LOGGER.info("Downloading %s ...", self.url) _LOGGER.debug("Location: %s", path) - download_from_mirrors([self.url], {}, tmp.file) + download_from_mirrors([self.url], {}, tmp.file, progress=progress) _LOGGER.debug("Extracting archive to %s ...", path) archive_extract_all(tmp.file, path) @@ -131,7 +165,12 @@ class GitSource(Source): self.ref = ref def download( - self, dir_suffix: str, force: bool = False, salt: str = "", namespace: str = "" + self, + dir_suffix: str, + force: bool = False, + salt: str = "", + namespace: str = "", + progress: Callable[[int], None] | None = None, ) -> Path: domain = DOMAIN if namespace: @@ -166,7 +205,12 @@ class LocalSource(Source): self.local_path = path def download( - self, dir_suffix: str, force: bool = False, salt: str = "", namespace: str = "" + self, + dir_suffix: str, + force: bool = False, + salt: str = "", + namespace: str = "", + progress: Callable[[int], None] | None = None, ) -> Path: src = Path(self.local_path) if not src.is_dir(): @@ -203,6 +247,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. @@ -251,7 +303,13 @@ class ConvertedLibrary: def get_require_name(self): return self.get_sanitized_name().replace("/", "__") - def download(self, force: bool = False, salt: str = "", namespace: str = ""): + def download( + self, + force: bool = False, + salt: str = "", + namespace: str = "", + progress: Callable[[int], None] | None = None, + ): """Fetch the library into the shared cache and record its ``path``. The cache directory is named after the sanitized library name; backends @@ -260,7 +318,11 @@ class ConvertedLibrary: ``get_require_name``). ``namespace`` keeps each backend's cache separate. """ self.path = self.source.download( - self.get_sanitized_name(), force=force, salt=salt, namespace=namespace + self.get_sanitized_name(), + force=force, + salt=salt, + namespace=namespace, + progress=progress, ) self.source_path = self.source.source_root(self.path) @@ -432,7 +494,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 +517,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 +531,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 +615,127 @@ 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.""" + 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 + 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"): + _LOGGER.warning( + "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 +744,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 +880,111 @@ 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 _content_lengths(urls: list[str]) -> list[int | None]: + """Content-Length per URL via HEAD requests; None when unknown.""" + import requests + + def head(url: str) -> int | None: + try: + resp = requests.head(url, timeout=10, allow_redirects=True) + if not resp.ok: + _LOGGER.debug("HEAD %s returned %s", url, resp.status_code) + return None + return int(resp.headers.get("content-length", 0)) or None + except (requests.RequestException, ValueError) as err: + _LOGGER.debug("HEAD %s failed: %s", url, err) + return None + + with ThreadPoolExecutor(max_workers=min(BATCH_DOWNLOAD_WORKERS, len(urls))) as ex: + return list(ex.map(head, urls)) + + +def _prefetch_wave( + wave: list[tuple[str, ConvertedLibrary]], salt: str, namespace: str +) -> None: + """Best-effort parallel download of a wave's registry archives. + + The walk's own ``download()`` call stays authoritative (it surfaces real + failures, with resume); bars are suppressed since parallel bars would + interleave. Duplicate URLs prefetch once so two threads never extract + into the same cache directory. + """ + components: list[ConvertedLibrary] = [] + seen: set[str] = set() + for _key, component in wave: + if not isinstance(component.source, URLSource): + continue + if component.source.url in seen: + continue + seen.add(component.source.url) + try: + cached = component.source.is_cached( + component.get_sanitized_name(), salt=salt, namespace=namespace + ) + except Exception as err: # noqa: BLE001 # pylint: disable=broad-exception-caught + # Best-effort: a failing probe prefetches (and re-downloads) + _LOGGER.debug("Cache probe for %s failed: %s", component.name, err) + cached = False + if cached: + # A completed extraction downloads nothing; a warm build must + # stay silent + continue + components.append(component) + if len(components) < 2: + return + _LOGGER.info( + "Downloading %d libraries: %s", + len(components), + ", ".join(c.name for c in components), + ) + # One combined bar over the batch, sized by HEAD requests. An unknown + # size would mean a silent multi-MB download; fall back to sequential + # downloads with their per-file bars instead. + sizes = _content_lengths([c.source.url for c in components]) + if not all(sizes): + # Name the culprits so the fallback is distinguishable from a hang + _LOGGER.debug( + "No Content-Length for %s; downloading sequentially", + ", ".join( + c.source.url + for c, size in zip(components, sizes, strict=True) + if not size + ), + ) + return + + def _fetch(component: ConvertedLibrary): + return lambda tracker: component.download( + salt=salt, namespace=namespace, progress=tracker + ) + + failures = run_batch_downloads( + BatchDownloadProgress("Downloading libraries", sum(sizes)), + [(component.name, _fetch(component)) for component in components], + ) + for name, err in failures: + # The sequential call below retries and raises the real error + _LOGGER.warning("Prefetch of %s failed (retrying sequentially): %s", name, err) + + def convert_libraries( libraries: list[Library], backend: LibraryBackend ) -> list[ConvertedLibrary]: @@ -713,10 +1010,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 +1022,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 +1070,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 @@ -792,105 +1081,126 @@ def convert_libraries( top_level_keys = set(top_level) worklist = deque(dict.fromkeys(top_level)) while worklist: - key = worklist.popleft() - node = nodes[key] + # Drain the frontier sequentially (spec resolution mutates shared + # node state), then prefetch the wave's registry archives in + # parallel; the per-component download() below stays authoritative. + wave: list[tuple[str, ConvertedLibrary]] = [] + while worklist: + 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. - requirements = frozenset(node.requirements) - if resolved_requirements.get(key) == requirements: - continue - resolved_requirements[key] = requirements + # 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 + resolved_requirements[key] = requirements - if node.is_git: - component = ConvertedLibrary(key, "*", GitSource(node.url, node.ref)) - elif node.is_local: - component = ConvertedLibrary(key, "*", LocalSource(node.local_path)) - else: - owner, name, version, url = _resolve_registry_version( - node.owner, node.pkgname, node.requirements - ) - component = ConvertedLibrary( - _owner_pkgname_to_name(owner, name), version, URLSource(url) - ) - component.download(salt=salt, namespace=backend.cache_key) + if node.is_git: + component = ConvertedLibrary(key, "*", GitSource(node.url, node.ref)) + elif node.is_local: + component = ConvertedLibrary(key, "*", LocalSource(node.local_path)) + else: + owner, name, version, url = _resolve_registry_version( + node.owner, node.pkgname, node.requirements + ) + component = ConvertedLibrary( + _owner_pkgname_to_name(owner, name), version, URLSource(url) + ) + wave.append((key, component)) + _prefetch_wave(wave, salt, backend.cache_key) + for key, component in wave: + node = nodes[key] + component.download(salt=salt, namespace=backend.cache_key) - source_dir = component.source_dir - library_json_path = source_dir / "library.json" - library_properties_path = source_dir / "library.properties" - 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. - _LOGGER.warning( - "Library %s at %s is missing library.json and library.properties; " - "re-downloading", - key, - source_dir, - ) - component.download(force=True, salt=salt, namespace=backend.cache_key) + source_dir = component.source_dir + library_json_path = source_dir / "library.json" + library_properties_path = source_dir / "library.properties" 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) - elif has_properties: - 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. - 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 has_json and not has_properties and not node.is_local: + # 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", + key, + source_dir, + ) + component.download(force=True, salt=salt, namespace=backend.cache_key) + 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) + elif has_properties: + component.data = parse_library_properties(library_properties_path) + else: + # 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}" + ) - 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. - 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)) - continue - components[key] = component + 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) - # 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: - continue try: - check_library_data(dependency, backend.platform, backend.framework) + check_library_data(component.data, backend.platform, backend.framework) except InvalidLibrary as e: - _LOGGER.debug("Skip dependency %s: %s", dependency.get("name"), str(e)) + # 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 + 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 - dep_name = _owner_pkgname_to_name( - dependency.get("owner"), dependency.get("name") - ) - if is_ignored(dep_name): - _LOGGER.debug("Skip ignored dependency %s", dep_name) - continue - # The version field may actually be a URL (git/archive dependency). - dep_version = dependency["version"] - dep_url = _url_or_none(dep_version) - if dep_url is not None: - dep_version = None - dep_key = add_spec(dep_name, dep_version, dep_url) - node.edges.add(dep_key) - worklist.append(dep_key) + 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"), component.name + ): + if "version" not in dependency: + # Cannot resolve from the registry; the arduino-backend + # PR adds the reconciliation that reports real drops + _LOGGER.debug( + "Skip version-less dependency %r of %s", + dependency.get("name"), + component.name, + ) + continue + 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_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). + dep_version = dependency["version"] + dep_url = _url_or_none(dep_version) + if dep_url is not None: + dep_version = None + dep_key = add_spec(dep_name, dep_version, dep_url) + node.edges.add(dep_key) + worklist.append(dep_key) # A git or local source wins over the same component requested from the # registry. That's intentional, but warn so the dropped registry spec isn't 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/tests/unit_tests/test_espidf_component.py b/tests/unit_tests/test_espidf_component.py index f9e048f6f4..c5f889a37a 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 == [ @@ -1187,6 +1066,36 @@ def test_idf_component_download_passes_salt() -> None: c.download(force=True, salt="abcd1234", namespace="idf") source.download.assert_called_once_with( - "owner/name", force=True, salt="abcd1234", namespace="idf" + "owner/name", force=True, salt="abcd1234", namespace="idf", progress=None ) 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_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index d8e7738569..b5296def66 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -2,6 +2,7 @@ # pylint: disable=protected-access +from concurrent.futures import ThreadPoolExecutor from contextlib import contextmanager import importlib.util import io @@ -14,7 +15,7 @@ import subprocess import sys import tarfile from types import SimpleNamespace -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest @@ -887,6 +888,58 @@ _PREFETCH_JSON = json.dumps( ) +def test_prefetch_leaves_unverifiable_entries_to_the_installer( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """An entry missing sha256 or size must not download unverified; the + installer handles it and fails loudly on a bad archive.""" + entries = json.loads(_PREFETCH_JSON) + del entries[0]["sha256"] + del entries[1]["size"] + entries.append( + { + "name": "gcc@14.2.0", + "url": "https://example.com/gcc.tar.gz", + "size": 67, + "sha256": "ef" * 32, + "dest": "gcc.tar.gz", + } + ) + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, json.dumps(entries), ""), + ), + patch("esphome.espidf.framework.download_with_resume") as download, + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch("esphome.espidf.framework.BatchDownloadProgress") as progress_cls, + ): + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + assert [call[0][0] for call in download.call_args_list] == [ + "https://example.com/gcc.tar.gz" + ] + assert download.call_args[1]["sha256"] == "ef" * 32 + progress_cls.assert_called_once_with("Downloading ESP-IDF tools", 67) + assert "cmake@3.30.2 has no sha256/size" in caplog.text + assert "ninja@1.12.1 has no sha256/size" in caplog.text + + +def test_prefetch_all_entries_unverifiable_is_a_noop(tmp_path: Path) -> None: + entries = json.loads(_PREFETCH_JSON) + for entry in entries: + del entry["sha256"] + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, json.dumps(entries), ""), + ), + patch("esphome.espidf.framework.download_with_resume") as download, + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + ): + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + download.assert_not_called() + + def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: with ( patch( @@ -895,16 +948,73 @@ def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: ), patch("esphome.espidf.framework.download_with_resume") as download, patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch("esphome.espidf.framework.BatchDownloadProgress") as progress_cls, ): + # Materialize the lazy mock before threads race its first creation + tracker = progress_cls.return_value.tracker.return_value _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) dist = get_idf_tools_path() / "dist" - assert download.call_count == 2 - assert download.call_args_list[0][0] == ( - "https://example.com/cmake.tar.gz", - dist / "cmake-3.30.2.tar.gz", - ) - assert download.call_args_list[0][1] == {"sha256": "ab" * 32, "size": 123} + # Archives download concurrently, so the call order is not fixed. + calls = {call[0]: call[1] for call in download.call_args_list} + assert set(calls) == { + ("https://example.com/cmake.tar.gz", dist / "cmake-3.30.2.tar.gz"), + ("https://example.com/ninja.zip", dist / "ninja.zip"), + } + kwargs = calls[("https://example.com/cmake.tar.gz", dist / "cmake-3.30.2.tar.gz")] + assert kwargs["sha256"] == "ab" * 32 + assert kwargs["size"] == 123 + # every archive reports into the one combined progress bar + progress_cls.assert_called_once_with("Downloading ESP-IDF tools", 123 + 45) + assert all(kw["progress"] is tracker for kw in calls.values()) + + +def test_prefetch_downloads_archives_concurrently(tmp_path: Path) -> None: + """More than one archive fans out over a bounded thread pool.""" + entries = [ + { + "name": f"tool{i}@1", + "url": f"https://example.com/tool{i}.tar.gz", + "size": 10, + "sha256": "ab" * 32, + "dest": f"tool{i}.tar.gz", + } + for i in range(6) + ] + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, json.dumps(entries), ""), + ), + patch("esphome.espidf.framework.download_with_resume") as download, + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch( + "esphome.framework_helpers.ThreadPoolExecutor", wraps=ThreadPoolExecutor + ) as pool, + ): + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + + pool.assert_called_once_with(max_workers=4) + assert download.call_count == 6 + + +def test_prefetch_single_archive_uses_one_worker(tmp_path: Path) -> None: + entries = json.loads(_PREFETCH_JSON)[:1] + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, json.dumps(entries), ""), + ), + patch("esphome.espidf.framework.download_with_resume") as download, + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch( + "esphome.framework_helpers.ThreadPoolExecutor", wraps=ThreadPoolExecutor + ) as pool, + ): + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + + pool.assert_called_once_with(max_workers=1) + assert download.call_count == 1 def test_prefetch_skips_already_downloaded_archives(tmp_path: Path) -> None: @@ -964,6 +1074,11 @@ def test_prefetch_one_failed_archive_does_not_stop_the_rest( ) -> None: """A single archive failing its download must not abort the prefetch of the remaining archives.""" + + def _fail_cmake_download(url: str, *args, **kwargs) -> None: + if "cmake" in url: + raise OSError("network down") + with ( patch( "esphome.espidf.framework.run_command", @@ -971,7 +1086,7 @@ def test_prefetch_one_failed_archive_does_not_stop_the_rest( ), patch( "esphome.espidf.framework.download_with_resume", - side_effect=[OSError("network down"), None], + side_effect=_fail_cmake_download, ) as download, patch("esphome.espidf.framework.get_system_python_path", return_value="python"), ): @@ -981,6 +1096,29 @@ def test_prefetch_one_failed_archive_does_not_stop_the_rest( assert "Could not prefetch cmake@3.30.2" in caplog.text +def test_prefetch_finishes_progress_bar_and_cancels_queue(tmp_path: Path) -> None: + """The batch bar is closed out after the pool, and the pool is shut down + with cancel_futures so Ctrl-C does not drain every queued archive.""" + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, _PREFETCH_JSON, ""), + ), + patch("esphome.espidf.framework.download_with_resume"), + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch("esphome.espidf.framework.BatchDownloadProgress") as progress_cls, + patch( + "esphome.framework_helpers.ThreadPoolExecutor", wraps=ThreadPoolExecutor + ) as pool_cls, + ): + pool = MagicMock(wraps=ThreadPoolExecutor(max_workers=2)) + pool_cls.return_value = pool + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + + pool.shutdown.assert_called_once_with(wait=True, cancel_futures=True) + progress_cls.return_value.done.assert_called_once_with() + + def test_prefetch_passes_targets_and_tools_to_script(tmp_path: Path) -> None: with ( patch( diff --git a/tests/unit_tests/test_framework_helpers.py b/tests/unit_tests/test_framework_helpers.py index 2022c15bfe..098dcb7725 100644 --- a/tests/unit_tests/test_framework_helpers.py +++ b/tests/unit_tests/test_framework_helpers.py @@ -21,6 +21,7 @@ import requests as req from esphome import framework_helpers from esphome.core import EsphomeError from esphome.framework_helpers import ( + BatchDownloadProgress, _7z_extract_all, _detect_archive_root, _is_transient_download_error, @@ -1112,6 +1113,108 @@ class TestDownloadWithResume: assert mock_get.call_args[1]["headers"] == {} assert dest.read_bytes() == b"data" + def test_progress_callback_reports_absolute_bytes(self, tmp_path: Path) -> None: + """With a callback no bar is drawn; the callback sees the running + byte count of this file, then its final verified size.""" + dest = tmp_path / "tool.tar.gz" + resp = _mock_response(b"") + resp.headers = {"content-length": "7"} + resp.iter_content.return_value = [b"1234", b"567"] + seen: list[int] = [] + with ( + patch("requests.get", return_value=resp), + patch("esphome.framework_helpers.ProgressBar") as bar, + ): + download_with_resume( + "https://example.com/t", dest, size=7, progress=seen.append + ) + assert seen == [0, 4, 7, 7] + bar.assert_not_called() + + def test_progress_callback_seeds_with_resume_offset(self, tmp_path: Path) -> None: + dest = tmp_path / "tool.tar.gz" + (tmp_path / "tool.tar.gz.part").write_bytes(b"12345") + good = hashlib.sha256(b"12345678").hexdigest() + seen: list[int] = [] + with patch("requests.get", return_value=_resumed_response(b"678")): + download_with_resume( + "https://example.com/t", dest, sha256=good, size=8, progress=seen.append + ) + assert seen[0] == 5 + assert seen[-1] == 8 + + def test_progress_callback_credits_already_complete_download( + self, tmp_path: Path + ) -> None: + """A verified dest from an earlier run still counts toward the batch.""" + dest = tmp_path / "tool.tar.gz" + dest.write_bytes(b"12345678") + seen: list[int] = [] + with patch("requests.get") as mock_get: + download_with_resume( + "https://example.com/t", dest, size=8, progress=seen.append + ) + mock_get.assert_not_called() + assert seen == [8] + + +class TestBatchDownloadProgress: + def test_sums_trackers_into_one_bar(self) -> None: + with patch("esphome.framework_helpers.ProgressBar") as bar_cls: + progress = BatchDownloadProgress("Downloading", 100) + a = progress.tracker() + b = progress.tracker() + a(10) + b(20) + a(30) + a(0) # a restart from zero takes that file's bytes back out + bar_cls.assert_called_once_with("Downloading") + updates = [c[0][0] for c in bar_cls.return_value.update.call_args_list] + assert updates == [0.1, 0.3, 0.5, 0.2] + + def test_clamps_at_one(self) -> None: + """Sizes are advisory; an over-delivering server never pushes past 100%.""" + with patch("esphome.framework_helpers.ProgressBar") as bar_cls: + progress = BatchDownloadProgress("Downloading", 10) + progress.tracker()(25) + assert bar_cls.return_value.update.call_args[0][0] == 1 + + def test_unknown_total_draws_nothing(self) -> None: + with patch("esphome.framework_helpers.ProgressBar") as bar_cls: + progress = BatchDownloadProgress("Downloading", 0) + progress.tracker()(5) + progress.done() + bar_cls.assert_not_called() + + def test_done_ends_an_unfinished_bar(self) -> None: + """A batch that stops short of 100% (a failed archive) still ends its + line so the next log message starts on a fresh row.""" + stream = io.StringIO() + stream.isatty = lambda: True # type: ignore[method-assign] + with patch("esphome.helpers.sys.stderr", stream): + progress = BatchDownloadProgress("Downloading", 10) + progress.tracker()(5) + progress.done() + assert stream.getvalue().endswith("50% \n") + + def test_done_before_any_frame_writes_nothing(self) -> None: + """A batch aborted before any tracker fired must not emit a stray + newline for a bar that was never drawn.""" + stream = io.StringIO() + stream.isatty = lambda: True # type: ignore[method-assign] + with patch("esphome.helpers.sys.stderr", stream): + BatchDownloadProgress("Downloading", 10).done() + assert stream.getvalue() == "" + + def test_done_after_full_bar_adds_nothing(self) -> None: + stream = io.StringIO() + stream.isatty = lambda: True # type: ignore[method-assign] + with patch("esphome.helpers.sys.stderr", stream): + progress = BatchDownloadProgress("Downloading", 10) + progress.tracker()(10) + progress.done() + assert stream.getvalue().endswith("100% Done...\r\n") + class TestDownloadFromMirrors: def test_success_returns_url_and_writes_content(self, tmp_path: Path) -> None: 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..5df8e3c9c5 --- /dev/null +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -0,0 +1,377 @@ +"""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 + + +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"] + + 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).""" + + (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): + + 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): + + 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_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") + + +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 + + +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(CPPPATH=["a"]) + env.Append(CPPPATH=["b"]) + assert caplog.text.count("env.Append(CPPPATH=...) is not captured") == 1 diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 0eede78656..25d074e26f 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -4,15 +4,18 @@ 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 +from types import SimpleNamespace 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 +26,8 @@ from esphome.platformio.library import ( _resolve_registry_version, check_library_data, convert_libraries, + join_flag_args, + split_flag_entry, ) @@ -150,11 +155,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, progress=None: dl_calls.append(urls), ) def fake_extract(fileobj, path): @@ -173,6 +197,12 @@ def test_urlsource_download_extracts_then_reuses_marker(setup_core, monkeypatch) assert out2 == out assert len(dl_calls) == 1 + # A batch caller passes a tracker and owns the messaging; no per-file INFO + with caplog_at_info() as records: + src.download("mylib-batch", progress=lambda done: None) + assert len(dl_calls) == 2 + assert not [r for r in records if "Downloading" in r.message] + def test_resolve_registry_version_raises_without_pkg_file(monkeypatch): registry = lib._make_registry_client() @@ -213,7 +243,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 +322,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 +565,322 @@ 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_prefetch_wave_downloads_registry_archives_in_parallel( + setup_core, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Registry archives in one wave download concurrently, deduped by URL; + git/local sources and failures are left to the sequential call.""" + calls: list[str] = [] + + def fake_download(self, force=False, salt="", namespace="", progress=None): + calls.append(self.source.url) + if progress is not None: + progress(0) + if "boom" in self.source.url: + raise RuntimeError("boom") + + monkeypatch.setattr(ConvertedLibrary, "download", fake_download) + monkeypatch.setattr(lib, "_content_lengths", lambda urls: [1] * len(urls)) + wave = [ + ("a", ConvertedLibrary("a", "1.0", URLSource("https://x/a.tar.gz"))), + ("b", ConvertedLibrary("b", "1.0", URLSource("https://x/b.tar.gz"))), + # Duplicate URL must prefetch once (two threads must never extract + # into the same cache directory) + ("b2", ConvertedLibrary("b2", "1.0", URLSource("https://x/b.tar.gz"))), + ("c", ConvertedLibrary("c", "1.0", URLSource("https://x/boom.tar.gz"))), + ("g", ConvertedLibrary("g", "*", lib.GitSource("https://x/g.git", None))), + ] + lib._prefetch_wave(wave, "", "idf") + assert sorted(calls) == [ + "https://x/a.tar.gz", + "https://x/b.tar.gz", + "https://x/boom.tar.gz", + ] + # The failure surfaces at default verbosity, after the bar + assert "Prefetch of c failed (retrying sequentially)" in caplog.text + + +def test_prefetch_wave_unknown_size_falls_back_to_sequential( + setup_core, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Any unknown HEAD size skips the parallel prefetch entirely so the + sequential downloads keep their per-file bars.""" + + def fail_download(self, force=False, salt="", namespace="", progress=None): + raise AssertionError("prefetched despite unknown size") + + monkeypatch.setattr(ConvertedLibrary, "download", fail_download) + monkeypatch.setattr(lib, "_content_lengths", lambda urls: [1, None]) + wave = [ + ("a", ConvertedLibrary("a", "1.0", URLSource("https://x/a.tar.gz"))), + ("b", ConvertedLibrary("b", "1.0", URLSource("https://x/b.tar.gz"))), + ] + caplog.set_level("DEBUG") + lib._prefetch_wave(wave, "", "idf") + # The culprit URL is named so the fallback is traceable + assert "No Content-Length for https://x/b.tar.gz" in caplog.text + + +def test_content_lengths_head_requests(monkeypatch: pytest.MonkeyPatch) -> None: + """Sizes come from HEAD Content-Length; a failing HEAD reads as 0 so + the combined bar is skipped rather than wrong.""" + import requests + + def fake_head(url, timeout, allow_redirects): + if "bad" in url: + raise requests.ConnectionError("down") + if "gone" in url: + return SimpleNamespace(ok=False, status_code=404, headers={}) + if "garbage" in url: + # A proxy/CDN doubling the header ("123, 123") or emitting junk + # must degrade to unknown, not ValueError the build + return SimpleNamespace(ok=True, headers={"content-length": "123, 123"}) + return SimpleNamespace(ok=True, headers={"content-length": "123"}) + + monkeypatch.setattr( + lib.requests if hasattr(lib, "requests") else requests, "head", fake_head + ) + # None marks an unknown size (probe failure or non-2xx), distinct + # from a genuine zero + assert lib._content_lengths( + ["https://x/a", "https://x/bad", "https://x/gone", "https://x/garbage"] + ) == [ + 123, + None, + None, + None, + ] + + +def test_prefetch_wave_cache_probe_failure_still_prefetches( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The cache probe is best-effort; a failing probe prefetches anyway.""" + calls: list[str] = [] + monkeypatch.setattr( + ConvertedLibrary, + "download", + lambda self, **kw: calls.append(self.source.url), + ) + monkeypatch.setattr( + URLSource, + "is_cached", + lambda self, *a, **kw: (_ for _ in ()).throw(RuntimeError("no core")), + ) + monkeypatch.setattr(lib, "_content_lengths", lambda urls: [1] * len(urls)) + wave = [ + ("a", ConvertedLibrary("a", "1.0", URLSource("https://x/a.tar.gz"))), + ("b", ConvertedLibrary("b", "1.0", URLSource("https://x/b.tar.gz"))), + ] + lib._prefetch_wave(wave, "", "idf") + assert sorted(calls) == ["https://x/a.tar.gz", "https://x/b.tar.gz"] + + +def test_prefetch_wave_warm_cache_is_silent( + setup_core, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Already-extracted archives download nothing; a warm build must not + print a Downloading line or draw a bar.""" + monkeypatch.setattr( + ConvertedLibrary, + "download", + lambda self, **kw: (_ for _ in ()).throw(AssertionError("downloaded")), + ) + wave = [] + for name in ("a", "b", "c"): + comp = ConvertedLibrary(name, "1.0", URLSource(f"https://x/{name}.tar.gz")) + marker_dir = comp.source._cache_dir(comp.get_sanitized_name(), "", "idf") + marker_dir.mkdir(parents=True) + (marker_dir / ".esphome_extracted").touch() + wave.append((name, comp)) + lib._prefetch_wave(wave, "", "idf") + assert "Downloading" not in caplog.text + + +def test_prefetch_wave_single_archive_skips_the_pool( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """One archive gains nothing from a pool; the sequential call keeps its + progress bar.""" + monkeypatch.setattr( + ConvertedLibrary, + "download", + lambda self, **kw: (_ for _ in ()).throw(AssertionError("prefetched")), + ) + lib._prefetch_wave( + [("a", ConvertedLibrary("a", "1.0", URLSource("https://x/a.tar.gz")))], + "", + "idf", + ) + + +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",), + ) + 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 030e79fa5a50b71c10fb99b1d686d5098ba1e888 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 22:04:47 -0500 Subject: [PATCH 3/3] Keep the empty-argument flag check next to the lexer that leaves the bare token --- esphome/platformio/library.py | 15 +++++++++++++++ tests/unit_tests/test_platformio_library.py | 7 +++++++ 2 files changed, 22 insertions(+) diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 24bd9db490..86139d61d7 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -645,6 +645,21 @@ def lex_build_flags(entries: str | list[str], owner: str) -> list[str]: BARE_ARG_FLAGS = frozenset({"-I", "-L", "-l", "-D"}) +def raise_on_empty_arg_flags(tokens: list[str], owner: str) -> None: + """Reject bare ``-I``/``-D``/``-L``/``-l`` tokens left by an empty glued + argument (``-D ""``). + + Lives next to ``join_flag_args`` because the bare token is its + postcondition: a trailing bare flag is warned and dropped there, so a + surviving one always means an empty argument. gcc would eat the next + flag as the argument (or add the CWD for ``-L``); always a typo. + """ + if empty := sorted({tok for tok in tokens if tok in BARE_ARG_FLAGS}): + raise EsphomeError( + f"{owner} contain empty-argument flag(s): {', '.join(empty)}" + ) + + 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.""" diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 25d074e26f..4cc5e6ba0a 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -656,6 +656,13 @@ def test_prefetch_wave_unknown_size_falls_back_to_sequential( assert "No Content-Length for https://x/b.tar.gz" in caplog.text +def test_raise_on_empty_arg_flags() -> None: + """A surviving bare flag means an empty glued argument; reject by name.""" + with pytest.raises(EsphomeError, match=r"build_flags contain empty-argument"): + lib.raise_on_empty_arg_flags(["-DFOO", "-D", "-l"], "build_flags") + lib.raise_on_empty_arg_flags(["-DFOO", "-Iinc"], "build_flags") + + def test_content_lengths_head_requests(monkeypatch: pytest.MonkeyPatch) -> None: """Sizes come from HEAD Content-Length; a failing HEAD reads as 0 so the combined bar is skipped rather than wrong."""