From 8b8ef9bc6105a87844aa7b99353d9b0265a12669 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 20:00:22 -0500 Subject: [PATCH 1/2] Clean up the archive on any ar failure; state the link-order contract plainly --- esphome/arduino/library.py | 6 ++-- esphome/build_gen/build_tool.py | 34 +++++++++++-------- tests/unit_tests/build_gen/test_build_tool.py | 26 ++++++++++++++ 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/esphome/arduino/library.py b/esphome/arduino/library.py index 5ed263eee6..456a8ff0d1 100644 --- a/esphome/arduino/library.py +++ b/esphome/arduino/library.py @@ -326,9 +326,9 @@ def resolve_libraries( for that core (e.g. ``espressif8266``/``esp8266``); ``cache_key`` keys the shared converter's download cache. - The returned order is unordered with respect to link dependencies - (bundled dependencies precede their dependents); the caller must link - the archives inside one ``--start-group``/``--end-group`` pair. + The returned list is not topologically sorted, so the caller must link + the archives inside one ``--start-group``/``--end-group`` pair (the + bundled-first grouping is incidental). """ bundled: list[ArduinoLibrary] = [] external: list[Library] = [] diff --git a/esphome/build_gen/build_tool.py b/esphome/build_gen/build_tool.py index d2497acce6..eada061c14 100644 --- a/esphome/build_gen/build_tool.py +++ b/esphome/build_gen/build_tool.py @@ -47,21 +47,27 @@ def _run_ar(ar: str, archive: str, rspfile: str) -> int: # 32767-char command-line limit it existed to avoid. "rc" creates, # "q" appends the remainder. op = "rc" - while objects: - batch = [objects.pop(0)] - batch_len = len(batch[0]) - while objects and batch_len + len(objects[0]) < 25000: - batch_len += len(objects[0]) + 1 - batch.append(objects.pop(0)) - rc = subprocess.run( - [ar, op, archive, *batch], check=False, close_fds=False - ).returncode - if rc != 0: - # A failed batch must not leave a truncated archive behind + ok = False + try: + while objects: + batch = [objects.pop(0)] + batch_len = len(batch[0]) + while objects and batch_len + len(objects[0]) < 25000: + batch_len += len(objects[0]) + 1 + batch.append(objects.pop(0)) + rc = subprocess.run( + [ar, op, archive, *batch], check=False, close_fds=False + ).returncode + if rc != 0: + return rc + op = "q" + ok = True + return 0 + finally: + if not ok: + # Any failure (bad exit, missing ar binary, interrupt) must not + # leave a truncated archive behind Path(archive).unlink(missing_ok=True) - return rc - op = "q" - return 0 def _run_copy(src: str, dst: str) -> int: diff --git a/tests/unit_tests/build_gen/test_build_tool.py b/tests/unit_tests/build_gen/test_build_tool.py index 05bc2483de..ba47d0a9b7 100644 --- a/tests/unit_tests/build_gen/test_build_tool.py +++ b/tests/unit_tests/build_gen/test_build_tool.py @@ -179,6 +179,32 @@ def test_ar_batch_failure_stops(tmp_path: Path) -> None: assert not archive.exists() +def test_ar_exception_leaves_no_partial_archive(tmp_path: Path) -> None: + """A missing ar binary mid-loop must not leave a truncated archive from + earlier successful batches.""" + archive = tmp_path / "lib.a" + rsp = tmp_path / "lib.a.rsp" + rsp.write_text("a.o\n") + with ( + patch.object( + build_tool.sys, + "argv", + ["build_tool", "ar", "ar-bin", str(archive), str(rsp)], + ), + patch.object( + build_tool.subprocess, + "run", + side_effect=lambda cmd, **kw: ( + archive.write_text("partial"), + (_ for _ in ()).throw(FileNotFoundError("no ar")), + ), + ), + pytest.raises(FileNotFoundError), + ): + build_tool.main() + 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.""" From 2acba9b3642900dbc42520abca70abeab2ab5cbd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 20:01:13 -0500 Subject: [PATCH 2/2] Materialize the tracker mock before the prefetch threads race its creation --- tests/unit_tests/test_espidf_framework.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index 70091fbf13..b51d535586 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -950,6 +950,8 @@ def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: patch("esphome.espidf.framework.get_system_python_path", return_value="python"), patch("esphome.espidf.framework.BatchDownloadProgress") as progress_cls, ): + # Materialize the lazy mock before threads race its first creation + tracker = progress_cls.return_value.tracker.return_value _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None) dist = get_idf_tools_path() / "dist" @@ -964,7 +966,6 @@ def test_prefetch_downloads_each_archive_with_resume(tmp_path: Path) -> None: assert kwargs["size"] == 123 # every archive reports into the one combined progress bar 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())