From 833af674bf7d9b2b082e780be979ec87c539f190 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 26 Aug 2026 23:20:06 -0500 Subject: [PATCH] Simplify: shared tool-step runner, probe args helper, buildtool touch stamp, knob-spelling parity --- .github/workflows/ci-docker.yml | 5 +- .github/workflows/ci.yml | 4 +- esphome/build_gen/arduino8266.py | 18 +-- esphome/build_gen/build_tool.py | 8 +- esphome/build_gen/espidf.py | 2 +- esphome/build_helpers/pch.py | 151 +++++++++--------- esphome/platformio/pch.py.script | 11 +- .../unit_tests/build_gen/test_arduino8266.py | 4 +- tests/unit_tests/build_gen/test_build_tool.py | 10 ++ tests/unit_tests/build_gen/test_espidf.py | 38 +++-- 10 files changed, 136 insertions(+), 115 deletions(-) diff --git a/.github/workflows/ci-docker.yml b/.github/workflows/ci-docker.yml index 7c1253364b..0ddef1153f 100644 --- a/.github/workflows/ci-docker.yml +++ b/.github/workflows/ci-docker.yml @@ -203,7 +203,10 @@ jobs: - nrf52 - host # Fail the job if the precompiled header silently degrades on the - # platforms where it must work (libretiny needs a toolchain bump) + # platforms where it must work. Excluded: bk72xx/rtl87xx/ln882x + # (libretiny GCC rejects its own pch until a toolchain bump), + # esp32-*-platformio (no pch; the toolchain is being dropped), + # nrf52 and host (no pch) include: - id: esp8266-arduino pch_strict: "1" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8880455ff8..acb7647990 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1203,8 +1203,8 @@ jobs: # ESP8266 Arduino built directly (no PlatformIO); compile validates # config first, so a separate config pass is redundant. Strict pch: - # the docker smoke test covers the PlatformIO default, this job is - # the only CI exercise of the native ninja pch (and its probe edge). + # exercises the native ninja pch (and its probe edge) against real + # component configs; the docker matrix smoke-tests both toolchains. ESPHOME_PCH_STRICT=1 python3 script/test_build_components.py -e compile -t esp8266-ard -c "$TEST_COMPONENTS" -f --toolchain arduino --fail-on-no-tests device-builder: diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index 2e562c8870..593d175db7 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -40,8 +40,10 @@ from esphome.build_helpers.pch import ( mark_pch_emitted, pch_checksum, pch_degraded, + pch_disabled_degraded, pch_enabled, pch_header_text, + pch_probe_args, pch_strict, ) from esphome.components.esp8266 import build_surgery @@ -1295,17 +1297,12 @@ 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 - # $out only expands in rule text, so the stamp command is - # baked into the rule (generation host == build host) - stamp = ( - "cmd /c copy /y nul $out >nul" if os.name == "nt" else "touch $out" - ) + probe = " ".join(pch_probe_args(PCH_HEADER_NAME, fatal=True)) lines.append("rule pchprobe") + # $out only expands in rule text, hence the inline stamp lines.append( - " command = $cxx $cxxflags $flags -Winvalid-pch" - " -Werror=invalid-pch" - f" -include {PCH_HEADER_NAME} -fsyntax-only -x c++" - f" {_q(Path(os.devnull))} && {stamp}" + f" command = $cxx $cxxflags $flags {probe}" + " && $python $buildtool touch $out" ) lines.append(" description = PCHPROBE $out") lines.append(f"build esphome_pch.probe: pchprobe {gch}") @@ -1315,8 +1312,7 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: src_cxx_override = ("$srccxxflags", pch_dep) mark_pch_emitted() else: - # Strict CI must not read "no pch at all" as success - pch_degraded("pch disabled by ESPHOME_PCH_ENABLE") + pch_disabled_degraded() src_objs = _ninja_compile_edges( lines, _collect_sources(src_dir), diff --git a/esphome/build_gen/build_tool.py b/esphome/build_gen/build_tool.py index 00aa1ec69d..d20fe78673 100644 --- a/esphome/build_gen/build_tool.py +++ b/esphome/build_gen/build_tool.py @@ -6,6 +6,7 @@ started esphome and must not depend on the package being importable. Subcommands: ar remove stale archive, then ``ar rcs`` copy copy a file + touch create/update a stamp file The ar rspfile carries one object path per line (the generating rule must use ``$in_newline``, never ``$in``). @@ -83,9 +84,14 @@ def _run_copy(src: str, dst: str) -> int: return 0 +def _run_touch(path: str) -> int: + Path(path).touch() + return 0 + + # mode -> (handler, expected operand count); surplus argv means a # mis-specified ninja rule and must error, not silently drop operands -_MODES = {"ar": (_run_ar, 3), "copy": (_run_copy, 2)} +_MODES = {"ar": (_run_ar, 3), "copy": (_run_copy, 2), "touch": (_run_touch, 1)} def main() -> int: diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index 7feb6c4c4e..132ec29df7 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -323,7 +323,7 @@ def prepare_pch() -> None: if not pch_enabled(): # Self-cleaning escape hatch: drop any previously built .gch pch.discard_pch(CORE.relative_build_path("build")) - pch.pch_degraded("pch disabled by ESPHOME_PCH_ENABLE") + pch.pch_disabled_degraded() return sdkconfig_path = CORE.relative_build_path(f"sdkconfig.{CORE.name}") try: diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index ada1b00149..e7363938bb 100644 --- a/esphome/build_helpers/pch.py +++ b/esphome/build_helpers/pch.py @@ -115,6 +115,29 @@ def pch_degraded(reason: str) -> None: raise EsphomeError(f"ESPHOME_PCH_STRICT: {reason}") +def pch_disabled_degraded() -> None: + """Strict CI must not read "no pch at all" as success.""" + pch_degraded("pch disabled by ESPHOME_PCH_ENABLE") + + +def pch_probe_args(header: str, fatal: bool = False) -> list[str]: + """Flags that load-check a built .gch via a syntax-only compile. + + ``fatal`` escalates a rejected pch to an error for consumers that + cannot inspect stderr (the ninja probe edge). + """ + return [ + "-Winvalid-pch", + *(["-Werror=invalid-pch"] if fatal else []), + "-include", + header, + "-fsyntax-only", + "-x", + "c++", + os.devnull, + ] + + 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. @@ -149,8 +172,7 @@ 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).""" if not pch_enabled(): - # Strict CI must not read "no pch at all" as success - pch_degraded("pch disabled by ESPHOME_PCH_ENABLE") + pch_disabled_degraded() return [] return ["post:pch.py"] @@ -391,100 +413,75 @@ def prepare_pch( pch_degraded("earlier failure latched") return _log_pch_in_use() - try: - result = subprocess.run( - cmd, - cwd=cmd_dir, - # C locale keeps diagnostics matchable by _TRANSIENT_ERRORS - env={**os.environ, "LC_ALL": "C"}, - capture_output=True, - text=True, - check=False, - timeout=300, - ) - error = None - if result.returncode < 0: + + def _run(run_cmd: list[str], what: str) -> subprocess.CompletedProcess | None: + """Spawn one pch tool step; environmental failures discard and + degrade (None): spawn/IO/timeout errors and signal kills never + latch the marker.""" + try: + proc = subprocess.run( + run_cmd, + cwd=cmd_dir, + # C locale keeps diagnostics matchable by _TRANSIENT_ERRORS + env={**os.environ, "LC_ALL": "C"}, + capture_output=True, + text=True, + check=False, + timeout=300, + ) + except (OSError, subprocess.SubprocessError) as err: + _LOGGER.warning("Precompiled header %s did not run: %s", what, err) + discard_pch(build_dir) + pch_degraded(f"{what} did not run: {err}") + return None + if proc.returncode < 0: # Killed by a signal (OOM, ^C): environmental, do not latch _LOGGER.warning( - "Precompiled header compile was killed (signal %d); retrying " - "next build", - -result.returncode, + "Precompiled header %s was killed (signal %d); retrying next build", + what, + -proc.returncode, ) discard_pch(build_dir) - pch_degraded(f"compile killed by signal {-result.returncode}") - return - if result.returncode != 0: - error = result.stderr.strip() or f"exit code {result.returncode}" - elif not gch.is_file(): - error = "compiler produced no .gch" - except (OSError, subprocess.SubprocessError) as err: - # Transient (timeout, spawn/IO): warn and retry next build, no marker - _LOGGER.warning("Precompiled header compile did not run: %s", err) - discard_pch(build_dir) - pch_degraded(f"compile did not run: {err}") - return - if error is not None: + pch_degraded(f"{what} killed by signal {-proc.returncode}") + return None + return proc + + def _fail(error: str, reason: str) -> None: + """Discard and degrade; deterministic failures also latch.""" _LOGGER.warning( "Precompiled header failed; compiling without it: %s", error[:400] ) - # This path latches, so keep the full compiler output recoverable - _LOGGER.debug("Full pch compile output: %s", error) + # 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 compile failure: {error[:200]}") + 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) - pch_degraded(f"compile failed: {error[:200]}") + pch_degraded(f"{reason}: {error[:200]}") + + result = _run(cmd, "compile") + if result is None: + return + error = None + if result.returncode != 0: + error = result.stderr.strip() or f"exit code {result.returncode}" + elif not gch.is_file(): + error = "compiler produced no .gch" + if error is not None: + _fail(error, "compile failed") return # Load probe: some toolchains build a .gch they then refuse to load # (per-process ASLR); dep flags are already stripped from cmd, so no - # -MF is needed - # cmd ends with the fixed "-x c++-header -c
-o " tail - probe_cmd = [ - *cmd[:-6], - "-Winvalid-pch", - "-include", - str(header), - "-fsyntax-only", - "-x", - "c++", - os.devnull, - ] - try: - probe = subprocess.run( - probe_cmd, - cwd=cmd_dir, - env={**os.environ, "LC_ALL": "C"}, - capture_output=True, - text=True, - check=False, - timeout=300, - ) - except (OSError, subprocess.SubprocessError) as err: - _LOGGER.warning("Precompiled header probe did not run: %s", err) - discard_pch(build_dir) - pch_degraded(f"probe did not run: {err}") - return - if probe.returncode < 0: - # Killed by a signal (OOM, ^C): environmental, do not latch - _LOGGER.warning( - "Precompiled header probe was killed (signal %d); retrying next build", - -probe.returncode, - ) - discard_pch(build_dir) - pch_degraded(f"probe killed by signal {-probe.returncode}") + # -MF is needed. cmd ends with the fixed "-x c++-header -c -o" tail. + probe = _run([*cmd[:-6], *pch_probe_args(str(header))], "probe") + if probe is None: return if probe.returncode != 0 or ".gch" in probe.stderr: - error = f"toolchain cannot load the pch: {probe.stderr.strip()[:400]}" - _LOGGER.warning("Precompiled header failed; compiling without it: %s", error) - discard_pch(build_dir) - if not any(m in error for m in _TRANSIENT_ERRORS): - failed_marker.write_text(checksum + "\n", encoding="utf-8") - os.utime(header) - pch_degraded(error) + _fail(probe.stderr.strip(), "toolchain cannot load the pch") return failed_marker.unlink(missing_ok=True) sum_path.write_text(checksum + "\n", encoding="utf-8") diff --git a/esphome/platformio/pch.py.script b/esphome/platformio/pch.py.script index 4c737460b7..dcd944d6d6 100644 --- a/esphome/platformio/pch.py.script +++ b/esphome/platformio/pch.py.script @@ -20,7 +20,8 @@ except Exception as err: # noqa: BLE001 -- not exported under -t nobuild # Precompile the src force-includes plus defines.h and force-include the # result into C++ src compiles only; their preprocessed output is unchanged. # Registration is gated host-side (pch_enabled()). Keep the closure, ccache -# values, and stamp flow in sync with build_helpers/pch.py. +# values, probe flow, stamp flow, and env-knob spellings in sync with +# build_helpers/pch.py. # Compiler failures that clear on their own must not latch the .failed marker _TRANSIENT_ERRORS = ("No space left", "Cannot allocate", "Resource temporarily") @@ -364,17 +365,21 @@ def _setup_pch() -> bool | None: return True +# Keep in sync with helpers.TRUTHY_ENV_STRINGS _strict = os.environ.get("ESPHOME_PCH_STRICT", "").strip().lower() in ( "1", "true", "yes", "on", + "enable", ) try: - if not _setup_pch() and _strict: - raise RuntimeError("ESPHOME_PCH_STRICT: precompiled header was not used") + _used = _setup_pch() except Exception: # noqa: BLE001 -- a speedup must never break the build if _strict: raise print("ESPHome: pch internal error; compiling without it") traceback.print_exc() +else: + if _strict and not _used: + raise RuntimeError("ESPHOME_PCH_STRICT: precompiled header was not used") diff --git a/tests/unit_tests/build_gen/test_arduino8266.py b/tests/unit_tests/build_gen/test_arduino8266.py index 56f8d7c867..b2384161dc 100644 --- a/tests/unit_tests/build_gen/test_arduino8266.py +++ b/tests/unit_tests/build_gen/test_arduino8266.py @@ -1872,8 +1872,8 @@ def test_write_project_pch_strict_emits_probe_edge( assert "build esphome_pch.probe: pchprobe esphome_pch.h.gch" in content assert "-Werror=invalid-pch" in content # $out only expands in rule text; an edge-level binding would emit a - # bare "touch " and fail every strict build - assert "&& touch $out" in content + # bare stamp command and fail every strict build + assert "&& $python $buildtool touch $out" in content assert "$stamp" not in content # With extra src flags the probe edge carries them like the .gch edge diff --git a/tests/unit_tests/build_gen/test_build_tool.py b/tests/unit_tests/build_gen/test_build_tool.py index b029c647ab..5beef92b10 100644 --- a/tests/unit_tests/build_gen/test_build_tool.py +++ b/tests/unit_tests/build_gen/test_build_tool.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os from pathlib import Path import subprocess import sys @@ -244,3 +245,12 @@ def test_copy_failure_leaves_no_partial_output(tmp_path: Path) -> None: ): assert build_tool.main() == 1 assert not dst.exists() + + +def test_touch_creates_and_updates_stamp(tmp_path: Path) -> None: + stamp = tmp_path / "esphome_pch.probe" + assert build_tool._run_touch(str(stamp)) == 0 + assert stamp.is_file() + os.utime(stamp, (1, 1)) + assert build_tool._run_touch(str(stamp)) == 0 + assert stamp.stat().st_mtime > 1 diff --git a/tests/unit_tests/build_gen/test_espidf.py b/tests/unit_tests/build_gen/test_espidf.py index c2d7d0906e..dea020eadf 100644 --- a/tests/unit_tests/build_gen/test_espidf.py +++ b/tests/unit_tests/build_gen/test_espidf.py @@ -903,30 +903,34 @@ def test_prepare_pch_probe_spawn_failure_degrades( prepare_pch() +@pytest.mark.parametrize( + ("stderr", "code"), + [("", -9), ("fatal: No space left on device", 1)], + ids=("signal-kill", "enospc"), +) def test_prepare_pch_probe_environmental_failures_do_not_latch( - tmp_path: Path, + tmp_path: Path, stderr: str, code: int ) -> None: """A signal-killed or ENOSPC probe retries next build, no marker.""" from esphome.build_gen.espidf import prepare_pch - for stderr, code in (("", -9), ("fatal: No space left on device", 1)): - dev = _make_pch_device(tmp_path, f"dev_pe{code}") - CORE.build_path = dev - gch = dev / "build" / "esphome_pch.h.gch" + dev = _make_pch_device(tmp_path, "dev_pe") + CORE.build_path = dev + gch = dev / "build" / "esphome_pch.h.gch" - def env_probe(cmd, _gch=gch, _stderr=stderr, _code=code, **kwargs): - if "-fsyntax-only" in cmd: - return subprocess.CompletedProcess(cmd, _code, "", _stderr) - _gch.write_bytes(b"gch") - return subprocess.CompletedProcess(cmd, 0, "", "") + def env_probe(cmd, **kwargs): + if "-fsyntax-only" in cmd: + return subprocess.CompletedProcess(cmd, code, "", stderr) + gch.write_bytes(b"gch") + return subprocess.CompletedProcess(cmd, 0, "", "") - with ( - patch.object(CORE, "name", "test"), - patch("esphome.build_helpers.pch.subprocess.run", side_effect=env_probe), - ): - prepare_pch() - assert not gch.exists() - assert not (dev / "build" / "esphome_pch.h.gch.failed").exists() + with ( + patch.object(CORE, "name", "test"), + patch("esphome.build_helpers.pch.subprocess.run", side_effect=env_probe), + ): + prepare_pch() + assert not gch.exists() + assert not (dev / "build" / "esphome_pch.h.gch.failed").exists() def test_prepare_pch_signal_kill_strict_raises(