Require a resolved ccache, dedupe the package specs, use the shared cache spec

esphome/arduino8266/framework.py

@@ -15,11 +15,11 @@ from __future__ import annotations
 
 import os
 from pathlib import Path
-from typing import Any, NamedTuple
+from typing import NamedTuple
 
-from esphome.build_helpers.ccache import ccache_defaults_env, resolve_ccache_path
+from esphome.build_helpers.ccache import ccache_defaults_env
 from esphome.build_helpers.ninja import find_ninja
-from esphome.build_helpers.tools_cache import tools_cache_path
+from esphome.build_helpers.tools_cache import ARDUINO8266_TOOLS_CACHE, tools_cache_path
 from esphome.core import EsphomeError, Version
 from esphome.framework_helpers import str_to_lst_of_str
 from esphome.platformio.registry import install_package, prefetch_packages
@@ -41,7 +41,7 @@ ESPHOME_ARDUINO8266_TOOLCHAIN_MIRRORS = str_to_lst_of_str(
 def get_arduino8266_tools_path() -> Path:
     # Machine-global so all projects share one install; see
     # espidf.framework.get_idf_tools_path for the location rationale.
-    return tools_cache_path("ESPHOME_ARDUINO8266_PREFIX", "arduino8266")
+    return tools_cache_path(*ARDUINO8266_TOOLS_CACHE)
 
 
 # 3.1.1 rather than 3.1.0: the registry has no package for 3.1.0, and the
@@ -90,8 +90,8 @@ class InstalledPaths(NamedTuple):
 def check_and_install(framework_version: Version) -> InstalledPaths:
     """Ensure framework, toolchain, and ninja are installed; return their paths."""
     if framework_version < MIN_FRAMEWORK_VERSION:
-        # Config validation will enforce this once the native backend is
-        # wired in; keep the module honest when called directly.
+        # Config validation enforces this too; keep the module honest when
+        # called directly.
         raise EsphomeError(
             f"The native toolchain requires the Arduino core "
             f">= {MIN_FRAMEWORK_VERSION}, got {framework_version}"
@@ -102,50 +102,33 @@ def check_and_install(framework_version: Version) -> InstalledPaths:
     framework_path = get_framework_path(package_version)
     downloads_dir = get_arduino8266_tools_path() / "downloads"
     toolchain_path = get_toolchain_path()
+    # One spec per package: the prefetch and the installs must agree
+    specs = (
+        (
+            FRAMEWORK_PACKAGE,
+            package_version,
+            framework_path,
+            ESPHOME_ARDUINO8266_FRAMEWORK_MIRRORS,
+            ("cores/esp8266", "tools/sdk", "libraries"),
+        ),
+        (
+            TOOLCHAIN_PACKAGE,
+            TOOLCHAIN_VERSION,
+            toolchain_path,
+            ESPHOME_ARDUINO8266_TOOLCHAIN_MIRRORS,
+            # xtensa-lx106-elf pins the target: every gcc package has a bin/
+            ("bin", "xtensa-lx106-elf"),
+        ),
+    )
     # Fetch both archives at once; the installs below verify and extract
-    prefetch_packages(
-        [
-            (
-                FRAMEWORK_PACKAGE,
-                package_version,
-                framework_path,
-                ESPHOME_ARDUINO8266_FRAMEWORK_MIRRORS,
-            ),
-            (
-                TOOLCHAIN_PACKAGE,
-                TOOLCHAIN_VERSION,
-                toolchain_path,
-                ESPHOME_ARDUINO8266_TOOLCHAIN_MIRRORS,
-            ),
-        ],
-        downloads_dir,
-    )
-    install_package(
-        FRAMEWORK_PACKAGE,
-        package_version,
-        framework_path,
-        ESPHOME_ARDUINO8266_FRAMEWORK_MIRRORS,
-        downloads_dir,
-        expect=("cores/esp8266", "tools/sdk", "libraries"),
-    )
-    install_package(
-        TOOLCHAIN_PACKAGE,
-        TOOLCHAIN_VERSION,
-        toolchain_path,
-        ESPHOME_ARDUINO8266_TOOLCHAIN_MIRRORS,
-        downloads_dir,
-        # xtensa-lx106-elf pins the target: every gcc package has a bin/
-        expect=("bin", "xtensa-lx106-elf"),
-    )
+    prefetch_packages([spec[:4] for spec in specs], downloads_dir)
+    for name, version, dest, mirrors, expect in specs:
+        install_package(name, version, dest, mirrors, downloads_dir, expect=expect)
     return InstalledPaths(
         framework=framework_path, toolchain=toolchain_path, ninja=ninja_path
     )
 
 
-# Sentinel: "resolve for me"; None is a real value meaning disabled.
-CCACHE_UNRESOLVED: Any = object()
-
-
 def toolchain_tool(toolchain_path: Path, name: str) -> Path:
     """Path to one toolchain tool (gcc, g++, ar, size, addr2line, ...).
 
@@ -156,9 +139,7 @@ def toolchain_tool(toolchain_path: Path, name: str) -> Path:
     return toolchain_path / "bin" / f"xtensa-lx106-elf-{name}{suffix}"
 
 
-def get_build_env(
-    toolchain_path: Path, ccache: str | None = CCACHE_UNRESOLVED
-) -> dict[str, str]:
+def get_build_env(toolchain_path: Path, ccache: str | None) -> dict[str, str]:
     env = os.environ.copy()
     # Drop empty entries: a trailing separator from an absent PATH would
     # make the shell search the current directory for tools
@@ -171,22 +152,13 @@ def get_build_env(
     return env
 
 
-def ccache_path() -> str | None:
-    """The ccache binary to prefix compiles with, or None when disabled.
-
-    Deliberately uncached: env/PATH can change between builds in a
-    long-lived host process.
-    """
-    return resolve_ccache_path()
-
-
-def ccache_env(ccache: str | None = CCACHE_UNRESOLVED) -> dict[str, str]:
+def ccache_env(ccache: str | None) -> dict[str, str]:
     """Return ccache settings for the build subprocess (not os.environ).
 
-    Values the user already set in the environment are respected.
+    ``ccache`` is the pre-resolved binary (resolve_ccache_path), or None
+    when disabled. Values the user already set in the environment are
+    respected.
     """
-    if ccache is CCACHE_UNRESOLVED:
-        ccache = ccache_path()
     if ccache is None:
         return {}
     return ccache_defaults_env(get_arduino8266_tools_path() / "ccache")

tests/unit_tests/test_arduino8266_framework.py

@@ -112,33 +112,15 @@ def test_check_and_install_returns_paths(tmp_path: Path) -> None:
 
 def test_get_build_env_prepends_toolchain_bin(tmp_path: Path) -> None:
     with patch.object(framework, "ccache_env", return_value={"CCACHE_DIR": "x"}):
-        env = framework.get_build_env(tmp_path)
+        env = framework.get_build_env(tmp_path, None)
     assert env["PATH"].startswith(str(tmp_path / "bin") + os.pathsep)
     assert env["CCACHE_DIR"] == "x"
 
 
-def test_ccache_path_delegates_uncached(
-    monkeypatch: pytest.MonkeyPatch,
-) -> None:
-    """Delegates on every call; the env/PATH decision must not freeze for
-    the process lifetime."""
-    monkeypatch.delenv("ESPHOME_CCACHE_ENABLE", raising=False)
-    with patch.object(
-        framework, "resolve_ccache_path", return_value="/usr/bin/ccache"
-    ) as mock_resolve:
-        assert framework.ccache_path() == "/usr/bin/ccache"
-        assert framework.ccache_path() == "/usr/bin/ccache"
-    assert mock_resolve.call_count == 2
-
-
 def test_ccache_env(tmp_path: Path) -> None:
-    with patch.object(framework, "ccache_path", return_value=None):
-        assert framework.ccache_env() == {}
-    with (
-        patch.object(framework, "ccache_path", return_value="/usr/bin/ccache"),
-        patch.dict(os.environ, {"CCACHE_NOHASHDIR": "false"}, clear=True),
-    ):
-        env = framework.ccache_env()
+    assert framework.ccache_env(None) == {}
+    with patch.dict(os.environ, {"CCACHE_NOHASHDIR": "false"}, clear=True):
+        env = framework.ccache_env("/usr/bin/ccache")
     # User-set values are respected; the rest get defaults
     assert "CCACHE_NOHASHDIR" not in env
     assert env["CCACHE_DEPEND"] == "1"
@@ -159,7 +141,7 @@ def test_get_build_env_without_path_has_no_empty_entry(tmp_path: Path) -> None:
         patch.dict(os.environ, {}, clear=True),
         patch.object(framework, "ccache_env", return_value={}),
     ):
-        env = framework.get_build_env(tmp_path)
+        env = framework.get_build_env(tmp_path, None)
     assert env["PATH"] == str(tmp_path / "bin")
     with (
         patch.dict(
@@ -167,20 +149,16 @@ def test_get_build_env_without_path_has_no_empty_entry(tmp_path: Path) -> None:
         ),
         patch.object(framework, "ccache_env", return_value={}),
     ):
-        env = framework.get_build_env(tmp_path)
+        env = framework.get_build_env(tmp_path, None)
     assert env["PATH"].split(os.pathsep) == [str(tmp_path / "bin"), "/usr/bin", "/bin"]
 
 
 def test_ccache_env_accepts_a_preresolved_path() -> None:
-    """A caller that already resolved ccache threads it through; the probe
-    must not run again (None means resolved-and-disabled)."""
-    with (
-        patch.dict(os.environ, {}, clear=True),
-        patch.object(framework, "ccache_path") as mock_resolve,
-    ):
+    """The caller resolves ccache once and threads it through; None means
+    resolved-and-disabled."""
+    with patch.dict(os.environ, {}, clear=True):
         assert framework.ccache_env(None) == {}
         env = framework.ccache_env("/usr/bin/ccache")
-    mock_resolve.assert_not_called()
     assert env["CCACHE_DIR"].endswith("ccache")