From 21fd74b2014c3bdb6c29bbb129702cc3da9f54fc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 25 Aug 2026 23:42:13 -0500 Subject: [PATCH] Tighten the review-round edges Only IncompatiblePlatform counts as a knowing skip; other malformed manifest data stays out of the satisfied set so the reconciliation can still flag a broken promise. Backend-provided names are recorded before the manifest-name evidence so the overlap warns once, in the backend's own suppression loop. ar runs rcs/qs so the symbol index is explicit, and the copy shim prints and returns 1 like its siblings instead of re-raising. --- esphome/arduino/library.py | 8 ++++- esphome/build_gen/build_tool.py | 11 ++++--- esphome/platformio/library.py | 20 ++++++----- tests/unit_tests/build_gen/test_build_tool.py | 33 +++++++++---------- 4 files changed, 40 insertions(+), 32 deletions(-) diff --git a/esphome/arduino/library.py b/esphome/arduino/library.py index dd1b738ac6..e224e62589 100644 --- a/esphome/arduino/library.py +++ b/esphome/arduino/library.py @@ -28,6 +28,7 @@ from esphome.platformio.library import ( LIBRARY_HEADER_SUFFIXES, SRC_FILE_EXTENSIONS, ConvertedLibrary, + IncompatiblePlatform, InvalidLibrary, LibraryBackend, _url_or_none, @@ -457,11 +458,16 @@ def resolve_libraries( # causes; debug keeps one fault from warning twice (pinned # by test_nonplatform_rejection_warns_once_through_real_converter) check_library_data(dep, pio_platform, None) - except InvalidLibrary as err: + except IncompatiblePlatform as err: # A knowing skip (platform filter), not a broken promise knowingly_skipped.add(name) _LOGGER.debug("Skip bundled candidate %s: %s", name, err) continue + except InvalidLibrary as err: + # Malformed manifest data never counts as satisfied; the + # walk owns the warning (see the warns-once test above) + _LOGGER.debug("Skip malformed bundled candidate %s: %s", name, err) + continue # Deferred: a later manifest name may satisfy this pending_bundled.setdefault(name) diff --git a/esphome/build_gen/build_tool.py b/esphome/build_gen/build_tool.py index eada061c14..a408b1586b 100644 --- a/esphome/build_gen/build_tool.py +++ b/esphome/build_gen/build_tool.py @@ -44,9 +44,9 @@ def _run_ar(ar: str, archive: str, rspfile: str) -> int: print(f"ar: no objects listed in {rspfile} for {archive}", file=sys.stderr) return 1 # Batch by argv length: expanding the rspfile gives back the Windows - # 32767-char command-line limit it existed to avoid. "rc" creates, - # "q" appends the remainder. - op = "rc" + # 32767-char command-line limit it existed to avoid. "rcs" creates, + # "qs" appends; the s keeps the symbol index explicit on every ar. + op = "rcs" ok = False try: while objects: @@ -60,7 +60,7 @@ def _run_ar(ar: str, archive: str, rspfile: str) -> int: ).returncode if rc != 0: return rc - op = "q" + op = "qs" ok = True return 0 finally: @@ -78,7 +78,8 @@ def _run_copy(src: str, dst: str) -> int: # SameFileError means dst IS src, where unlinking destroys the input if not isinstance(err, shutil.SameFileError): Path(dst).unlink(missing_ok=True) - raise + print(f"copy: {src} -> {dst} failed: {err}", file=sys.stderr) + return 1 return 0 diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 20211a9fc0..f4136a7b36 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -943,6 +943,17 @@ def _reconcile_versionless_skips( if dep_name in components: # A version-less dep's request key is the name itself continue + if ( + not dep_owner + and backend.provides is not None + and backend.provides(dep_name) + ): + # provides() only satisfies owner-less names (same guard as + # the walk's skip); record for the post-emit reconciliation. + # Checked before the manifest-name evidence so the overlap + # case warns once, in the backend's own suppression loop + backend.provided_requests.add(dep_name) + continue if dep_name in resolved_manifest_names: # Name-only evidence: a coincidental collision must stay # visible where the user could pin it @@ -954,15 +965,6 @@ def _reconcile_versionless_skips( requester, ) continue - if ( - not dep_owner - and backend.provides is not None - and backend.provides(dep_name) - ): - # provides() only satisfies owner-less names (same guard as - # the walk's skip); record for the post-emit reconciliation - backend.provided_requests.add(dep_name) - continue warned.add(dep_name) log( "Dependency %s of %s has no version to resolve and nothing " diff --git a/tests/unit_tests/build_gen/test_build_tool.py b/tests/unit_tests/build_gen/test_build_tool.py index ba47d0a9b7..b029c647ab 100644 --- a/tests/unit_tests/build_gen/test_build_tool.py +++ b/tests/unit_tests/build_gen/test_build_tool.py @@ -3,7 +3,6 @@ from __future__ import annotations from pathlib import Path -import shutil import subprocess import sys from unittest.mock import MagicMock, patch @@ -31,7 +30,7 @@ def test_ar_removes_stale_archive(tmp_path: Path) -> None: assert build_tool.main() == 0 assert not archive.exists() # The rspfile is expanded by the shim (GNU ar would escape backslashes) - assert mock_run.call_args[0][0] == ["ar-bin", "rc", str(archive), "a.o"] + assert mock_run.call_args[0][0] == ["ar-bin", "rcs", str(archive), "a.o"] def test_copy(tmp_path: Path) -> None: @@ -83,7 +82,7 @@ def test_ar_expands_rspfile_without_escaping(tmp_path) -> None: assert build_tool.main() == 0 assert mock_run.call_args[0][0] == [ "ar-bin", - "rc", + "rcs", str(tmp_path / "lib.a"), "obj/a.o", "sub\\b.o", @@ -106,7 +105,7 @@ def test_ar_unquotes_ninja_escaped_paths(tmp_path: Path) -> None: assert rc == 0 assert mock_run.call_args.args[0] == [ "/usr/bin/ar", - "rc", + "rcs", "lib.a", "obj/a b.o", "obj/c.o", @@ -129,7 +128,7 @@ def test_ar_empty_object_list_fails( def test_ar_batches_long_object_lists(tmp_path: Path) -> None: """The expanded argv must stay under the Windows 32767-char limit: a - long object list creates with rc, then appends with q.""" + long object list creates with rcs, then appends with qs.""" archive = tmp_path / "lib.a" rsp = tmp_path / "lib.a.rsp" objects = [f"dir/{'x' * 120}_{i}.o" for i in range(400)] @@ -147,8 +146,8 @@ def test_ar_batches_long_object_lists(tmp_path: Path) -> None: assert build_tool.main() == 0 calls = [c[0][0] for c in mock_run.call_args_list] assert len(calls) > 1 - assert calls[0][1] == "rc" - assert all(c[1] == "q" for c in calls[1:]) + assert calls[0][1] == "rcs" + assert all(c[1] == "qs" for c in calls[1:]) assert [o for c in calls for o in c[3:]] == objects assert all(sum(len(a) + 1 for a in c) < 32000 for c in calls) @@ -215,18 +214,19 @@ def test_surplus_arguments_error(capsys: pytest.CaptureFixture[str]) -> None: assert "expected 2 arguments, got 3" in capsys.readouterr().err -def test_copy_same_file_keeps_the_input(tmp_path: Path) -> None: - """A same-file copy (dst IS src) must not unlink the input.""" +def test_copy_same_file_keeps_the_input( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A same-file copy (dst IS src) must not unlink the input, and fails + with a message and exit code like the other shim paths.""" src = tmp_path / "firmware.bin" src.write_bytes(b"image") - with ( - patch.object( - build_tool.sys, "argv", ["build_tool", "copy", str(src), str(src)] - ), - pytest.raises(shutil.SameFileError), + with patch.object( + build_tool.sys, "argv", ["build_tool", "copy", str(src), str(src)] ): - build_tool.main() + assert build_tool.main() == 1 assert src.read_bytes() == b"image" + assert "failed" in capsys.readouterr().err def test_copy_failure_leaves_no_partial_output(tmp_path: Path) -> None: @@ -241,7 +241,6 @@ def test_copy_failure_leaves_no_partial_output(tmp_path: Path) -> None: "argv", ["build_tool", "copy", str(tmp_path / "src.bin"), str(dst)], ), - pytest.raises(OSError), ): - build_tool.main() + assert build_tool.main() == 1 assert not dst.exists()