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.
This commit is contained in:
J. Nick Koston
2026-08-22 15:25:37 -05:00
parent ff8f9786e9
commit 040c91259d
4 changed files with 181 additions and 42 deletions
+58 -24
View File
@@ -27,8 +27,10 @@ from esphome.platformio.library import (
LIBRARY_HEADER_SUFFIXES, LIBRARY_HEADER_SUFFIXES,
SRC_FILE_EXTENSIONS, SRC_FILE_EXTENSIONS,
ConvertedLibrary, ConvertedLibrary,
IncompatiblePlatform,
InvalidLibrary, InvalidLibrary,
LibraryBackend, LibraryBackend,
_url_or_none,
check_library_data, check_library_data,
collect_filtered_files, collect_filtered_files,
convert_libraries, convert_libraries,
@@ -219,18 +221,18 @@ def _collect_lib_sources(
len(dropped), len(dropped),
", ".join(sorted(dropped)), ", ".join(sorted(dropped)),
) )
if not lib.sources and not any( if (
Path(f).suffix.lower() in LIBRARY_HEADER_SUFFIXES for f in matched 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 # Matched headers mean a header-only library; a declared filter
# sources yields an empty archive that fails far away at link # matching nothing (or only inert files) is a manifest/tree problem.
if "srcFilter" in build or "srcDir" in build: # The truly empty tree raises via _assert_tree_has_code.
_LOGGER.warning( _LOGGER.warning(
"Library %s declares srcFilter/srcDir but no source files matched", "Library %s declares srcFilter/srcDir but no source files matched",
name, name,
) )
else:
_LOGGER.warning("Library %s has no sources or headers", name)
def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: 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, name,
) )
lib = _library_info(name, lib_dir, data) lib = _library_info(name, lib_dir, data)
if not lib.sources and not any( _assert_tree_has_code(
Path(p).suffix.lower() in LIBRARY_HEADER_SUFFIXES for p in walk_files(lib_dir) name,
): lib_dir,
# An empty or half-extracted bundled directory can never link; a "the framework install may be incomplete (run 'esphome clean-all')",
# 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')"
)
return lib 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: def _external_short_name(name: str) -> str:
"""The short library name of a requested spec. """The short library name of a requested spec.
@@ -411,6 +420,10 @@ def resolve_libraries(
continue continue
if name in bundled_names or is_lib_ignored(name, lib_ignore): if name in bundled_names or is_lib_ignored(name, lib_ignore):
continue 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): if dep.get("owner") or not _provided(name):
# Owner-less names in the framework tree prefer the bundled # Owner-less names in the framework tree prefer the bundled
# copy (PIO's process_dependencies); everything else resolves # copy (PIO's process_dependencies); everything else resolves
@@ -421,9 +434,19 @@ def resolve_libraries(
# mismatch; re-checking would warn twice # mismatch; re-checking would warn twice
check_library_data(dep, pio_platform, None) check_library_data(dep, pio_platform, None)
except InvalidLibrary as err: except InvalidLibrary as err:
# The shared walk already reported any non-platform cause; if isinstance(err, IncompatiblePlatform) or "version" not in dep:
# warning again here would read as two distinct failures # The platform skip is routine; the walk's version-less
_LOGGER.debug("Skip bundled candidate %s: %s", name, err) # 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 continue
# Deferred: a later-emitted library's manifest name may satisfy # Deferred: a later-emitted library's manifest name may satisfy
# this; adding now could double the archive # this; adding now could double the archive
@@ -433,6 +456,11 @@ def resolve_libraries(
apply_extra_script( apply_extra_script(
component, board_mcu=lambda: board_mcu, pio_platform=pio_platform 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): if isinstance(manifest_name := component.data.get("name"), str):
converted_manifest_names.add(manifest_name) converted_manifest_names.add(manifest_name)
converted.append( converted.append(
@@ -456,7 +484,13 @@ def resolve_libraries(
), ),
) )
for name in pending_bundled: 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 continue
bundled_names.add(name) bundled_names.add(name)
bundled.append(_bundled_library(framework_path, name)) bundled.append(_bundled_library(framework_path, name))
+21 -5
View File
@@ -65,16 +65,32 @@ def _run_ar(ar: str, archive: str, rspfile: str) -> int:
def _run_copy(src: str, dst: 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 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: def main() -> int:
mode = sys.argv[1] mode = sys.argv[1]
if mode == "ar": if entry := _MODES.get(mode):
return _run_ar(*sys.argv[2:5]) handler, argc = entry
if mode == "copy": args = sys.argv[2:]
return _run_copy(*sys.argv[2:4]) 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) print(f"unknown build_tool mode: {mode}", file=sys.stderr)
return 1 return 1
@@ -176,3 +176,31 @@ def test_ar_batch_failure_stops(tmp_path: Path) -> None:
assert mock_run.call_count == 1 assert mock_run.call_count == 1
# The failed batch must not leave a truncated archive behind # The failed batch must not leave a truncated archive behind
assert not archive.exists() 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()
+74 -13
View File
@@ -86,6 +86,7 @@ def _webserver(tmp_path: Path, data: dict) -> ConvertedLibrary:
_add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6")
lib_dir = tmp_path / "converted" / "webserver" lib_dir = tmp_path / "converted" / "webserver"
(lib_dir / "src").mkdir(parents=True) (lib_dir / "src").mkdir(parents=True)
(lib_dir / "src" / "server.cpp").write_text("")
return _converted("esp32async__ESPAsyncWebServer", lib_dir, data) 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.""" """Build ESPAsyncWebServer (depending on ESPAsyncTCP) plus resolved TCP."""
ws_dir = tmp_path / "converted" / "webserver" ws_dir = tmp_path / "converted" / "webserver"
(ws_dir / "src").mkdir(parents=True) (ws_dir / "src").mkdir(parents=True)
(ws_dir / "src" / "server.cpp").write_text("")
tcp_dir = tmp_path / "converted" / "tcp" tcp_dir = tmp_path / "converted" / "tcp"
(tcp_dir / "src").mkdir(parents=True) (tcp_dir / "src").mkdir(parents=True)
(tcp_dir / "src" / "tcp.cpp").write_text("")
ws = _converted( ws = _converted(
"esp32async__ESPAsyncWebServer", "esp32async__ESPAsyncWebServer",
ws_dir, ws_dir,
@@ -182,22 +185,26 @@ def test_library_info_declared_filter_matches_nothing_warns(
) -> None: ) -> None:
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
data = {"build": {"srcFilter": ["+<nothing/*>"]}} data = {"build": {"srcFilter": ["+<nothing/*>"]}}
lib = component._library_info("x", read_path, data) lib = component._library_info("x", read_path, data)
assert not lib.sources assert not lib.sources
assert "declares srcFilter/srcDir but no source files matched" in caplog.text assert "declares srcFilter/srcDir but no source files matched" in caplog.text
def test_library_info_empty_tree_warns( def test_empty_converted_tree_raises_at_emit(tmp_path: Path) -> None:
tmp_path: Path, caplog: pytest.LogCaptureFixture """A converted tree with no sources and no headers is a broken download;
) -> None: fail by name like the bundled case."""
"""No sources and no headers is an empty archive waiting to fail at framework = _make_framework(tmp_path)
link; warn by name even without a declared filter.""" _add_library("Some/Empty", "1.0.0")
read_path = tmp_path / "lib" lib_dir = tmp_path / "converted" / "empty"
(read_path / "src").mkdir(parents=True) (lib_dir / "src").mkdir(parents=True)
lib = component._library_info("x", read_path, {}) converted = _converted("some__Empty", lib_dir, {"build": {}})
assert not lib.sources with (
assert "has no sources or headers" in caplog.text _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: 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 = tmp_path / "converted" / "external"
lib_dir.mkdir(parents=True) lib_dir.mkdir(parents=True)
(lib_dir / "main.cpp").write_text("")
converted = _converted( converted = _converted(
"some__External", lib_dir, {"dependencies": [{"name": "Wire"}]} "some__External", lib_dir, {"dependencies": [{"name": "Wire"}]}
) )
@@ -291,6 +299,7 @@ def test_library_info_trailing_bare_flag_warns(
) -> None: ) -> None:
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (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"]}}) lib = component._library_info("x", read_path, {"build": {"flags": ["-DA=1 -l"]}})
assert lib.flags == ["-DA=1"] assert lib.flags == ["-DA=1"]
assert lib.link_libs == [] assert lib.link_libs == []
@@ -302,6 +311,7 @@ def test_library_info_missing_explicit_include_warns(
) -> None: ) -> None:
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
lib = component._library_info("x", read_path, {"build": {"flags": ["-Inope"]}}) lib = component._library_info("x", read_path, {"build": {"flags": ["-Inope"]}})
assert lib.include_dirs == [(read_path / "src").resolve()] assert lib.include_dirs == [(read_path / "src").resolve()]
assert "include dir nope which does not exist" in caplog.text 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 = tmp_path / "converted" / "external"
lib_dir.mkdir(parents=True) lib_dir.mkdir(parents=True)
(lib_dir / "main.cpp").write_text("")
converted = _converted( converted = _converted(
"some__External", lib_dir, {"dependencies": [{"name": "Wire"}]} "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.""" the generator's contract; default is archive."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (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("x", read_path, {}).lib_archive is True
assert ( assert (
component._library_info( component._library_info(
@@ -460,6 +472,47 @@ def test_nonplatform_rejection_warns_once_through_real_converter(
assert caplog.text.count("manifest is corrupt") == 1 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( def test_short_name_collision_with_bundled_name_warns(
tmp_path: Path, caplog: pytest.LogCaptureFixture tmp_path: Path, caplog: pytest.LogCaptureFixture
) -> None: ) -> None:
@@ -473,6 +526,7 @@ def test_short_name_collision_with_bundled_name_warns(
{"build": {}, "dependencies": [{"name": "Wire"}]}, {"build": {}, "dependencies": [{"name": "Wire"}]},
) )
(tmp_path / "conv" / "src").mkdir(parents=True) (tmp_path / "conv" / "src").mkdir(parents=True)
(tmp_path / "conv" / "src" / "a.cpp").write_text("")
with _emitting_converter(converted): with _emitting_converter(converted):
libs = _resolve(framework) libs = _resolve(framework)
assert "Wire" not in [lib.name for lib in libs] 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.""" """A declared-but-falsy srcDir must not silently fall back to the probe."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
with pytest.raises(EsphomeError, match="does not exist"): with pytest.raises(EsphomeError, match="does not exist"):
component._library_info("x", read_path, {"build": {"srcDir": declared}}) 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.""" """bool("false") is True; the string forms must parse, not coerce."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
lib = component._library_info("x", read_path, {"build": {"libArchive": value}}) lib = component._library_info("x", read_path, {"build": {"libArchive": value}})
assert lib.lib_archive is expected 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.""" """precompiled/ldflags properties are not honored; the drop is named."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
component._library_info( component._library_info(
"x", read_path, {"precompiled": "true", "ldflags": "-lfoo", "build": {}} "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.""" """A typo'd libArchive fails by name like the other build fields."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
with pytest.raises(EsphomeError, match="malformed libArchive value 'archive-me'"): with pytest.raises(EsphomeError, match="malformed libArchive value 'archive-me'"):
component._library_info("x", read_path, {"build": {"libArchive": "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.""" """Malformed includeDir/srcFilter fail naming the library like srcDir."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
with pytest.raises(EsphomeError, match=match): with pytest.raises(EsphomeError, match=match):
component._library_info("x", read_path, {"build": build}) 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.""" """The dot_a_linkage property uses the same strict table as libArchive."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (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": {}}) lib = component._library_info("x", read_path, {"dot_a_linkage": value, "build": {}})
assert lib.lib_archive is expected 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.""" """A typo'd dot_a_linkage must not silently flip link semantics."""
read_path = tmp_path / "lib" read_path = tmp_path / "lib"
(read_path / "src").mkdir(parents=True) (read_path / "src").mkdir(parents=True)
(read_path / "src" / "stub.cpp").write_text("")
with pytest.raises(EsphomeError, match="malformed dot_a_linkage value 'yes'"): with pytest.raises(EsphomeError, match="malformed dot_a_linkage value 'yes'"):
component._library_info("x", read_path, {"dot_a_linkage": "yes", "build": {}}) 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") _add_library("Someone/WireLib", "9.9.9")
ws_dir = tmp_path / "converted" / "webserver" ws_dir = tmp_path / "converted" / "webserver"
(ws_dir / "src").mkdir(parents=True) (ws_dir / "src").mkdir(parents=True)
(ws_dir / "src" / "stub.cpp").write_text("")
wire_dir = tmp_path / "converted" / "wire" wire_dir = tmp_path / "converted" / "wire"
(wire_dir / "src").mkdir(parents=True) (wire_dir / "src").mkdir(parents=True)
(wire_dir / "src" / "wire.cpp").write_text("")
ws = _converted( ws = _converted(
"esp32async__ESPAsyncWebServer", "esp32async__ESPAsyncWebServer",
ws_dir, ws_dir,
@@ -939,9 +1002,7 @@ def test_empty_bundled_library_warns(
framework = _make_framework(tmp_path) framework = _make_framework(tmp_path)
(framework / "libraries" / "Empty").mkdir() (framework / "libraries" / "Empty").mkdir()
_add_library("Empty", None) _add_library("Empty", None)
with pytest.raises( with pytest.raises(EsphomeError, match="Library Empty has no sources or headers"):
EsphomeError, match="Bundled library Empty has no sources or headers"
):
_resolve(framework) _resolve(framework)