Quote empty tokens, honest docstrings, strict registry payloads, required layout checks, honest patch targets

esphome/build_helpers/ccache.py

@@ -1,7 +1,9 @@
 """Shared ccache policy for build backends.
 
-One place for the probe, the enable/override rules, and the ``CCACHE_*``
-defaults, so the backends cannot drift apart.
+One place for the ``CCACHE_*`` defaults (every backend) and for the probe
+and enable rules (backends that call ``resolve_ccache_path``: PlatformIO
+and the native Arduino build). The ESP-IDF backend keeps its own
+``IDF_CCACHE_ENABLE`` gate and does not probe.
 """
 
 from __future__ import annotations

esphome/build_helpers/ninja.py

@@ -64,12 +64,14 @@ def shell_token(tok: str, force: bool = False) -> str:
     Lexing strips the quoting a user wrote (``-DX="a b"`` becomes the single
     token ``-DX=a b``); re-quote on the way out so the compiler receives the
     same argv element SCons would pass under PlatformIO. After ninja
-    un-doubles ``$$``, sh still expands ``$VAR`` while CreateProcess passes
-    it literally -- the same divergence SCons-under-sh has, so this stays
-    PlatformIO parity.
+    un-doubles ``$$``, sh still applies every expansion double quotes allow
+    (``$VAR``, ``$(...)``, backticks) while CreateProcess passes them
+    literally; SCons on POSIX spawns without a shell, so this is a known,
+    deliberate divergence for tokens carrying those characters.
     """
     tok = tok.replace("$", "$$")  # ninja would expand a bare $ to nothing
-    if force or _NEEDS_QUOTE.search(tok):
+    if force or not tok or _NEEDS_QUOTE.search(tok):
+        # An empty token must become "" or it vanishes from the argv
         return quote_arg(tok)
     return tok
 

esphome/platformio/registry.py

@@ -71,13 +71,25 @@ def registry_download(package: str, version: str) -> tuple[str, str, int | None]
             f"The package registry returned invalid JSON for {package}: {err}"
         ) from err
     systype = get_systype()
-    for ver in data.get("versions", []):
+    versions = data.get("versions")
+    if not isinstance(versions, list):
+        # A schema change or an error/captive-portal payload must not be
+        # reported as "version not found"
+        raise EsphomeError(
+            f"Unexpected package registry response for {package}: {str(data)[:200]}"
+        )
+    for ver in versions:
         if ver.get("name") != version:
             continue
         for file in ver.get("files", []):
-            # A bare string would make ``in`` a substring test
-            systems = file.get("system") or "*"
-            if isinstance(systems, str):
+            # Only a MISSING key means "any system"; an explicitly empty
+            # list must not match (a wrong-architecture download would be
+            # cached as a good install). A bare string would make ``in`` a
+            # substring test.
+            systems = file.get("system")
+            if systems is None:
+                systems = ["*"]
+            elif isinstance(systems, str):
                 systems = [systems]
             if "*" in systems or systype in systems:
                 sha256 = (file.get("checksum") or {}).get("sha256")
@@ -101,7 +113,7 @@ def install_package(
     dest: Path,
     mirrors: list[str],
     downloads_dir: Path,
-    expect: Collection[str] = (),
+    expect: Collection[str],
 ) -> None:
     """Download, verify, and extract one package if not already installed.
 

tests/unit_tests/build_helpers/test_ccache.py

