diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index c967798a4e..ae9f6a28bd 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -1303,7 +1303,9 @@ 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)) + probe = " ".join( + pch_probe_args(PCH_HEADER_NAME, source=str(Path(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 dc9f3830ed..f0d7b7c885 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -298,7 +298,9 @@ def _pch_cmake() -> str: """ if not pch_enabled(): return "" - # Strict inverts: a per-process consumer rejection reds the build + # 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" return f""" # ESPHome precompiled header (see esphome/build_helpers/pch.py). diff --git a/esphome/build_helpers/pch.py b/esphome/build_helpers/pch.py index e1814690df..078ac51d9c 100644 --- a/esphome/build_helpers/pch.py +++ b/esphome/build_helpers/pch.py @@ -103,8 +103,20 @@ def pch_enabled() -> bool: def pch_strict() -> bool: - """CI knob: ``ESPHOME_PCH_STRICT=1`` turns pch degrade paths fatal.""" - return parse_enable_env("ESPHOME_PCH_STRICT") is True + """CI knob: ``ESPHOME_PCH_STRICT=1`` turns pch degrade paths fatal. + + 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 def pch_degraded(reason: str) -> None: @@ -120,11 +132,12 @@ def pch_disabled_degraded() -> None: pch_degraded("pch disabled by ESPHOME_PCH_ENABLE") -def pch_probe_args(header: str) -> list[str]: +def pch_probe_args(header: str, source: str = "-") -> list[str]: """Flags that load-check a built .gch via a syntax-only compile. Rejection must be a nonzero exit (never just a wording match), so the - invalid-pch class is always escalated. + invalid-pch class is always escalated. ``source`` defaults to stdin + (host independent); the ninja probe edge passes a real file. """ return [ "-Winvalid-pch", @@ -134,7 +147,7 @@ def pch_probe_args(header: str) -> list[str]: "-fsyntax-only", "-x", "c++", - os.devnull, + source, ] @@ -403,7 +416,9 @@ def prepare_pch( return failed_marker = Path(f"{gch}.failed") - def _run(run_cmd: list[str], what: str) -> subprocess.CompletedProcess | None: + def _run( + run_cmd: list[str], what: str, stdin: str | None = None + ) -> 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.""" @@ -413,6 +428,7 @@ def prepare_pch( cwd=cmd_dir, # C locale keeps diagnostics matchable by _TRANSIENT_ERRORS env={**os.environ, "LC_ALL": "C"}, + input=stdin, capture_output=True, text=True, check=False, @@ -458,15 +474,27 @@ def prepare_pch( from cmd, so no -MF is needed; cmd ends with the fixed "-x c++-header -c -o" tail. A cached-header rejection may not reproduce (per-process), so that caller passes latch=False.""" - # The fixed tail pch_compile_command appends; the slice below - # depends on it - assert cmd[-6:-4] == ["-x", "c++-header"], cmd[-6:] - probe = _run([*cmd[:-6], *pch_probe_args(str(header))], "probe") + if cmd[-6:-4] != ["-x", "c++-header"]: + # The slice below depends on pch_compile_command's fixed tail + _LOGGER.warning("Unexpected pch command shape: %s", cmd[-6:]) + discard_pch(build_dir) + pch_degraded("unexpected pch command shape") + return + base = cmd[:-6] + probe = _run([*base, *pch_probe_args(str(header))], "probe", stdin="") if probe is None: return - if probe.returncode != 0 or ".gch" in probe.stderr: + if probe.returncode != 0: error = probe.stderr.strip() or f"exit code {probe.returncode}" - if latch: + # 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="" + ) + if baseline is None: + return + if latch and baseline.returncode == 0: _fail(error, "toolchain cannot load the pch") else: _LOGGER.warning( diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 93cda274f3..69258b9163 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -539,6 +539,14 @@ def run_compile(config, verbose: bool) -> int: 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 diff --git a/esphome/platformio/pch.py.script b/esphome/platformio/pch.py.script index d9eb389ae9..ee56a33bc1 100644 --- a/esphome/platformio/pch.py.script +++ b/esphome/platformio/pch.py.script @@ -26,14 +26,17 @@ 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", -) +# Keep in sync with helpers.TRUTHY_ENV_STRINGS / FALSY_ENV_STRINGS +_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") +): + # 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") _INCLUDE_RE = re.compile(rb'^\s*#\s*include\s+["<]([^">]+)[">]', re.MULTILINE) _CORE_HEADER = "esphome/core/defines.h" @@ -130,28 +133,9 @@ def _compile_gch(cxx, flags, header: Path, gch: Path, proj_dir: Path): 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 = ( - ["-MF", os.devnull] - if any(f in ("-MD", "-MMD", "-M", "-MM") for f in flags) - else [] - ) +def _probe_run(cxx, flags, extra, proj_dir: Path): probe = subprocess.run( # noqa: PLW1510 - [ - cxx, - *flags, - *dep_redirect, - "-Winvalid-pch", - "-include", - str(header), - "-fsyntax-only", - "-x", - "c++", - "-", - ], + [cxx, *flags, *extra, "-fsyntax-only", "-x", "c++", "-"], cwd=proj_dir, env={**os.environ, "LC_ALL": "C"}, input="", @@ -160,9 +144,32 @@ def _probe_gch(cxx, flags, header: Path, proj_dir: Path): ) if probe.returncode < 0: raise OSError(f"probe killed by signal {-probe.returncode}") - if probe.returncode != 0 or ".gch" in probe.stderr: - return f"toolchain cannot load the pch: {probe.stderr.strip()}" - return None + return probe + + +def _probe_gch(cxx, flags, header: Path, proj_dir: Path): + """Load-check an existing .gch; error string or None. Rejection must + be a nonzero exit (keep in sync with pch_probe_args); a baseline run + without the pch keeps environmental failures from being blamed on it.""" + # -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 = ( + ["-MF", os.devnull] + if any(f in ("-MD", "-MMD", "-M", "-MM") for f in flags) + else [] + ) + probe = _probe_run( + cxx, + flags, + [*dep_redirect, "-Winvalid-pch", "-Werror=invalid-pch", "-include", str(header)], + proj_dir, + ) + if probe.returncode == 0: + return None + baseline = _probe_run(cxx, flags, dep_redirect, proj_dir) + if baseline.returncode != 0: + raise OSError(f"probe cannot run at all: {baseline.stderr.strip()[:200]}") + return f"toolchain cannot load the pch: {probe.stderr.strip()}" def _read_stamp(path: Path) -> str: diff --git a/tests/unit_tests/build_gen/test_espidf.py b/tests/unit_tests/build_gen/test_espidf.py index e06ed6c428..25011a0aca 100644 --- a/tests/unit_tests/build_gen/test_espidf.py +++ b/tests/unit_tests/build_gen/test_espidf.py @@ -1240,8 +1240,11 @@ def test_prepare_pch_probe_rejection_latches_and_degrades( def rejecting(cmd, **kwargs): if "-fsyntax-only" in cmd: + if "-include" not in cmd: + # Baseline without the pch passes: the pch is to blame + return subprocess.CompletedProcess(cmd, 0, "", "") return subprocess.CompletedProcess( - cmd, 0, "", "warning: esphome_pch.h.gch: had text segment " + cmd, 1, "", "error: esphome_pch.h.gch: had text segment " ) gch.write_bytes(b"gch") return subprocess.CompletedProcess(cmd, 0, "", "") @@ -1289,8 +1292,10 @@ def test_prepare_pch_strict_reprobes_cached_gch( def reject(cmd, **kwargs): assert "-fsyntax-only" in cmd, "cached path must not recompile" + if "-include" not in cmd: + return subprocess.CompletedProcess(cmd, 0, "", "") return subprocess.CompletedProcess( - cmd, 0, "", "warning: esphome_pch.h.gch: had text segment " + cmd, 1, "", "error: esphome_pch.h.gch: had text segment " ) monkeypatch.setenv("ESPHOME_PCH_STRICT", "1") @@ -1304,3 +1309,78 @@ def test_prepare_pch_strict_reprobes_cached_gch( # Per-process rejection may not reproduce: the cached path must not # latch the pch off for later non-strict builds assert not (dev / "build" / "esphome_pch.h.gch.failed").exists() + + +def test_prepare_pch_unexpected_command_shape_degrades( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A tail the probe slice cannot trust discards and degrades.""" + from esphome.build_gen.espidf import prepare_pch + from esphome.core import EsphomeError + + monkeypatch.setenv("ESPHOME_PCH_STRICT", "1") + dev = _make_pch_device(tmp_path, "dev_sh") + CORE.build_path = dev + gch = dev / "build" / "esphome_pch.h.gch" + + def ok(cmd, **kwargs): + 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=ok), + patch( + "esphome.build_helpers.pch.pch_compile_command", + return_value=( + ["g++", "-DX=1", "-c", "x", "-o", "y", "extra"], + dev / "build", + ), + ), + pytest.raises(EsphomeError, match="command shape"), + ): + prepare_pch() + assert not gch.exists() + + # Non-strict: same shape problem degrades without raising + monkeypatch.delenv("ESPHOME_PCH_STRICT") + with ( + patch.object(CORE, "name", "test"), + patch("esphome.build_helpers.pch.subprocess.run", side_effect=ok), + patch( + "esphome.build_helpers.pch.pch_compile_command", + return_value=( + ["g++", "-DX=1", "-c", "x", "-o", "y", "extra"], + dev / "build", + ), + ), + ): + prepare_pch() + assert not gch.exists() + + +def test_prepare_pch_probe_baseline_spawn_failure_is_transient( + tmp_path: Path, +) -> None: + """A baseline that cannot spawn is environmental: no marker.""" + from esphome.build_gen.espidf import prepare_pch + + dev = _make_pch_device(tmp_path, "dev_bs") + CORE.build_path = dev + gch = dev / "build" / "esphome_pch.h.gch" + + def flaky(cmd, **kwargs): + if "-fsyntax-only" in cmd: + if "-include" in cmd: + return subprocess.CompletedProcess(cmd, 1, "", "boom") + raise OSError("baseline spawn failed") + 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=flaky), + ): + prepare_pch() + assert not gch.exists() + assert not (dev / "build" / "esphome_pch.h.gch.failed").exists() diff --git a/tests/unit_tests/build_helpers/test_pch.py b/tests/unit_tests/build_helpers/test_pch.py index 3b5650fae8..0a66f6a629 100644 --- a/tests/unit_tests/build_helpers/test_pch.py +++ b/tests/unit_tests/build_helpers/test_pch.py @@ -253,3 +253,14 @@ def test_pch_extra_scripts_strict_raises_when_disabled( monkeypatch.setenv("ESPHOME_PCH_STRICT", "1") with pytest.raises(EsphomeError, match="disabled"): pch.pch_extra_scripts() + + +def test_pch_strict_rejects_unrecognized_values( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A typo must not silently disable the gate.""" + from esphome.core import EsphomeError + + monkeypatch.setenv("ESPHOME_PCH_STRICT", "yolo") + with pytest.raises(EsphomeError, match="Unrecognized ESPHOME_PCH_STRICT"): + pch.pch_strict() diff --git a/tests/unit_tests/test_platformio_pch_script.py b/tests/unit_tests/test_platformio_pch_script.py index 4287ddd511..84b8a10ee9 100644 --- a/tests/unit_tests/test_platformio_pch_script.py +++ b/tests/unit_tests/test_platformio_pch_script.py @@ -100,12 +100,14 @@ def _fake_cxx( body += f"echo {fail_msg or 'boom'} >&2\nexit 1\n" else: # Only the c++-header compile has a -o; the load probe has none - body += 'out=""; prev=""; mf=0; dep=0\nfor a in "$@"; do [ "$prev" = "-o" ] && out="$a"; prev="$a"; [ "$a" = "-MF" ] && mf=1; case "$a" in -M|-MM|-MD|-MMD) dep=1;; esac; done\n' + body += 'out=""; prev=""; mf=0; dep=0; inc=0\nfor a in "$@"; do [ "$prev" = "-o" ] && out="$a"; prev="$a"; [ "$a" = "-MF" ] && mf=1; [ "$a" = "-include" ] && inc=1; case "$a" in -M|-MM|-MD|-MMD) dep=1;; esac; done\n' # Real cc1plus rejects -MF without a dependency flag body += 'if [ "$mf" = 1 ] && [ "$dep" = 0 ]; then echo "cc1plus: error: to generate dependencies you must specify either \x27-M\x27 or \x27-MM\x27" >&2; exit 1; fi\n' body += '[ -n "$out" ] && echo gch > "$out"\n' if reject_pch: - body += 'case " $* " in *c++-header*) ;; *) echo "warning: esphome_pch.h.gch: had text segment at different address" >&2;; esac\n' + # -Werror=invalid-pch makes rejection a nonzero exit; the + # baseline (no -include) still passes + body += 'case " $* " in *c++-header*) ;; *) if [ "$inc" = 1 ]; then echo "error: esphome_pch.h.gch: had text segment at different address" >&2; exit 1; fi;; esac\n' body += f'case " $* " in *c++-header*) exit 0;; *) exit {probe_exit};; esac\n' cxx.write_text("#!/bin/sh\n" + body) cxx.chmod(cxx.stat().st_mode | stat.S_IEXEC) @@ -293,12 +295,14 @@ def test_pch_script_spawn_failure_is_transient( assert "did not run" in capsys.readouterr().out -def test_pch_script_probe_nonzero_exit_falls_back(tmp_path: Path) -> None: - """A probe failure whose stderr never mentions .gch must still count.""" +def test_pch_script_probe_environment_failure_does_not_latch( + tmp_path: Path, +) -> None: + """Probe AND baseline failing is environmental: no marker, retry.""" scons_env = _run_script(tmp_path, probe_exit=1) proj = tmp_path / "dev" assert not (proj / "esphome_pch.h.gch").exists() - assert (proj / "esphome_pch.h.gch.failed").is_file() + assert not (proj / "esphome_pch.h.gch.failed").exists() assert scons_env.prepended == []