From 35bb8388a23441e4b0ac7e270780fa4287dfbb0c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 22:58:44 -0500 Subject: [PATCH] Defer superseded wave downloads, dedupe tool dests, validate captured buckets, right-size the log levels --- esphome/espidf/framework.py | 6 ++++ esphome/framework_helpers.py | 2 +- esphome/platformio/extra_script.py | 18 ++++++++--- esphome/platformio/library.py | 31 +++++++++++++----- tests/unit_tests/test_espidf_framework.py | 20 ++++++++++++ .../test_platformio_extra_script.py | 24 ++++++++++++++ tests/unit_tests/test_platformio_library.py | 32 +++++++++++++++++++ 7 files changed, 120 insertions(+), 13 deletions(-) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index a31a339fee..e012117b4f 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -730,9 +730,15 @@ def _prefetch_idf_tool_archives( return dist_path = get_idf_tools_path() / "dist" entries = [] + seen_dests: set[str] = set() for entry in json.loads(stdout): if (dist_path / entry["dest"]).is_file(): continue + if entry["dest"] in seen_dests: + # Two workers on one .part file would interleave + # seek/truncate writes; mirror the library prefetch's dedupe + continue + seen_dests.add(entry["dest"]) # 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). diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index 4d0574372a..a6fe533a61 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -756,7 +756,7 @@ def run_batch_downloads( 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. + can exit; in-flight ones still finish. ``jobs`` must be non-empty. """ failures: list[tuple[str, Exception]] = [] diff --git a/esphome/platformio/extra_script.py b/esphome/platformio/extra_script.py index 2d5667a3e5..f1847dc959 100644 --- a/esphome/platformio/extra_script.py +++ b/esphome/platformio/extra_script.py @@ -229,8 +229,18 @@ def captured_as_build_flags( generated build files stay portable. """ flags: list[str] = [] + + def _strs(bucket: list, kind: str) -> list[str]: + # Third-party scripts legally append SCons nodes, ints, or dicts; + # stringifying those into flags would hand the compiler garbage + good = [entry for entry in bucket if isinstance(entry, str)] + for entry in bucket: + if not isinstance(entry, str): + _LOGGER.warning("Ignoring unsupported %s entry %r", kind, entry) + return good + library_root = library_dir.resolve() - for path in result.libpath: + for path in _strs(result.libpath, "LIBPATH"): # Anchor relative paths to library_dir; the script's CWD has been # restored by now resolved = (library_dir / path).resolve() @@ -238,7 +248,7 @@ def captured_as_build_flags( flags.append(f"-L{resolved.relative_to(library_root)}") except ValueError: flags.append(f"-L{resolved}") - flags.extend(f"-l{lib}" for lib in result.libs) + flags.extend(f"-l{lib}" for lib in _strs(result.libs, "LIBS")) for define in result.cppdefines: # SCons also accepts dict/list CPPDEFINES; formatting those blind # would hand the compiler garbage like -D{'FOO': '1'} @@ -248,6 +258,6 @@ def captured_as_build_flags( flags.append(f"-D{define}") else: _LOGGER.warning("Ignoring unsupported CPPDEFINES entry %r", define) - flags.extend(result.linkflags) - flags.extend(result.cppflags) + flags.extend(_strs(result.linkflags, "LINKFLAGS")) + flags.extend(_strs(result.cppflags, "CPPFLAGS")) return flags diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 86139d61d7..79933c07e8 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -57,6 +57,9 @@ DEFAULT_BUILD_INCLUDE_DIR = "include" DEFAULT_BUILD_FLAGS = [] # Suffix -> compiler kind (PlatformIO's CSUFFIXES/CXXSUFFIXES/ASSUFFIXES). # "asm" merges SCons's AS and ASPP sets: all compile as assembler-with-cpp. +# The kind values drive the ESP8266 native ninja rules (later in this +# chain); existing backends consume only the keys. Note .C/.C++ join the +# suffix set here, matching PlatformIO's CXXSUFFIXES. SOURCE_KIND_FOR_SUFFIX: dict[str, str] = { ".c": "c", ".cpp": "cxx", @@ -649,6 +652,9 @@ def raise_on_empty_arg_flags(tokens: list[str], owner: str) -> None: """Reject bare ``-I``/``-D``/``-L``/``-l`` tokens left by an empty glued argument (``-D ""``). + Consumed by the ESP8266 native build generator (later in this chain) + for user build_flags; library manifests deliberately stay warn-and-drop. + Lives next to ``join_flag_args`` because the bare token is its postcondition: a trailing bare flag is warned and dropped there, so a surviving one always means an empty argument. gcc would eat the next @@ -683,7 +689,9 @@ def warn_properties_depends(name: str, data: object) -> None: ``library.properties`` spelling would otherwise drop silently. """ if isinstance(data, dict) and not data.get("dependencies") and data.get("depends"): - _LOGGER.warning( + # INFO: common and unactionable for transitive libraries; a WARNING + # on every build would train users to ignore the stream + _LOGGER.info( "Library %s declares dependencies via library.properties " "depends=, which are not resolved automatically; add them with " "add_library() if needed", @@ -965,18 +973,14 @@ def _prefetch_wave( components.append(component) if len(components) < 2: return - _LOGGER.info( - "Downloading %d libraries: %s", - len(components), - ", ".join(c.name for c in components), - ) # One combined bar over the batch, sized by HEAD requests. An unknown # size would mean a silent multi-MB download; fall back to sequential # downloads with their per-file bars instead. sizes = _content_lengths([c.source.url for c in components]) if not all(sizes): - # Name the culprits so the fallback is distinguishable from a hang - _LOGGER.debug( + # Announced before the sequential per-file downloads take over, so + # the fallback is distinguishable from a hang + _LOGGER.info( "No Content-Length for %s; downloading sequentially", ", ".join( c.source.url @@ -985,6 +989,11 @@ def _prefetch_wave( ), ) return + _LOGGER.info( + "Downloading %d libraries: %s", + len(components), + ", ".join(c.name for c in components), + ) def _fetch(component: ConvertedLibrary): return lambda tracker: component.download( @@ -1126,6 +1135,12 @@ def convert_libraries( _prefetch_wave(wave, salt, backend.cache_key) for key, component in wave: node = nodes[key] + if frozenset(node.requirements) != resolved_requirements[key]: + # An earlier wave entry grew this node's requirements after + # the drain resolved it; downloading the superseded version + # would be wasted work, and the next wave re-resolves it + worklist.append(key) + continue component.download(salt=salt, namespace=backend.cache_key) source_dir = component.source_dir diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index b5296def66..351111fd0b 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -940,6 +940,26 @@ def test_prefetch_all_entries_unverifiable_is_a_noop(tmp_path: Path) -> None: download.assert_not_called() +def test_prefetch_dedupes_entries_by_dest(tmp_path: Path) -> None: + """Two entries resolving to one dest would interleave writes into the + same .part file; only the first downloads.""" + entries = json.loads(_PREFETCH_JSON) + dup = dict(entries[0]) | {"name": "cmake-alias@3.30.2"} + entries.append(dup) + 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.BatchDownloadProgress"), + ): + _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) + dests = [call[0][1].name for call in download.call_args_list] + assert dests.count("cmake-3.30.2.tar.gz") == 1 + + def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: with ( patch( diff --git a/tests/unit_tests/test_platformio_extra_script.py b/tests/unit_tests/test_platformio_extra_script.py index 5df8e3c9c5..f7ac268bf4 100644 --- a/tests/unit_tests/test_platformio_extra_script.py +++ b/tests/unit_tests/test_platformio_extra_script.py @@ -169,6 +169,30 @@ def test_apply_extra_script_callable_target_and_str_flags(tmp_path) -> None: assert c.data["build"]["flags"] == ["-DBASE=1", "-lesp8266"] +def test_captured_nonstring_buckets_warn_and_skip(tmp_path, caplog) -> None: + """Non-string LIBS/LINKFLAGS/CPPFLAGS/LIBPATH entries (legal SCons + nodes) are skipped by name instead of stringified into garbage flags.""" + (tmp_path / "src").mkdir() + script = tmp_path / "extra.py" + script.write_text( + "env.Append(LIBS=['m', 42], LINKFLAGS=['-Wl,-x', {'no': 1}], " + "CPPFLAGS=['-Os', 3.5], LIBPATH=['libs', 7])\n" + ) + (tmp_path / "libs").mkdir() + + c = IDFComponent("owner/name", "1.0", source=URLSource("http://dummy")) + c.path = tmp_path + c.data = {"build": {"extraScript": "extra.py"}} + + apply_extra_script(c, board_mcu=lambda: "esp8266", pio_platform="espressif8266") + + flags = c.data["build"]["flags"] + assert "-lm" in flags and "-Wl,-x" in flags and "-Os" in flags + assert not any("42" in f or "no" in f or "3.5" in f for f in flags) + assert "Ignoring unsupported LIBS entry 42" in caplog.text + assert "Ignoring unsupported LIBPATH entry 7" in caplog.text + + def test_captured_dict_cppdefines_warn_and_skip(tmp_path, caplog) -> None: """A dict CPPDEFINES entry (legal SCons) must warn and skip; formatting it blind would hand the compiler -D{'FOO': '1'} garbage.""" diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 4cc5e6ba0a..e1defb0710 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -255,6 +255,37 @@ def _patch_download_with_manifests(monkeypatch, tmp_path, manifests, *, properti _patch_registry_resolve(monkeypatch) +def test_wave_requirement_growth_defers_the_superseded_download(tmp_path, monkeypatch): + """A's manifest constrains B while B sits in the same wave: B's + drain-time resolution is superseded, so its download defers to the + next wave instead of fetching a version that is immediately replaced.""" + download_names: list[str] = [] + manifests = { + "esphome/A": { + "name": "A", + "build": {}, + "dependencies": {"esphome/B": ">=1.0"}, + }, + "esphome/B": {"name": "B", "build": {}}, + } + + def fake_download(self, force=False, salt="", namespace="", progress=None): + download_names.append(self.name) + self.path = tmp_path / self.get_require_name() + self.path.mkdir(parents=True, exist_ok=True) + (self.path / "library.json").write_text(json.dumps(manifests[self.name])) + + monkeypatch.setattr(ConvertedLibrary, "download", fake_download) + _patch_registry_resolve(monkeypatch) + top = convert_libraries( + [Library("esphome/A", "1.0.0", None), Library("esphome/B", None, None)], + _backend(), + ) + assert sorted(c.name for c in top) == ["esphome/A", "esphome/B"] + # B downloads exactly once, after its requirement set stabilized + assert download_names.count("esphome/B") == 1 + + def test_convert_libraries_parses_library_properties(tmp_path, monkeypatch): # A manifest provided as library.properties (Arduino style) instead of # library.json must still be parsed and converted. @@ -814,6 +845,7 @@ def test_walk_warns_for_properties_only_depends( {"esphome/A": "name=A\nversion=1.0\ndepends=Wire, SPI\n"}, properties=("esphome/A",), ) + caplog.set_level("INFO") convert_libraries([Library("esphome/A", "1.0.0", None)], _backend()) assert "declares dependencies via library.properties" in caplog.text