Share the guarded pch prepare contract, simplify wiring and tests

esphome/build_gen/espidf.py

@@ -300,11 +300,6 @@ def _pch_cmake() -> str:
     return pch.pch_cmake_consumer("${COMPONENT_LIB}", "${app_sources}")
 
 
-def discard_pch() -> None:
-    """Drop the pch sidecars in the IDF build dir."""
-    pch.discard_pch(CORE.relative_build_path("build"))
-
-
 def prepare_pch() -> None:
     """Build the .gch right before ninja, after every reconfigure, so the
     compile_commands.json flags and the sdkconfig are the settled ones."""

esphome/build_helpers/pch.py

@@ -7,7 +7,7 @@ it too; Arduino.h visibility there is intended (esphome#8693).
 
 from __future__ import annotations
 
-from collections.abc import Iterable
+from collections.abc import Callable, Iterable
 from contextlib import suppress
 from dataclasses import dataclass
 import hashlib
@@ -213,6 +213,30 @@ def ccache_pch_env() -> dict[str, str]:
     return env
 
 
+def guarded_prepare(build_dir: Path, prepare: Callable[[], None]) -> None:
+    """Run a backend's pch preparation; an optional speedup must never
+    abort the build. Owns the failure ordering both native backends need:
+    strict is read first so its own knob error cannot mask the real
+    failure, discard_pch raises itself if a stale .gch survives (silently
+    wrong output), and the header is ensured afterwards so a consumer-side
+    OBJECT_DEPENDS stays satisfiable without forcing rebuilds when the
+    header already exists."""
+    try:
+        prepare()
+    except Exception:  # noqa: BLE001  # pylint: disable=broad-exception-caught
+        strict = pch_strict()
+        discard_pch(build_dir)
+        if strict:
+            raise
+        header = build_dir / PCH_HEADER_NAME
+        if not header.exists():
+            with suppress(OSError):
+                header.touch()
+        _LOGGER.warning(
+            "Precompiled header setup failed; compiling without it", exc_info=True
+        )
+
+
 def pch_extra_scripts() -> list[str]:
     """The extra_scripts entries a PlatformIO platform registers for the
     pch; empty when disabled (the script itself has no enable check)."""

esphome/components/nrf52/__init__.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import asyncio
-from contextlib import suppress
 import logging
 from pathlib import Path
 import re
@@ -816,7 +815,7 @@ def _generate_cmake_lists() -> bool:
         ]
 
     if consumer := pch_cmake_consumer("app", "${APP_SOURCES}"):
-        lines += consumer.rstrip("\n").splitlines()
+        lines += consumer.splitlines()
 
     if link_flags:
         lines += [
@@ -838,7 +837,10 @@ def _app_build_dir(build_dir: Path) -> Path:
     Sysbuild (SDK >= 2.9.2) nests the app in a domain dir named after the
     app source dir ("zephyr"); older SDKs configure it at the top level.
     In the non-sysbuild layout build_dir/zephyr is the Zephyr output dir,
-    which has no CMakeCache.txt, so the probe cannot misfire."""
+    which has no CMakeCache.txt, so the probe cannot misfire. A probe of
+    the on-disk layout (rather than the SDK-version check the artifact
+    copy uses) stays truthful mid-build and if sysbuild is ever toggled
+    independently of the version."""
     sysbuild_app = build_dir / "zephyr"
     if (sysbuild_app / "CMakeCache.txt").is_file():
         return sysbuild_app
@@ -858,17 +860,12 @@ def _prepare_pch(app_dir: Path) -> None:
     )
     # New layout first (Zephyr >= 3.4 nests under zephyr/); fixed candidates
     # keep the .sum identity deterministic and skip walking generated/
-    autoconf = next(
-        (
-            candidate
-            for candidate in (
-                app_dir / "zephyr" / "include" / "generated" / "zephyr" / "autoconf.h",
-                app_dir / "zephyr" / "include" / "generated" / "autoconf.h",
-            )
-            if candidate.exists()
-        ),
-        None,
-    )
+    generated = app_dir / "zephyr" / "include" / "generated"
+    autoconf = None
+    for candidate in (generated / "zephyr" / "autoconf.h", generated / "autoconf.h"):
+        if candidate.exists():
+            autoconf = candidate
+            break
     if autoconf is None:
         # Fail closed: autoconf.h is the .sum's Kconfig identity
         _LOGGER.warning("No autoconf.h found; compiling without the pch")
@@ -960,7 +957,8 @@ def run_compile(args, config: ConfigType) -> bool:
         # so an existing DB is settled, and --cmake-only reconfigures.
         # Sysbuild configures the app image during its own configure, so the
         # app's flags and autoconf.h are settled after this phase too.
-        if not (_app_build_dir(build_dir) / "compile_commands.json").is_file():
+        app_dir = _app_build_dir(build_dir)
+        if not (app_dir / "compile_commands.json").is_file():
             if not run_command_ok(
                 west_cmd + ["--cmake-only", "--", "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON"],
                 env=env,
@@ -968,13 +966,17 @@ def run_compile(args, config: ConfigType) -> bool:
                 cwd=str(paths["framework_path"]),
             ):
                 raise EsphomeError("nRF52 native build configure failed")
