From c7a96f77d286a1a7d1706b0798a21a6f138e2287 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 22 Aug 2026 11:41:59 -0500 Subject: [PATCH] Trim comment essays and hoist function-local test imports --- esphome/arduino/library.py | 96 ++++++------------- esphome/build_gen/build_tool.py | 11 +-- esphome/platformio/library.py | 39 +++----- tests/unit_tests/build_gen/test_build_tool.py | 4 +- tests/unit_tests/test_arduino_library.py | 44 +++------ tests/unit_tests/test_platformio_library.py | 10 +- 6 files changed, 64 insertions(+), 140 deletions(-) diff --git a/esphome/arduino/library.py b/esphome/arduino/library.py index 5d7712e4ad..80e7aa0043 100644 --- a/esphome/arduino/library.py +++ b/esphome/arduino/library.py @@ -1,25 +1,12 @@ """Arduino-core backend for the shared PlatformIO library converter. -Turns the libraries registered via ``cg.add_library()`` into build inputs for -a native Arduino build. Bare names that exist under the framework's bundled -``libraries/`` directory (ESP8266WiFi, Wire, SPI, ...) are read straight from -the framework tree; everything else goes through the shared -resolution/download pipeline in ``esphome.platformio.library``. Nothing here -is core-specific: the caller names the PlatformIO platform, MCU, and cache -key of the Arduino core it builds. +Bundled names build straight from the framework tree; everything else goes +through ``esphome.platformio.library``. Mirrors ``lib_ldf_mode=off``: each +library builds its own archive; all include dirs join one global path. -Known deviations: flat-layout (``library.properties``, no ``src/``) -libraries get the recursive default source filter rather than PlatformIO's -root-only Arduino-1.0 filter (no bundled library is affected), and the -Arduino ``dot_a_linkage`` property is honored even though PlatformIO -ignores it. Bundled libraries never run a manifest ``extraScript`` (a -warning names the library if one declares it). Manifest ``-I`` build -flags join the global include path rather than staying private to the -library's own sources as under PlatformIO. - -Mirrors PlatformIO's ``lib_ldf_mode=off`` behavior: each library builds into -its own static archive and every library's include dir joins one global -include path. +Deviations from PlatformIO: flat-layout libraries get the recursive default +source filter; ``dot_a_linkage`` is honored; bundled libraries never run a +manifest ``extraScript``; manifest ``-I`` flags join the global include path. """ from __future__ import annotations @@ -101,12 +88,8 @@ def _warn_properties_depends(name: str, data: object) -> None: def _manifest_build(name: str, data: object) -> dict: - """The manifest's ``build`` section, validated by name. - - A bare json.load imposes no shape; a malformed manifest must name the - library instead of an AttributeError deep in a traceback (and must do so - before apply_extra_script dereferences the same section). - """ + """The manifest's ``build`` section; a malformed manifest must fail + naming the library, not with an AttributeError.""" build = data.get("build", {}) if isinstance(data, dict) else None if not isinstance(build, dict): raise EsphomeError(f"Library {name} has a malformed manifest") @@ -119,9 +102,8 @@ def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: # PIO's source-dir resolution: manifest srcDir, else src/Src, else the root if "srcDir" in build: - # An explicitly declared srcDir (falsy included) that does not - # resolve is unambiguously a manifest/tree error; a silently empty - # source set would surface as link errors far from the cause + # A declared srcDir (falsy included) that does not resolve is a + # manifest error src_dir = build["srcDir"] if not ( isinstance(src_dir, str) and src_dir and (read_path / src_dir).is_dir() @@ -138,11 +120,8 @@ def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: # PlatformIO shell-lexes each build.flags entry flag_tokens = lex_build_flags(build.get("flags", []), f"library {name}") - # build.libArchive is PIO behavior; dot_a_linkage is honored as a - # deliberate extra (Arduino IDE's property, which PIO ignores) so - # properties-only libraries can opt out of archiving too. Both parse - # through the same strict table: bool("false") is True, and a typo'd - # value must not silently change link semantics. + # dot_a_linkage (Arduino IDE's property, ignored by PIO) is a deliberate + # extra. Strict parse: bool("false") is True. def _parse_archive(key: str, raw: object) -> bool: if isinstance(raw, bool): return raw @@ -233,9 +212,7 @@ def _bundled_library(framework_path: Path, name: str) -> ArduinoLibrary: manifest = lib_dir / "library.properties" data = parse_library_properties(manifest) if manifest.is_file() else {} if isinstance(data, dict): - # The dependency walk never runs for bundled libraries (a no-op for - # the ESP8266 core, whose bundled manifests declare none); on a core - # where one does, the skip must be visible before link errors + # Bundled manifest deps are never walked; make the skip visible if data.get("dependencies"): _LOGGER.warning( "Bundled library %s declares dependencies, which are not " @@ -296,10 +273,8 @@ def resolve_libraries( # PlatformIO's lib_ignore covers framework-bundled libraries too; the # shared converter only filters the registry/git ones. lib_ignore = lib_ignore_set() - # One memoized answer to "does the framework bundle this name?" for the - # classification loop, the provides hook, and the dependency walk: the - # safety guard and the dir probe must stay fused (path traversal), and - # common names (Wire, SPI) are asked repeatedly + # Memoized "does the framework bundle this name?"; the safety guard and + # dir probe must stay fused (path traversal) _provided = functools.cache( lambda name: ( _is_safe_library_name(name) @@ -309,16 +284,11 @@ def resolve_libraries( for library in CORE.platformio_libraries.values(): if is_lib_ignored(library.name, lib_ignore): continue - # Only a bare name with a matching framework directory is bundled: a - # version pin means a registry package ("pngle@1.1.0"), and a bare - # name without the directory resolves from the registry at the - # latest version, matching PlatformIO (a typo fails loudly as a - # registry lookup error). + # Bundled only for a bare name with a matching framework dir; pinned + # or unmatched names resolve from the registry, as under PlatformIO. if not library.repository and not library.version and _provided(library.name): - # A bundled library's own manifest dependencies are not walked. - # PlatformIO would walk them even under lib_ldf_mode=off, but no - # library bundled with the ESP8266 core declares any, so the walk - # is a no-op there; core add_library() calls list what they need. + # Bundled libraries' own manifest deps are not walked (none of + # the ESP8266 core's declare any; _bundled_library warns if one does) bundled.append(_bundled_library(framework_path, library.name)) else: external.append(library) @@ -328,9 +298,8 @@ def resolve_libraries( 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 bundled name to - # add (a duplicate archive shows up as duplicate-symbol link errors) + # Deps matching a separately-requested external are already in the build + # (a duplicate archive means duplicate-symbol link errors) external_short_names = { _external_short_name(lib.name) for lib in external if lib.name } @@ -360,18 +329,14 @@ def resolve_libraries( ): continue if dep.get("owner") or not _provided(name): - # 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. + # Owner-less names in the framework tree prefer the bundled + # copy (PIO's process_dependencies); everything else resolves + # via the converter, and the walk reports any real drops continue if not dependency_is_usable(dep, pio_platform, "arduino", component.name): continue - # 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 + # Deferred: a later-emitted library's manifest name may satisfy + # this; adding now could double the archive pending_bundled.setdefault(name) def _emit(component: ConvertedLibrary) -> None: @@ -388,9 +353,6 @@ def resolve_libraries( _add_bundled_dependencies(component) if external: - # Every converter drop path raises (an incompatible top-level is a - # RuntimeError, resolution and download failures raise), so the - # return needs no re-verification here. convert_libraries( external, LibraryBackend( @@ -398,10 +360,8 @@ def resolve_libraries( framework="arduino", emit=_emit, cache_key=cache_key, - # The graph walk must not resolve a bundled name from the - # registry ({"Wire": "*"} in a manifest); the bundled copy - # is added by _add_bundled_dependencies after emit. Unsafe - # names are simply not provided. + # The walk must not resolve bundled names from the registry; + # _add_bundled_dependencies adds them after emit provides=_provided, ), ) diff --git a/esphome/build_gen/build_tool.py b/esphome/build_gen/build_tool.py index 4cdbb88c28..6395f84dd0 100644 --- a/esphome/build_gen/build_tool.py +++ b/esphome/build_gen/build_tool.py @@ -21,14 +21,9 @@ def main() -> int: # Remove first: ``ar rc`` replaces members but never drops ones whose # source was removed from the build, which would leak stale objects. Path(archive).unlink(missing_ok=True) - # Expand the response file here instead of passing @rspfile: GNU ar - # treats backslashes in response files as escapes, corrupting Windows - # paths ("sub\a.o" -> "suba.o"). - # One path per line (rspfile_content = $in_newline). ninja shell-quotes - # a path containing specials, so undo a simple surrounding quote per - # line. Expanding into argv trades away the OS command-line length - # limit rspfiles dodge; the relative object paths here stay far - # below it. + # GNU ar treats backslashes in response files as escapes (corrupts + # Windows paths), so expand the rspfile into argv, stripping the + # simple surrounding quote ninja adds to special paths. objects = [ line[1:-1] if len(line) >= 2 and line[0] == line[-1] and line[0] in "'\"" diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 56010bf5e1..a37c7fe5bf 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -298,10 +298,9 @@ class LibraryBackend: framework: str emit: Callable[["ConvertedLibrary"], None] cache_key: str - # When set, an owner-less manifest dependency this returns True for is - # skipped by the graph walk: the backend provides it outside the - # registry (e.g. a library bundled with the Arduino core), mirroring - # PlatformIO's process_dependencies preference for bundled builders. + # Owner-less dependency names this returns True for are skipped by the + # walk; the backend supplies them outside the registry (e.g. core-bundled + # libraries). provides: Callable[[str], bool] | None = None @@ -630,13 +629,8 @@ def join_flag_args(tokens: Iterable[str], owner: str) -> list[str]: def dependency_is_usable( dep: dict, platform: str | None, framework: str, requester: str ) -> bool: - """Whether a manifest dependency passes the compatibility filter. - - The routine cross-platform skip logs at debug; any other - ``InvalidLibrary`` cause is a dropped dependency and warns naming the - requester (unreachable from ``check_library_data`` today, which raises - only for the platform filter). - """ + """Compatibility filter for a manifest dependency: platform mismatches + skip at debug, any other ``InvalidLibrary`` warns naming the requester.""" try: check_library_data(dep, platform, framework) except IncompatiblePlatform as e: @@ -696,9 +690,7 @@ def normalize_dependencies( continue normalized.append(entry) elif isinstance(entry, str) and entry: - # PIO also accepts a bare list of names ("dependencies": - # ["Wire"]); dropping them here would hide a real dependency - # from every caller's visibility warning + # PIO also accepts a bare list of names ("dependencies": ["Wire"]) normalized.append({"name": entry}) else: _LOGGER.warning( @@ -1022,10 +1014,8 @@ def convert_libraries( component.data.get("dependencies"), component.name ): if "name" not in dependency or "version" not in dependency: - # Version-less deps cannot resolve from the registry. - # Deferred: only the final resolution set can tell a real - # drop from a name another manifest resolves later, so the - # reconciliation after emit owns the warning + # Version-less deps cannot resolve from the registry; the + # post-emit reconciliation owns the drop warning _LOGGER.debug( "Skip version-less dependency %r of %s", dependency.get("name"), @@ -1053,9 +1043,8 @@ def convert_libraries( # The backend adds it from its own tree; resolving it here # would fetch a same-named registry package instead if (pin := dependency.get("version")) and pin != "*": - # The declared constraint is discarded for the bundled - # copy; a too-old bundled library must not surface as - # link errors with no stated cause + # The version pin is discarded for the bundled copy; make + # the substitution visible _LOGGER.warning( "Dependency %s pins version %s; using the library " "bundled with the framework instead", @@ -1123,11 +1112,9 @@ def convert_libraries( for component in components.values(): backend.emit(component) - # A version-less dependency is satisfied when its request key resolved, - # a resolved component's manifest name matches, or the backend provides - # it from its own tree (e.g. the arduino bundled libraries, added by the - # backend after emit). Anything else is a real drop that would otherwise - # surface as link errors far from the cause. + # Warn for version-less deps nothing satisfied (request key, manifest + # name, or backend provides()); a silent drop surfaces 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, dep_owner, requester in skipped_versionless: diff --git a/tests/unit_tests/build_gen/test_build_tool.py b/tests/unit_tests/build_gen/test_build_tool.py index 875ffcb773..7f56abe9cb 100644 --- a/tests/unit_tests/build_gen/test_build_tool.py +++ b/tests/unit_tests/build_gen/test_build_tool.py @@ -3,6 +3,8 @@ from __future__ import annotations from pathlib import Path +import subprocess +import sys from unittest.mock import MagicMock, patch import pytest @@ -50,8 +52,6 @@ def test_unknown_mode(capsys: pytest.CaptureFixture[str]) -> None: def test_runs_as_script(tmp_path: Path) -> None: """The ninja rules invoke the file as a plain script.""" - import subprocess - import sys src = tmp_path / "a.bin" src.write_text("x") diff --git a/tests/unit_tests/test_arduino_library.py b/tests/unit_tests/test_arduino_library.py index 42e1bfc1c9..c310c964d9 100644 --- a/tests/unit_tests/test_arduino_library.py +++ b/tests/unit_tests/test_arduino_library.py @@ -12,7 +12,13 @@ import pytest from esphome.arduino import library as component from esphome.const import KEY_CORE, KEY_TARGET_PLATFORM, PLATFORM_ESP8266 from esphome.core import CORE, EsphomeError, Library -from esphome.platformio.library import ConvertedLibrary, LibraryBackend +import esphome.platformio.library as pio_library +from esphome.platformio.library import ( + ConvertedLibrary, + IncompatiblePlatform, + InvalidLibrary, + LibraryBackend, +) @pytest.fixture(autouse=True) @@ -212,9 +218,7 @@ def test_resolve_libraries_bundled(tmp_path: Path) -> None: def test_resolve_libraries_registry_name_is_external( tmp_path: Path, version: str | None ) -> None: - """A name that is not bundled reaches the converter: bare resolves from - the registry at the latest version (matching PlatformIO and the - documented libraries: key) and a version pin is a registry package.""" + """A name that is not bundled reaches the converter, bare or pinned.""" framework = _make_framework(tmp_path) _add_library("pngle", version) with patch.object(component, "convert_libraries", return_value=[]) as mock_convert: @@ -410,12 +414,8 @@ def test_bundled_dependency_nonplatform_rejection_warns( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: """An InvalidLibrary whose cause is not the platform filter is visible.""" - from esphome.platformio.library import InvalidLibrary - framework = _make_framework(tmp_path) converted = _webserver(tmp_path, {"build": {}, "dependencies": [{"name": "Wire"}]}) - import esphome.platformio.library as pio_library - with ( _emitting_converter(converted), patch.object( @@ -466,9 +466,7 @@ def test_library_info_lib_archive_parse( def test_bundled_dependency_dict_shorthand_prefers_bundled(tmp_path: Path) -> None: - """The {"Wire": "*"} dict shorthand (version="*", no owner) must resolve - to the bundled library, matching PIO's process_dependencies, instead of - being routed to the registry.""" + """The {"Wire": "*"} dict shorthand resolves to the bundled library.""" framework = _make_framework(tmp_path) converted = _webserver(tmp_path, {"build": {}, "dependencies": {"Wire": "*"}}) with _emitting_converter(converted): @@ -481,12 +479,8 @@ def test_bundled_dependency_platform_rejection_is_debug( ) -> None: """The typed IncompatiblePlatform (the routine cross-platform skip) stays at debug regardless of message wording.""" - from esphome.platformio.library import IncompatiblePlatform - framework = _make_framework(tmp_path) converted = _webserver(tmp_path, {"build": {}, "dependencies": [{"name": "Wire"}]}) - import esphome.platformio.library as pio_library - with ( _emitting_converter(converted), patch.object( @@ -626,11 +620,8 @@ def test_bundled_library_non_dict_manifest_skips_probes_and_raises( def test_dict_shorthand_dependency_skips_registry_through_real_converter( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """{"Wire": "*"} in a real manifest must never reach the registry: the - graph walk skips backend-provided names and the bundled copy is added - after emit (no converter mock; a registry touch fails the test).""" - import esphome.platformio.library as pio_library - + """{"Wire": "*"} resolves to the bundled copy without touching the + registry (real converter).""" framework = _make_framework(tmp_path) _local_lib(tmp_path, {"Wire": "*"}) # Pin the component cache to tmp_path (data_dir honors an ambient @@ -702,8 +693,6 @@ def test_pinned_bundled_dependency_substitution_warns( ) -> None: """A non-* version pin on a backend-provided dependency is discarded for the bundled copy; the substitution must be visible.""" - import esphome.platformio.library as pio_library - framework = _make_framework(tmp_path) _local_lib(tmp_path, {"Wire": "^2.0.0"}) monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) @@ -720,9 +709,7 @@ def test_pinned_bundled_dependency_substitution_warns( def test_transitively_resolved_dependency_does_not_warn( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """A version-less dependency the walk resolved as another library's - registry dependency is present in the build; the skipping warning must - stay quiet for it.""" + """A dependency the walk already resolved does not warn.""" framework = _make_framework(tmp_path) _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") ws, tcp = _ws_tcp_pair(tmp_path) @@ -754,9 +741,8 @@ def test_external_short_name(spec: str, expected: str) -> None: 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.""" + """A name a converted library's manifest provides is not also added + from the framework tree, 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" @@ -814,8 +800,6 @@ def test_versionless_dependency_with_provider_stays_quiet( ) -> None: """With a provides backend the version-less skip is routine (debug) and the bundled copy is picked up after emit.""" - import esphome.platformio.library as pio_library - framework = _make_framework(tmp_path) _local_lib(tmp_path, [{"name": "Wire"}]) monkeypatch.setenv("ESPHOME_DATA_DIR", str(tmp_path / ".esphome")) diff --git a/tests/unit_tests/test_platformio_library.py b/tests/unit_tests/test_platformio_library.py index 98c0462e9f..febaf830a5 100644 --- a/tests/unit_tests/test_platformio_library.py +++ b/tests/unit_tests/test_platformio_library.py @@ -679,9 +679,8 @@ def test_walk_warns_for_nonplatform_invalid_library( 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.""" + """An owner-qualified version-less dependency is not satisfied by + provides(); it must still warn.""" _patch_download_with_manifests( monkeypatch, tmp_path, @@ -756,9 +755,8 @@ def test_versionless_url_ish_dependency_name_warns_cleanly( def test_versionless_dependency_matching_resolved_manifest_name_stays_quiet( tmp_path, monkeypatch, caplog: pytest.LogCaptureFixture ) -> None: - """A bare dependency name satisfied by a component requested under an - owner-qualified spec (manifest names match) is not a drop; a nameless - entry is skipped without a reconciliation warning.""" + """A bare name satisfied by an owner-qualified component's manifest + name is not a drop.""" _patch_download_with_manifests( monkeypatch, tmp_path,