@@ -0,0 +1,79 @@
+"""Tests for the shared ccache policy in esphome.build_helpers.ccache."""
+
+from __future__ import annotations
+
+import os
+from pathlib import Path
+from types import SimpleNamespace
+from unittest.mock import patch
+
+import pytest
+
+from esphome.build_helpers import ccache
+
+
+def test_resolve_opt_out() -> None:
+    with patch.dict(os.environ, {"ESPHOME_CCACHE_ENABLE": "0"}):
+        assert ccache.resolve_ccache_path() is None
+
+
+def test_resolve_no_binary(caplog: pytest.LogCaptureFixture) -> None:
+    with (
+        patch.dict(os.environ, {}, clear=True),
+        patch("shutil.which", return_value=None),
+    ):
+        assert ccache.resolve_ccache_path() is None
+    assert "no ccache binary" not in caplog.text
+
+
+def test_resolve_probe_failure() -> None:
+    with (
+        patch.dict(os.environ, {}, clear=True),
+        patch("shutil.which", return_value="/usr/bin/ccache"),
+        patch(
+            "esphome.build_helpers.ccache.subprocess.run", side_effect=OSError("boom")
+        ),
+    ):
+        assert ccache.resolve_ccache_path() is None
+
+
+def test_resolve_explicit_skips_probe_and_warns_missing(
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    with (
+        patch.dict(os.environ, {"ESPHOME_CCACHE_ENABLE": "1"}, clear=True),
+        patch("shutil.which", return_value="/usr/bin/ccache"),
+        patch.object(ccache, "_ccache_runs", side_effect=AssertionError),
+    ):
+        assert ccache.resolve_ccache_path() == "/usr/bin/ccache"
+    with (
+        patch.dict(os.environ, {"ESPHOME_CCACHE_ENABLE": "1"}, clear=True),
+        patch("shutil.which", return_value=None),
+    ):
+        assert ccache.resolve_ccache_path() is None
+    assert "no ccache binary is on PATH" in caplog.text
+
+
+def test_probe_spawns_with_close_fds_false() -> None:
+    with patch("esphome.build_helpers.ccache.subprocess.run") as mock_run:
+        assert ccache._ccache_runs("/usr/bin/ccache") is True
+    assert mock_run.call_args.kwargs["close_fds"] is False
+
+
+def test_defaults_env(tmp_path: Path) -> None:
+    with (
+        patch("esphome.core.CORE", SimpleNamespace(build_path=tmp_path / "b")),
+        patch.dict(os.environ, {"CCACHE_NOHASHDIR": "false"}, clear=True),
+    ):
+        env = ccache.ccache_defaults_env(tmp_path / "cache")
+    assert env["CCACHE_DIR"] == str(tmp_path / "cache")
+    assert env["CCACHE_DEPEND"] == "1"
+    assert "CCACHE_NOHASHDIR" not in env  # user value respected
+
+
+def test_defaults_env_requires_build_path() -> None:
+    with (
+        patch("esphome.core.CORE", SimpleNamespace(build_path=None)),
+        pytest.raises(ValueError, match="build_path"),
+    ):
+        ccache.ccache_defaults_env(Path("/x"))

tests/unit_tests/build_helpers/test_ninja.py

@@ -76,3 +76,8 @@ def test_shell_token_quotes_shell_metacharacters() -> None:
 def test_quote_path_force_quotes() -> None:
     assert ninja_helper.quote_path(Path("a b")) == '"a b"'
     assert ninja_helper.quote_path("simple") == '"simple"'
+
+
+def test_shell_token_empty_token_is_quoted() -> None:
+    """An empty argv element must survive as an explicit pair of quotes."""
+    assert ninja_helper.shell_token("") == '""'

tests/unit_tests/test_platformio_registry.py

@@ -204,7 +204,7 @@ def test_install_package_skips_when_marker_exists(tmp_path: Path) -> None:
     dest.mkdir()
     (dest / ".esphome_extracted").touch()
     with patch.object(registry, "download_from_mirrors") as mock_download:
-        registry.install_package("pkg", "1.0.0", dest, [], tmp_path / "dl")
+        registry.install_package("pkg", "1.0.0", dest, [], tmp_path / "dl", expect=())
     mock_download.assert_not_called()
 
 
@@ -218,7 +218,9 @@ def test_install_package_downloads_via_mirrors(tmp_path: Path) -> None:
     ):
         # Extraction is expected to create the directory
         mock_extract.side_effect = lambda *_a, **_kw: dest.mkdir()
-        registry.install_package("pkg", "1.0.0", dest, mirrors, tmp_path / "dl")
+        registry.install_package(
+            "pkg", "1.0.0", dest, mirrors, tmp_path / "dl", expect=()
+        )
     assert mock_download.call_args[0][0] is mirrors
     assert mock_download.call_args[0][1] == {
         "VERSION": "1.0.0",
@@ -240,7 +242,7 @@ def test_install_package_downloads_via_registry(tmp_path: Path) -> None:
         ),
     ):
         mock_extract.side_effect = lambda *_a, **_kw: dest.mkdir()