+            # The configure phase creates the sysbuild domain dir: re-resolve
+            app_dir = _app_build_dir(build_dir)
             # The pch includes zephyr/kernel.h, whose syscall headers are
-            # generated at build time (same target the clang-tidy flow uses)
+            # generated at build time. clang_tidy.py gets them with a single
+            # `west build -t`, but under sysbuild that target only exists in
+            # the app domain's ninja, not the top-level one, so build it there
             if not run_command_ok(
                 [
                     "cmake",
                     "--build",
-                    str(_app_build_dir(build_dir)),
+                    str(app_dir),
                     "--target",
                     "zephyr_generated_headers",
                 ],
@@ -983,24 +985,10 @@ def run_compile(args, config: ConfigType) -> bool:
                 cwd=str(paths["framework_path"]),
             ):
                 raise EsphomeError("nRF52 Zephyr header generation failed")
+    else:
+        app_dir = _app_build_dir(build_dir)
 
-    # An optional speedup must never abort the build
-    app_dir = _app_build_dir(build_dir)
-    try:
-        _prepare_pch(app_dir)
-    except Exception:  # noqa: BLE001  # pylint: disable=broad-exception-caught
-        # Strict first: its own knob error must not mask the real failure
-        strict = pch.pch_strict()
-        # Raises itself if a stale .gch survives (silently wrong output)
-        pch.discard_pch(app_dir)
-        if strict:
-            raise
-        # Best effort: OBJECT_DEPENDS needs the header even without a pch
-        with suppress(OSError):
-            (app_dir / PCH_HEADER_NAME).touch()
-        _LOGGER.warning(
-            "Precompiled header setup failed; compiling without it", exc_info=True
-        )
+    pch.guarded_prepare(app_dir, lambda: _prepare_pch(app_dir))
 
     if not run_command_ok(
         west_cmd,

esphome/espidf/toolchain.py

@@ -528,24 +528,11 @@ def run_compile(config, verbose: bool) -> int:
             return result.returncode
         _patch_memory_segments()
 
-    # After every reconfigure so compile_commands and sdkconfig are settled.
-    # An optional speedup must never abort the build
-    from esphome.build_gen.espidf import discard_pch, prepare_pch
+    # After every reconfigure so compile_commands and sdkconfig are settled
+    from esphome.build_gen.espidf import prepare_pch
+    from esphome.build_helpers.pch import guarded_prepare
 
-    try:
-        prepare_pch()
-    except Exception:  # noqa: BLE001  # pylint: disable=broad-exception-caught
-        from esphome.build_helpers.pch import pch_strict
-
-        # Strict first: its own knob error must not mask the real failure
-        strict = pch_strict()
-        # Raises itself if a stale .gch survives (silently wrong output)
-        discard_pch()
-        if strict:
-            raise
-        _LOGGER.warning(
-            "Precompiled header setup failed; compiling without it", exc_info=True
-        )
+    guarded_prepare(CORE.relative_build_path("build"), prepare_pch)
 
     # Build
     args = []

tests/unit_tests/components/nrf52/test_pch.py

@@ -25,10 +25,13 @@ def build_dir(tmp_path: Path) -> Path:
     return d
 
 
-def _write_autoconf(build_dir: Path, text: str = "#define CONFIG_GPIO 1\n") -> Path:
+_AUTOCONF_TEXT = "#define CONFIG_GPIO 1\n"
+
+
+def _write_autoconf(build_dir: Path) -> Path:
     autoconf = build_dir / "zephyr" / "include" / "generated" / "zephyr" / "autoconf.h"
     autoconf.parent.mkdir(parents=True)
-    autoconf.write_text(text)
+    autoconf.write_text(_AUTOCONF_TEXT)
     return autoconf
 
 
@@ -58,6 +61,8 @@ def test_prepare_pch_missing_autoconf_degrades(
         nrf52._prepare_pch(build_dir)
     assert not prepare.called
     assert "No autoconf.h found" in caplog.text
+    # The header is written first so OBJECT_DEPENDS stays satisfied
+    assert (build_dir / "esphome_pch.h").is_file()
 
 
 def test_prepare_pch_missing_autoconf_strict_raises(
@@ -93,15 +98,8 @@ def test_app_build_dir_top_level_layout(build_dir: Path) -> None:
     assert nrf52._app_build_dir(build_dir) == build_dir
 
 
-def test_prepare_pch_writes_header_before_degrading(build_dir: Path) -> None:
-    # OBJECT_DEPENDS must be satisfied even when the pch degrades
-    with patch.object(nrf52.pch, "prepare_pch"):
-        nrf52._prepare_pch(build_dir)
-    assert (build_dir / "esphome_pch.h").is_file()
-
-
 def test_prepare_pch_extras_carry_build_identity(build_dir: Path) -> None:
-    _write_autoconf(build_dir, "#define CONFIG_GPIO 1\n")
+    _write_autoconf(build_dir)
     with (
         patch.dict(CORE.data, {KEY_CORE: {KEY_FRAMEWORK_VERSION: "2.9.2"}}),
         patch.object(
@@ -115,12 +113,7 @@ def test_prepare_pch_extras_carry_build_identity(build_dir: Path) -> None:
     (passed_dir, headers, extras) = prepare.call_args.args
     assert passed_dir == build_dir
     assert headers == nrf52.PCH_DEFAULT_HEADERS
-    assert list(extras) == [
-        "2.9.2",
-        "adafruit_feather",
-        "#define CONFIG_GPIO 1\n",
-        "-Os",
-    ]
+    assert list(extras) == ["2.9.2", "adafruit_feather", _AUTOCONF_TEXT, "-Os"]
 
 
 def _generate_cmake(tmp_path: Path) -> str:
@@ -139,27 +132,19 @@ def _generate_cmake(tmp_path: Path) -> str:
     return (tmp_path / "build" / "zephyr" / "CMakeLists.txt").read_text()
 
 
-def test_cmake_lists_pch_block_default(tmp_path: Path) -> None:
+def test_cmake_lists_include_pch_consumer_block(tmp_path: Path) -> None:
+    # Content contract is pinned by the shared pch_cmake_consumer tests;
+    # here only that the block reaches the generated CMakeLists
     text = _generate_cmake(tmp_path)
-    assert "-Winvalid-pch" in text
-    assert "-Wno-error=invalid-pch" in text
-    # Relative -include; the only build-dir reference is the OBJECT_DEPENDS
-    assert '"$<$<COMPILE_LANGUAGE:CXX>:esphome_pch.h>"' in text
+    assert "target_compile_options(app PRIVATE" in text
     assert 'OBJECT_DEPENDS "${CMAKE_BINARY_DIR}/esphome_pch.h"' in text
 
 
-def test_cmake_lists_pch_block_strict_escalates(
-    monkeypatch: pytest.MonkeyPatch, tmp_path: Path
-) -> None:
-    monkeypatch.setenv("ESPHOME_PCH_STRICT", "1")
-    assert "-Werror=invalid-pch" in _generate_cmake(tmp_path)
-
-
 def test_cmake_lists_pch_block_disabled(
     monkeypatch: pytest.MonkeyPatch, tmp_path: Path
 ) -> None:
     monkeypatch.setenv("ESPHOME_PCH_ENABLE", "0")
-    assert "invalid-pch" not in _generate_cmake(tmp_path)
+    assert "esphome_pch.h" not in _generate_cmake(tmp_path)
 
 
 class TestRunCompilePhases:
@@ -245,23 +230,13 @@ class TestRunCompilePhases:
         assert env["CCACHE_PCH_EXTSUM"] == "true"
         assert env["CCACHE_SLOPPINESS"] == "pch_defines,time_macros"
 
-    def test_settled_db_skips_cmake_phase(self, compile_ctx) -> None:
+    @pytest.mark.parametrize("sysbuild", [False, True])
+    def test_settled_db_skips_cmake_phase(self, sysbuild: bool, compile_ctx) -> None:
         run_cmd, prepare, build_dir = compile_ctx
-        build_dir.mkdir(parents=True)
-        # A present cache keeps the pristine wipe from dropping the DB
-        (build_dir / "CMakeCache.txt").write_text("")
-        (build_dir / "compile_commands.json").write_text("[]")
-        run_cmd.side_effect = [False]
-        with pytest.raises(EsphomeError, match="nRF52 native build failed"):
-            self._run()
-        assert run_cmd.call_count == 1
-        assert "--cmake-only" not in run_cmd.call_args.args[0]
-        assert prepare.called
-
-    def test_settled_sysbuild_db_skips_cmake_phase(self, compile_ctx) -> None:
-        run_cmd, prepare, build_dir = compile_ctx
-        app = build_dir / "zephyr"
+        app = build_dir / "zephyr" if sysbuild else build_dir
         app.mkdir(parents=True)
+        # A present top-level cache keeps the pristine wipe from dropping
+        # the DB; the app-dir cache is the sysbuild layout marker
         (build_dir / "CMakeCache.txt").write_text("")
         (app / "CMakeCache.txt").write_text("")
         (app / "compile_commands.json").write_text("[]")

tests/unit_tests/test_espidf_toolchain.py

@@ -684,7 +684,7 @@ def test_run_compile_aborts_when_stale_pch_survives_discard(
         patch.object(toolchain, "print_summary"),
         patch("esphome.build_gen.espidf.prepare_pch", side_effect=RuntimeError("boom")),
         patch(
-            "esphome.build_gen.espidf.discard_pch",
+            "esphome.build_helpers.pch.discard_pch",
             side_effect=EsphomeError("Could not discard the stale precompiled header"),
         ),
         pytest.raises(EsphomeError, match="Could not discard"),