From 8a5d53899cb891d6db657167e944a817cfc94cbf Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 08:40:35 -0500 Subject: [PATCH] Simplify: strict knob via parse_enable_env, one probe tail, latch folded into _fail, discard owns its policy --- esphome/build_gen/arduino8266.py | 9 +-- esphome/build_gen/espidf.py | 2 +- esphome/build_helpers/ccache.py | 9 ++- esphome/build_helpers/pch.py | 83 +++++++++++----------- esphome/espidf/toolchain.py | 15 +--- esphome/platformio/pch.py.script | 16 ++--- tests/unit_tests/build_helpers/test_pch.py | 45 ++++++++++++ tests/unit_tests/test_espidf_toolchain.py | 19 ++--- 8 files changed, 118 insertions(+), 80 deletions(-) diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index ae9f6a28bd..93ad7fd519 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -39,6 +39,7 @@ from esphome.build_helpers.pch import ( PCH_HEADER_NAME, mark_pch_emitted, pch_checksum, + pch_consumer_escalation, pch_degraded, pch_disabled_degraded, pch_enabled, @@ -1292,9 +1293,7 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: # strict inverts it so any consumer rejection reds the build # (rejection is per-process, so the probe alone cannot prove # the consumers) - escalation = ( - "-Werror=invalid-pch" if pch_strict() else "-Wno-error=invalid-pch" - ) + escalation = pch_consumer_escalation() cxx_parts = src_other + [ f"-Winvalid-pch {escalation} -include {PCH_HEADER_NAME}" ] @@ -1303,9 +1302,7 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: if pch_strict(): # Consumers wait on the probe stamp, so an unloadable .gch # reds the build here instead of warning ~100 times - probe = " ".join( - pch_probe_args(PCH_HEADER_NAME, source=str(Path(os.devnull))) - ) + probe = " ".join(pch_probe_args(PCH_HEADER_NAME, source=os.devnull)) lines.append("rule pchprobe") # $out only expands in rule text, hence the inline stamp lines.append( diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index f0d7b7c885..6309f8c1ec 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -301,7 +301,7 @@ def _pch_cmake() -> str: # 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 = "-Werror=invalid-pch" if pch.pch_strict() else "-Wno-error=invalid-pch" + 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 diff --git a/esphome/build_helpers/ccache.py b/esphome/build_helpers/ccache.py index 1915b4a20c..32724b4a8e 100644 --- a/esphome/build_helpers/ccache.py +++ b/esphome/build_helpers/ccache.py @@ -21,12 +21,13 @@ def _ccache_runs(ccache: str) -> bool: ) -def parse_enable_env(name: str) -> bool | None: +def parse_enable_env(name: str, strict: bool = False) -> bool | None: """Strictly parse an on/off environment knob; None when unset or invalid. ``bool(str)`` truthiness would flip ``no``/``off`` to enabled, so only 1/true/yes/on and 0/false/no/off count; anything else warns and reads - as unset so the caller's default policy applies. + as unset so the caller's default policy applies — or raises when + ``strict`` (a typo must not silently disable a CI gate). """ raw = os.environ.get(name) if raw is None: @@ -39,6 +40,10 @@ def parse_enable_env(name: str) -> bool | None: return True if lowered in FALSY_ENV_STRINGS: return False + if strict: + from esphome.core import EsphomeError + + raise EsphomeError(f"Unrecognized {name}={raw!r}; use 1 or 0") _LOGGER.warning("Ignoring unrecognized %s=%r; use 1 or 0", name, raw) return None diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index 078ac51d9c..4c6e3783ed 100644 --- a/esphome/build_helpers/pch.py +++ b/esphome/build_helpers/pch.py @@ -108,15 +108,7 @@ def pch_strict() -> bool: A set-but-unrecognized value raises: a typo must not silently turn the gate into a no-op that proves nothing. """ - parsed = parse_enable_env("ESPHOME_PCH_STRICT") - if parsed is None and os.environ.get("ESPHOME_PCH_STRICT") is not None: - from esphome.core import EsphomeError - - raise EsphomeError( - f"Unrecognized ESPHOME_PCH_STRICT=" - f"{os.environ['ESPHOME_PCH_STRICT']!r}; use 1 or 0" - ) - return parsed is True + return parse_enable_env("ESPHOME_PCH_STRICT", strict=True) is True def pch_degraded(reason: str) -> None: @@ -132,6 +124,11 @@ def pch_disabled_degraded() -> None: pch_degraded("pch disabled by ESPHOME_PCH_ENABLE") +def pch_probe_tail(source: str = "-") -> list[str]: + """The syntax-only compile shared by the probe and its baseline.""" + return ["-fsyntax-only", "-x", "c++", source] + + def pch_probe_args(header: str, source: str = "-") -> list[str]: """Flags that load-check a built .gch via a syntax-only compile. @@ -144,13 +141,16 @@ def pch_probe_args(header: str, source: str = "-") -> list[str]: "-Werror=invalid-pch", "-include", header, - "-fsyntax-only", - "-x", - "c++", - source, + *pch_probe_tail(source), ] +def pch_consumer_escalation() -> str: + """Consumer-side invalid-pch flag: strict reds the build on rejection + (per-process, so the probe alone cannot prove the consumers).""" + return "-Werror=invalid-pch" if pch_strict() else "-Wno-error=invalid-pch" + + 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. @@ -354,13 +354,24 @@ def discard_pch(build_dir: Path) -> None: 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. + .gch must not force a full rebuild every build. A .gch that survives + an unlink failure would be consumed silently (wrong output, not a + slow build), so that raises. """ header = build_dir / PCH_HEADER_NAME gch = Path(f"{header}.gch") had_gch = gch.is_file() - gch.unlink(missing_ok=True) - Path(f"{gch}.sum").unlink(missing_ok=True) + try: + gch.unlink(missing_ok=True) + Path(f"{gch}.sum").unlink(missing_ok=True) + except OSError as err: + if gch.is_file(): + from esphome.core import EsphomeError + + raise EsphomeError( + f"Could not discard the stale precompiled header: {err}" + ) from err + _LOGGER.warning("Could not discard the pch sidecars: %s", err) if had_gch and header.is_file(): os.utime(header) @@ -451,21 +462,18 @@ def prepare_pch( return None return proc - def _fail(error: str, reason: str) -> None: - """Discard and degrade; deterministic failures also latch.""" + def _fail(error: str, reason: str, latch: bool) -> None: + """Discard and degrade; deterministic failures latch when asked.""" _LOGGER.warning( "Precompiled header failed; compiling without it: %s", error[:400] ) # Latching paths keep the full compiler output recoverable _LOGGER.debug("Full pch output: %s", error) discard_pch(build_dir) - if any(m in error for m in _TRANSIENT_ERRORS): - # Resource exhaustion clears on its own; retry next build - pch_degraded(f"transient {reason}: {error[:200]}") - return - # Skip retries until a header/flag/backend-identity/command change - failed_marker.write_text(checksum + "\n", encoding="utf-8") - os.utime(header) + if latch and not any(m in error for m in _TRANSIENT_ERRORS): + # Skip retries until a header/flag/backend-identity/command change + failed_marker.write_text(checksum + "\n", encoding="utf-8") + os.utime(header) pch_degraded(f"{reason}: {error[:200]}") def _probe(latch: bool = True) -> None: @@ -486,23 +494,16 @@ def prepare_pch( return if probe.returncode != 0: error = probe.stderr.strip() or f"exit code {probe.returncode}" - # Disambiguate: only blame the pch when the same compile passes - # without it; anything else is environmental and must not latch - # pch_probe_args minus the warning flags and the -include pair - baseline = _run( - [*base, *pch_probe_args(str(header))[4:]], "probe baseline", stdin="" - ) + # Disambiguate: only blame (and latch on) the pch when the same + # compile passes without it; anything else is environmental + baseline = _run([*base, *pch_probe_tail()], "probe baseline", stdin="") if baseline is None: return - if latch and baseline.returncode == 0: - _fail(error, "toolchain cannot load the pch") - else: - _LOGGER.warning( - "Precompiled header failed; compiling without it: %s", - error[:400], - ) - discard_pch(build_dir) - pch_degraded(f"toolchain cannot load the pch: {error[:200]}") + _fail( + error, + "toolchain cannot load the pch", + latch=latch and baseline.returncode == 0, + ) if gch.is_file() and _read_stamp(sum_path) == checksum: _log_pch_in_use() @@ -529,7 +530,7 @@ def prepare_pch( elif not gch.is_file(): error = "compiler produced no .gch" if error is not None: - _fail(error, "compile failed") + _fail(error, "compile failed", latch=True) return _probe() if not gch.is_file(): diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 69258b9163..a1e3f055fd 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -535,21 +535,10 @@ def run_compile(config, verbose: bool) -> int: try: prepare_pch() except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught - # Discard so a stale .gch can never be consumed - try: - discard_pch() - except OSError as discard_err: - # A stale .gch that survives would be consumed silently: that - # is wrong output, not a slow build, so it must abort - from esphome.build_helpers.pch import PCH_HEADER_NAME - - if CORE.relative_build_path("build", f"{PCH_HEADER_NAME}.gch").is_file(): - raise EsphomeError( - f"Could not discard the stale precompiled header: {discard_err}" - ) from discard_err - _LOGGER.warning("Could not discard the stale pch: %s", discard_err) from esphome.build_helpers.pch import pch_strict + # Raises itself if a stale .gch survives (silently wrong output) + discard_pch() if pch_strict(): raise _LOGGER.warning( diff --git a/esphome/platformio/pch.py.script b/esphome/platformio/pch.py.script index ee56a33bc1..76c074aee2 100644 --- a/esphome/platformio/pch.py.script +++ b/esphome/platformio/pch.py.script @@ -27,14 +27,12 @@ except Exception as err: # noqa: BLE001 -- not exported under -t nobuild _TRANSIENT_ERRORS = ("No space left", "Cannot allocate", "Resource temporarily") # Keep in sync with helpers.TRUTHY_ENV_STRINGS / FALSY_ENV_STRINGS +_TRUTHY = ("1", "true", "yes", "on", "enable") +_FALSY = ("", "0", "false", "no", "off", "disable") _STRICT_RAW = os.environ.get("ESPHOME_PCH_STRICT") _STRICT_VALUE = (_STRICT_RAW or "").strip().lower() -_STRICT = _STRICT_VALUE in ("1", "true", "yes", "on", "enable") -if ( - _STRICT_RAW is not None - and not _STRICT - and _STRICT_VALUE not in ("", "0", "false", "no", "off", "disable") -): +_STRICT = _STRICT_VALUE in _TRUTHY +if _STRICT_RAW is not None and _STRICT_VALUE not in _TRUTHY + _FALSY: # A typo must not silently turn the gate into a no-op raise RuntimeError(f"Unrecognized ESPHOME_PCH_STRICT={_STRICT_RAW!r}; use 1 or 0") @@ -186,9 +184,9 @@ def _setup_pch() -> bool | None: try: from SCons.Script import COMMAND_LINE_TARGETS except ImportError: - # No SCons is an anomaly under PlatformIO: strict must not - # read the unknown state as success - return not _STRICT + # No SCons is an anomaly under PlatformIO: the unknown state + # must not read as success (strict decides fatality at the gate) + return False # Expected under -t nobuild (nothing compiles); a missing # projenv on a real compile must not pass strict return "nobuild" in [str(t) for t in COMMAND_LINE_TARGETS] diff --git a/tests/unit_tests/build_helpers/test_pch.py b/tests/unit_tests/build_helpers/test_pch.py index 0a66f6a629..e55b2cf4fc 100644 --- a/tests/unit_tests/build_helpers/test_pch.py +++ b/tests/unit_tests/build_helpers/test_pch.py @@ -264,3 +264,48 @@ def test_pch_strict_rejects_unrecognized_values( monkeypatch.setenv("ESPHOME_PCH_STRICT", "yolo") with pytest.raises(EsphomeError, match="Unrecognized ESPHOME_PCH_STRICT"): pch.pch_strict() + + +def test_discard_pch_raises_when_gch_survives( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A .gch an unlink failure leaves behind would be consumed silently.""" + from pathlib import Path as _P + + from esphome.core import EsphomeError + + (tmp_path / "esphome_pch.h").write_text("") + gch = tmp_path / "esphome_pch.h.gch" + gch.write_bytes(b"gch") + real_unlink = _P.unlink + + def failing_unlink(self, missing_ok=False): + if self.name.endswith(".gch"): + raise OSError("readonly") + return real_unlink(self, missing_ok=missing_ok) + + monkeypatch.setattr(_P, "unlink", failing_unlink) + with pytest.raises(EsphomeError, match="Could not discard"): + pch.discard_pch(tmp_path) + + +def test_discard_pch_warns_when_only_sidecar_unlink_fails( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + from pathlib import Path as _P + + (tmp_path / "esphome_pch.h").write_text("") + (tmp_path / "esphome_pch.h.gch").write_bytes(b"gch") + (tmp_path / "esphome_pch.h.gch.sum").write_text("x") + real_unlink = _P.unlink + + def failing_unlink(self, missing_ok=False): + if self.name.endswith(".sum"): + raise OSError("readonly") + return real_unlink(self, missing_ok=missing_ok) + + monkeypatch.setattr(_P, "unlink", failing_unlink) + pch.discard_pch(tmp_path) + assert "Could not discard the pch sidecars" in caplog.text diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index da02334736..19de30c7c6 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -669,12 +669,12 @@ def test_get_core_framework_version_from_core_data(): assert toolchain._get_core_framework_version() == "5.5.4" -def test_run_compile_logs_failed_pch_discard( - setup_core: Path, - monkeypatch: pytest.MonkeyPatch, - caplog: pytest.LogCaptureFixture, +def test_run_compile_aborts_when_stale_pch_survives_discard( + setup_core: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """A discard failure in the catch-all must be visible, not silent.""" + """An undiscardable stale .gch means silently wrong output: abort.""" + from esphome.core import EsphomeError + monkeypatch.setenv("ESPHOME_PCH_ENABLE", "1") _setup_build(setup_core) @@ -683,10 +683,13 @@ def test_run_compile_logs_failed_pch_discard( 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")), - patch("esphome.build_gen.espidf.discard_pch", side_effect=OSError("readonly")), + patch( + "esphome.build_gen.espidf.discard_pch", + side_effect=EsphomeError("Could not discard the stale precompiled header"), + ), + pytest.raises(EsphomeError, match="Could not discard"), ): - assert toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) == 0 - assert "Could not discard the stale pch" in caplog.text + toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) def test_run_compile_strict_reraises_pch_failure(