From 1059ecf50c54eed656e775cd936a82a9b9878b39 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 25 Aug 2026 17:48:01 -0500 Subject: [PATCH] Fold header order into pch checksum, gate rebuild-forcing touch, guard malformed compile DBs --- esphome/build_gen/espidf.py | 41 +++++++++--- esphome/espidf/toolchain.py | 11 ++- tests/unit_tests/build_gen/test_espidf.py | 81 +++++++++++++++++++++++ tests/unit_tests/test_espidf_toolchain.py | 7 ++ 4 files changed, 129 insertions(+), 11 deletions(-) diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index 3ec2bea0dc..92f227731b 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -358,13 +358,17 @@ def _pch_compile_command(build_dir: Path, header: Path, gch: Path) -> list[str] # Configure already succeeded, so an unusable DB is a real anomaly _LOGGER.warning("No usable compile database, skipping pch: %s", err) return None + if not isinstance(entries, list): + _LOGGER.warning("Malformed compile database, skipping pch") + return None # Windows compile DBs use backslashes; normalize both sides src_prefix = str(CORE.relative_src_path()).replace("\\", "/") entry = next( ( e for e in entries - if e.get("file", "").replace("\\", "/").startswith(src_prefix) + if isinstance(e, dict) + and e.get("file", "").replace("\\", "/").startswith(src_prefix) and e.get("file", "").endswith(_CXX_SOURCE_SUFFIXES) ), None, @@ -379,6 +383,11 @@ def _pch_compile_command(build_dir: Path, header: Path, gch: Path) -> list[str] # launcher; the .gch must be compiled directly if tokens and is_launcher(tokens[0]): tokens = tokens[1:] + if not tokens: + # An "arguments"-style or empty entry must skip cleanly, not spawn + # a compiler-less argv that warns on every build + _LOGGER.warning("Compile database entry has no usable command, skipping pch") + return None args: list[str] = [] arg_it = iter(tokens) for tok in arg_it: @@ -398,6 +407,22 @@ def _pch_compile_command(build_dir: Path, header: Path, gch: Path) -> list[str] return [*args, "-x", "c++-header", "-c", str(header), "-o", str(gch)] +def discard_pch() -> None: + """Remove the pch sidecars so a stale .gch is never consumed. + + Bumps the header only when a .gch was actually removed: TUs compiled + against it have incomplete depfiles, while a repeat failure with no + .gch must not force a full rebuild every build. + """ + header = CORE.relative_build_path(_PCH_BUILD_HEADER) + gch = Path(f"{header}.gch") + had_gch = gch.is_file() + gch.unlink(missing_ok=True) + Path(f"{gch}.sum").unlink(missing_ok=True) + if had_gch and header.is_file(): + os.utime(header) + + def prepare_pch() -> None: """Compile the prefix header's .gch and write its ccache .sum. @@ -416,18 +441,18 @@ def prepare_pch() -> None: cmd = _pch_compile_command(build_dir, header, gch) if cmd is None: # Freshness cannot be validated; a leftover .gch must not be consumed - gch.unlink(missing_ok=True) - sum_path.unlink(missing_ok=True) + discard_pch() return sdkconfig_path = CORE.relative_build_path(f"sdkconfig.{CORE.name}") try: sdkconfig = sdkconfig_path.read_text(encoding="utf-8") except OSError as err: - # Folding the error in keeps distinct unreadable states from colliding + # Path-independent marker: str(err) embeds the per-device path and + # would defeat cross-device .sum sharing _LOGGER.warning( "Could not read %s for the pch checksum: %s", sdkconfig_path, err ) - sdkconfig = f"unreadable:{err}" + sdkconfig = f"unreadable:{type(err).__name__}:{err.errno}" # Build-path stripped so identical configs hash identically across devices cmd_id = ( " ".join(cmd) @@ -438,6 +463,8 @@ def prepare_pch() -> None: CORE.relative_src_path(), _PCH_HEADERS, ( + # The closure is sorted, so root order only enters via the text + pch_header_text(_PCH_HEADERS), str(idf_version()), CORE.cpp_standard or "", sdkconfig, @@ -474,9 +501,7 @@ def prepare_pch() -> None: except (OSError, subprocess.SubprocessError) as err: # Transient (timeout, spawn/IO): warn and retry next build, no marker _LOGGER.warning("Precompiled header compile did not run: %s", err) - gch.unlink(missing_ok=True) - sum_path.unlink(missing_ok=True) - os.utime(header) + discard_pch() return if error is not None: _LOGGER.warning( diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index d501f6f2af..0eccf5af79 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -1,5 +1,6 @@ """ESP-IDF direct build API for ESPHome.""" +from contextlib import suppress from dataclasses import dataclass, field import hashlib import json @@ -530,13 +531,17 @@ def run_compile(config, verbose: bool) -> int: # After every reconfigure so compile_commands and sdkconfig are settled. # An optional speedup must never abort the build - from esphome.build_gen.espidf import prepare_pch + from esphome.build_gen.espidf import discard_pch, prepare_pch try: prepare_pch() - except Exception as err: # noqa: BLE001 # pylint: disable=broad-exception-caught + except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught + # Discard so an unexpected error can never leave a stale .gch that + # GCC would silently consume; exc_info keeps the failure diagnosable + with suppress(OSError): + discard_pch() _LOGGER.warning( - "Precompiled header setup failed; compiling without it: %s", err + "Precompiled header setup failed; compiling without it", exc_info=True ) # Build diff --git a/tests/unit_tests/build_gen/test_espidf.py b/tests/unit_tests/build_gen/test_espidf.py index f5a8fed9cd..9b7e78f4c1 100644 --- a/tests/unit_tests/build_gen/test_espidf.py +++ b/tests/unit_tests/build_gen/test_espidf.py @@ -4,6 +4,7 @@ from __future__ import annotations import json import logging +import os from pathlib import Path import subprocess from unittest.mock import patch @@ -645,6 +646,59 @@ def test_pch_compile_command_variants(tmp_path: Path) -> None: ] +def test_pch_compile_command_rejects_unusable_entries(tmp_path: Path) -> None: + """Malformed DB shapes and command-less entries skip cleanly instead of + producing a compiler-less argv retried every build.""" + from esphome.build_gen.espidf import _pch_compile_command + + build = tmp_path / "build" + build.mkdir() + header = build / "esphome_pch.h" + gch = build / "esphome_pch.h.gch" + db = build / "compile_commands.json" + src_file = str(tmp_path / "src" / "esphome" / "a.cpp") + + db.write_text(json.dumps({"not": "a list"})) + assert _pch_compile_command(build, header, gch) is None + + db.write_text(json.dumps(["just a string"])) + assert _pch_compile_command(build, header, gch) is None + + # arguments-style entry (allowed by the spec, unused by CMake) + db.write_text( + json.dumps([{"arguments": ["g++", "-c", src_file], "file": src_file}]) + ) + assert _pch_compile_command(build, header, gch) is None + + +def test_pch_header_list_order_is_in_checksum( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Reordering _PCH_HEADERS keeps the include closure identical, but the + generated header text differs, so the .gch must rebuild.""" + import esphome.build_gen.espidf as espidf_mod + + dev = _make_pch_device(tmp_path, "dev_r") + CORE.build_path = dev + gch = dev / "build" / "esphome_pch.h.gch" + + def fake_compile(cmd, **kwargs): + gch.write_bytes(b"gch") + return subprocess.CompletedProcess(cmd, 0, "", "") + + with ( + patch.object(CORE, "name", "test"), + patch("esphome.build_gen.espidf.subprocess.run", side_effect=fake_compile), + ): + espidf_mod.prepare_pch() + first = (dev / "build" / "esphome_pch.h.gch.sum").read_text() + monkeypatch.setattr( + espidf_mod, "_PCH_HEADERS", tuple(reversed(espidf_mod._PCH_HEADERS)) + ) + espidf_mod.prepare_pch() + assert (dev / "build" / "esphome_pch.h.gch.sum").read_text() != first + + def test_prepare_pch_failure_writes_marker_and_skips_retry(tmp_path: Path) -> None: from esphome.build_gen.espidf import prepare_pch @@ -679,6 +733,8 @@ def test_prepare_pch_spawn_oserror_is_transient(tmp_path: Path) -> None: calls.append(cmd) raise OSError("no such compiler") + header = dev / "build" / "esphome_pch.h" + before = header.stat().st_mtime_ns with ( patch.object(CORE, "name", "test"), patch("esphome.build_gen.espidf.subprocess.run", side_effect=raising), @@ -688,6 +744,31 @@ def test_prepare_pch_spawn_oserror_is_transient(tmp_path: Path) -> None: assert not (dev / "build" / "esphome_pch.h.gch.failed").exists() assert not (dev / "build" / "esphome_pch.h.gch.sum").exists() assert len(calls) == 2 + # No .gch was ever in play, so the header must not be re-touched into + # forcing a full rebuild on every failing build + assert header.stat().st_mtime_ns == before + + +def test_prepare_pch_transient_with_stale_gch_bumps_header(tmp_path: Path) -> None: + """A stale .gch removed on a transient failure must dirty its consumers.""" + from esphome.build_gen.espidf import prepare_pch + + dev = _make_pch_device(tmp_path, "dev_s") + CORE.build_path = dev + gch = dev / "build" / "esphome_pch.h.gch" + gch.write_bytes(b"stale") + header = dev / "build" / "esphome_pch.h" + os.utime(header, (1, 1)) + with ( + patch.object(CORE, "name", "test"), + patch( + "esphome.build_gen.espidf.subprocess.run", + side_effect=OSError("no such compiler"), + ), + ): + prepare_pch() + assert not gch.exists() + assert header.stat().st_mtime_ns > 1_000_000_000 def test_prepare_pch_disabled_is_noop( diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index e7d5df896e..7458d44764 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -675,6 +675,12 @@ def test_run_compile_invokes_prepare_pch_and_survives_failure( """The pch hook runs before the build and a failure never aborts it.""" monkeypatch.setenv("ESPHOME_PCH_ENABLE", "1") _setup_build(setup_core) + # A stale .gch must be discarded on the failure path, never consumed + build = setup_core / "build" / "test" / "build" + build.mkdir(parents=True, exist_ok=True) + (build / "esphome_pch.h").write_text("") + stale_gch = build / "esphome_pch.h.gch" + stale_gch.write_bytes(b"stale") with ( patch.object(toolchain, "need_reconfigure", return_value=False), @@ -686,3 +692,4 @@ def test_run_compile_invokes_prepare_pch_and_survives_failure( ): assert toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) == 0 prepare.assert_called_once() + assert not stale_gch.exists()