-        registry.install_package("pkg", "1.0.0", dest, [], tmp_path / "dl")
+        registry.install_package("pkg", "1.0.0", dest, [], tmp_path / "dl", expect=())
     assert mock_download.call_args[0][0] == "http://x/pkg.tar.gz"
     assert mock_download.call_args[1] == {"sha256": "abc123", "size": 42}
 
@@ -291,7 +293,9 @@ def test_install_package_marker_rechecked_under_lock(tmp_path: Path) -> None:
         patch.object(registry, "download_from_mirrors") as mock_download,
         patch.object(registry, "rmdir") as mock_rmdir,
     ):
-        registry.install_package("pkg", "1.0.0", dest, ["http://m"], tmp_path / "dl")
+        registry.install_package(
+            "pkg", "1.0.0", dest, ["http://m"], tmp_path / "dl", expect=()
+        )
     mock_download.assert_not_called()
     mock_rmdir.assert_not_called()
 
@@ -306,5 +310,39 @@ def test_install_package_uses_hard_lock(tmp_path: Path) -> None:
         patch.object(registry, "get_systype", return_value="linux_x86_64"),
     ):
         mock_extract.side_effect = lambda *_a, **_kw: dest.mkdir(exist_ok=True)
-        registry.install_package("pkg", "1.0.0", dest, ["http://m"], tmp_path / "dl")
+        registry.install_package(
+            "pkg", "1.0.0", dest, ["http://m"], tmp_path / "dl", expect=()
+        )
     assert mock_lock.call_args.kwargs["fallback_to_soft"] is False
+
+
+def test_registry_download_empty_system_list_does_not_match() -> None:
+    """An explicitly empty system list must not act as a wildcard."""
+    with (
+        _registry_response([{"system": [], "download_url": "http://x/any"}]),
+        patch.object(registry, "get_systype", return_value="linux_x86_64"),
+        pytest.raises(EsphomeError, match="No pkg 1.0.0 build"),
+    ):
+        registry.registry_download("pkg", "1.0.0")
+
+
+def test_registry_download_unexpected_payload_is_named() -> None:
+    """An error envelope without a versions list is not 'version not found'."""
+
+    def fake_download(mirrors: list[str], substitutions: dict, target) -> str:
+        target.write(json.dumps({"message": "rate limited"}).encode())
+        return "http://x"
+
+    with (
+        patch.object(registry, "download_from_mirrors", side_effect=fake_download),
+        pytest.raises(EsphomeError, match="Unexpected package registry response"),
+    ):
+        registry.registry_download("pkg", "1.0.0")
+
+
+def test_registry_download_missing_system_key_matches_any() -> None:
+    """A file with no system key at all serves every host."""
+    with _registry_response(
+        [{"download_url": "http://x/any", "checksum": {"sha256": "abc"}, "size": 1}]
+    ):
+        assert registry.registry_download("pkg", "1.0.0") == ("http://x/any", "abc", 1)

tests/unit_tests/test_platformio_toolchain.py

@@ -432,7 +432,7 @@ def test_ccache_env_enabled_by_default(setup_core: Path) -> None:
     with (
         patch.dict(os.environ, {}, clear=True),
         patch("shutil.which", return_value="/usr/bin/ccache"),
-        patch("esphome.framework_helpers.subprocess.run"),
+        patch("esphome.build_helpers.ccache.subprocess.run"),
     ):
         env = toolchain._ccache_env()
 
@@ -495,7 +495,7 @@ def test_ccache_env_disabled_when_probe_fails(
     with (
         patch.dict(os.environ, {}, clear=True),
         patch("shutil.which", return_value="/usr/bin/ccache"),
-        patch("esphome.framework_helpers.subprocess.run", side_effect=probe_error),
+        patch("esphome.build_helpers.ccache.subprocess.run", side_effect=probe_error),
     ):
         env = toolchain._ccache_env()
 
@@ -509,7 +509,7 @@ def test_ccache_env_forced_on_skips_probe(setup_core: Path) -> None:
     with (
         patch.dict(os.environ, {"ESPHOME_CCACHE_ENABLE": "1"}, clear=True),
         patch("shutil.which", return_value="/usr/bin/ccache"),
-        patch("esphome.framework_helpers.subprocess.run") as mock_probe,
+        patch("esphome.build_helpers.ccache.subprocess.run") as mock_probe,
     ):
         env = toolchain._ccache_env()
 
@@ -537,9 +537,9 @@ def test_ccache_env_strips_win_long_path_prefix(setup_core: Path) -> None:
         patch.dict(os.environ, {}, clear=True),
         # shutil.which is patched, so the win32 code path of the real
         # implementation (which crashes on a POSIX host) is never reached.
-        patch("esphome.platformio.toolchain.sys.platform", "win32"),
+        patch("esphome.framework_helpers.sys.platform", "win32"),
         patch("shutil.which", return_value=prefixed),
-        patch("esphome.framework_helpers.subprocess.run") as mock_probe,
+        patch("esphome.build_helpers.ccache.subprocess.run") as mock_probe,
     ):
         env = toolchain._ccache_env()
 
