From 564f3e31c1c04b62726624f9c00de120bd38c703 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 25 Aug 2026 17:44:54 -0500 Subject: [PATCH 1/2] Harden idedata -include anchoring, probe exit check, package identity, and failure diagnostics --- esphome/build_helpers/idedata.py | 14 +++- esphome/platformio/pch.py.script | 67 ++++++++++++------- .../unit_tests/build_helpers/test_idedata.py | 37 ++++++++-- .../unit_tests/test_platformio_pch_script.py | 65 ++++++++++++++++-- 4 files changed, 146 insertions(+), 37 deletions(-) diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 9522535f60..29f70b835c 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -201,9 +201,17 @@ def parse_entry( if tok in ("-c", "-o"): next(it, None) # drop the flag and its argument (input/output) elif tok == "-include": - # Resolve like -I so cached idedata works from any cwd (the pch - # include is emitted relative to the build dir) - cxx_flags.extend(("-include", _include(next(it, "")))) + # -include searches the compile cwd first, then the -I chain, so + # only re-anchor paths that really live next to the compile (the + # pch); a name meant for the -I chain must stay untouched + raw = next(it, "") + if not raw: + _LOGGER.warning("Dropping -include with no argument") + else: + resolved = _include(raw) + cxx_flags.extend( + ("-include", resolved if Path(resolved).is_file() else raw) + ) elif tok.startswith("-D"): # ``.strip()`` handles tokens like ``-D CONFIGURED=1`` (a single # quoted arg with a space after -D) that some flags arrive as. diff --git a/esphome/platformio/pch.py.script b/esphome/platformio/pch.py.script index 6e0ba70ddc..45bb9ddb05 100644 --- a/esphome/platformio/pch.py.script +++ b/esphome/platformio/pch.py.script @@ -5,6 +5,7 @@ import posixpath import re import shlex import subprocess +import traceback # pylint: disable=E0602 Import("env", "projenv") # noqa: F821 @@ -107,6 +108,11 @@ def _setup_pch() -> None: try: version = platform.get_package_version(package) except KeyError: + # Only trust KeyError as "absent" when the package really is not + # installed; an unresolved manifest must not hash as a constant + if platform.get_package(package) is not None: + print(f"ESPHome: skipping precompiled header: no version for {package}") + return version = None # absent optional package except Exception as err: # noqa: BLE001 # Without trustworthy package identity a stale .gch could be @@ -141,8 +147,15 @@ def _setup_pch() -> None: for local in sorted(p for p in inc_dir.rglob("*") if p.is_file()): try: data = local.read_bytes() - except OSError: - data = b"" + except OSError as err: + print(f"ESPHome: could not read {local} for the pch checksum: {err}") + try: + # mtime/size keep a changed-but-unreadable header shifting + # the digest without putting device paths in it + st = local.stat() + data = f"".encode() + except OSError: + data = b"" digest.update(str(local.relative_to(proj_dir)).encode()) digest.update(data) digest.update(b"\0") @@ -181,27 +194,30 @@ def _setup_pch() -> None: # macOS arm64 rejects it per-process: "had text segment at # different address"); probe once so consumers never pay for a # pch that every compile would silently reject - probe = subprocess.run( # noqa: PLW1510 - [ - cxx, - *flags, - "-MF", - os.devnull, - "-Winvalid-pch", - "-include", - str(header), - "-fsyntax-only", - "-x", - "c++", - "-", - ], - cwd=proj_dir, - input="", - capture_output=True, - text=True, - ) - if ".gch" in probe.stderr: - error = f"toolchain cannot load the pch: {probe.stderr.strip()}" + try: + probe = subprocess.run( # noqa: PLW1510 + [ + cxx, + *flags, + "-MF", + os.devnull, + "-Winvalid-pch", + "-include", + str(header), + "-fsyntax-only", + "-x", + "c++", + "-", + ], + cwd=proj_dir, + input="", + capture_output=True, + text=True, + ) + if probe.returncode != 0 or ".gch" in probe.stderr: + error = f"toolchain cannot load the pch: {probe.stderr.strip()}" + except OSError as err: + error = str(err) if error is not None: print("ESPHome: precompiled header failed; compiling without it") print(error) @@ -230,5 +246,6 @@ def _setup_pch() -> None: try: _setup_pch() -except Exception as err: # noqa: BLE001 -- a speedup must never break the build - print(f"ESPHome: precompiled header setup failed; compiling without it: {err}") +except Exception: # noqa: BLE001 -- a speedup must never break the build + print("ESPHome: precompiled header setup failed; compiling without it") + traceback.print_exc() diff --git a/tests/unit_tests/build_helpers/test_idedata.py b/tests/unit_tests/build_helpers/test_idedata.py index 0a192fd438..dbbf647060 100644 --- a/tests/unit_tests/build_helpers/test_idedata.py +++ b/tests/unit_tests/build_helpers/test_idedata.py @@ -85,13 +85,13 @@ def test_parse_entry_resolves_relative_includes() -> None: assert all(Path(inc).is_absolute() for inc in includes) -def test_parse_entry_resolves_force_include_path() -> None: +def test_parse_entry_resolves_force_include_path(tmp_path: Path) -> None: """The pch -include is emitted relative to the build dir; idedata must resolve it so cached flags work from any cwd.""" - directory = f"{ABS}build/proj" + (tmp_path / "esphome_pch.h").write_text("") entry = _entry( - directory, - f"{directory}/src/esphome/x.cpp", + str(tmp_path), + f"{tmp_path}/src/esphome/x.cpp", "g++ -include esphome_pch.h -c x.cpp", ) @@ -100,7 +100,34 @@ def test_parse_entry_resolves_force_include_path() -> None: idx = cxx_flags.index("-include") resolved = cxx_flags[idx + 1] assert Path(resolved).is_absolute() - assert resolved.endswith("build/proj/esphome_pch.h") + assert resolved == str(tmp_path / "esphome_pch.h").replace("\\", "/") + + +def test_parse_entry_keeps_search_chain_force_include(tmp_path: Path) -> None: + """-include names resolved via the -I chain (libretiny's Arduino.h) must + not be re-anchored to a nonexistent build-dir path.""" + entry = _entry( + str(tmp_path), + f"{tmp_path}/src/esphome/x.cpp", + "g++ -include Arduino.h -c x.cpp", + ) + + _, _, _, cxx_flags = idedata.parse_entry(entry) + + assert cxx_flags[cxx_flags.index("-include") + 1] == "Arduino.h" + + +def test_parse_entry_drops_trailing_force_include( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + entry = _entry( + str(tmp_path), f"{tmp_path}/src/esphome/x.cpp", "g++ -c x.cpp -include" + ) + + _, _, _, cxx_flags = idedata.parse_entry(entry) + + assert "-include" not in cxx_flags + assert "no argument" in caplog.text def test_parse_entry_skips_dependency_flags() -> None: diff --git a/tests/unit_tests/test_platformio_pch_script.py b/tests/unit_tests/test_platformio_pch_script.py index 4e4e5ca8e5..0f879eef47 100644 --- a/tests/unit_tests/test_platformio_pch_script.py +++ b/tests/unit_tests/test_platformio_pch_script.py @@ -26,12 +26,25 @@ class _FakePlatform: raise KeyError(name) return "1.2.3" + def get_package(self, name: str) -> object | None: + return None + class _BrokenPlatform(_FakePlatform): def get_package_version(self, name: str) -> str: raise RuntimeError("manifest parse error") +class _UnresolvedPlatform(_FakePlatform): + """KeyError from a package that IS installed: unresolved identity.""" + + def get_package_version(self, name: str) -> str: + raise KeyError(name) + + def get_package(self, name: str) -> object: + return object() + + class _FakeSConsEnv(dict): """Just enough of a SCons construction environment for pch.py.""" @@ -66,11 +79,17 @@ class _FakeSConsEnv(dict): self.prepended = CXXFLAGS -def _fake_cxx(tmp_path: Path, fail: bool = False, reject_pch: bool = False) -> Path: +def _fake_cxx( + tmp_path: Path, + fail: bool = False, + reject_pch: bool = False, + probe_exit: int = 0, +) -> Path: """A compiler stand-in that records its argv and writes the -o target. With reject_pch it builds the .gch fine but, like GCC 10 on macOS arm64, - warns on any consuming compile that the .gch cannot be loaded. + warns on any consuming compile that the .gch cannot be loaded; probe_exit + sets the exit code of non-header compiles (the load probe). """ cxx = tmp_path / "fake-gxx" body = ( @@ -84,7 +103,7 @@ def _fake_cxx(tmp_path: Path, fail: bool = False, reject_pch: bool = False) -> P body += '[ -n "$out" ] && echo gch > "$out"\n' if reject_pch: body += 'case " $* " in *c++-header*) ;; *) echo "warning: esphome_pch.h.gch: had text segment at different address" >&2;; esac\n' - body += "exit 0\n" + body += f'case " $* " in *c++-header*) exit 0;; *) exit {probe_exit};; esac\n' cxx.write_text("#!/bin/sh\n" + body) cxx.chmod(cxx.stat().st_mode | stat.S_IEXEC) return cxx @@ -95,6 +114,7 @@ def _run_script( flags: list[str] | None = None, fail: bool = False, reject_pch: bool = False, + probe_exit: int = 0, env_vars: dict[str, str] | None = None, name: str = "dev", platform_cls: type[_FakePlatform] = _FakePlatform, @@ -103,7 +123,7 @@ def _run_script( src = proj / "src" (src / "esphome" / "core").mkdir(parents=True, exist_ok=True) (src / "esphome" / "core" / "defines.h").write_text("#define USE_X\n") - cxx = _fake_cxx(tmp_path, fail=fail, reject_pch=reject_pch) + cxx = _fake_cxx(tmp_path, fail=fail, reject_pch=reject_pch, probe_exit=probe_exit) args = (proj, src, str(cxx), flags or ["-DX=1"], platform_cls) # Distinct objects: the script must scope ccache/flags to projenv only global_env = _FakeSConsEnv(*args) @@ -199,6 +219,22 @@ def test_pch_script_probe_rejection_falls_back( assert "toolchain cannot load the pch" in capsys.readouterr().out +def test_pch_script_probe_nonzero_exit_falls_back(tmp_path: Path) -> None: + """A probe failure whose stderr never mentions .gch must still count.""" + scons_env = _run_script(tmp_path, probe_exit=1) + proj = tmp_path / "dev" + assert not (proj / "esphome_pch.h.gch").exists() + assert (proj / "esphome_pch.h.gch.failed").is_file() + assert scons_env.prepended == [] + + +def test_pch_script_unresolved_package_version_skips_pch(tmp_path: Path) -> None: + """A KeyError for an installed package is unresolved identity, not absence.""" + scons_env = _run_script(tmp_path, platform_cls=_UnresolvedPlatform) + assert not (tmp_path / "dev" / "esphome_pch.h.gch").exists() + assert scons_env.prepended == [] + + def test_pch_script_package_version_error_skips_pch(tmp_path: Path) -> None: """Without trustworthy package identity a stale .gch could survive an upgrade, so the script must not build one at all.""" @@ -238,3 +274,24 @@ def test_pch_script_hashes_project_local_include_dirs(tmp_path: Path) -> None: (tmp_path / "fake-gxx.argv").unlink(missing_ok=True) _run_script(tmp_path, flags=flags) assert (proj / "esphome_pch.h.gch.sum").read_text() != first + + +@pytest.mark.skipif(os.geteuid() == 0, reason="root ignores file modes") +def test_pch_script_unreadable_local_header_warns_and_varies( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """An unreadable generated header still shifts the digest via mtime/size.""" + proj = tmp_path / "dev" + override = proj / "lwip_override" + override.mkdir(parents=True) + secret = override / "lwipopts.h" + secret.write_text("#define TCP_MSS 1460\n") + secret.chmod(0) + flags = ["-DX=1", "-I", str(override)] + _run_script(tmp_path, flags=flags) + first = (proj / "esphome_pch.h.gch.sum").read_text() + assert "could not read" in capsys.readouterr().out + os.utime(secret, (1, 1)) + (tmp_path / "fake-gxx.argv").unlink(missing_ok=True) + _run_script(tmp_path, flags=flags) + assert (proj / "esphome_pch.h.gch.sum").read_text() != first From 1059ecf50c54eed656e775cd936a82a9b9878b39 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 25 Aug 2026 17:48:01 -0500 Subject: [PATCH 2/2] 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()