From c89c2d975d24a96e88e7c601155a750609cce6f6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 00:25:38 -0500 Subject: [PATCH] Strict escalates consumer invalid-pch, re-probes cached headers everywhere, and defaults the matrix to strict --- .github/workflows/ci-docker.yml | 27 ++++------ esphome/build_gen/arduino8266.py | 12 ++++- esphome/build_helpers/pch.py | 6 ++- esphome/espidf/toolchain.py | 5 +- esphome/platformio/pch.py.script | 54 ++++++++++++------- .../unit_tests/build_gen/test_arduino8266.py | 3 ++ tests/unit_tests/test_espidf_toolchain.py | 20 +++++++ .../unit_tests/test_platformio_pch_script.py | 32 +++++++++++ 8 files changed, 120 insertions(+), 39 deletions(-) diff --git a/.github/workflows/ci-docker.yml b/.github/workflows/ci-docker.yml index 0ddef1153f..eb3f4f70e7 100644 --- a/.github/workflows/ci-docker.yml +++ b/.github/workflows/ci-docker.yml @@ -202,22 +202,17 @@ jobs: - ln882x-arduino - nrf52 - host - # Fail the job if the precompiled header silently degrades on the - # 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) + # Strict by default so a new matrix id cannot silently join in the + # degrade-quietly mode the knob exists to catch; the knob is inert + # where no pch code runs (esp32-*-platformio, nrf52, host). + # Opt-outs: libretiny GCC rejects its own pch until a toolchain bump. include: - - id: esp8266-arduino - pch_strict: "1" - - id: esp8266-arduino-native - pch_strict: "1" - - id: esp32-idf-esp-idf - pch_strict: "1" - - id: esp32-arduino-esp-idf - pch_strict: "1" - - id: rp2040-arduino - pch_strict: "1" + - id: bk72xx-arduino + pch_strict: "0" + - id: rtl87xx-arduino + pch_strict: "0" + - id: ln882x-arduino + pch_strict: "0" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Download image artifact @@ -229,7 +224,7 @@ jobs: - name: Compile ${{ matrix.id }} run: | docker run --rm \ - -e ESPHOME_PCH_STRICT="${{ matrix.pch_strict || '0' }}" \ + -e ESPHOME_PCH_STRICT="${{ matrix.pch_strict || '1' }}" \ -v "${{ github.workspace }}/docker/test_configs:/config" \ "ghcr.io/esphome/esphome-amd64:${{ needs.check-docker.outputs.tag }}" \ compile "${{ matrix.id }}.yaml" diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index 593d175db7..aabdaaaf4f 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -1288,9 +1288,15 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: if src_other: lines.append(f" flags = {' '.join(src_other)}") # Relative -include: absolute would break cross-device ccache. - # -Wno-error keeps a rejected .gch a warning under user -Werror + # -Wno-error keeps a rejected .gch a warning under user -Werror; + # 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" + ) cxx_parts = src_other + [ - f"-Winvalid-pch -Wno-error=invalid-pch -include {PCH_HEADER_NAME}" + f"-Winvalid-pch {escalation} -include {PCH_HEADER_NAME}" ] lines.append(f"srccxxflags = {' '.join(cxx_parts)}") pch_dep = gch @@ -1305,6 +1311,8 @@ def write_project(paths: InstalledPaths, ccache: str | None) -> bool: " && $python $buildtool touch $out" ) lines.append(" description = PCHPROBE $out") + # Runs when the .gch is (re)built; strict consumer -Werror + # covers a cached .gch this process cannot load lines.append(f"build esphome_pch.probe: pchprobe {gch}") if src_other: lines.append(f" flags = {' '.join(src_other)}") diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index 1bf05bd906..5a57baaecc 100644 --- a/esphome/build_helpers/pch.py +++ b/esphome/build_helpers/pch.py @@ -457,7 +457,11 @@ def prepare_pch( 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 = _run([*cmd[:-6], *pch_probe_args(str(header))], "probe") + # The fixed tail pch_compile_command appends; the slice below + # depends on it + assert cmd[-6:-4] == ["-x", "c++-header"], cmd[-6:] + # fatal: rejection must be a nonzero exit, not a wording match + probe = _run([*cmd[:-6], *pch_probe_args(str(header), fatal=True)], "probe") if probe is None: return if probe.returncode != 0 or ".gch" in probe.stderr: diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 3db8e21757..93cda274f3 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -1,6 +1,5 @@ """ESP-IDF direct build API for ESPHome.""" -from contextlib import suppress from dataclasses import dataclass, field import hashlib import json @@ -537,8 +536,10 @@ def run_compile(config, verbose: bool) -> int: prepare_pch() except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught # Discard so a stale .gch can never be consumed - with suppress(OSError): + try: discard_pch() + except OSError as discard_err: + _LOGGER.warning("Could not discard the stale pch: %s", discard_err) from esphome.build_helpers.pch import pch_strict if pch_strict(): diff --git a/esphome/platformio/pch.py.script b/esphome/platformio/pch.py.script index c875fa1c23..12760615b7 100644 --- a/esphome/platformio/pch.py.script +++ b/esphome/platformio/pch.py.script @@ -26,6 +26,15 @@ except Exception as err: # noqa: BLE001 -- not exported under -t nobuild # Compiler failures that clear on their own must not latch the .failed marker _TRANSIENT_ERRORS = ("No space left", "Cannot allocate", "Resource temporarily") +# Keep in sync with helpers.TRUTHY_ENV_STRINGS +_STRICT = os.environ.get("ESPHOME_PCH_STRICT", "").strip().lower() in ( + "1", + "true", + "yes", + "on", + "enable", +) + _INCLUDE_RE = re.compile(rb'^\s*#\s*include\s+["<]([^">]+)[">]', re.MULTILINE) _CORE_HEADER = "esphome/core/defines.h" @@ -118,6 +127,11 @@ def _compile_gch(cxx, flags, header: Path, gch: Path, proj_dir: Path): raise OSError(f"compiler killed by signal {-result.returncode}") if result.returncode != 0: return result.stderr + return _probe_gch(cxx, flags, header, proj_dir) + + +def _probe_gch(cxx, flags, header: Path, proj_dir: Path): + """Load-check an existing .gch; error string or None.""" # -MF is only legal alongside a dependency flag; pass it solely to # redirect a depfile that -MD/-MMD in the flags would otherwise write dep_redirect = ( @@ -165,8 +179,9 @@ def _setup_pch() -> bool | None: try: from SCons.Script import COMMAND_LINE_TARGETS except ImportError: - # No SCons: cannot tell, stay lenient - return True + # No SCons is an anomaly under PlatformIO: strict must not + # read the unknown state as success + return not _STRICT # 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] @@ -293,12 +308,22 @@ def _setup_pch() -> bool | None: checksum = digest.hexdigest() # The ccache .sum sidecar doubles as the freshness stamp - if ( - not header.is_file() - or not gch.is_file() - or not sum_path.is_file() - or (_read_stamp(sum_path) != checksum) - ): + fresh = ( + header.is_file() + and gch.is_file() + and sum_path.is_file() + and (_read_stamp(sum_path) == checksum) + ) + if fresh and _STRICT: + # Rejection is per-process: strict re-proves a cached .gch loads + # (mirrors the pch_strict() re-probe in build_helpers/pch.py) + error = _probe_gch(cxx, flags, header, proj_dir) + if error is not None: + print(f"ESPHome: {error}") + gch.unlink(missing_ok=True) + sum_path.unlink(missing_ok=True) + return + if not fresh: failed_marker = Path(f"{gch}.failed") if _read_stamp(failed_marker) == checksum: print( @@ -365,21 +390,14 @@ 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: _used = _setup_pch() except Exception: # noqa: BLE001 -- a speedup must never break the build - if _strict: + if _STRICT: raise print("ESPHome: pch internal error; compiling without it") traceback.print_exc() else: - if _strict and not _used: + 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 b2384161dc..9e29156155 100644 --- a/tests/unit_tests/build_gen/test_arduino8266.py +++ b/tests/unit_tests/build_gen/test_arduino8266.py @@ -1875,6 +1875,9 @@ def test_write_project_pch_strict_emits_probe_edge( # bare stamp command and fail every strict build assert "&& $python $buildtool touch $out" in content assert "$stamp" not in content + # Strict consumers escalate: a per-TU rejection must red the build + assert "srccxxflags = -Winvalid-pch -Werror=invalid-pch" in content + assert "-Wno-error=invalid-pch" not in content # With extra src flags the probe edge carries them like the .gch edge CORE.platformio_options["build_src_flags"] = ( diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index 4f0b24d077..da02334736 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -669,6 +669,26 @@ 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, +) -> None: + """A discard failure in the catch-all must be visible, not silent.""" + monkeypatch.setenv("ESPHOME_PCH_ENABLE", "1") + _setup_build(setup_core) + + with ( + patch.object(toolchain, "need_reconfigure", return_value=False), + 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")), + ): + assert toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) == 0 + assert "Could not discard the stale pch" in caplog.text + + def test_run_compile_strict_reraises_pch_failure( setup_core: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/unit_tests/test_platformio_pch_script.py b/tests/unit_tests/test_platformio_pch_script.py index 902dbbf5dc..afd64b32d6 100644 --- a/tests/unit_tests/test_platformio_pch_script.py +++ b/tests/unit_tests/test_platformio_pch_script.py @@ -460,12 +460,44 @@ def test_pch_script_unreadable_local_header_skips_pch( assert "skipping precompiled header" in capsys.readouterr().out +def test_pch_script_strict_reprobes_cached_gch(tmp_path: Path) -> None: + """Rejection is per-process: strict re-proves a cached .gch loads.""" + _run_script(tmp_path) + proj = tmp_path / "dev" + assert (proj / "esphome_pch.h.gch").is_file() + (tmp_path / "fake-gxx.argv").unlink(missing_ok=True) + # Second run: cache fresh, but the toolchain now rejects loads + with pytest.raises(RuntimeError, match="not used"): + _run_script(tmp_path, reject_pch=True, env_vars={"ESPHOME_PCH_STRICT": "1"}) + assert not (proj / "esphome_pch.h.gch").exists() + + def test_pch_script_strict_raises_when_pch_not_used(tmp_path: Path) -> None: """ESPHOME_PCH_STRICT fails the build instead of degrading.""" with pytest.raises(RuntimeError, match="ESPHOME_PCH_STRICT"): _run_script(tmp_path, fail=True, env_vars={"ESPHOME_PCH_STRICT": "1"}) +def test_pch_script_strict_fails_without_scons(tmp_path: Path) -> None: + """No SCons under PlatformIO is an anomaly; strict must not pass.""" + proj = tmp_path / "dev" + (proj / "src").mkdir(parents=True) + + def strict_import(*names: str) -> None: + if "projenv" in names: + raise RuntimeError("Import of non-existent variable 'projenv'") + + env = _FakeSConsEnv(proj, proj / "src", "g++", ["-DX=1"]) + with ( + patch.dict(os.environ, {"ESPHOME_PCH_STRICT": "1"}, clear=True), + pytest.raises(RuntimeError, match="not used"), + ): + exec( # noqa: S102 + compile(_SCRIPT.read_text(), "pch.py", "exec"), + {"Import": strict_import, "env": env}, + ) + + def test_pch_script_strict_reraises_internal_errors(tmp_path: Path) -> None: """The catch-all must not swallow programming errors in strict mode.""" with pytest.raises(TypeError):