diff --git a/esphome/__main__.py b/esphome/__main__.py index c17471dbbb..308d0d6653 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -857,18 +857,14 @@ def compile_program(args: ArgsProtocol, config: ConfigType) -> int: toolchain.create_factory_bin() toolchain.create_ota_bin() toolchain.create_elf_copy() + from esphome.build_helpers.idedata import IDEDATA_BEST_EFFORT_ERRORS + try: if toolchain.get_idedata() is None: _LOGGER.warning("No idedata was generated for this build") - except ( - EsphomeError, - LookupError, - OSError, - RuntimeError, - ValueError, - ) as err: - # Broad on purpose: the firmware already built; an idedata - # failure must not fail a successful build. + except IDEDATA_BEST_EFFORT_ERRORS as err: + # The firmware already built; an idedata failure must not fail + # a successful build. _LOGGER.warning("Could not generate idedata: %s", err) _LOGGER.debug("Idedata failure detail", exc_info=True) else: diff --git a/esphome/build_helpers/idedata.py b/esphome/build_helpers/idedata.py index f58b0ec8f8..567a5fa29e 100644 --- a/esphome/build_helpers/idedata.py +++ b/esphome/build_helpers/idedata.py @@ -21,6 +21,17 @@ import subprocess from esphome.core import EsphomeError from esphome.helpers import write_file +# Everything idedata generation may raise after a successful link. Broad on +# purpose, and shared by every consumer: idedata is a bonus artifact, so +# these must be caught and warned about, never allowed to fail the build. +IDEDATA_BEST_EFFORT_ERRORS = ( + EsphomeError, + LookupError, + OSError, + RuntimeError, + ValueError, +) + _LOGGER = logging.getLogger(__name__) # C++ translation-unit suffixes used to identify ESPHome source files. @@ -305,9 +316,24 @@ def idedata_from_build(compile_commands: Path, launcher: str | None = None) -> d build_includes: dict[str, None] = dict.fromkeys( rep_includes if _is_esphome_src(representative["file"]) else () ) + + def _shape(entry: dict) -> str: + # The command minus its TU-specific paths: entries sharing a shape + # carry identical include sets (one ninja rule), so tokenize once + # per shape instead of once per TU + return ( + entry["command"] + .replace(entry.get("file", ""), "") + .replace(entry.get("output", ""), "") + ) + + seen_shapes = {_shape(representative)} for entry in entries: if entry is representative or not _is_esphome_src(entry["file"]): continue + if (shape := _shape(entry)) in seen_shapes: + continue + seen_shapes.add(shape) for inc in parse_entry(entry, launcher)[2]: build_includes.setdefault(inc, None) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index e6601eb767..a31a339fee 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -1,7 +1,6 @@ """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 @@ -25,6 +24,7 @@ from esphome.framework_helpers import ( get_python_env_executable_path, get_system_python_path, rmdir, + run_batch_downloads, run_command, run_command_ok, str_to_lst_of_str, @@ -692,12 +692,6 @@ 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, @@ -735,17 +729,16 @@ def _prefetch_idf_tool_archives( ) return dist_path = get_idf_tools_path() / "dist" - pending = [ - entry - for entry in json.loads(stdout) - if not (dist_path / entry["dest"]).is_file() - ] - # tools.json always carries sha256 and size; an entry missing either - # must not be downloaded unverified here, so leave it to the - # installer (which fails loudly on a bad archive). - entries = [e for e in pending if e.get("sha256") and e.get("size")] - for entry in pending: - if entry not in entries: + entries = [] + for entry in json.loads(stdout): + if (dist_path / entry["dest"]).is_file(): + continue + # tools.json always carries sha256 and size; an entry missing + # either must not be downloaded unverified here, so leave it to + # the installer (which fails loudly on a bad archive). + if entry.get("sha256") and entry.get("size"): + entries.append(entry) + else: _LOGGER.warning( "Tool %s has no sha256/size in the download list; " "leaving it to the installer", @@ -758,42 +751,28 @@ def _prefetch_idf_tool_archives( len(entries), ", ".join(entry["name"] for entry in entries), ) + # Every entry carries a size (checked above), so the combined bar can # be trusted. Unlike the library prefetch there is no sequential # fallback: per-file bars from several threads would interleave, and # skipping the prefetch would lose the resume workaround for #17703. - progress = BatchDownloadProgress( - "Downloading ESP-IDF tools", sum(entry["size"] for entry in entries) + def _download(entry: dict): + return lambda tracker: download_with_resume( + entry["url"], + dist_path / entry["dest"], + sha256=entry["sha256"], + size=entry["size"], + progress=tracker, + ) + + # A failed archive is retried by the installer itself (without + # resume); keep prefetching the rest. + failures = run_batch_downloads( + BatchDownloadProgress( + "Downloading ESP-IDF tools", sum(entry["size"] for entry in entries) + ), + [(entry["name"], _download(entry)) for entry in entries], ) - # 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() - try: - download_with_resume( - entry["url"], - dist_path / entry["dest"], - sha256=entry["sha256"], - size=entry["size"], - 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) - failures.append((entry["name"], e)) - - 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() 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 diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index 06696c0153..4d0574372a 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -1,6 +1,7 @@ """Generic toolchain installation helpers shared across framework implementations.""" from collections.abc import Callable, Iterable +from concurrent.futures import ThreadPoolExecutor from contextlib import ExitStack import hashlib import io @@ -737,6 +738,46 @@ def _stream_response_to_file( own_bar.update(1) +# Concurrent downloads per batch; enough to hide latency without +# hammering the host or the mirrors. +BATCH_DOWNLOAD_WORKERS = 4 + + +def run_batch_downloads( + progress: "BatchDownloadProgress", + jobs: list[tuple[str, Callable[[Callable[[int], None]], None]]], + max_workers: int = BATCH_DOWNLOAD_WORKERS, +) -> list[tuple[str, Exception]]: + """Run download jobs concurrently, reporting into one combined bar. + + ``jobs`` holds ``(name, fetch)`` pairs where ``fetch(tracker)`` performs + one download reporting absolute byte counts to ``tracker``. Failures are + collected (list.append is atomic under the GIL) and returned after the + bar is done, so the caller's warnings never land on the bar's row; a + failed job credits its tracker 0 so the bar can still complete. Ctrl-C + drops queued jobs instead of downloading them all before the process + can exit; in-flight ones still finish. + """ + failures: list[tuple[str, Exception]] = [] + + def _run(name: str, fetch: Callable[[Callable[[int], None]], None]) -> None: + tracker = progress.tracker() + try: + fetch(tracker) + except Exception as err: # noqa: BLE001 # pylint: disable=broad-exception-caught + failures.append((name, err)) + tracker(0) + + ex = ThreadPoolExecutor(max_workers=min(max_workers, len(jobs))) + try: + for future in [ex.submit(_run, name, fetch) for name, fetch in jobs]: + future.result() + finally: + ex.shutdown(wait=True, cancel_futures=True) + progress.done() + return failures + + class BatchDownloadProgress: """One progress bar across several concurrent ``download_with_resume`` calls. diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 7d69d9b894..24bd9db490 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -32,10 +32,12 @@ from urllib.request import url2pathname from esphome import git from esphome.core import CORE, EsphomeError, Library from esphome.framework_helpers import ( + BATCH_DOWNLOAD_WORKERS, BatchDownloadProgress, archive_extract_all, download_from_mirrors, rmdir, + run_batch_downloads, ) _LOGGER = logging.getLogger(__name__) @@ -896,10 +898,6 @@ def is_lib_ignored(name: str | None, lib_ignore: set[str]) -> bool: ) -# A few streams saturate most links without hammering the registry -_DOWNLOAD_WORKERS = 4 - - def _content_lengths(urls: list[str]) -> list[int | None]: """Content-Length per URL via HEAD requests; None when unknown.""" import requests @@ -915,7 +913,7 @@ def _content_lengths(urls: list[str]) -> list[int | None]: _LOGGER.debug("HEAD %s failed: %s", url, err) return None - with ThreadPoolExecutor(max_workers=min(_DOWNLOAD_WORKERS, len(urls))) as ex: + with ThreadPoolExecutor(max_workers=min(BATCH_DOWNLOAD_WORKERS, len(urls))) as ex: return list(ex.map(head, urls)) @@ -972,28 +970,16 @@ def _prefetch_wave( ), ) return - progress = BatchDownloadProgress("Downloading libraries", sum(sizes)) - # 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 _fetch(component: ConvertedLibrary) -> None: - tracker = progress.tracker() - try: - component.download(salt=salt, namespace=namespace, progress=tracker) - except Exception as err: # noqa: BLE001 # pylint: disable=broad-exception-caught - failures.append((component.name, err)) - tracker(0) + def _fetch(component: ConvertedLibrary): + return lambda tracker: component.download( + salt=salt, namespace=namespace, progress=tracker + ) - ex = ThreadPoolExecutor(max_workers=min(_DOWNLOAD_WORKERS, len(components))) - try: - for future in [ex.submit(_fetch, component) for component in components]: - 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() + failures = run_batch_downloads( + BatchDownloadProgress("Downloading libraries", sum(sizes)), + [(component.name, _fetch(component)) for component in components], + ) for name, err in failures: # The sequential call below retries and raises the real error _LOGGER.warning("Prefetch of %s failed (retrying sequentially): %s", name, err) diff --git a/tests/unit_tests/build_helpers/test_idedata.py b/tests/unit_tests/build_helpers/test_idedata.py index 491daa0ec0..ff91ca34c1 100644 --- a/tests/unit_tests/build_helpers/test_idedata.py +++ b/tests/unit_tests/build_helpers/test_idedata.py @@ -151,6 +151,40 @@ def test_is_esphome_src_handles_backslash_paths() -> None: assert not idedata._is_esphome_src(r"C:\b\src\esphome\core\app.h") +def test_idedata_from_build_dedupes_identical_command_shapes( + tmp_path: Path, +) -> None: + """Translation units sharing one ninja rule (same command modulo + file/output) carry + identical includes, so only one per shape is tokenized; a differing + shape still contributes its includes.""" + + def _tu(name: str, inc: str) -> dict: + # ninja's compdb embeds the file and output strings verbatim + file = f"{ABS}build/src/esphome/core/{name}.cpp" + return _entry( + f"{ABS}build", file, f"/tools/g++ -I{ABS}inc/{inc} -c {file} -o {name}.o" + ) | {"output": f"{name}.o"} + + entries = [_tu(name, "shared") for name in ("application", "component", "helpers")] + entries.append(_tu("extra", "extra")) + compile_commands = tmp_path / "compile_commands.json" + compile_commands.write_text(json.dumps(entries)) + with ( + patch.object(idedata, "get_toolchain_includes", return_value=[]), + patch.object(idedata, "parse_entry", wraps=idedata.parse_entry) as spy, + ): + data = idedata.idedata_from_build(compile_commands) + includes = set(data["includes"]["build"]) + assert f"{ABS}inc/shared".replace("\\", "/") in { + i.replace("\\", "/") for i in includes + } + assert any("inc/extra" in i for i in includes) + # Representative + one distinct shape; the two same-shape duplicates + # are never tokenized + assert spy.call_count == 2 + + def test_idedata_from_build(tmp_path: Path) -> None: """Full transform: representative entry + include union + toolchain dirs.""" compile_commands = tmp_path / "compile_commands.json" diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index b51d535586..b5296def66 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -989,7 +989,7 @@ def test_prefetch_downloads_archives_concurrently(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.ThreadPoolExecutor", wraps=ThreadPoolExecutor + "esphome.framework_helpers.ThreadPoolExecutor", wraps=ThreadPoolExecutor ) as pool, ): _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) @@ -1008,7 +1008,7 @@ def test_prefetch_single_archive_uses_one_worker(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.ThreadPoolExecutor", wraps=ThreadPoolExecutor + "esphome.framework_helpers.ThreadPoolExecutor", wraps=ThreadPoolExecutor ) as pool, ): _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) @@ -1108,7 +1108,7 @@ def test_prefetch_finishes_progress_bar_and_cancels_queue(tmp_path: Path) -> Non 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 + "esphome.framework_helpers.ThreadPoolExecutor", wraps=ThreadPoolExecutor ) as pool_cls, ): pool = MagicMock(wraps=ThreadPoolExecutor(max_workers=2))