From 7a3f700206f574206f06b8c5adc77fc47f0a6139 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 20 Aug 2026 15:41:39 -0500 Subject: [PATCH] Act on bad-compiler detection, atomic logged idedata cache, memoized entry parsing, callable board_mcu --- esphome/build_helpers/idedata.py | 69 +++++++++++++----- esphome/espidf/component.py | 20 ++--- esphome/platformio/extra_script.py | 10 +-- .../unit_tests/build_helpers/test_idedata.py | 73 ++++++++++++++++--- tests/unit_tests/test_espidf_component.py | 9 ++- .../test_platformio_extra_script.py | 16 ++-- 6 files changed, 138 insertions(+), 59 deletions(-) diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 86391e688b..42dbccfdae 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -11,7 +11,6 @@ consumers (IDE integration, clang-tidy) expect: from __future__ import annotations -import functools import json import logging import os @@ -20,6 +19,8 @@ import re import shlex import subprocess +from esphome.helpers import write_file + _LOGGER = logging.getLogger(__name__) # C++ translation-unit suffixes used to identify ESPHome source files. @@ -132,11 +133,16 @@ _COMPILER_STEM = re.compile( ) -@functools.cache +# Deduplicates the warning across a compile DB of hundreds of entries; +# cleared per idedata build (a module-level functools.cache would suppress +# the warning for the process lifetime in non-forking hosts). +_warned_not_a_compiler: set[str] = set() + + def _warn_not_a_compiler(token: str) -> None: - # A stale compile DB built with a launcher the current run no longer - # configures would otherwise cache the launcher as the compiler path. - # Cached so a database of hundreds of entries warns once per path. + if token in _warned_not_a_compiler: + return + _warned_not_a_compiler.add(token) _LOGGER.warning("compile_commands entry does not start with a compiler: %s", token) @@ -163,6 +169,14 @@ def parse_entry( # build, so this is a comparison, not a guess by name. if launcher is not None and tokens[0] == launcher: tokens = tokens[1:] + if ( + not _COMPILER_STEM.search(Path(tokens[0]).stem) + and len(tokens) > 1 + and _COMPILER_STEM.search(Path(tokens[1]).stem) + ): + # A stale compile DB built with a launcher the current run no longer + # configures: the real compiler is the next token. + tokens = tokens[1:] if not _COMPILER_STEM.search(Path(tokens[0]).stem): _warn_not_a_compiler(tokens[0]) # token0 is the compiler path; the rest of the command already uses forward @@ -268,8 +282,10 @@ def load_or_build_idedata( 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 + except ValueError 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: # Caches written before cc_path was emitted stay newer than # compile_commands.json forever, so rebuild them on the field rather @@ -280,29 +296,48 @@ def load_or_build_idedata( data = idedata_from_build(compile_commands, launcher) data["prog_path"] = str(elf_path) - cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") + if _COMPILER_STEM.search(Path(data["cxx_path"]).stem): + 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") + else: + # parse_entry already warned; serve the data for this run but never + # persist a known-bad compiler path (the cache would outlive the + # timestamp check and hide the fault) + _LOGGER.debug("Not caching idedata with unrecognized compiler path") 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). """ + _warned_not_a_compiler.clear() entries = json.loads(Path(compile_commands).read_text(encoding="utf-8")) - cxx_path, defines, _, cxx_flags = parse_entry(_pick_entry(entries), launcher) + + # ninja-generated compile DBs repeat one identical multi-KB command per + # source file; parse each distinct (directory, command) once. + parsed: dict[tuple[str, str], tuple[str, list[str], list[str], list[str]]] = {} + + def _parse(entry: dict) -> tuple[str, list[str], list[str], list[str]]: + key = (entry["directory"], entry["command"]) + if key not in parsed: + parsed[key] = parse_entry(entry, launcher) + return parsed[key] + + cxx_path, defines, _, cxx_flags = _parse(_pick_entry(entries)) build_includes: dict[str, None] = {} for entry in entries: if not _is_esphome_src(entry["file"]): continue - for inc in parse_entry(entry, launcher)[2]: + for inc in _parse(entry)[2]: build_includes.setdefault(inc, None) return { diff --git a/esphome/espidf/component.py b/esphome/espidf/component.py index 604a61af00..5e16a51623 100644 --- a/esphome/espidf/component.py +++ b/esphome/espidf/component.py @@ -42,17 +42,6 @@ def _idf_framework() -> str: return "arduino" if CORE.using_arduino else "espidf" -def _apply_extra_script(component: IDFComponent) -> None: - 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", - ) - - def generate_cmakelists_txt(component: IDFComponent) -> str: """ Generate a CMakeLists.txt file for an ESP-IDF component. @@ -272,7 +261,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/platformio/extra_script.py b/esphome/platformio/extra_script.py index f1c2ad7a9a..726c3e04bb 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -45,15 +45,15 @@ _LOGGER = logging.getLogger(__name__) def apply_extra_script( component: ConvertedLibrary, - board_mcu: str | Callable[[], str], + board_mcu: Callable[[], str], pio_platform: str, ) -> None: """Run a library's PIO ``extraScript`` and fold its captured env vars into ``component.data["build"]["flags"]`` so the backend's -L/-l/-D extraction picks them up. Shared by the ESP-IDF and ESP8266 Arduino backends. - ``board_mcu`` may be a callable so a backend whose target lookup needs - build state (the esp32 variant) resolves it only when a script will run. + ``board_mcu`` is a callable so a backend whose target lookup needs build + state (the esp32 variant) resolves it only when a script will run. ``pio_platform`` is exposed to the script as PlatformIO's ``PIOPLATFORM``. """ extra_script = component.data.get("build", {}).get("extraScript") @@ -80,12 +80,10 @@ def apply_extra_script( component.name, ) return - if callable(board_mcu): - board_mcu = board_mcu() result = run_extra_script( script_path, library_dir=source_path, - board_mcu=board_mcu, + board_mcu=board_mcu(), pio_platform=pio_platform, ) extra_flags = captured_as_build_flags(result, library_dir=source_path) diff --git a/tests/unit_tests/build_helpers/test_idedata.py b/tests/unit_tests/build_helpers/test_idedata.py index e65802dd74..6c44192439 100644 --- a/tests/unit_tests/build_helpers/test_idedata.py +++ b/tests/unit_tests/build_helpers/test_idedata.py @@ -278,30 +278,39 @@ def test_parse_entry_strips_launcher_prefix() -> None: ) assert cxx_path == "/tools/xtensa-lx106-elf-g++" assert defines == ["USE_ESP8266"] - # Without a configured launcher nothing is stripped, even a token that - # happens to be named ccache -- but the surprise is warned about (once - # per path, however many entries the compile DB has) - idedata._warn_not_a_compiler.cache_clear() - cxx_path, _, _, _ = idedata.parse_entry(entry) - assert cxx_path == "/opt/homebrew/bin/ccache" -def test_parse_entry_warns_when_first_token_is_not_a_compiler( +def test_parse_entry_recovers_from_unconfigured_launcher( caplog: pytest.LogCaptureFixture, ) -> None: - idedata._warn_not_a_compiler.cache_clear() + """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", ) - idedata.parse_entry(entry) - assert "does not start with a compiler" in caplog.text - caplog.clear() - idedata.parse_entry(entry, launcher="/opt/homebrew/bin/ccache") + cxx_path, _, _, _ = idedata.parse_entry(entry) + assert cxx_path == "/tools/xtensa-lx106-elf-g++" assert "does not start with a compiler" not in caplog.text +def test_parse_entry_warns_when_first_token_is_not_a_compiler( + caplog: pytest.LogCaptureFixture, +) -> None: + """An unrecoverable non-compiler leading token is warned about, once per + path however many entries the compile DB has.""" + idedata._warned_not_a_compiler.clear() + entry = _entry( + f"{ABS}build", + f"{ABS}build/src/esphome/core/application.cpp", + "/usr/bin/python3 wrapper.py -c a.cpp -o a.o", + ) + idedata.parse_entry(entry) + idedata.parse_entry(entry) + assert caplog.text.count("does not start with a compiler") == 1 + + def _write_compile_commands(tmp_path: Path) -> Path: compile_commands = tmp_path / "compile_commands.json" compile_commands.write_text( @@ -402,3 +411,43 @@ def test_parse_entry_accepts_versioned_compilers() -> None: assert idedata._COMPILER_STEM.search(stem) assert not idedata._COMPILER_STEM.search("ccache") assert not idedata._COMPILER_STEM.search("distcc") + + +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_never_caches_bad_compiler(tmp_path: Path) -> None: + """Idedata whose compiler path failed the sanity check is served for this + run but not persisted, so the next build re-parses.""" + 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", + "/usr/bin/python3 wrapper.py -c app.cpp -o app.cpp.o", + ) + ] + ) + ) + cache = tmp_path / "c.json" + 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"] == "/usr/bin/python3" + assert not cache.exists() diff --git a/tests/unit_tests/test_espidf_component.py b/tests/unit_tests/test_espidf_component.py index 0ffc7fc61e..c4a910be23 100644 --- a/tests/unit_tests/test_espidf_component.py +++ b/tests/unit_tests/test_espidf_component.py @@ -1069,10 +1069,11 @@ def test_idf_component_download_passes_salt() -> None: assert c.path == Path("/converted/owner/name") -def test_apply_extra_script_wrapper_wires_esp32_target(tmp_path, monkeypatch): - """The espidf wrapper resolves the esp32 variant into the shared helper.""" +def test_emit_idf_component_wires_esp32_target(tmp_path, monkeypatch): + """Emitting a component resolves the esp32 variant into the shared + extraScript helper.""" from esphome.components import esp32 as esp32_module - from esphome.espidf.component import _apply_extra_script + from esphome.espidf.component import _emit_idf_component monkeypatch.setattr(esp32_module, "get_esp32_variant", lambda: "ESP32") (tmp_path / "src").mkdir() @@ -1081,5 +1082,5 @@ def test_apply_extra_script_wrapper_wires_esp32_target(tmp_path, monkeypatch): c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) c.path = tmp_path c.data = {"build": {"extraScript": "extra.py"}} - _apply_extra_script(c) + _emit_idf_component(c) assert c.data["build"]["flags"] == ["-lesp32"] diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index 16e9c9b58b..d88a9fc7cb 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -117,7 +117,7 @@ def test_apply_extra_script_path_traversal_is_rejected(tmp_path): c.path = library_dir c.data = {"build": {"extraScript": "../evil.py"}} - apply_extra_script(c, board_mcu="esp32", pio_platform="espressif32") + 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. @@ -135,14 +135,14 @@ def test_apply_extra_script_merges_into_existing_flags(tmp_path): c.path = tmp_path c.data = {"build": {"extraScript": "extra.py", "flags": ["-DEXISTING"]}} - apply_extra_script(c, board_mcu="esp32", pio_platform="espressif32") + 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_callable_target_and_str_flags(tmp_path) -> None: - """The shared helper resolves a callable board_mcu lazily and normalizes + """The shared helper resolves the board_mcu callable lazily and normalizes a string ``build.flags`` value into a list before extending it.""" from esphome.platformio.extra_script import apply_extra_script @@ -176,7 +176,7 @@ def test_apply_extra_script_no_script_and_no_flags(tmp_path) -> None: script = tmp_path / "noop.py" script.write_text("pass\n") c.data = {"build": {"extraScript": "noop.py"}} - apply_extra_script(c, board_mcu="esp8266", pio_platform="espressif8266") + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") assert "flags" not in c.data["build"] @@ -191,7 +191,7 @@ def test_apply_extra_script_ignores_uncaptured_env_calls(tmp_path) -> None: 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="esp8266", pio_platform="espressif8266") + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") assert c.data["build"]["flags"] == ["-lsingle"] @@ -204,7 +204,7 @@ def test_apply_extra_script_swallows_script_errors(tmp_path, caplog) -> None: 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="esp8266", pio_platform="espressif8266") + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") assert "flags" not in c.data["build"] assert "skipping" in caplog.text @@ -218,7 +218,7 @@ def test_apply_extra_script_pio_platform(tmp_path) -> None: 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="esp8266", pio_platform="espressif8266") + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") assert c.data["build"]["flags"] == ["-lespressif8266"] @@ -230,5 +230,5 @@ def test_apply_extra_script_missing_script_logged(tmp_path, caplog) -> None: c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) c.path = tmp_path c.data = {"build": {"extraScript": "nope.py"}} - apply_extra_script(c, board_mcu="esp8266", pio_platform="espressif8266") + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") assert "not found" in caplog.text