From cf084bc70fe6cdc3f5ede9122f8e1860e30b01bd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 10:53:26 -0500 Subject: [PATCH] Share the CMake pch consumer block, apply review suggestions --- esphome/build_gen/espidf.py | 27 +++--------- esphome/build_helpers/pch.py | 27 ++++++++++++ esphome/components/nrf52/__init__.py | 44 +++++++++---------- tests/unit_tests/build_helpers/test_pch.py | 28 ++++++++++++ tests/unit_tests/components/nrf52/test_pch.py | 24 ++++++++-- 5 files changed, 103 insertions(+), 47 deletions(-) diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index 6309f8c1ec..e8dc85a150 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -291,30 +291,13 @@ target_link_options(${{COMPONENT_LIB}} PUBLIC def _pch_cmake() -> str: - """The src component's precompiled-header block (C++ TUs only). + """Consumer block appended to the component CMakeLists. - The -include stays relative (resolved from the compiler cwd, the build - dir); an absolute path would poison ccache keys. + Strict inverts: a per-process consumer rejection reds the build. + Baked at generation: a knob flip takes effect when the CMakeLists is + rewritten (every esphome compile); a hand-run idf.py keeps the old one """ - if not pch_enabled(): - return "" - # Strict inverts: a per-process consumer rejection reds the build. - # Baked at generation: a knob flip takes effect when the CMakeLists is - # rewritten (every esphome compile); a hand-run idf.py keeps the old one - escalation = pch.pch_consumer_escalation() - return f""" -# ESPHome precompiled header (see esphome/build_helpers/pch.py). -# OBJECT_DEPENDS is on the header, not the .gch: pch-baked headers drop -# out of TU depfiles, and prepare_pch() touches the header on rebuild. -target_compile_options(${{COMPONENT_LIB}} PRIVATE - "$<$:-Winvalid-pch>" - "$<$:{escalation}>" - "$<$:-include>" - "$<$:{PCH_HEADER_NAME}>" -) -set_source_files_properties(${{app_sources}} PROPERTIES - OBJECT_DEPENDS "${{CMAKE_BINARY_DIR}}/{PCH_HEADER_NAME}") -""" + return pch.pch_cmake_consumer("${COMPONENT_LIB}", "${app_sources}") def discard_pch() -> None: diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index 0bbc0e718d..bbaeef420c 100644 --- a/esphome/build_helpers/pch.py +++ b/esphome/build_helpers/pch.py @@ -156,6 +156,33 @@ def pch_consumer_escalation() -> str: return "-Werror=invalid-pch" if pch_strict() else "-Wno-error=invalid-pch" +def pch_cmake_consumer(target: str, sources_var: str) -> str: + """Emit the CMake block making ``target``'s C++ sources consume the + pch; empty when disabled. Shared by every CMake-based backend so the + consumer contract (flags, relative include, header dependency) + cannot drift between them. + + OBJECT_DEPENDS is on the header, not the .gch: pch-baked headers drop + out of TU depfiles, and prepare_pch() touches the header on rebuild. + The -include stays relative (resolved from the compiler cwd, the build + dir); an absolute path would poison ccache keys. + """ + if not pch_enabled(): + return "" + escalation = pch_consumer_escalation() + return f""" +# ESPHome precompiled header (see esphome/build_helpers/pch.py) +target_compile_options({target} PRIVATE + "$<$:-Winvalid-pch>" + "$<$:{escalation}>" + "$<$:-include>" + "$<$:{PCH_HEADER_NAME}>" +) +set_source_files_properties({sources_var} PROPERTIES + OBJECT_DEPENDS "${{CMAKE_BINARY_DIR}}/{PCH_HEADER_NAME}") +""" + + def ccache_pch_env() -> dict[str, str]: """Settings ccache needs to cache compiles that consume the .gch; empty unless this build actually emitted one. User-set values win. diff --git a/esphome/components/nrf52/__init__.py b/esphome/components/nrf52/__init__.py index 9107ff5149..e5b542d878 100644 --- a/esphome/components/nrf52/__init__.py +++ b/esphome/components/nrf52/__init__.py @@ -1,6 +1,7 @@ from __future__ import annotations import asyncio +from contextlib import suppress import logging from pathlib import Path import re @@ -13,7 +14,7 @@ from esphome.build_helpers.pch import ( PCH_DEFAULT_HEADERS, PCH_HEADER_NAME, mark_pch_emitted, - pch_consumer_escalation, + pch_cmake_consumer, pch_enabled, pch_header_text, ) @@ -814,24 +815,8 @@ def _generate_cmake_lists() -> bool: ")", ] - if pch_enabled(): - # ESPHome precompiled header (see esphome/build_helpers/pch.py). - # OBJECT_DEPENDS is on the header, not the .gch: pch-baked headers - # drop out of TU depfiles, and prepare_pch() touches the header on - # rebuild. The relative -include resolves from the compiler cwd - # (the build dir); an absolute path would poison ccache keys. - escalation = pch_consumer_escalation() - lines += [ - "", - "target_compile_options(app PRIVATE", - ' "$<$:-Winvalid-pch>"', - f' "$<$:{escalation}>"', - ' "$<$:-include>"', - f' "$<$:{PCH_HEADER_NAME}>"', - ")", - "set_source_files_properties(${APP_SOURCES} PROPERTIES", - f' OBJECT_DEPENDS "${{CMAKE_BINARY_DIR}}/{PCH_HEADER_NAME}")', - ] + if consumer := pch_cmake_consumer("app", "${APP_SOURCES}"): + lines += consumer.rstrip("\n").splitlines() if link_flags: lines += [ @@ -871,7 +856,19 @@ def _prepare_pch(app_dir: Path) -> None: write_file_if_changed( app_dir / PCH_HEADER_NAME, pch_header_text(PCH_DEFAULT_HEADERS) ) - autoconf = next(app_dir.glob("zephyr/include/generated/**/autoconf.h"), 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, + ) if autoconf is None: # Fail closed: autoconf.h is the .sum's Kconfig identity _LOGGER.warning("No autoconf.h found; compiling without the pch") @@ -970,7 +967,7 @@ def run_compile(args, config: ConfigType) -> bool: stream_output=True, cwd=str(paths["framework_path"]), ): - raise EsphomeError("nRF52 native build failed") + raise EsphomeError("nRF52 native build configure failed") # The pch includes zephyr/kernel.h, whose syscall headers are # generated at build time (same target the clang-tidy flow uses) if not run_command_ok( @@ -985,7 +982,7 @@ def run_compile(args, config: ConfigType) -> bool: stream_output=True, cwd=str(paths["framework_path"]), ): - raise EsphomeError("nRF52 native build failed") + raise EsphomeError("nRF52 Zephyr header generation failed") # An optional speedup must never abort the build app_dir = _app_build_dir(build_dir) @@ -998,6 +995,9 @@ def run_compile(args, config: ConfigType) -> bool: 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 ) diff --git a/tests/unit_tests/build_helpers/test_pch.py b/tests/unit_tests/build_helpers/test_pch.py index cb931816c4..c6e04bf1d0 100644 --- a/tests/unit_tests/build_helpers/test_pch.py +++ b/tests/unit_tests/build_helpers/test_pch.py @@ -232,6 +232,34 @@ def test_pch_strict( assert pch.pch_strict() is expected +def test_pch_cmake_consumer_substitutes_target_and_sources( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv("ESPHOME_PCH_ENABLE", raising=False) + monkeypatch.delenv("ESPHOME_PCH_STRICT", raising=False) + block = pch.pch_cmake_consumer("app", "${APP_SOURCES}") + assert "target_compile_options(app PRIVATE" in block + assert '"$<$:-Winvalid-pch>"' in block + assert "-Wno-error=invalid-pch" in block + assert '"$<$:esphome_pch.h>"' in block + assert "set_source_files_properties(${APP_SOURCES} PROPERTIES" in block + assert 'OBJECT_DEPENDS "${CMAKE_BINARY_DIR}/esphome_pch.h"' in block + + +def test_pch_cmake_consumer_strict_escalates( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("ESPHOME_PCH_STRICT", "1") + assert "-Werror=invalid-pch" in pch.pch_cmake_consumer("app", "${APP_SOURCES}") + + +def test_pch_cmake_consumer_empty_when_disabled( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("ESPHOME_PCH_ENABLE", "0") + assert pch.pch_cmake_consumer("app", "${APP_SOURCES}") == "" + + def test_pch_degraded_raises_only_in_strict( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/unit_tests/components/nrf52/test_pch.py b/tests/unit_tests/components/nrf52/test_pch.py index ae90570c4a..420ff063f5 100644 --- a/tests/unit_tests/components/nrf52/test_pch.py +++ b/tests/unit_tests/components/nrf52/test_pch.py @@ -221,17 +221,30 @@ class TestRunCompilePhases: def test_generated_headers_failure_raises(self, compile_ctx) -> None: run_cmd, prepare, _ = compile_ctx run_cmd.side_effect = [True, False] - with pytest.raises(EsphomeError, match="nRF52 native build failed"): + with pytest.raises(EsphomeError, match="header generation failed"): self._run() assert not prepare.called def test_cmake_phase_failure_raises(self, compile_ctx) -> None: run_cmd, prepare, _ = compile_ctx run_cmd.side_effect = [False] - with pytest.raises(EsphomeError, match="nRF52 native build failed"): + with pytest.raises(EsphomeError, match="configure failed"): self._run() assert not prepare.called + def test_ccache_pch_env_reaches_west(self, compile_ctx) -> None: + run_cmd, _, _ = compile_ctx + run_cmd.side_effect = [False] + # clear=True also drops ambient CCACHE_*/ESPHOME_PCH_* overrides + with ( + patch.dict("os.environ", {}, clear=True), + pytest.raises(EsphomeError, match="configure failed"), + ): + self._run() + env = run_cmd.call_args.kwargs["env"] + 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: run_cmd, prepare, build_dir = compile_ctx build_dir.mkdir(parents=True) @@ -275,13 +288,18 @@ class TestRunCompilePhases: def test_prepare_failure_never_aborts_the_build( self, compile_ctx, caplog: pytest.LogCaptureFixture ) -> None: - run_cmd, prepare, _ = compile_ctx + run_cmd, prepare, build_dir = compile_ctx + build_dir.mkdir(parents=True) + # Keep the pristine wipe from dropping the dir the fallback touches + (build_dir / "CMakeCache.txt").write_text("") prepare.side_effect = RuntimeError("boom") run_cmd.side_effect = [True, True, False] with pytest.raises(EsphomeError, match="nRF52 native build failed"): self._run() assert run_cmd.call_count == 3 assert "Precompiled header setup failed" in caplog.text + # The fallback still satisfies OBJECT_DEPENDS + assert (build_dir / "esphome_pch.h").is_file() def test_prepare_failure_strict_raises( self, monkeypatch: pytest.MonkeyPatch, compile_ctx