@@ -588,7 +588,7 @@ def test_ccache_env_respects_user_values_and_refreshes_basedir(
     with (
         patch.dict(os.environ, user_env, clear=True),
         patch("shutil.which", return_value="/usr/bin/ccache"),
-        patch("esphome.framework_helpers.subprocess.run"),
+        patch("esphome.build_helpers.ccache.subprocess.run"),
     ):
         env = toolchain._ccache_env()
 
@@ -607,7 +607,7 @@ def test_run_platformio_cli_passes_ccache_env_to_subprocess_only(
     with (
         patch.dict(os.environ, {}, clear=False),
         patch("shutil.which", return_value="/usr/bin/ccache"),
-        patch("esphome.framework_helpers.subprocess.run"),
+        patch("esphome.build_helpers.ccache.subprocess.run"),
     ):
         os.environ.pop("ESPHOME_CCACHE_ENABLE", None)
         mock_run_external_process.return_value = 0
@@ -629,7 +629,7 @@ def test_ccache_env_requires_build_path(setup_core: Path) -> None:
     with (
         patch.dict(os.environ, {}, clear=True),
         patch("shutil.which", return_value="/usr/bin/ccache"),
-        patch("esphome.framework_helpers.subprocess.run"),
+        patch("esphome.build_helpers.ccache.subprocess.run"),
         pytest.raises(ValueError, match="CORE.build_path must be set"),
     ):
         toolchain._ccache_env()
@@ -643,7 +643,7 @@ def test_run_platformio_cli_merges_caller_env(
 
     with (
         patch("shutil.which", return_value="/usr/bin/ccache"),
-        patch("esphome.framework_helpers.subprocess.run"),
+        patch("esphome.build_helpers.ccache.subprocess.run"),
     ):
         mock_run_external_process.return_value = 0
         toolchain.run_platformio_cli(
@@ -871,7 +871,7 @@ def test_strip_win_long_path_prefix(
     platform: str, input_path: str, expected: str
 ) -> None:
     r"""``\\?\`` and ``\\?\UNC\`` prefixes are stripped only on win32."""
-    with patch("esphome.platformio.toolchain.sys.platform", platform):
+    with patch("esphome.framework_helpers.sys.platform", platform):
         assert toolchain.strip_win_long_path_prefix(input_path) == expected
 
 
@@ -898,7 +898,7 @@ def test_run_platformio_cli_strips_win_long_path_prefix(
         # so the stdlib sees it too) would send shutil.which down the Windows
         # code path, which crashes on a POSIX host.
         patch.dict(os.environ, {"ESPHOME_CCACHE_ENABLE": "0"}, clear=False),
-        patch("esphome.platformio.toolchain.sys.platform", "win32"),
+        patch("esphome.framework_helpers.sys.platform", "win32"),
         patch("esphome.platformio.toolchain.sys.executable", prefixed_exe),
     ):
         # Pop any pre-existing PYTHONEXEPATH so the assertion below reflects
@@ -930,7 +930,7 @@ def test_run_platformio_cli_does_not_set_pythonexepath_without_strip(
 
     with (
         patch.dict(os.environ, {}, clear=False),
-        patch("esphome.platformio.toolchain.sys.platform", "linux"),
+        patch("esphome.framework_helpers.sys.platform", "linux"),
         patch("esphome.platformio.toolchain.sys.executable", plain_exe),
     ):
         os.environ.pop("PYTHONEXEPATH", None)