From 040c91259da0600bbe6972bc9499b861247621d3 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 15:25:37 -0500 Subject: [PATCH] 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)