From 45cbe0aa4baf2ef0deedf7d0809448170a94e934 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 21 Aug 2026 13:12:57 -0500 Subject: [PATCH] Parse IDF_CCACHE_ENABLE strictly through the shared helper, fix the ccache docstrings parse_enable_env carries the 1/true/yes/on and 0/false/no/off table for both knobs, so IDF_CCACHE_ENABLE=off disables instead of reading as truthy and suppressing the shared opt-out. The ccache module docstring and the ESP-IDF _ccache_env docstring now describe the precedence this PR actually ships, and the probe test duplicated by the helper move is dropped from the PlatformIO toolchain tests. --- esphome/build_helpers/ccache.py | 41 +++++++++++-------- esphome/espidf/framework.py | 25 +++++++---- tests/unit_tests/test_espidf_framework.py | 22 ++++++++++ tests/unit_tests/test_platformio_toolchain.py | 9 ---- 4 files changed, 62 insertions(+), 35 deletions(-) diff --git a/esphome/build_helpers/ccache.py b/esphome/build_helpers/ccache.py index ff3ae3c915..81bafe874e 100644 --- a/esphome/build_helpers/ccache.py +++ b/esphome/build_helpers/ccache.py @@ -3,9 +3,10 @@ ``ccache_defaults_env`` serves the backends that export ``CCACHE_*`` into a build subprocess (native ESP-IDF and Arduino); ``resolve_ccache_path`` carries the probe and enable rules (PlatformIO and the native Arduino -build). The ESP-IDF backend keeps its own ``IDF_CCACHE_ENABLE`` gate and -does not probe; PlatformIO feeds its SCons wrapper script through env -channels instead of ``CCACHE_*`` defaults. +build). The ESP-IDF backend keeps ``IDF_CCACHE_ENABLE`` as a +higher-precedence override and falls back to the shared resolver (probe +included) when it is unset; PlatformIO feeds its SCons wrapper script +through env channels instead of ``CCACHE_*`` defaults. """ from __future__ import annotations @@ -27,6 +28,25 @@ def _ccache_runs(ccache: str) -> bool: ) +def parse_enable_env(name: str) -> 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. + """ + raw = os.environ.get(name) + if raw is None: + return None + lowered = raw.strip().lower() + if lowered in ("1", "true", "yes", "on"): + return True + if lowered in ("0", "false", "no", "off"): + return False + _LOGGER.warning("Ignoring unrecognized %s=%r; use 1 or 0", name, raw) + return None + + def resolve_ccache_path() -> str | None: """The ccache binary to wrap compiles with, or None when disabled. @@ -39,20 +59,7 @@ def resolve_ccache_path() -> str | None: """ import shutil - # Strict parse: bool(str) truthiness would flip "no"/"off" to enabled - # and silently skip the probe - raw = os.environ.get("ESPHOME_CCACHE_ENABLE") - explicit: bool | None = None - if raw is not None: - lowered = raw.strip().lower() - if lowered in ("1", "true", "yes", "on"): - explicit = True - elif lowered in ("0", "false", "no", "off"): - explicit = False - else: - _LOGGER.warning( - "Ignoring unrecognized ESPHOME_CCACHE_ENABLE=%r; use 1 or 0", raw - ) + explicit = parse_enable_env("ESPHOME_CCACHE_ENABLE") if explicit is False: return None ccache = shutil.which("ccache") diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index fed6481166..3977c6560c 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -11,7 +11,11 @@ import re import shutil from typing import Any, NoReturn -from esphome.build_helpers.ccache import ccache_defaults_env, resolve_ccache_path +from esphome.build_helpers.ccache import ( + ccache_defaults_env, + parse_enable_env, + resolve_ccache_path, +) from esphome.build_helpers.tools_cache import tools_cache_path from esphome.core import Version from esphome.framework_helpers import ( @@ -27,7 +31,7 @@ from esphome.framework_helpers import ( run_command_ok, str_to_lst_of_str, ) -from esphome.helpers import get_bool_env, write_file_if_changed +from esphome.helpers import write_file_if_changed _LOGGER = logging.getLogger(__name__) @@ -1133,8 +1137,10 @@ def check_esp_idf_install( def _ccache_env() -> dict[str, str]: """Return ccache settings for ESP-IDF compiles. - Enabled by default whenever the ``ccache`` binary is on PATH; set - ``IDF_CCACHE_ENABLE=0`` in the environment to opt out. The cache lives under + Enabled by default whenever a runnable ``ccache`` binary is on PATH. + ``IDF_CCACHE_ENABLE=0`` opts out and ``=1`` forces it on; when that knob + is unset the shared ``ESPHOME_CCACHE_ENABLE`` applies (same 0/1 forms, + unrecognized values warn and count as unset). The cache lives under the IDF tools path (the machine-global cache dir, or ``ESPHOME_ESP_IDF_PREFIX``), so it is shared across all projects and removed by ``esphome clean-all`` along with the framework. @@ -1154,17 +1160,18 @@ def _ccache_env() -> dict[str, str]: # ESPHOME_CCACHE_ENABLE, which resolve_ccache_path parses; without it a # user disabling ccache to debug a miscompile would silently keep it # enabled here. - if "IDF_CCACHE_ENABLE" in os.environ: - if not get_bool_env("IDF_CCACHE_ENABLE"): - return {} - elif resolve_ccache_path() is None: + idf_knob = parse_enable_env("IDF_CCACHE_ENABLE") + if idf_knob is False: + return {} + if idf_knob is None and resolve_ccache_path() is None: # ESP-IDF silently skips ccache without the binary; don't enable it. return {} # ccache is enabled past here; the shared helper carries the CCACHE_* # policy (and the fail-loud build_path guard). env = ccache_defaults_env(get_idf_tools_path() / "ccache") - if "IDF_CCACHE_ENABLE" not in os.environ: + if idf_knob is None: + # An unparsable IDF_CCACHE_ENABLE must not leak to idf.py as truthy env["IDF_CCACHE_ENABLE"] = "1" return env diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index a33c95b91d..32e2d7a3fb 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -1457,6 +1457,28 @@ def test_ccache_env_honors_shared_esphome_opt_out(tmp_path: Path) -> None: assert _ccache_env() == {} +@pytest.mark.parametrize("value", ["off", "no"]) +def test_ccache_env_idf_knob_parses_strictly(tmp_path: Path, value: str) -> None: + """IDF_CCACHE_ENABLE uses the same strict table as the shared knob, so + "off" disables instead of reading as truthy.""" + p1, p2, p3 = _ccache_patches(tmp_path, "/usr/bin/ccache", tmp_path / "build") + with patch.dict("os.environ", {"IDF_CCACHE_ENABLE": value}, clear=True), p1, p2, p3: + assert _ccache_env() == {} + + +def test_ccache_env_idf_knob_unrecognized_warns_and_defers( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """An unparsable IDF_CCACHE_ENABLE warns, defers to the shared resolver, + and is not forwarded to idf.py as truthy.""" + p1, p2, p3 = _ccache_patches(tmp_path, "/usr/bin/ccache", tmp_path / "build") + env_vars = {"IDF_CCACHE_ENABLE": "enabled"} + with patch.dict("os.environ", env_vars, clear=True), p1, p2, p3: + env = _ccache_env() + assert "unrecognized IDF_CCACHE_ENABLE" in caplog.text + assert env["IDF_CCACHE_ENABLE"] == "1" + + def test_ccache_env_idf_knob_wins_over_shared_opt_out(tmp_path: Path) -> None: """IDF_CCACHE_ENABLE=1 takes precedence over ESPHOME_CCACHE_ENABLE=0.""" p1, p2, p3 = _ccache_patches(tmp_path, None, tmp_path / "build") diff --git a/tests/unit_tests/test_platformio_toolchain.py b/tests/unit_tests/test_platformio_toolchain.py index 4b960b0c58..63c40f3609 100644 --- a/tests/unit_tests/test_platformio_toolchain.py +++ b/tests/unit_tests/test_platformio_toolchain.py @@ -1941,12 +1941,3 @@ def test_run_platformio_cli_invokes_heal( with patch.object(toolchain, "heal_platformio_python_env") as mock_heal: toolchain.run_platformio_cli("test") mock_heal.assert_called_once() - - -def test_ccache_probe_spawns_with_close_fds_false() -> None: - """The probe follows the repo-wide posix_spawn convention.""" - from esphome.build_helpers import ccache - - with patch("subprocess.run") as mock_run: - assert ccache._ccache_runs("/usr/bin/ccache") is True - assert mock_run.call_args.kwargs["close_fds"] is False