From 278abf2771fa25b18ad6a1d35f2dc7108c0e6efe Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 25 Aug 2026 15:53:54 -0500 Subject: [PATCH] Treat pch spawn errors as transient, keep user force-includes, fold command into checksum --- esphome/build_gen/espidf.py | 75 +++++++++++++++-------- esphome/build_helpers/idedata.py | 13 ++-- esphome/espidf/toolchain.py | 10 ++- tests/unit_tests/build_gen/test_espidf.py | 69 +++++++++++++++++++-- tests/unit_tests/test_espidf_framework.py | 9 +++ tests/unit_tests/test_espidf_toolchain.py | 19 ++++++ 6 files changed, 158 insertions(+), 37 deletions(-) diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index f461e0353d..3ec2bea0dc 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -1,6 +1,5 @@ """ESP-IDF direct build generator for ESPHome.""" -import hashlib import json import logging import os @@ -41,7 +40,9 @@ _LOGGER = logging.getLogger(__name__) # rest. Deliberately hard-coded: frequency-derived sets measured no better # and kept selecting headers that cannot compile standalone (X-macro, # platform-variant). Every entry must be safe to include first in an -# empty TU. +# empty TU. Caveat: application.h/automation.h become ambiently visible, +# so a TU missing those #includes still builds here but not on other +# platforms; ESPHOME_PCH_ENABLE=0 restores the strict view. _PCH_HEADERS = ( PCH_CORE_HEADER, "esphome/core/component.h", @@ -58,7 +59,7 @@ _PCH_BUILD_HEADER = f"build/{PCH_HEADER_NAME}" # Compile-command tokens dropped when retargeting a TU's flags at the # prefix header: source/output/depfile flags with an argument, and the # argument-less depfile flags (the pch compile must not touch depfiles) -_PCH_STRIP_FLAGS_WITH_ARG = frozenset({"-include", "-o", "-c", "-MT", "-MF", "-MQ"}) +_PCH_STRIP_FLAGS_WITH_ARG = frozenset({"-o", "-c", "-MT", "-MF", "-MQ"}) _PCH_STRIP_FLAGS = frozenset({"-MD", "-MMD", "-MP", "-MM", "-M"}) _CXX_SOURCE_SUFFIXES = (".cpp", ".cc", ".cxx") @@ -354,7 +355,8 @@ def _pch_compile_command(build_dir: Path, header: Path, gch: Path) -> list[str] (build_dir / "compile_commands.json").read_text(encoding="utf-8") ) except (OSError, json.JSONDecodeError) as err: - _LOGGER.debug("No usable compile database, skipping pch: %s", err) + # Configure already succeeded, so an unusable DB is a real anomaly + _LOGGER.warning("No usable compile database, skipping pch: %s", err) return None # Windows compile DBs use backslashes; normalize both sides src_prefix = str(CORE.relative_src_path()).replace("\\", "/") @@ -368,10 +370,10 @@ def _pch_compile_command(build_dir: Path, header: Path, gch: Path) -> list[str] None, ) if entry is None: - _LOGGER.debug("No src C++ entry in the compile database, skipping pch") + _LOGGER.warning("No src C++ entry in the compile database, skipping pch") return None tokens = expand_response_files( - split_command(entry["command"]), Path(entry.get("directory", build_dir)) + split_command(entry.get("command", "")), Path(entry.get("directory", build_dir)) ) # A DB recorded with ccache enabled prefixes the compiler with the # launcher; the .gch must be compiled directly @@ -385,6 +387,13 @@ def _pch_compile_command(build_dir: Path, header: Path, gch: Path) -> list[str] continue if tok in _PCH_STRIP_FLAGS: continue + if tok == "-include": + # Drop only the injected prefix; user force-includes must reach + # the .gch compile or GCC rejects it over the macro mismatch + inc = next(arg_it, "") + if not inc.endswith(PCH_HEADER_NAME): + args.extend(("-include", inc)) + continue args.append(tok) return [*args, "-x", "c++-header", "-c", str(header), "-o", str(gch)] @@ -394,8 +403,9 @@ def prepare_pch() -> None: Runs right before ninja, after every reconfigure, so the flags in compile_commands.json and the sdkconfig are the settled ones. The .sum - doubles as the freshness stamp; a failed .gch compile falls back to - the plain header include. + doubles as the freshness stamp and folds in the compile command, so a + flag-only change rebuilds the .gch. A failed compile falls back to the + plain header include. """ if not pch_enabled(): return @@ -403,12 +413,27 @@ def prepare_pch() -> None: header = CORE.relative_build_path(_PCH_BUILD_HEADER) gch = Path(f"{header}.gch") sum_path = Path(f"{gch}.sum") + 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) + return + sdkconfig_path = CORE.relative_build_path(f"sdkconfig.{CORE.name}") try: - sdkconfig = CORE.relative_build_path(f"sdkconfig.{CORE.name}").read_text( - encoding="utf-8" + sdkconfig = sdkconfig_path.read_text(encoding="utf-8") + except OSError as err: + # Folding the error in keeps distinct unreadable states from colliding + _LOGGER.warning( + "Could not read %s for the pch checksum: %s", sdkconfig_path, err ) - except OSError: - sdkconfig = "no-sdkconfig" + sdkconfig = f"unreadable:{err}" + # Build-path stripped so identical configs hash identically across devices + cmd_id = ( + " ".join(cmd) + .replace(str(Path(CORE.build_path).resolve()), "") + .replace(str(CORE.build_path), "") + ) checksum = pch_checksum( CORE.relative_src_path(), _PCH_HEADERS, @@ -418,6 +443,7 @@ def prepare_pch() -> None: sdkconfig, *get_project_compile_flags(), *get_project_cxx_compile_flags(), + cmd_id, ), ) if ( @@ -426,21 +452,15 @@ def prepare_pch() -> None: and sum_path.read_text(encoding="utf-8").strip() == checksum ): return - cmd = _pch_compile_command(build_dir, header, gch) - if cmd is None: - # The checksum is stale; a leftover .gch must not be consumed - gch.unlink(missing_ok=True) - sum_path.unlink(missing_ok=True) - return - # Keyed on the checksum and the compile command: a failure caused by - # the command alone must retry when the command changes - marker_key = f"{checksum} {hashlib.sha256(' '.join(cmd).encode()).hexdigest()}" failed_marker = Path(f"{gch}.failed") if ( failed_marker.is_file() - and failed_marker.read_text(encoding="utf-8").strip() == marker_key + and failed_marker.read_text(encoding="utf-8").strip() == checksum ): - _LOGGER.debug("Pch previously failed for these inputs; skipping") + _LOGGER.info( + "Precompiled header disabled after an earlier failure; delete %s to retry", + failed_marker, + ) return try: result = subprocess.run( @@ -452,7 +472,12 @@ def prepare_pch() -> None: elif not gch.is_file(): error = "compiler produced no .gch" except (OSError, subprocess.SubprocessError) as err: - error = str(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) + return if error is not None: _LOGGER.warning( "Precompiled header failed; compiling without it: %s", error[:400] @@ -460,7 +485,7 @@ def prepare_pch() -> None: gch.unlink(missing_ok=True) sum_path.unlink(missing_ok=True) # Skip retries until a header/flag/sdkconfig/command change - failed_marker.write_text(marker_key + "\n", encoding="utf-8") + failed_marker.write_text(checksum + "\n", encoding="utf-8") os.utime(header) return failed_marker.unlink(missing_ok=True) diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index 8a46203890..97faf0f504 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -198,10 +198,15 @@ def parse_entry( it = iter(tokens[1:]) for tok in it: - if tok in ("-c", "-o", "-include"): - # Drop the flag and its argument; the injected relative - # -include esphome_pch.h does not resolve outside the build dir - next(it, None) + if tok in ("-c", "-o"): + next(it, None) # drop the flag and its argument (input/output) + continue + if tok == "-include": + # Drop only the injected pch include, whose relative path does + # not resolve outside the build dir; keep other force-includes + inc = next(it, "") + if not inc.endswith("esphome_pch.h"): + cxx_flags.extend(("-include", inc)) 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/espidf/toolchain.py b/esphome/espidf/toolchain.py index a5d145e3c1..d501f6f2af 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -528,10 +528,16 @@ def run_compile(config, verbose: bool) -> int: return result.returncode _patch_memory_segments() - # After every reconfigure so compile_commands and sdkconfig are settled + # 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 - prepare_pch() + try: + prepare_pch() + except Exception as err: # noqa: BLE001 # pylint: disable=broad-exception-caught + _LOGGER.warning( + "Precompiled header setup failed; compiling without it: %s", err + ) # Build args = [] diff --git a/tests/unit_tests/build_gen/test_espidf.py b/tests/unit_tests/build_gen/test_espidf.py index 22c3f7ef8d..f5a8fed9cd 100644 --- a/tests/unit_tests/build_gen/test_espidf.py +++ b/tests/unit_tests/build_gen/test_espidf.py @@ -667,20 +667,27 @@ def test_prepare_pch_failure_writes_marker_and_skips_retry(tmp_path: Path) -> No assert (dev / "build" / "esphome_pch.h.gch.failed").exists() -def test_prepare_pch_spawn_oserror_degrades(tmp_path: Path) -> None: +def test_prepare_pch_spawn_oserror_is_transient(tmp_path: Path) -> None: + """Spawn/IO failures retry on the next build instead of latching.""" from esphome.build_gen.espidf import prepare_pch dev = _make_pch_device(tmp_path, "dev_o") CORE.build_path = dev + calls = [] + + def raising(cmd, **kwargs): + calls.append(cmd) + raise OSError("no such compiler") + with ( patch.object(CORE, "name", "test"), - patch( - "esphome.build_gen.espidf.subprocess.run", - side_effect=OSError("no such compiler"), - ), + patch("esphome.build_gen.espidf.subprocess.run", side_effect=raising), ): prepare_pch() - assert (dev / "build" / "esphome_pch.h.gch.failed").exists() + prepare_pch() + assert not (dev / "build" / "esphome_pch.h.gch.failed").exists() + assert not (dev / "build" / "esphome_pch.h.gch.sum").exists() + assert len(calls) == 2 def test_prepare_pch_disabled_is_noop( @@ -810,3 +817,53 @@ def test_component_cmakelists_pch_object_depends() -> None: content = get_component_cmakelists() assert 'OBJECT_DEPENDS "${CMAKE_BINARY_DIR}/esphome_pch.h"' in content + + +def test_prepare_pch_command_change_invalidates_sum(tmp_path: Path) -> None: + """A flag-only change in the compile DB must rebuild the .gch.""" + from esphome.build_gen.espidf import prepare_pch + + dev = _make_pch_device(tmp_path, "dev_c") + 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), + ): + prepare_pch() + first = (dev / "build" / "esphome_pch.h.gch.sum").read_text() + db = dev / "build" / "compile_commands.json" + db.write_text(db.read_text().replace("-DX=1", "-DX=2")) + prepare_pch() + assert (dev / "build" / "esphome_pch.h.gch.sum").read_text() != first + + +def test_prepare_pch_keeps_user_force_includes(tmp_path: Path) -> None: + from esphome.build_gen.espidf import _pch_compile_command + + dev = _make_pch_device(tmp_path, "dev_u") + CORE.build_path = dev + build = dev / "build" + src_file = str(dev / "src" / "esphome" / "a.cpp") + build.joinpath("compile_commands.json").write_text( + json.dumps( + [ + { + "directory": str(build), + "command": ( + "g++ -include user.h -include esphome_pch.h " + f"-o a.obj -c {src_file}" + ), + "file": src_file, + } + ] + ) + ) + cmd = _pch_compile_command(build, build / "esphome_pch.h", build / "x.gch") + assert "user.h" in cmd + assert "esphome_pch.h" not in " ".join(cmd[:-3]) diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index a49c292e2a..7e48593ad3 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -1991,3 +1991,12 @@ def test_ccache_env_opt_in_with_usable_binary( env = _ccache_env() assert env["IDF_CCACHE_ENABLE"] == "1" assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +def test_ccache_env_exports_pch_settings(tmp_path: Path) -> None: + # The pch cannot cache under ccache without these + p1, p2, p3 = _ccache_patches(tmp_path, "/usr/bin/ccache", tmp_path / "build") + with patch.dict("os.environ", {}, clear=True), p1, p2, p3: + env = _ccache_env() + assert env["CCACHE_SLOPPINESS"] == "pch_defines,time_macros" + assert env["CCACHE_PCH_EXTSUM"] == "true" diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index 2cd5d4e1cb..e7d5df896e 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -667,3 +667,22 @@ def test_get_core_framework_version_from_core_data(): CORE.data = {KEY_ESP32: {KEY_IDF_VERSION: cv.Version(5, 5, 4)}} assert toolchain._get_core_framework_version() == "5.5.4" + + +def test_run_compile_invokes_prepare_pch_and_survives_failure( + setup_core: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The pch hook runs before the build and a failure never aborts it.""" + monkeypatch.setenv("ESPHOME_PCH_ENABLE", "1") + _setup_build(setup_core) + + with ( + patch.object(toolchain, "need_reconfigure", return_value=False), + patch.object(toolchain, "run_idf_py", return_value=0), + patch.object(toolchain, "print_summary"), + patch( + "esphome.build_gen.espidf.prepare_pch", side_effect=RuntimeError("boom") + ) as prepare, + ): + assert toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) == 0 + prepare.assert_called_once()