From ce15f9a3318c6a4e5e0bf27ec1a3c5806157947f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 18:58:02 -0500 Subject: [PATCH] Verify pre-existing tool archives and escalate surviving torn dirs --- esphome/espidf/framework.py | 16 +++++-- esphome/espidf/install_tool_archives.py | 54 +++++++++-------------- tests/unit_tests/test_espidf_framework.py | 47 +++++++++++++++++--- 3 files changed, 75 insertions(+), 42 deletions(-) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index 8b8b80fb43b..9d5d7f9c1ac 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -306,11 +306,19 @@ def _run_idf_tools_script( str(idf_framework_root), *(args or []), ] + # Explicit paths: the scripts dir (sibling imports must survive + # PYTHONSAFEPATH), the esphome package root, and the framework's idf_tools + pythonpath = os.pathsep.join( + ( + str(_SCRIPTS_DIR), + str(_SCRIPTS_DIR.parents[1]), + str(Path(idf_framework_root) / "tools"), + ) + ) return run_command( cmd, msg=msg, - env=(env or os.environ) - | {"PYTHONPATH": str(Path(idf_framework_root) / "tools")}, + env=(env or os.environ) | {"PYTHONPATH": pythonpath}, stream_output=stream_output, ) @@ -729,9 +737,9 @@ def _prefetch_idf_tool_archives( dist_path = get_idf_tools_path() / "dist" entries = [] seen_dests: set[str] = set() + # Pre-existing archives are not skipped: download_with_resume keeps + # them only on a sha256 match, so the pre-extraction can trust dist/ for entry in json.loads(stdout): - if (dist_path / entry["dest"]).is_file(): - continue # Never download unverified: an entry without sha256/size is # left to the installer, which fails loudly on a bad archive. # Checked before the dedupe so it cannot shadow a verifiable diff --git a/esphome/espidf/install_tool_archives.py b/esphome/espidf/install_tool_archives.py index fdc24540dcf..a1104d5c409 100644 --- a/esphome/espidf/install_tool_archives.py +++ b/esphome/espidf/install_tool_archives.py @@ -1,7 +1,8 @@ """Extract prefetched ESP-IDF tool archives in parallel. Run via ``python -...`` with idf_tools on PYTHONPATH and IDF_TOOLS_PATH set. +...`` with idf_tools and the esphome package root on PYTHONPATH +and IDF_TOOLS_PATH set. Drives idf_tools' own ``IDFTool.install()`` so extraction semantics match the sequential installer, which still runs afterwards as the authority and redoes anything this best-effort pass failed on. Archives are trusted from @@ -11,16 +12,14 @@ the prefetch's sha256 verification, not re-hashed here. # pylint: disable=import-error # idf_tools is on PYTHONPATH at runtime only from concurrent.futures import ThreadPoolExecutor -from contextlib import suppress -import os from pathlib import Path -import shutil -import stat import sys from _tool_resolution import archive_name, init_idf_tools, iter_tool_downloads from idf_tools import ToolBinaryError, g +from esphome.helpers import rmtree + def collect_pending( targets_csv: str, tool_specs: list[str] @@ -46,36 +45,26 @@ def collect_pending( return pending -def _rmtree(path: str) -> None: - """Best-effort removal; clears the read-only bits that block deletion on - Windows (esphome.helpers.rmtree is not importable here).""" - - def _onexc(func, p, exc): # pragma: no cover # Windows read-only files - if os.access(p, os.W_OK): - raise exc - # Preserve the mode: 0o600 would strip a directory's execute bit - # and make the installer's own rmtree fail on the survivor - Path(p).chmod(Path(p).stat().st_mode | stat.S_IWUSR) - func(p) - - with suppress(OSError): - shutil.rmtree(path, onexc=_onexc) - if Path(path).exists(): # pragma: no cover - # A surviving torn dir may pass the installer's binary probe - print(f"could not remove {path}", file=sys.stderr) - - -def install_one(tool: object, name: str, version: str) -> bool: +def install_one(tool: object, name: str, version: str) -> bool | None: + """True on success, False on a cleaned-up failure, None when the torn + dest dir survived and could fool the installer's binary probe.""" try: tool.install(version) # check_binary_valid exits via SystemExit; the installer redoes failures except (Exception, SystemExit) as e: # noqa: BLE001 # pylint: disable=broad-exception-caught - # A torn dest dir must not look installed to the installer - _rmtree(tool.get_path_for_version(version)) print( f"pre-extracting {name}@{version} failed, leaving it to the installer: {e}", file=sys.stderr, ) + # A torn dest dir must not look installed to the installer + dest = tool.get_path_for_version(version) + try: + rmtree(dest) + except FileNotFoundError: # pragma: no cover # failed before mkdir + pass + except OSError as cleanup_err: + print(f"could not remove {dest}: {cleanup_err}", file=sys.stderr) + return None return False return True @@ -99,12 +88,13 @@ def main() -> None: ex.submit(install_one, tool, name, version) for (name, version), tool in pending.items() ] - # Every job failing is a systematic fault; a nonzero exit makes the - # caller log it instead of silently degrading to a sequential install - failed = sum(not future.result() for future in futures) + # A survivor could fool the installer; every job failing is systematic. + # Either way a nonzero exit makes the caller warn + results = [future.result() for future in futures] + failed = sum(result is not True for result in results) if failed: - print(f"{failed} of {len(futures)} pre-extractions failed", file=sys.stderr) - if failed == len(futures): + print(f"{failed} of {len(results)} pre-extractions failed", file=sys.stderr) + if None in results or failed == len(results): sys.exit(1) diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index 267a96294be..6dbb5ebe9f0 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -1026,7 +1026,9 @@ def test_prefetch_downloads_archives_concurrently(tmp_path: Path) -> None: assert download.call_count == 6 -def test_prefetch_skips_already_downloaded_archives(tmp_path: Path) -> None: +def test_prefetch_reverifies_already_downloaded_archives(tmp_path: Path) -> None: + """A pre-existing archive is not skipped: download_with_resume keeps it + only when the sha256 matches, so the pre-extraction can trust it.""" dist = get_idf_tools_path() / "dist" dist.mkdir(parents=True) (dist / "cmake-3.30.2.tar.gz").write_bytes(b"cached") @@ -1040,9 +1042,10 @@ def test_prefetch_skips_already_downloaded_archives(tmp_path: Path) -> None: ): _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) - # only the missing archive is downloaded - assert download.call_count == 1 - assert download.call_args[0][1] == dist / "ninja.zip" + assert sorted(call[0][1] for call in download.call_args_list) == [ + dist / "cmake-3.30.2.tar.gz", + dist / "ninja.zip", + ] @pytest.mark.parametrize( @@ -1163,10 +1166,17 @@ def test_prefetch_passes_targets_and_tools_to_script(tmp_path: Path) -> None: cmd = run.call_args[0][0] assert cmd[-3:] == ["esp32,esp32c3", "required", "cmake"] assert cmd[1].endswith("get_tool_downloads.py") - # the script inherits the caller's env plus the framework tools PYTHONPATH + # the script inherits the caller's env plus an explicit PYTHONPATH: + # sibling scripts, the esphome package root, the framework's idf_tools env = run.call_args[1]["env"] assert env["IDF_TOOLS_PATH"] == "/x" - assert env["PYTHONPATH"] == str(tmp_path / "tools") + assert env["PYTHONPATH"] == os.pathsep.join( + ( + str(_ESPIDF_SCRIPTS_DIR), + str(_ESPIDF_SCRIPTS_DIR.parents[1]), + str(tmp_path / "tools"), + ) + ) def test_framework_install_prefetches_before_installer( @@ -2202,3 +2212,28 @@ def test_install_tool_archives_inprocess_dedupes_and_skips( assert (tools / "ninja" / "1.12.1" / ".installed").is_file() assert not (tools / "installed-tool").exists() assert not (tools / "broken-tool").exists() + + +def test_install_tool_archives_surviving_torn_dir_escalates( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """A torn dir that survives cleanup could fool the installer; the exit + is nonzero even though the other tool succeeded.""" + import esphome.helpers + + _make_dist(tmp_path, "cmake.tar.gz", "ninja-v1.zip") + monkeypatch.setenv("TEST_FAIL_INSTALL", "ninja") + monkeypatch.setattr( + esphome.helpers, "rmtree", MagicMock(side_effect=OSError("busy")) + ) + with pytest.raises(SystemExit) as excinfo: + _run_espidf_script_inprocess( + tmp_path, monkeypatch, "install_tool_archives.py", "esp32", "4", "required" + ) + assert excinfo.value.code == 1 + err = capsys.readouterr().err + assert "could not remove" in err + assert "1 of 2 pre-extractions failed" in err + assert (tmp_path / "tp" / "tools" / "cmake" / "3.30.2" / ".installed").is_file()