From d45e0074a398756610f09f84e588e734b5705101 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 11:01:46 -0500 Subject: [PATCH] Share the guarded pch prepare contract, simplify wiring and tests --- esphome/build_gen/espidf.py | 5 -- esphome/build_helpers/pch.py | 26 +++++++- esphome/components/nrf52/__init__.py | 56 +++++++---------- esphome/espidf/toolchain.py | 21 ++----- tests/unit_tests/components/nrf52/test_pch.py | 63 ++++++------------- tests/unit_tests/test_espidf_toolchain.py | 2 +- 6 files changed, 71 insertions(+), 102 deletions(-) diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index e8dc85a150..3af65510a3 100644 --- a/esphome/build_gen/espidf.py +++ b/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.""" diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index bbaeef420c..72c87721f8 100644 --- a/esphome/build_helpers/pch.py +++ b/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).""" diff --git a/esphome/components/nrf52/__init__.py b/esphome/components/nrf52/__init__.py index e5b542d878..f16d5caaac 100644 --- a/esphome/components/nrf52/__init__.py +++ b/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, diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 5d7f96c716..f6f072a680 100644 --- a/esphome/espidf/toolchain.py +++ b/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 = [] diff --git a/tests/unit_tests/components/nrf52/test_pch.py b/tests/unit_tests/components/nrf52/test_pch.py index 420ff063f5..77e92f8d95 100644 --- a/tests/unit_tests/components/nrf52/test_pch.py +++ b/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 '"$<$: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("[]") diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index 19de30c7c6..22f3624bcb 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/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"),