From d0f38d7644ee0f1fca35c9541813d205f07cf429 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 19 Aug 2026 12:03:39 -0500 Subject: [PATCH 1/5] [espidf] Download tool archives in parallel during the install prefetch --- esphome/espidf/framework.py | 37 ++++++++--- esphome/framework_helpers.py | 75 +++++++++++++++++++--- tests/unit_tests/test_espidf_framework.py | 74 +++++++++++++++++++-- tests/unit_tests/test_framework_helpers.py | 72 +++++++++++++++++++++ 4 files changed, 235 insertions(+), 23 deletions(-) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index 0f6ef873b8..85e9c88171 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -1,6 +1,7 @@ """ESP-IDF framework tools for ESPHome.""" from collections.abc import Callable +from concurrent.futures import ThreadPoolExecutor from ctypes.util import find_library import json import logging @@ -15,6 +16,7 @@ import platformdirs from esphome.core import CORE, Version from esphome.framework_helpers import ( + BatchDownloadProgress, PathType, archive_extract_all, create_venv, @@ -690,6 +692,12 @@ def _patch_tools_json_demote_unused_tools(framework_path: Path) -> None: ) +# Tool archives are large (tens to hundreds of MB) and served by GitHub / +# dl.espressif.com; a few streams at once saturate most links without +# hammering the host. Smaller than external_files' 8: those are tiny files. +_PREFETCH_WORKERS = 4 + + def _prefetch_idf_tool_archives( framework_path: Path, targets_str: str, @@ -702,10 +710,10 @@ def _prefetch_idf_tool_archives( which makes large archives effectively impossible to fetch on unstable connections (#17703). This asks the framework's idf_tools (via ``get_tool_downloads.py``) which archives the coming install needs, then - downloads each into ``/dist`` with - ``download_with_resume``. The installer then finds the verified archives - already in place ("file ... is already downloaded") and never touches the - network. + downloads them into ``/dist`` with + ``download_with_resume``, a few at a time under one combined progress + bar. The installer then finds the verified archives already in place + ("file ... is already downloaded") and never touches the network. Strictly best-effort: any failure here just logs and returns, leaving ``idf_tools.py install`` to download whatever is missing exactly as @@ -732,21 +740,34 @@ def _prefetch_idf_tool_archives( for entry in json.loads(stdout) if not (dist_path / entry["dest"]).is_file() ] - for index, entry in enumerate(entries, start=1): - _LOGGER.info( - "Downloading %s (%d/%d) ...", entry["name"], index, len(entries) - ) + if not entries: + return + _LOGGER.info( + "Downloading %d ESP-IDF tool archive(s): %s", + len(entries), + ", ".join(entry["name"] for entry in entries), + ) + progress = BatchDownloadProgress( + "Downloading ESP-IDF tools", + sum(entry["size"] or 0 for entry in entries), + ) + + def _download(entry: dict) -> None: try: download_with_resume( entry["url"], dist_path / entry["dest"], sha256=entry["sha256"], size=entry["size"], + progress=progress.tracker(), ) except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught # Keep prefetching the remaining archives; the installer # will retry this one itself (without resume). _LOGGER.warning("Could not prefetch %s: %s", entry["name"], e) + + with ThreadPoolExecutor(max_workers=min(_PREFETCH_WORKERS, len(entries))) as ex: + list(ex.map(_download, entries)) except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught # The installer downloads anything missing itself; never let the # prefetch become a new way for the install to fail. diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index b8a43220ff..76ae6b801f 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -1,6 +1,6 @@ """Generic toolchain installation helpers shared across framework implementations.""" -from collections.abc import Iterable +from collections.abc import Callable, Iterable from contextlib import ExitStack import hashlib import io @@ -10,6 +10,7 @@ import os from pathlib import Path import subprocess import sys +import threading import time from typing import IO, TYPE_CHECKING @@ -697,7 +698,11 @@ def _response_validator(resp: "requests.Response") -> str | None: def _stream_response_to_file( - resp: "requests.Response", f: IO[bytes], offset: int, size: int | None = None + resp: "requests.Response", + f: IO[bytes], + offset: int, + size: int | None = None, + progress: Callable[[int], None] | None = None, ) -> None: """Stream an open ``_open_ranged`` response body into ``f`` at ``offset``. @@ -705,21 +710,60 @@ def _stream_response_to_file( (effective offset 0) discards the stale bytes. ``offset`` also seeds the progress bar so a resumed download shows overall progress. ``size`` is the known full file size; when None it is derived from the response's - content-length, and without either there is no progress bar. + content-length, and without either there is no progress bar. With + ``progress`` set, no bar is drawn here; the callback gets the absolute + byte count, seeded with ``offset`` and then after each chunk. """ f.seek(offset) f.truncate(offset) total_size = size or offset + _content_length(resp) downloaded = offset - progress = ProgressBar("Downloading") if total_size > 0 else None + own_bar: ProgressBar | None = None + if progress is None: + own_bar = ProgressBar("Downloading") if total_size > 0 else None + progress = ( + (lambda done: own_bar.update(done / total_size)) + if own_bar + else (lambda _: None) + ) + progress(downloaded) for chunk in resp.iter_content(chunk_size=256 * 1024): if chunk: f.write(chunk) downloaded += len(chunk) - if progress is not None: - progress.update(downloaded / total_size) - if progress is not None: - progress.update(1) + progress(downloaded) + if own_bar is not None: + own_bar.update(1) + + +class BatchDownloadProgress: + """One progress bar across several concurrent ``download_with_resume`` calls. + + Each ``tracker()`` is a ``progress`` callback for one download; it reports + that file's absolute byte count and the bar shows the sum over ``total``. + The lock also serialises the bar's stderr writes, so worker threads never + interleave frames. With an unknown ``total`` (0) nothing is drawn. + """ + + def __init__(self, header: str, total: int) -> None: + self._bar = ProgressBar(header) if total > 0 else None + self._total = total + self._sum = 0 + self._lock = threading.Lock() + + def tracker(self) -> Callable[[int], None]: + last = 0 + + def update(done: int) -> None: + nonlocal last + if self._bar is None: + return + with self._lock: + self._sum += done - last + last = done + self._bar.update(min(self._sum / self._total, 1)) + + return update def download_with_resume( @@ -732,6 +776,7 @@ def download_with_resume( attempts: int = 5, timeout: int = 30, retry_connect_errors: bool = True, + progress: Callable[[int], None] | None = None, ) -> None: """Download ``url`` to ``dest``, resuming partial downloads. @@ -754,6 +799,12 @@ def download_with_resume( of consuming attempts — for callers with their own fallback, like ``download_from_mirrors``. + ``progress``, when given, replaces the built-in progress bar: it is called + with the absolute number of bytes of ``dest`` obtained so far (including + a resumed prefix, and the final size once the file is verified), so a + caller running several downloads at once can draw one combined bar (see + ``BatchDownloadProgress``). + Raises EsphomeError when all attempts are exhausted. """ # Imported lazily: requests is a heavy import (~85ms) and is only needed @@ -777,6 +828,8 @@ def download_with_resume( if dest.is_file() and (sha256 is not None or size is not None): try: _verify_file(dest, sha256, size) + if progress is not None: + progress(dest.stat().st_size) return except EsphomeError: dest.unlink() @@ -822,7 +875,7 @@ def download_with_resume( # Recorded so a later run can prove an If-Range # resume of this part file safe. _write_download_meta(meta, url, validator, expected_total) - _stream_response_to_file(resp, f, offset, size) + _stream_response_to_file(resp, f, offset, size, progress) # else: a previous run already wrote every byte (or more) but # was killed before the rename below. Skip the network entirely # — a Range request past EOF would draw HTTP 416 — and let @@ -831,6 +884,10 @@ def download_with_resume( expected_size = size if size is not None else expected_total _verify_file(part, sha256, expected_size or None) + if progress is not None: + # Also credits a part file an earlier run completed without + # streaming anything this time. + progress(part.stat().st_size) if not expected_size and sha256 is None: # No sha, no size, and the server sent no usable # content-length: nothing can prove the download complete diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index d8e7738569..af14b35d0d 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -2,6 +2,7 @@ # pylint: disable=protected-access +from concurrent.futures import ThreadPoolExecutor from contextlib import contextmanager import importlib.util import io @@ -899,12 +900,68 @@ def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) dist = get_idf_tools_path() / "dist" - assert download.call_count == 2 - assert download.call_args_list[0][0] == ( - "https://example.com/cmake.tar.gz", - dist / "cmake-3.30.2.tar.gz", + # Archives download concurrently, so the call order is not fixed. + calls = {call[0]: call[1] for call in download.call_args_list} + assert set(calls) == { + ("https://example.com/cmake.tar.gz", dist / "cmake-3.30.2.tar.gz"), + ("https://example.com/ninja.zip", dist / "ninja.zip"), + } + kwargs = calls[("https://example.com/cmake.tar.gz", dist / "cmake-3.30.2.tar.gz")] + assert kwargs["sha256"] == "ab" * 32 + assert kwargs["size"] == 123 + # every archive reports into the one combined progress bar + assert all( + kw["progress"].__qualname__.startswith("BatchDownloadProgress.tracker") + for kw in calls.values() ) - assert download.call_args_list[0][1] == {"sha256": "ab" * 32, "size": 123} + + +def test_prefetch_downloads_archives_concurrently(tmp_path: Path) -> None: + """More than one archive fans out over a bounded thread pool.""" + entries = [ + { + "name": f"tool{i}@1", + "url": f"https://example.com/tool{i}.tar.gz", + "size": 10, + "sha256": "ab" * 32, + "dest": f"tool{i}.tar.gz", + } + for i in range(6) + ] + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, json.dumps(entries), ""), + ), + patch("esphome.espidf.framework.download_with_resume") as download, + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch( + "esphome.espidf.framework.ThreadPoolExecutor", wraps=ThreadPoolExecutor + ) as pool, + ): + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + + pool.assert_called_once_with(max_workers=4) + assert download.call_count == 6 + + +def test_prefetch_single_archive_uses_one_worker(tmp_path: Path) -> None: + entries = json.loads(_PREFETCH_JSON)[:1] + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, json.dumps(entries), ""), + ), + patch("esphome.espidf.framework.download_with_resume") as download, + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch( + "esphome.espidf.framework.ThreadPoolExecutor", wraps=ThreadPoolExecutor + ) as pool, + ): + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + + pool.assert_called_once_with(max_workers=1) + assert download.call_count == 1 def test_prefetch_skips_already_downloaded_archives(tmp_path: Path) -> None: @@ -964,6 +1021,11 @@ def test_prefetch_one_failed_archive_does_not_stop_the_rest( ) -> None: """A single archive failing its download must not abort the prefetch of the remaining archives.""" + + def _fail_cmake_download(url: str, *args, **kwargs) -> None: + if "cmake" in url: + raise OSError("network down") + with ( patch( "esphome.espidf.framework.run_command", @@ -971,7 +1033,7 @@ def test_prefetch_one_failed_archive_does_not_stop_the_rest( ), patch( "esphome.espidf.framework.download_with_resume", - side_effect=[OSError("network down"), None], + side_effect=_fail_cmake_download, ) as download, patch("esphome.espidf.framework.get_system_python_path", return_value="python"), ): diff --git a/tests/unit_tests/test_framework_helpers.py b/tests/unit_tests/test_framework_helpers.py index 2022c15bfe..ffcb79155d 100644 --- a/tests/unit_tests/test_framework_helpers.py +++ b/tests/unit_tests/test_framework_helpers.py @@ -21,6 +21,7 @@ import requests as req from esphome import framework_helpers from esphome.core import EsphomeError from esphome.framework_helpers import ( + BatchDownloadProgress, _7z_extract_all, _detect_archive_root, _is_transient_download_error, @@ -1112,6 +1113,77 @@ class TestDownloadWithResume: assert mock_get.call_args[1]["headers"] == {} assert dest.read_bytes() == b"data" + def test_progress_callback_reports_absolute_bytes(self, tmp_path: Path) -> None: + """With a callback no bar is drawn; the callback sees the running + byte count of this file, then its final verified size.""" + dest = tmp_path / "tool.tar.gz" + resp = _mock_response(b"") + resp.headers = {"content-length": "7"} + resp.iter_content.return_value = [b"1234", b"567"] + seen: list[int] = [] + with ( + patch("requests.get", return_value=resp), + patch("esphome.framework_helpers.ProgressBar") as bar, + ): + download_with_resume( + "https://example.com/t", dest, size=7, progress=seen.append + ) + assert seen == [0, 4, 7, 7] + bar.assert_not_called() + + def test_progress_callback_seeds_with_resume_offset(self, tmp_path: Path) -> None: + dest = tmp_path / "tool.tar.gz" + (tmp_path / "tool.tar.gz.part").write_bytes(b"12345") + good = hashlib.sha256(b"12345678").hexdigest() + seen: list[int] = [] + with patch("requests.get", return_value=_resumed_response(b"678")): + download_with_resume( + "https://example.com/t", dest, sha256=good, size=8, progress=seen.append + ) + assert seen[0] == 5 + assert seen[-1] == 8 + + def test_progress_callback_credits_already_complete_download( + self, tmp_path: Path + ) -> None: + """A verified dest from an earlier run still counts toward the batch.""" + dest = tmp_path / "tool.tar.gz" + dest.write_bytes(b"12345678") + seen: list[int] = [] + with patch("requests.get") as mock_get: + download_with_resume( + "https://example.com/t", dest, size=8, progress=seen.append + ) + mock_get.assert_not_called() + assert seen == [8] + + +class TestBatchDownloadProgress: + def test_sums_trackers_into_one_bar(self) -> None: + with patch("esphome.framework_helpers.ProgressBar") as bar_cls: + progress = BatchDownloadProgress("Downloading", 100) + a = progress.tracker() + b = progress.tracker() + a(10) + b(20) + a(30) + a(0) # a restart from zero takes that file's bytes back out + bar_cls.assert_called_once_with("Downloading") + updates = [c[0][0] for c in bar_cls.return_value.update.call_args_list] + assert updates == [0.1, 0.3, 0.5, 0.2] + + def test_clamps_at_one(self) -> None: + """Sizes are advisory; an over-delivering server never pushes past 100%.""" + with patch("esphome.framework_helpers.ProgressBar") as bar_cls: + progress = BatchDownloadProgress("Downloading", 10) + progress.tracker()(25) + assert bar_cls.return_value.update.call_args[0][0] == 1 + + def test_unknown_total_draws_nothing(self) -> None: + with patch("esphome.framework_helpers.ProgressBar") as bar_cls: + BatchDownloadProgress("Downloading", 0).tracker()(5) + bar_cls.assert_not_called() + class TestDownloadFromMirrors: def test_success_returns_url_and_writes_content(self, tmp_path: Path) -> None: From fe53ff6b6b4051121a73278c1b2b9c7456d62347 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 19 Aug 2026 13:28:42 -0500 Subject: [PATCH 2/5] Finish the batch bar, cancel queued archives on Ctrl-C, report known sizes --- esphome/espidf/framework.py | 21 +++++++++++++----- esphome/framework_helpers.py | 12 ++++++++--- tests/unit_tests/test_espidf_framework.py | 25 +++++++++++++++++++++- tests/unit_tests/test_framework_helpers.py | 24 ++++++++++++++++++++- 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index 85e9c88171..95d168c85c 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -747,27 +747,38 @@ def _prefetch_idf_tool_archives( len(entries), ", ".join(entry["name"] for entry in entries), ) + # tools.json always carries sizes; should one be missing the bar + # could not be trusted, so draw none rather than a wrong one. + sizes = [entry["size"] for entry in entries] progress = BatchDownloadProgress( - "Downloading ESP-IDF tools", - sum(entry["size"] or 0 for entry in entries), + "Downloading ESP-IDF tools", sum(sizes) if all(sizes) else 0 ) def _download(entry: dict) -> None: + tracker = progress.tracker() try: download_with_resume( entry["url"], dist_path / entry["dest"], sha256=entry["sha256"], size=entry["size"], - progress=progress.tracker(), + progress=tracker, ) except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught # Keep prefetching the remaining archives; the installer # will retry this one itself (without resume). + tracker(0) _LOGGER.warning("Could not prefetch %s: %s", entry["name"], e) - with ThreadPoolExecutor(max_workers=min(_PREFETCH_WORKERS, len(entries))) as ex: - list(ex.map(_download, entries)) + ex = ThreadPoolExecutor(max_workers=min(_PREFETCH_WORKERS, len(entries))) + try: + for future in [ex.submit(_download, entry) for entry in entries]: + future.result() + finally: + # On Ctrl-C drop the queued archives instead of downloading them + # all before the process can exit; in-flight ones still finish. + ex.shutdown(wait=True, cancel_futures=True) + progress.done() except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught # The installer downloads anything missing itself; never let the # prefetch become a new way for the install to fail. diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index 76ae6b801f..5247619ab9 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -742,7 +742,9 @@ class BatchDownloadProgress: Each ``tracker()`` is a ``progress`` callback for one download; it reports that file's absolute byte count and the bar shows the sum over ``total``. The lock also serialises the bar's stderr writes, so worker threads never - interleave frames. With an unknown ``total`` (0) nothing is drawn. + interleave frames. With an unknown ``total`` (0) nothing is drawn. Call + ``done()`` once every download has finished (or failed) so a bar that + never reached 100% still ends its line before the next log message. """ def __init__(self, header: str, total: int) -> None: @@ -765,6 +767,10 @@ class BatchDownloadProgress: return update + def done(self) -> None: + if self._bar is not None and self._bar.last_progress != 100: + self._bar.done() + def download_with_resume( url: str, @@ -829,7 +835,7 @@ def download_with_resume( try: _verify_file(dest, sha256, size) if progress is not None: - progress(dest.stat().st_size) + progress(size if size is not None else dest.stat().st_size) return except EsphomeError: dest.unlink() @@ -887,7 +893,7 @@ def download_with_resume( if progress is not None: # Also credits a part file an earlier run completed without # streaming anything this time. - progress(part.stat().st_size) + progress(expected_size or part.stat().st_size) if not expected_size and sha256 is None: # No sha, no size, and the server sent no usable # content-length: nothing can prove the download complete diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index af14b35d0d..f06190fee7 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -15,7 +15,7 @@ import subprocess import sys import tarfile from types import SimpleNamespace -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest @@ -1043,6 +1043,29 @@ def test_prefetch_one_failed_archive_does_not_stop_the_rest( assert "Could not prefetch cmake@3.30.2" in caplog.text +def test_prefetch_finishes_progress_bar_and_cancels_queue(tmp_path: Path) -> None: + """The batch bar is closed out after the pool, and the pool is shut down + with cancel_futures so Ctrl-C does not drain every queued archive.""" + with ( + patch( + "esphome.espidf.framework.run_command", + return_value=(True, _PREFETCH_JSON, ""), + ), + patch("esphome.espidf.framework.download_with_resume"), + patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch("esphome.espidf.framework.BatchDownloadProgress") as progress_cls, + patch( + "esphome.espidf.framework.ThreadPoolExecutor", wraps=ThreadPoolExecutor + ) as pool_cls, + ): + pool = MagicMock(wraps=ThreadPoolExecutor(max_workers=2)) + pool_cls.return_value = pool + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + + pool.shutdown.assert_called_once_with(wait=True, cancel_futures=True) + progress_cls.return_value.done.assert_called_once_with() + + def test_prefetch_passes_targets_and_tools_to_script(tmp_path: Path) -> None: with ( patch( diff --git a/tests/unit_tests/test_framework_helpers.py b/tests/unit_tests/test_framework_helpers.py index ffcb79155d..96da38e606 100644 --- a/tests/unit_tests/test_framework_helpers.py +++ b/tests/unit_tests/test_framework_helpers.py @@ -1181,9 +1181,31 @@ class TestBatchDownloadProgress: def test_unknown_total_draws_nothing(self) -> None: with patch("esphome.framework_helpers.ProgressBar") as bar_cls: - BatchDownloadProgress("Downloading", 0).tracker()(5) + progress = BatchDownloadProgress("Downloading", 0) + progress.tracker()(5) + progress.done() bar_cls.assert_not_called() + def test_done_ends_an_unfinished_bar(self) -> None: + """A batch that stops short of 100% (a failed archive) still ends its + line so the next log message starts on a fresh row.""" + stream = io.StringIO() + stream.isatty = lambda: True # type: ignore[method-assign] + with patch("esphome.helpers.sys.stderr", stream): + progress = BatchDownloadProgress("Downloading", 10) + progress.tracker()(5) + progress.done() + assert stream.getvalue().endswith("50% \n") + + def test_done_after_full_bar_adds_nothing(self) -> None: + stream = io.StringIO() + stream.isatty = lambda: True # type: ignore[method-assign] + with patch("esphome.helpers.sys.stderr", stream): + progress = BatchDownloadProgress("Downloading", 10) + progress.tracker()(10) + progress.done() + assert stream.getvalue().endswith("100% Done...\r\n") + class TestDownloadFromMirrors: def test_success_returns_url_and_writes_content(self, tmp_path: Path) -> None: From 12238eec3bede436dc833f6fd01aa6a66676d2a6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 19 Aug 2026 13:57:15 -0500 Subject: [PATCH 3/5] Log prefetch failures after the bar, assert on the shared progress instance --- esphome/espidf/framework.py | 12 +++++++++--- tests/unit_tests/test_espidf_framework.py | 8 ++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index 95d168c85c..ee46364abf 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -747,12 +747,16 @@ def _prefetch_idf_tool_archives( len(entries), ", ".join(entry["name"] for entry in entries), ) - # tools.json always carries sizes; should one be missing the bar - # could not be trusted, so draw none rather than a wrong one. + # tools.json always carries sizes; should one be missing the combined + # bar could not be trusted, so show no bar at all (per-file bars from + # several threads would interleave) rather than a wrong one. sizes = [entry["size"] for entry in entries] progress = BatchDownloadProgress( "Downloading ESP-IDF tools", sum(sizes) if all(sizes) else 0 ) + # Reported after the bar is done so the warnings do not land on + # its row; list.append is atomic under the GIL. + failures: list[tuple[str, Exception]] = [] def _download(entry: dict) -> None: tracker = progress.tracker() @@ -768,7 +772,7 @@ def _prefetch_idf_tool_archives( # Keep prefetching the remaining archives; the installer # will retry this one itself (without resume). tracker(0) - _LOGGER.warning("Could not prefetch %s: %s", entry["name"], e) + failures.append((entry["name"], e)) ex = ThreadPoolExecutor(max_workers=min(_PREFETCH_WORKERS, len(entries))) try: @@ -779,6 +783,8 @@ def _prefetch_idf_tool_archives( # all before the process can exit; in-flight ones still finish. ex.shutdown(wait=True, cancel_futures=True) progress.done() + for name, e in failures: + _LOGGER.warning("Could not prefetch %s: %s", name, e) except Exception as e: # noqa: BLE001 # pylint: disable=broad-exception-caught # The installer downloads anything missing itself; never let the # prefetch become a new way for the install to fail. diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index f06190fee7..a453f6eece 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -896,6 +896,7 @@ def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: ), patch("esphome.espidf.framework.download_with_resume") as download, patch("esphome.espidf.framework.get_system_python_path", return_value="python"), + patch("esphome.espidf.framework.BatchDownloadProgress") as progress_cls, ): _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) @@ -910,10 +911,9 @@ def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: assert kwargs["sha256"] == "ab" * 32 assert kwargs["size"] == 123 # every archive reports into the one combined progress bar - assert all( - kw["progress"].__qualname__.startswith("BatchDownloadProgress.tracker") - for kw in calls.values() - ) + progress_cls.assert_called_once_with("Downloading ESP-IDF tools", 123 + 45) + tracker = progress_cls.return_value.tracker.return_value + assert all(kw["progress"] is tracker for kw in calls.values()) def test_prefetch_downloads_archives_concurrently(tmp_path: Path) -> None: From b361342bde80628110e3b2eca9672274266b0182 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 19 Aug 2026 15:12:37 -0500 Subject: [PATCH 4/5] Do not end a progress bar that never drew a frame --- esphome/framework_helpers.py | 8 +++++++- tests/unit_tests/test_framework_helpers.py | 9 +++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index 5247619ab9..0daa446ed9 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -768,7 +768,13 @@ class BatchDownloadProgress: return update def done(self) -> None: - if self._bar is not None and self._bar.last_progress != 100: + # Nothing to end unless a frame was drawn and it was not the final + # one (update(1) already emitted its own newline). + if ( + self._bar is not None + and self._bar.last_progress is not None + and self._bar.last_progress != 100 + ): self._bar.done() diff --git a/tests/unit_tests/test_framework_helpers.py b/tests/unit_tests/test_framework_helpers.py index 96da38e606..098dcb7725 100644 --- a/tests/unit_tests/test_framework_helpers.py +++ b/tests/unit_tests/test_framework_helpers.py @@ -1197,6 +1197,15 @@ class TestBatchDownloadProgress: progress.done() assert stream.getvalue().endswith("50% \n") + def test_done_before_any_frame_writes_nothing(self) -> None: + """A batch aborted before any tracker fired must not emit a stray + newline for a bar that was never drawn.""" + stream = io.StringIO() + stream.isatty = lambda: True # type: ignore[method-assign] + with patch("esphome.helpers.sys.stderr", stream): + BatchDownloadProgress("Downloading", 10).done() + assert stream.getvalue() == "" + def test_done_after_full_bar_adds_nothing(self) -> None: stream = io.StringIO() stream.isatty = lambda: True # type: ignore[method-assign] From 040c91259da0600bbe6972bc9499b861247621d3 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 15:25:37 -0500 Subject: [PATCH 5/5] Mirror the URL rule in the bundled probe; fail on empty converted trees; harden build_tool argv A URL-pinned dependency now skips the bundled probe (the walk resolves the fork; adding the bundled copy would double the archive). A versioned bundled candidate's non-platform manifest fault warns here since it skips the walk's usability filter via provides(); version-less causes stay at debug (the walk already warned). The pending drain logs a manifest-name suppression at debug and drops its unreachable bundled_names re-check. A converted tree with no sources and no headers now fails by name at emit like the bundled case (test scaffolds gained real source files). build_tool validates each mode's operand count and a failed copy unlinks the partial output. --- esphome/arduino/library.py | 82 ++++++++++++----- esphome/build_gen/build_tool.py | 26 ++++-- tests/unit_tests/build_gen/test_build_tool.py | 28 ++++++ tests/unit_tests/test_arduino_library.py | 87 ++++++++++++++++--- 4 files changed, 181 insertions(+), 42 deletions(-) diff --git a/esphome/arduino/library.py b/esphome/arduino/library.py index 6932196207..2c849a2fb0 100644 --- a/esphome/arduino/library.py +++ b/esphome/arduino/library.py @@ -27,8 +27,10 @@ from esphome.platformio.library import ( LIBRARY_HEADER_SUFFIXES, SRC_FILE_EXTENSIONS, ConvertedLibrary, + IncompatiblePlatform, InvalidLibrary, LibraryBackend, + _url_or_none, check_library_data, collect_filtered_files, convert_libraries, @@ -219,18 +221,18 @@ def _collect_lib_sources( len(dropped), ", ".join(sorted(dropped)), ) - if not lib.sources and not any( - Path(f).suffix.lower() in LIBRARY_HEADER_SUFFIXES for f in matched + if ( + not lib.sources + and ("srcFilter" in build or "srcDir" in build) + and not any(Path(f).suffix.lower() in LIBRARY_HEADER_SUFFIXES for f in matched) ): - # Matched headers mean a header-only library; anything else with no - # sources yields an empty archive that fails far away at link - if "srcFilter" in build or "srcDir" in build: - _LOGGER.warning( - "Library %s declares srcFilter/srcDir but no source files matched", - name, - ) - else: - _LOGGER.warning("Library %s has no sources or headers", name) + # Matched headers mean a header-only library; a declared filter + # matching nothing (or only inert files) is a manifest/tree problem. + # The truly empty tree raises via _assert_tree_has_code. + _LOGGER.warning( + "Library %s declares srcFilter/srcDir but no source files matched", + name, + ) def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: @@ -284,18 +286,25 @@ def _bundled_library(framework_path: Path, name: str) -> ArduinoLibrary: name, ) lib = _library_info(name, lib_dir, data) - if not lib.sources and not any( - Path(p).suffix.lower() in LIBRARY_HEADER_SUFFIXES for p in walk_files(lib_dir) - ): - # An empty or half-extracted bundled directory can never link; a - # warning would scroll away and resurface as undefined symbols - raise EsphomeError( - f"Bundled library {name} has no sources or headers; the " - "framework install may be incomplete (run 'esphome clean-all')" - ) + _assert_tree_has_code( + name, + lib_dir, + "the framework install may be incomplete (run 'esphome clean-all')", + ) return lib +def _assert_tree_has_code(name: str, root: Path, hint: str) -> None: + """An empty or half-extracted tree can never link; fail by name (a + warning would scroll away and resurface as undefined symbols).""" + if not any( + Path(p).suffix in SRC_FILE_EXTENSIONS + or Path(p).suffix.lower() in LIBRARY_HEADER_SUFFIXES + for p in walk_files(root) + ): + raise EsphomeError(f"Library {name} has no sources or headers; {hint}") + + def _external_short_name(name: str) -> str: """The short library name of a requested spec. @@ -411,6 +420,10 @@ def resolve_libraries( continue if name in bundled_names or is_lib_ignored(name, lib_ignore): continue + if _url_or_none(dep.get("version")) is not None: + # A URL names one specific source (the walk resolves it as + # git); the bundled copy must never be added on top + continue if dep.get("owner") or not _provided(name): # Owner-less names in the framework tree prefer the bundled # copy (PIO's process_dependencies); everything else resolves @@ -421,9 +434,19 @@ def resolve_libraries( # mismatch; re-checking would warn twice check_library_data(dep, pio_platform, None) except InvalidLibrary as err: - # The shared walk already reported any non-platform cause; - # warning again here would read as two distinct failures - _LOGGER.debug("Skip bundled candidate %s: %s", name, err) + if isinstance(err, IncompatiblePlatform) or "version" not in dep: + # The platform skip is routine; the walk's version-less + # filter already warned for other version-less causes + _LOGGER.debug("Skip bundled candidate %s: %s", name, err) + else: + # Versioned deps skip the walk's filter via provides(); + # this is the only place the fault can be seen + _LOGGER.warning( + "Skipping bundled dependency %s of %s: %s", + name, + component.name, + err, + ) continue # Deferred: a later-emitted library's manifest name may satisfy # this; adding now could double the archive @@ -433,6 +456,11 @@ def resolve_libraries( apply_extra_script( component, board_mcu=lambda: board_mcu, pio_platform=pio_platform ) + _assert_tree_has_code( + component.get_require_name(), + component.source_dir, + "the download may be incomplete (run 'esphome clean-all')", + ) if isinstance(manifest_name := component.data.get("name"), str): converted_manifest_names.add(manifest_name) converted.append( @@ -456,7 +484,13 @@ def resolve_libraries( ), ) for name in pending_bundled: - if name in converted_manifest_names or name in bundled_names: + if name in converted_manifest_names: + # Exact manifest-name evidence: the converted library is this + # library, so the bundled copy would double the archive + _LOGGER.debug( + "Bundled %s suppressed by a converted library's manifest name", + name, + ) continue bundled_names.add(name) bundled.append(_bundled_library(framework_path, name)) diff --git a/esphome/build_gen/build_tool.py b/esphome/build_gen/build_tool.py index 3cbb4fb61f..f77cd9d497 100644 --- a/esphome/build_gen/build_tool.py +++ b/esphome/build_gen/build_tool.py @@ -65,16 +65,32 @@ def _run_ar(ar: str, archive: str, rspfile: str) -> int: def _run_copy(src: str, dst: str) -> int: - shutil.copyfile(src, dst) + try: + shutil.copyfile(src, dst) + except OSError: + # Never leave a partially written output (e.g. a firmware image) + Path(dst).unlink(missing_ok=True) + raise return 0 +# mode -> (handler, expected operand count); surplus argv means a +# mis-specified ninja rule and must error, not silently drop operands +_MODES = {"ar": (_run_ar, 3), "copy": (_run_copy, 2)} + + def main() -> int: mode = sys.argv[1] - if mode == "ar": - return _run_ar(*sys.argv[2:5]) - if mode == "copy": - return _run_copy(*sys.argv[2:4]) + if entry := _MODES.get(mode): + handler, argc = entry + args = sys.argv[2:] + if len(args) != argc: + print( + f"build_tool {mode}: expected {argc} arguments, got {len(args)}", + file=sys.stderr, + ) + return 1 + return handler(*args) print(f"unknown build_tool mode: {mode}", file=sys.stderr) return 1 diff --git a/tests/unit_tests/build_gen/test_build_tool.py b/tests/unit_tests/build_gen/test_build_tool.py index 9a361211ad..5698e8521e 100644 --- a/tests/unit_tests/build_gen/test_build_tool.py +++ b/tests/unit_tests/build_gen/test_build_tool.py @@ -176,3 +176,31 @@ def test_ar_batch_failure_stops(tmp_path: Path) -> None: assert mock_run.call_count == 1 # The failed batch must not leave a truncated archive behind assert not archive.exists() + + +def test_surplus_arguments_error(capsys: pytest.CaptureFixture[str]) -> None: + """A mis-specified ninja rule passing extra operands errors instead of + silently dropping them.""" + with patch.object( + build_tool.sys, "argv", ["build_tool", "copy", "a", "b", "extra"] + ): + assert build_tool.main() == 1 + assert "expected 2 arguments, got 3" in capsys.readouterr().err + + +def test_copy_failure_leaves_no_partial_output(tmp_path: Path) -> None: + """A failed copy unlinks the destination; a partial firmware image must + never be left on disk.""" + dst = tmp_path / "firmware.factory.bin" + dst.write_text("stale") + with ( + patch.object(build_tool.shutil, "copyfile", side_effect=OSError("disk full")), + patch.object( + build_tool.sys, + "argv", + ["build_tool", "copy", str(tmp_path / "src.bin"), str(dst)], + ), + pytest.raises(OSError), + ): + build_tool.main() + assert not dst.exists() diff --git a/tests/unit_tests/test_arduino_library.py b/tests/unit_tests/test_arduino_library.py index 17315e9a27..75204e6816 100644 --- a/tests/unit_tests/test_arduino_library.py +++ b/tests/unit_tests/test_arduino_library.py @@ -86,6 +86,7 @@ def _webserver(tmp_path: Path, data: dict) -> ConvertedLibrary: _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") lib_dir = tmp_path / "converted" / "webserver" (lib_dir / "src").mkdir(parents=True) + (lib_dir / "src" / "server.cpp").write_text("") return _converted("esp32async__ESPAsyncWebServer", lib_dir, data) @@ -108,8 +109,10 @@ def _ws_tcp_pair(tmp_path: Path) -> tuple[ConvertedLibrary, ConvertedLibrary]: """Build ESPAsyncWebServer (depending on ESPAsyncTCP) plus resolved TCP.""" ws_dir = tmp_path / "converted" / "webserver" (ws_dir / "src").mkdir(parents=True) + (ws_dir / "src" / "server.cpp").write_text("") tcp_dir = tmp_path / "converted" / "tcp" (tcp_dir / "src").mkdir(parents=True) + (tcp_dir / "src" / "tcp.cpp").write_text("") ws = _converted( "esp32async__ESPAsyncWebServer", ws_dir, @@ -182,22 +185,26 @@ def test_library_info_declared_filter_matches_nothing_warns( ) -> None: read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") data = {"build": {"srcFilter": ["+"]}} lib = component._library_info("x", read_path, data) assert not lib.sources assert "declares srcFilter/srcDir but no source files matched" in caplog.text -def test_library_info_empty_tree_warns( - tmp_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - """No sources and no headers is an empty archive waiting to fail at - link; warn by name even without a declared filter.""" - read_path = tmp_path / "lib" - (read_path / "src").mkdir(parents=True) - lib = component._library_info("x", read_path, {}) - assert not lib.sources - assert "has no sources or headers" in caplog.text +def test_empty_converted_tree_raises_at_emit(tmp_path: Path) -> None: + """A converted tree with no sources and no headers is a broken download; + fail by name like the bundled case.""" + framework = _make_framework(tmp_path) + _add_library("Some/Empty", "1.0.0") + lib_dir = tmp_path / "converted" / "empty" + (lib_dir / "src").mkdir(parents=True) + converted = _converted("some__Empty", lib_dir, {"build": {}}) + with ( + _emitting_converter(converted), + pytest.raises(EsphomeError, match="no sources or headers; the download"), + ): + _resolve(framework) def test_library_info_no_src_dir(tmp_path: Path) -> None: @@ -275,6 +282,7 @@ def test_resolve_libraries_bundled_dep_already_present(tmp_path: Path) -> None: lib_dir = tmp_path / "converted" / "external" lib_dir.mkdir(parents=True) + (lib_dir / "main.cpp").write_text("") converted = _converted( "some__External", lib_dir, {"dependencies": [{"name": "Wire"}]} ) @@ -291,6 +299,7 @@ def test_library_info_trailing_bare_flag_warns( ) -> None: read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") lib = component._library_info("x", read_path, {"build": {"flags": ["-DA=1 -l"]}}) assert lib.flags == ["-DA=1"] assert lib.link_libs == [] @@ -302,6 +311,7 @@ def test_library_info_missing_explicit_include_warns( ) -> None: read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") lib = component._library_info("x", read_path, {"build": {"flags": ["-Inope"]}}) assert lib.include_dirs == [(read_path / "src").resolve()] assert "include dir nope which does not exist" in caplog.text @@ -343,6 +353,7 @@ def test_resolve_libraries_lib_ignore_covers_bundled_dependencies( lib_dir = tmp_path / "converted" / "external" lib_dir.mkdir(parents=True) + (lib_dir / "main.cpp").write_text("") converted = _converted( "some__External", lib_dir, {"dependencies": [{"name": "Wire"}]} ) @@ -373,6 +384,7 @@ def test_library_info_lib_archive_flag(tmp_path: Path) -> None: the generator's contract; default is archive.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") assert component._library_info("x", read_path, {}).lib_archive is True assert ( component._library_info( @@ -460,6 +472,47 @@ def test_nonplatform_rejection_warns_once_through_real_converter( assert caplog.text.count("manifest is corrupt") == 1 +def test_url_pinned_bundled_name_not_doubled(tmp_path: Path) -> None: + """A URL-pinned dependency names one specific source; the bundled copy + of the same short name must never be added on top of the fork.""" + framework = _make_framework(tmp_path) + converted = _webserver( + tmp_path, + { + "build": {}, + "dependencies": [ + {"name": "Wire", "version": "https://github.com/x/wire-fork.git"} + ], + }, + ) + with _emitting_converter(converted): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + + +def test_versioned_bundled_candidate_fault_warns( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A versioned bundled-name dependency skips the walk's usability filter + via provides(), so a non-platform fault warns here.""" + framework = _make_framework(tmp_path) + converted = _webserver( + tmp_path, + {"build": {}, "dependencies": [{"name": "Wire", "version": "*"}]}, + ) + with ( + _emitting_converter(converted), + patch.object( + component, + "check_library_data", + side_effect=InvalidLibrary("manifest is corrupt"), + ), + ): + libs = _resolve(framework) + assert "Wire" not in [lib.name for lib in libs] + assert "Skipping bundled dependency Wire" in caplog.text + + def test_short_name_collision_with_bundled_name_warns( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: @@ -473,6 +526,7 @@ def test_short_name_collision_with_bundled_name_warns( {"build": {}, "dependencies": [{"name": "Wire"}]}, ) (tmp_path / "conv" / "src").mkdir(parents=True) + (tmp_path / "conv" / "src" / "a.cpp").write_text("") with _emitting_converter(converted): libs = _resolve(framework) assert "Wire" not in [lib.name for lib in libs] @@ -508,6 +562,7 @@ def test_library_info_falsy_declared_src_dir_raises( """A declared-but-falsy srcDir must not silently fall back to the probe.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") with pytest.raises(EsphomeError, match="does not exist"): component._library_info("x", read_path, {"build": {"srcDir": declared}}) @@ -529,6 +584,7 @@ def test_library_info_lib_archive_parse( """bool("false") is True; the string forms must parse, not coerce.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") lib = component._library_info("x", read_path, {"build": {"libArchive": value}}) assert lib.lib_archive is expected @@ -539,6 +595,7 @@ def test_library_info_dropped_link_fields_warn( """precompiled/ldflags properties are not honored; the drop is named.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") component._library_info( "x", read_path, {"precompiled": "true", "ldflags": "-lfoo", "build": {}} ) @@ -604,6 +661,7 @@ def test_library_info_lib_archive_malformed_raises(tmp_path: Path) -> None: """A typo'd libArchive fails by name like the other build fields.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") with pytest.raises(EsphomeError, match="malformed libArchive value 'archive-me'"): component._library_info("x", read_path, {"build": {"libArchive": "archive-me"}}) @@ -674,6 +732,7 @@ def test_library_info_malformed_build_fields_are_named( """Malformed includeDir/srcFilter fail naming the library like srcDir.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") with pytest.raises(EsphomeError, match=match): component._library_info("x", read_path, {"build": build}) @@ -693,6 +752,7 @@ def test_library_info_dot_a_linkage_parses_strictly( """The dot_a_linkage property uses the same strict table as libArchive.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") lib = component._library_info("x", read_path, {"dot_a_linkage": value, "build": {}}) assert lib.lib_archive is expected @@ -701,6 +761,7 @@ def test_library_info_dot_a_linkage_malformed_raises(tmp_path: Path) -> None: """A typo'd dot_a_linkage must not silently flip link semantics.""" read_path = tmp_path / "lib" (read_path / "src").mkdir(parents=True) + (read_path / "src" / "stub.cpp").write_text("") with pytest.raises(EsphomeError, match="malformed dot_a_linkage value 'yes'"): component._library_info("x", read_path, {"dot_a_linkage": "yes", "build": {}}) @@ -901,8 +962,10 @@ def test_converted_manifest_name_suppresses_bundled_dependency( _add_library("Someone/WireLib", "9.9.9") ws_dir = tmp_path / "converted" / "webserver" (ws_dir / "src").mkdir(parents=True) + (ws_dir / "src" / "stub.cpp").write_text("") wire_dir = tmp_path / "converted" / "wire" (wire_dir / "src").mkdir(parents=True) + (wire_dir / "src" / "wire.cpp").write_text("") ws = _converted( "esp32async__ESPAsyncWebServer", ws_dir, @@ -939,9 +1002,7 @@ def test_empty_bundled_library_warns( framework = _make_framework(tmp_path) (framework / "libraries" / "Empty").mkdir() _add_library("Empty", None) - with pytest.raises( - EsphomeError, match="Bundled library Empty has no sources or headers" - ): + with pytest.raises(EsphomeError, match="Library Empty has no sources or headers"): _resolve(framework)