From 3b2fc9f59aaa3f95ebda485c6f549e9578f0fbba Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 11:24:49 -0500 Subject: [PATCH] Close the owner-qualified drop gap and the duplicate-archive gap in the library backend An owner-qualified version-less dependency was skipped by both the arduino backend (owner set) and the reconciliation (provides() knew the short name), so nothing logged the drop; provides() now only satisfies owner-less names, mirroring the walk's backend-provided guard. A dict dependency entry with no name at all now warns in the normalizer like any other malformed entry. Bundled dependency additions are deferred until conversion finishes and skipped when a converted library's manifest already provides the name, so a name that is both bundled and registry-resolved cannot build twice. External short names come from the request spec (custom Name=url form included) instead of the URL tail. The empty-bundled-library probe now walks the whole library tree against a shared header-suffix constant, and the dead versioned-dependency branch is folded into the surviving check. --- esphome/arduino/library.py | 57 ++++++++++++----- esphome/platformio/library.py | 24 +++++-- tests/unit_tests/test_arduino_library.py | 64 ++++++++++++++++++- tests/unit_tests/test_platformio_library.py | 69 ++++++++++++++++++--- 4 files changed, 182 insertions(+), 32 deletions(-) diff --git a/esphome/arduino/library.py b/esphome/arduino/library.py index fabbe6d142..5d7712e4ad 100644 --- a/esphome/arduino/library.py +++ b/esphome/arduino/library.py @@ -35,6 +35,7 @@ from esphome.platformio.extra_script import apply_extra_script from esphome.platformio.library import ( DEFAULT_BUILD_INCLUDE_DIR, DEFAULT_BUILD_SRC_FILTER, + HEADER_FILE_EXTENSIONS, SRC_FILE_EXTENSIONS, ConvertedLibrary, LibraryBackend, @@ -253,9 +254,7 @@ def _bundled_library(framework_path: Path, name: str) -> ArduinoLibrary: ) lib = _library_info(name, lib_dir, data) if not lib.sources and not any( - Path(p).suffix in (".h", ".hpp", ".hh", ".inc") - for d in lib.include_dirs - for p in walk_files(d) + Path(p).suffix.lower() in HEADER_FILE_EXTENSIONS 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 @@ -266,6 +265,19 @@ def _bundled_library(framework_path: Path, name: str) -> ArduinoLibrary: return lib +def _external_short_name(name: str) -> str: + """The short library name of a requested spec. + + "owner/Name" and plain names take the last path segment; the + "Name=" custom-name form takes the declared name (the URL tail is + a repository path, not a library name). + """ + head, sep, tail = name.partition("=") + if sep and "://" in tail: + return head + return name.rsplit("/", maxsplit=1)[-1] + + def resolve_libraries( framework_path: Path, *, pio_platform: str, board_mcu: str, cache_key: str ) -> list[ArduinoLibrary]: @@ -313,10 +325,15 @@ def resolve_libraries( converted: list[ArduinoLibrary] = [] bundled_names = {lib.name for lib in bundled} + converted_manifest_names: set[str] = set() + # Ordered set of bundled dependency names to add once conversion is done + pending_bundled: dict[str, None] = {} # Short names of the separately-requested externals: a manifest - # dependency matching one is already in the build, not a drop (a false - # "skipping" warning teaches users to ignore the real one) - external_short_names = {lib.name.split("/")[-1] for lib in external if lib.name} + # dependency matching one is already in the build, not a bundled name to + # add (a duplicate archive shows up as duplicate-symbol link errors) + external_short_names = { + _external_short_name(lib.name) for lib in external if lib.name + } def _add_bundled_dependencies(component: ConvertedLibrary) -> None: # A version-less bare-name dependency ("Hash" in ESPAsyncWebServer) @@ -342,26 +359,27 @@ def resolve_libraries( or is_lib_ignored(name, lib_ignore) ): continue - if "version" in dep and (dep.get("owner") or not _provided(name)): - # The converter resolves versioned deps from the registry. An - # owner-less versioned name that exists in the framework tree - # ({"Wire": "*"} normalizes to version="*") falls through to - # the bundled path below, matching PlatformIO's - # process_dependencies preference for bundled builders. - continue if dep.get("owner") or not _provided(name): - # The shared walk's post-emit reconciliation reports drops - # (it alone knows the final resolution set); nothing to add + # The converter resolves owner-qualified and non-bundled + # names from the registry; an owner-less name that exists in + # the framework tree prefers the bundled copy ({"Wire": "*"} + # normalizes to version="*"), matching PlatformIO's + # process_dependencies. The shared walk's post-emit + # reconciliation reports any real drops. continue if not dependency_is_usable(dep, pio_platform, "arduino", component.name): continue - bundled_names.add(name) - bundled.append(_bundled_library(framework_path, name)) + # Deferred: a later-emitted converted library may satisfy this + # name (its manifest name is only known at its own emit), and + # adding the bundled copy too would double the archive + pending_bundled.setdefault(name) def _emit(component: ConvertedLibrary) -> None: apply_extra_script( component, board_mcu=lambda: board_mcu, pio_platform=pio_platform ) + if isinstance(manifest_name := component.data.get("name"), str): + converted_manifest_names.add(manifest_name) converted.append( _library_info( component.get_require_name(), component.source_dir, component.data @@ -387,5 +405,10 @@ def resolve_libraries( provides=_provided, ), ) + for name in pending_bundled: + if name in converted_manifest_names or name in bundled_names: + continue + bundled_names.add(name) + bundled.append(_bundled_library(framework_path, name)) return bundled + converted diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index db25e5309c..110ab1483e 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -67,6 +67,11 @@ SOURCE_KIND_FOR_SUFFIX: dict[str, str] = { ".ASM": "asm", } SRC_FILE_EXTENSIONS = list(SOURCE_KIND_FOR_SUFFIX) +# Suffixes that count as headers when probing whether a library has any +# usable files at all (compare against Path.suffix.lower()) +HEADER_FILE_EXTENSIONS = frozenset( + {".h", ".hpp", ".hh", ".hxx", ".inc", ".ipp", ".tcc"} +) DOMAIN = "pio_components" @@ -683,7 +688,7 @@ def normalize_dependencies( for entry in dependencies: if isinstance(entry, dict): name = entry.get("name") - if "name" in entry and (not isinstance(name, str) or not name): + if not isinstance(name, str) or not name: # A dependency name must be a non-empty string; every # consumer indexes or joins it _LOGGER.warning( @@ -929,7 +934,7 @@ def convert_libraries( resolved_requirements: dict[str, frozenset[str]] = {} top_level_keys = set(top_level) # (name, requester) pairs reconciled against the final resolution set - skipped_versionless: list[tuple[Any, str]] = [] + skipped_versionless: list[tuple[Any, Any, str]] = [] worklist = deque(dict.fromkeys(top_level)) while worklist: key = worklist.popleft() @@ -1029,7 +1034,9 @@ def convert_libraries( dependency.get("name"), component.name, ) - skipped_versionless.append((dependency.get("name"), component.name)) + skipped_versionless.append( + (dependency.get("name"), dependency.get("owner"), component.name) + ) continue if not dependency_is_usable( dependency, backend.platform, backend.framework, component.name @@ -1126,7 +1133,7 @@ def convert_libraries( # surface as link errors far from the cause. resolved_manifest_names = {c.data.get("name") for c in components.values()} warned: set[str] = set() - for dep_name, requester in skipped_versionless: + for dep_name, dep_owner, requester in skipped_versionless: if not isinstance(dep_name, str) or not dep_name or dep_name in warned: continue if dep_name in components: @@ -1134,7 +1141,14 @@ def convert_libraries( continue if dep_name in resolved_manifest_names: continue - if backend.provides is not None and backend.provides(dep_name): + if ( + not dep_owner + and backend.provides is not None + and backend.provides(dep_name) + ): + # provides() only satisfies owner-less names: the walk's + # backend-provided skip has the same owner guard, so an + # owner-qualified version-less dep was added by nobody continue warned.add(dep_name) _LOGGER.warning( diff --git a/tests/unit_tests/test_arduino_library.py b/tests/unit_tests/test_arduino_library.py index 0d3ed004e7..42e1bfc1c9 100644 --- a/tests/unit_tests/test_arduino_library.py +++ b/tests/unit_tests/test_arduino_library.py @@ -387,8 +387,8 @@ def test_library_info_lib_archive_flag(tmp_path: Path) -> None: def test_resolve_libraries_dep_warnings( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """A nameless dependency entry warns; an owner-without-version entry is - left to the shared walk's reconciliation (no local warning).""" + """A nameless dependency entry warns in the shared normalizer; an + owner-without-version entry is left to the walk's reconciliation.""" framework = _make_framework(tmp_path) converted = _webserver( tmp_path, @@ -402,7 +402,7 @@ def test_resolve_libraries_dep_warnings( ) with _emitting_converter(converted): _resolve(framework) - assert "malformed dependency entry" in caplog.text + assert "Ignoring unrecognized dependency entry" in caplog.text assert "Orphan" not in caplog.text @@ -735,6 +735,64 @@ def test_transitively_resolved_dependency_does_not_warn( assert "Skipping" not in caplog.text +@pytest.mark.parametrize( + ("spec", "expected"), + [ + ("owner/Name", "Name"), + ("Name", "Name"), + ("Foo=file:///srv/Wire", "Foo"), + ("Foo=https://github.com/x/Wire", "Foo"), + # An "=" without a URL is a registry name, not the custom-name form + ("FOO=BAR", "FOO=BAR"), + ("https://github.com/x/Wire", "Wire"), + ], +) +def test_external_short_name(spec: str, expected: str) -> None: + assert component._external_short_name(spec) == expected + + +def test_converted_manifest_name_suppresses_bundled_dependency( + tmp_path: Path, +) -> None: + """A dependency name a converted library's manifest provides is not + also added from the framework tree (a duplicate archive would surface + as duplicate-symbol link errors), even when the provider emits later.""" + framework = _make_framework(tmp_path) + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + # Requested under a different short name; only the manifest says "Wire" + _add_library("Someone/WireLib", "9.9.9") + ws_dir = tmp_path / "converted" / "webserver" + (ws_dir / "src").mkdir(parents=True) + wire_dir = tmp_path / "converted" / "wire" + (wire_dir / "src").mkdir(parents=True) + ws = _converted( + "esp32async__ESPAsyncWebServer", + ws_dir, + {"build": {}, "dependencies": [{"name": "Wire"}]}, + ) + registry_wire = _converted( + "someone__WireLib", wire_dir, {"name": "Wire", "build": {}} + ) + with _emitting_converter(ws, registry_wire): + libs = _resolve(framework) + # The bundled Wire is not added alongside the registry-resolved one + assert [lib.name for lib in libs] == [ + "esp32async__ESPAsyncWebServer", + "someone__WireLib", + ] + + +def test_bundled_library_root_headers_pass_the_probe(tmp_path: Path) -> None: + """Headers anywhere in the bundled tree (uncommon suffixes and case + included) prove the install is intact, even with an empty src dir.""" + framework = _make_framework(tmp_path) + lib_dir = framework / "libraries" / "HeaderOnly" + (lib_dir / "src").mkdir(parents=True) + (lib_dir / "impl.HXX").write_text("") + lib = component._bundled_library(framework, "HeaderOnly") + assert lib.sources == [] + + def test_empty_bundled_library_warns( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index c3a7da6091..67acebe505 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -26,9 +26,13 @@ from esphome.platformio.library import ( ) -def _backend(emit=lambda component: None) -> LibraryBackend: +def _backend(emit=lambda component: None, provides=None) -> LibraryBackend: return LibraryBackend( - platform="espressif32", framework="espidf", emit=emit, cache_key="idf" + platform="espressif32", + framework="espidf", + emit=emit, + cache_key="idf", + provides=provides, ) @@ -596,11 +600,14 @@ def test_normalize_dependencies_forms(caplog) -> None: """Every PIO-legal spelling normalizes; unrecognizable entries warn.""" from esphome.platformio.library import normalize_dependencies - assert normalize_dependencies(["Wire", {"name": "SPI"}, 5, ""], "libx") == [ + assert normalize_dependencies( + ["Wire", {"name": "SPI"}, 5, "", {"version": "1.0"}], "libx" + ) == [ {"name": "Wire"}, {"name": "SPI"}, ] - assert caplog.text.count("unrecognized dependency entry") == 2 + # The int, the empty string, and the nameless dict all warn + assert caplog.text.count("unrecognized dependency entry") == 3 # A plain string is names, never iterated into characters assert normalize_dependencies("Wire, SPI") == [ {"name": "Wire"}, @@ -630,12 +637,20 @@ def test_versionless_dependency_without_provider_warns( _patch_download_with_manifests( monkeypatch, tmp_path, - {"esphome/A": {"name": "A", "dependencies": [{"name": "Hash"}]}}, + { + "esphome/A": { + "name": "A", + # The duplicate entry warns once (reconciliation dedup) + "dependencies": [{"name": "Hash"}, {"name": "Hash"}], + } + }, ) convert_libraries([Library("esphome/A", None, None)], _backend()) assert ( - "Hash of esphome/A has no version to resolve and nothing provides it" - in caplog.text + caplog.text.count( + "Hash of esphome/A has no version to resolve and nothing provides it" + ) + == 1 ) @@ -663,6 +678,46 @@ def test_walk_warns_for_nonplatform_invalid_library( assert "Skipping dependency B of esphome/A: manifest is corrupt" in caplog.text +def test_versionless_owner_qualified_dependency_warns_despite_provides( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """The backend's provides() only covers owner-less names (the walk's + backend-provided skip has the same guard), so an owner-qualified + version-less dependency that nobody adds must still warn.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + { + "esphome/A": { + "name": "A", + "dependencies": [{"name": "Wire", "owner": "Foo"}], + } + }, + ) + convert_libraries( + [Library("esphome/A", None, None)], + _backend(provides=lambda name: name == "Wire"), + ) + assert "Wire of esphome/A has no version to resolve" in caplog.text + + +def test_versionless_provided_dependency_stays_quiet( + tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture +) -> None: + """An owner-less version-less dependency the backend provides is added + by the backend after emit; no reconciliation warning.""" + _patch_download_with_manifests( + monkeypatch, + tmp_path, + {"esphome/A": {"name": "A", "dependencies": [{"name": "Wire"}]}}, + ) + convert_libraries( + [Library("esphome/A", None, None)], + _backend(provides=lambda name: name == "Wire"), + ) + assert "has no version to resolve" not in caplog.text + + def test_versionless_dependency_requested_top_level_stays_quiet( tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture ) -> None: