From 0cb307aea573ef72ee3d35dbf23d7bd87f54a911 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 21 Aug 2026 12:53:39 -0500 Subject: [PATCH 1/2] Type the platform rejection, prefer bundled deps, parse manifests strictly check_library_data's platform filter raises IncompatiblePlatform (an InvalidLibrary subclass) so the arduino backend branches on the type instead of substring-matching the message. The {"Wire": "*"} dict shorthand resolves to the bundled library like PIO's process_dependencies instead of a registry lookup. libArchive parses booleans and true/false strings and warns on anything else (bool(str) made "false" archive). A declared-but-falsy srcDir raises instead of silently probing. The converter-drop warning names the missing requests, and the bundled-walk comment states what PIO actually does. --- esphome/arduino/library.py | 71 ++++++++++++------ esphome/platformio/library.py | 10 ++- tests/unit_tests/test_arduino_library.py | 96 +++++++++++++++++++++++- 3 files changed, 151 insertions(+), 26 deletions(-) diff --git a/esphome/arduino/library.py b/esphome/arduino/library.py index dee58b5fed..f699d79704 100644 --- a/esphome/arduino/library.py +++ b/esphome/arduino/library.py @@ -32,6 +32,7 @@ from esphome.platformio.library import ( DEFAULT_BUILD_SRC_FILTER, SRC_FILE_EXTENSIONS, ConvertedLibrary, + IncompatiblePlatform, InvalidLibrary, LibraryBackend, check_library_data, @@ -73,16 +74,19 @@ def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: build = data.get("build", {}) # PIO's source-dir resolution: manifest srcDir, else src/Src, else the root - src_dir = build.get("srcDir") or next( - (d for d in ("src", "Src") if (read_path / d).is_dir()), "." - ) - if "srcDir" in build and not (read_path / src_dir).is_dir(): - # Unlike the default probes, an explicitly declared srcDir that does - # not resolve is unambiguously a manifest/tree error; a silently - # empty source set would surface as link errors far from the cause - raise EsphomeError( - f"Library {name} declares srcDir {src_dir} which does not exist" - ) + 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 + src_dir = build["srcDir"] + if not ( + isinstance(src_dir, str) and src_dir and (read_path / src_dir).is_dir() + ): + raise EsphomeError( + f"Library {name} declares srcDir {src_dir!r} which does not exist" + ) + else: + src_dir = next((d for d in ("src", "Src") if (read_path / d).is_dir()), ".") src_filter = ensure_list(build.get("srcFilter", DEFAULT_BUILD_SRC_FILTER)) # PlatformIO shell-lexes each build.flags entry @@ -92,7 +96,20 @@ def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: # deliberate extra (Arduino IDE's property, which PIO ignores) so # properties-only libraries can opt out of archiving too if "libArchive" in build: - lib_archive = bool(build["libArchive"]) + raw_archive = build["libArchive"] + if isinstance(raw_archive, bool): + lib_archive = raw_archive + elif str(raw_archive).strip().lower() in ("true", "false"): + lib_archive = str(raw_archive).strip().lower() == "true" + else: + # bool("false") is True; an unparsable value must not silently + # archive a library whose author disabled archiving + _LOGGER.warning( + "Library %s has an unrecognized libArchive value %r; assuming true", + name, + raw_archive, + ) + lib_archive = True elif "dot_a_linkage" in data: lib_archive = str(data["dot_a_linkage"]).lower() == "true" else: @@ -192,9 +209,10 @@ def resolve_libraries( and "/" not in library.name and (framework_path / "libraries" / library.name).is_dir() ): - # A bundled library's own manifest dependencies are deliberately - # not walked (PlatformIO's lib_ldf_mode=off does not either); - # core add_library() calls list what they need explicitly. + # 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.append(_bundled_library(framework_path, library.name)) else: external.append(library) @@ -217,11 +235,13 @@ def resolve_libraries( continue if name in bundled_names or is_lib_ignored(name, lib_ignore): continue - if "version" in dep: - # The converter resolves versioned deps from the registry. - # Note: the dict shorthand {"Wire": "*"} normalizes to - # version="*" and takes this path even for a bundled name; - # use the list form for bundled dependencies. + bundled_dir = framework_path / "libraries" / name + if "version" in dep and (dep.get("owner") or not bundled_dir.is_dir()): + # 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"): # Owner but no version: the converter skips it too, so this @@ -233,7 +253,7 @@ def resolve_libraries( component.name, ) continue - if not (framework_path / "libraries" / name).is_dir(): + if not bundled_dir.is_dir(): # The shared converter skips version-less deps too, so this # is the only place the drop can be made visible before the # missing sources surface as link errors. @@ -251,7 +271,7 @@ def resolve_libraries( # manifest is routine (every ESPAsyncWebServer build hits # it), so the platform filter stays at debug; any other # cause means a dropped dependency and must be visible - if "platform" in str(err).lower(): + if isinstance(err, IncompatiblePlatform): _LOGGER.debug("Skipping bundled dependency %s: %s", name, err) else: _LOGGER.warning( @@ -287,12 +307,15 @@ def resolve_libraries( ) if len(resolved) < len(external): # A requested library the converter dropped would otherwise - # surface only as link errors far from the cause + # surface only as link errors far from the cause; name the + # requests that went missing, not just the survivors + resolved_names = {c.name for c in resolved} + dropped = [str(lib) for lib in external if lib.name not in resolved_names] _LOGGER.warning( - "%d of %d requested libraries were not resolved (resolved: %s)", + "%d of %d requested libraries were not resolved (missing: %s)", len(external) - len(resolved), len(external), - ", ".join(sorted(c.name for c in resolved)) or "none", + ", ".join(sorted(dropped)) or "unknown", ) return bundled + converted diff --git a/esphome/platformio/library.py b/esphome/platformio/library.py index 7349c46144..34c7a272cc 100644 --- a/esphome/platformio/library.py +++ b/esphome/platformio/library.py @@ -209,6 +209,14 @@ class InvalidLibrary(Exception): pass +class IncompatiblePlatform(InvalidLibrary): + """The manifest's platform filter rejected the target platform. + + A distinct type so callers can treat the routine cross-platform skip + differently from other manifest problems without matching message text. + """ + + class ConvertedLibrary: """A resolved PlatformIO library plus its parsed manifest and on-disk path. @@ -438,7 +446,7 @@ def check_library_data(data: dict, platform: str | None, framework: str): valid_platforms = platform is None or "*" in platforms or platform in platforms if not valid_platforms: - raise InvalidLibrary(f"Unsupported library platforms: {platforms}") + raise IncompatiblePlatform(f"Unsupported library platforms: {platforms}") frameworks = data.get("frameworks", "*") if isinstance(frameworks, str): diff --git a/tests/unit_tests/test_arduino_library.py b/tests/unit_tests/test_arduino_library.py index f0bff7b729..59a1f9fa45 100644 --- a/tests/unit_tests/test_arduino_library.py +++ b/tests/unit_tests/test_arduino_library.py @@ -291,7 +291,7 @@ def test_library_info_missing_declared_src_dir_raises(tmp_path: Path) -> None: """An explicitly declared srcDir that does not exist is a manifest error.""" read_path = tmp_path / "lib" read_path.mkdir() - with pytest.raises(EsphomeError, match="srcDir nosrc which does not exist"): + with pytest.raises(EsphomeError, match="srcDir 'nosrc' which does not exist"): component._library_info("x", read_path, {"build": {"srcDir": "nosrc"}}) @@ -425,6 +425,8 @@ def test_resolve_libraries_warns_when_converter_drops_a_request( cache_key="arduino8266", ) assert "1 of 1 requested libraries were not resolved" in caplog.text + # The actionable fact is which request went missing, not the survivors + assert "missing: pngle" in caplog.text def test_bundled_dependency_nonplatform_rejection_warns( @@ -458,3 +460,95 @@ def test_bundled_dependency_nonplatform_rejection_warns( ) assert "Skipping bundled dependency Wire" in caplog.text assert "manifest is corrupt" in caplog.text + + +@pytest.mark.parametrize("declared", ["", None]) +def test_library_info_falsy_declared_src_dir_raises( + tmp_path: Path, declared: str | None +) -> None: + """A declared-but-falsy srcDir must not silently fall back to the probe.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + with pytest.raises(EsphomeError, match="does not exist"): + component._library_info("x", read_path, {"build": {"srcDir": declared}}) + + +@pytest.mark.parametrize( + ("value", "expected", "warns"), + [ + (False, False, False), + ("false", False, False), + ("False", False, False), + ("true", True, False), + ("archive-me", True, True), + ], +) +def test_library_info_lib_archive_parse( + tmp_path: Path, + value: object, + expected: bool, + warns: bool, + caplog: pytest.LogCaptureFixture, +) -> None: + """bool("false") is True; the string forms must parse, not coerce.""" + read_path = tmp_path / "lib" + (read_path / "src").mkdir(parents=True) + lib = component._library_info("x", read_path, {"build": {"libArchive": value}}) + assert lib.lib_archive is expected + assert ("unrecognized libArchive" in caplog.text) is warns + + +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.""" + framework = _make_framework(tmp_path) + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + lib_dir = tmp_path / "converted" / "webserver" + (lib_dir / "src").mkdir(parents=True) + converted = _converted( + "esp32async__ESPAsyncWebServer", + lib_dir, + {"build": {}, "dependencies": {"Wire": "*"}}, + ) + with _emitting_converter(converted): + libs = component.resolve_libraries( + framework, + pio_platform="espressif8266", + board_mcu="esp8266", + cache_key="arduino8266", + ) + assert "Wire" in [lib.name for lib in libs] + + +def test_bundled_dependency_platform_rejection_is_debug( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> 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) + _add_library("ESP32Async/ESPAsyncWebServer", "3.9.6") + lib_dir = tmp_path / "converted" / "webserver" + (lib_dir / "src").mkdir(parents=True) + converted = _converted( + "esp32async__ESPAsyncWebServer", + lib_dir, + {"build": {}, "dependencies": [{"name": "Wire"}]}, + ) + with ( + _emitting_converter(converted), + patch.object( + component, + "check_library_data", + side_effect=IncompatiblePlatform("nothing about the p-word here"), + ), + ): + component.resolve_libraries( + framework, + pio_platform="espressif8266", + board_mcu="esp8266", + cache_key="arduino8266", + ) + assert "Skipping bundled dependency Wire" not in caplog.text From 784cf6819f26a614a828e0bd71188c066e3ef3c9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 21 Aug 2026 12:55:23 -0500 Subject: [PATCH 2/2] Refuse inconsistent memory-layout configs, validate the cached linker script Unknown or conflicting VTABLES_IN_* defines raise instead of warning and picking arbitrarily (a typo previously won the sorted pick and died in the SDK header's #error). Custom MMU sizes without the CUSTOM knob raise too: PlatformIO warns but its defaults win the compile line, while here the user's tokens would win and compile against a layout the linker script does not implement. The cached linker script is re-checked for its SECTIONS block so a truncated file regenerates, the testing-mode flash-ld read gets the same OSError guard as the gcc spawn, the ParseFlags docstring states its actual coverage, and the ld test mocks are quiet so the clean path asserts no warnings. --- esphome/build_gen/arduino8266.py | 57 ++++++++++++----- .../unit_tests/build_gen/test_arduino8266.py | 63 ++++++++++++------- 2 files changed, 83 insertions(+), 37 deletions(-) diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index ae38bc547a..6f302833a1 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -215,10 +215,15 @@ def _resolve_build_config(defines: dict[str, str]) -> _BuildConfig: # build flags, whose iteration order varies between processes. vtables_knobs = sorted(name for name in defines if name.startswith("VTABLES_IN_")) known_vtables = {"VTABLES_IN_FLASH", "VTABLES_IN_DRAM", "VTABLES_IN_IRAM"} + # A typo would otherwise win the sorted pick and end in the SDK header's + # #error, and a conflicting pair would resolve arbitrarily; both are + # config errors, not build-time surprises if unknown := [k for k in vtables_knobs if k not in known_vtables]: - _LOGGER.warning("Unknown VTABLES_IN_* define(s): %s", ", ".join(unknown)) + raise EsphomeError(f"Unknown VTABLES_IN_* define(s): {', '.join(unknown)}") if len(vtables_knobs) > 1: - _LOGGER.warning("Multiple VTABLES_IN_* defines; using %s", vtables_knobs[0]) + raise EsphomeError( + f"Conflicting VTABLES_IN_* defines: {', '.join(vtables_knobs)}" + ) vtables = vtables_knobs[0] if vtables_knobs else "VTABLES_IN_FLASH" mmu = next((variant for knob, variant in _MMU_VARIANTS if knob in defines), None) @@ -237,13 +242,14 @@ def _resolve_build_config(defines: dict[str, str]) -> _BuildConfig: ) else: if "MMU_IRAM_SIZE" in defines or "MMU_ICACHE_SIZE" in defines: - # Same diagnostic the PlatformIO builder prints: without the - # knob the linker script keeps the default layout while the - # compile line carries the custom sizes - _LOGGER.warning( - "Detected custom MMU flags; use " - "-DPIO_FRAMEWORK_ARDUINO_MMU_CUSTOM to disable the " - "default configuration" + # PlatformIO only warns here and appends its defaults last so + # they win the compile line; in this generator the user's + # tokens would come last instead, compiling against a memory + # layout the linker script does not implement. Refuse rather + # than reproduce the upstream footgun with worse odds. + raise EsphomeError( + "Custom MMU_IRAM_SIZE/MMU_ICACHE_SIZE build flags require " + "-DPIO_FRAMEWORK_ARDUINO_MMU_CUSTOM" ) mmu = list(_MMU_DEFAULT) @@ -306,7 +312,10 @@ def _project_flags( Every entry is shell-lexed the way PlatformIO's ``ParseFlags`` does, so a linker flag anywhere in an entry reaches the link line and ``build_unflags`` matches individual tokens (``-Os`` inside ``-Os -g3``). - Lexed tokens are re-quoted at emission via ``_shell_token``. + Only the flag forms ESPHome emits are classified (``-Wl,``/``-L``/``-l`` + and compile flags); rarities like plain-form ``-T``/``-u``/``-Xlinker`` + route to the compile line, unlike full ParseFlags. Lexed tokens are + re-quoted at emission via ``_shell_token``. """ compile_flags: list[str] = [] link_flags: list[str] = [] @@ -372,11 +381,20 @@ def generate_ld_scripts( # any behavioral edit in build_surgery self-invalidates the cache + f" {build_surgery.surgery_fingerprint()}" ) - if not ( - output.is_file() - and stamp.is_file() - and stamp.read_text(encoding="utf-8") == stamp_content - ): + + def _cached_ld_is_valid() -> bool: + if not ( + output.is_file() + and stamp.is_file() + and stamp.read_text(encoding="utf-8") == stamp_content + ): + return False + # A truncated or externally edited script must force regeneration, + # not be reused on existence alone (the SECTIONS check below only + # guards the generation path) + return "SECTIONS" in output.read_text(encoding="utf-8") + + if not _cached_ld_is_valid(): try: result = subprocess.run( cmd, capture_output=True, text=True, check=False, close_fds=False @@ -409,10 +427,17 @@ def generate_ld_scripts( # A patched copy of the flash ld in the build dir; resolved through # the same -L path as the SDK original it shadows. flash_ld = framework / "tools" / "sdk" / "ld" / flash_ld_name + try: + flash_ld_text = flash_ld.read_text(encoding="utf-8") + except OSError as err: + # Same half-extracted-cache hazard as the gcc spawn above + raise EsphomeError( + f"Could not read {flash_ld}: {err}; run 'esphome clean-all' and retry" + ) from err write_file_if_changed( ld_dir / f"testing_{flash_ld_name}", build_surgery.apply_testing_memory_patches( - flash_ld.read_text(encoding="utf-8"), + flash_ld_text, ("dram0_0_seg", "irom0_0_seg"), ), ) diff --git a/tests/unit_tests/build_gen/test_arduino8266.py b/tests/unit_tests/build_gen/test_arduino8266.py index e7ea3b6e77..8303ec0322 100644 --- a/tests/unit_tests/build_gen/test_arduino8266.py +++ b/tests/unit_tests/build_gen/test_arduino8266.py @@ -153,7 +153,7 @@ def test_defines_match_platformio_builder() -> None: ] -def _make_framework(tmp_path: Path) -> dict[str, Path]: +def _make_framework(tmp_path: Path) -> InstalledPaths: framework = tmp_path / "framework" core = framework / "cores" / "esp8266" core.mkdir(parents=True) @@ -270,9 +270,14 @@ def test_generate_ld_scripts(tmp_path: Path) -> None: paths = _make_framework(tmp_path) _set_flags("-DFP_IN_IROM") - result = MagicMock(returncode=0, stdout=_COMMON_LD_H_OUTPUT) - with patch.object(arduino8266.subprocess, "run", return_value=result) as mock_run: + result = MagicMock(returncode=0, stdout=_COMMON_LD_H_OUTPUT, stderr="") + with ( + patch.object(arduino8266.subprocess, "run", return_value=result) as mock_run, + patch.object(arduino8266._LOGGER, "warning") as mock_warn, + ): ld_dir = _run_generate_ld_scripts(paths) + # A clean preprocessor run must be quiet + mock_warn.assert_not_called() content = (ld_dir / "local.eagle.app.v6.common.ld").read_text() assert RATETABLE_RULE in content cmd = mock_run.call_args[0][0] @@ -297,6 +302,20 @@ def test_generate_ld_scripts(tmp_path: Path) -> None: mock_run.assert_called_once() +def test_generate_ld_scripts_corrupt_cache_regenerates(tmp_path: Path) -> None: + """A truncated cached linker script regenerates even with a fresh stamp.""" + paths = _make_framework(tmp_path) + result = MagicMock(returncode=0, stdout=_COMMON_LD_H_OUTPUT, stderr="") + with patch.object(arduino8266.subprocess, "run", return_value=result): + ld_dir = _run_generate_ld_scripts(paths) + output = ld_dir / "local.eagle.app.v6.common.ld" + output.write_text("truncated garbage") + with patch.object(arduino8266.subprocess, "run", return_value=result) as mock_run: + _run_generate_ld_scripts(paths) + mock_run.assert_called_once() + assert RATETABLE_RULE in output.read_text() + + def test_generate_ld_scripts_failure(tmp_path: Path) -> None: paths = _make_framework(tmp_path) @@ -320,7 +339,7 @@ def test_generate_ld_scripts_testing_mode(tmp_path: Path) -> None: "}\n" ) CORE.testing_mode = True - result = MagicMock(returncode=0, stdout=_COMMON_LD_H_OUTPUT) + result = MagicMock(returncode=0, stdout=_COMMON_LD_H_OUTPUT, stderr="") with patch.object(arduino8266.subprocess, "run", return_value=result): ld_dir = _run_generate_ld_scripts(paths) patched = (ld_dir / "testing_eagle.flash.4m.ld").read_text() @@ -415,15 +434,14 @@ def test_flag_defines_joins_spaced_define() -> None: assert "" not in defines -def test_build_config_custom_mmu_without_knob_warns( - caplog: pytest.LogCaptureFixture, -) -> None: - """Custom MMU sizes without the CUSTOM knob keep the default layout and - warn, as the PlatformIO builder does.""" +def test_build_config_custom_mmu_without_knob_raises() -> None: + """Custom MMU sizes without the CUSTOM knob would compile against a + layout the linker script does not implement; refuse instead of warning + (PlatformIO warns, but its defaults win the compile line; ours would + not).""" _set_flags("-DMMU_IRAM_SIZE=0xC000") - config = _resolve_build_config(_flag_defines(set())) - assert config.mmu_defines == ["MMU_IRAM_SIZE=0x8000", "MMU_ICACHE_SIZE=0x8000"] - assert "Detected custom MMU flags" in caplog.text + with pytest.raises(EsphomeError, match="PIO_FRAMEWORK_ARDUINO_MMU_CUSTOM"): + _resolve_build_config(_flag_defines(set())) def test_flag_defines_lexes_quoted_single_tokens() -> None: @@ -534,15 +552,18 @@ def test_flag_defines_respects_unflags() -> None: assert config.vtables == "VTABLES_IN_FLASH" -def test_vtables_unknown_and_conflicting_warn( - caplog: pytest.LogCaptureFixture, -) -> None: - _set_flags("-DVTABLES_IN_BANANA", "-DVTABLES_IN_DRAM") - config = _resolve_build_config(_flag_defines(set())) - assert "Unknown VTABLES_IN_*" in caplog.text - assert "Multiple VTABLES_IN_*" in caplog.text - # Deterministic pick, as before - assert config.vtables == "VTABLES_IN_BANANA" +def test_vtables_unknown_raises() -> None: + """A typo'd knob would win the sorted pick and die in the SDK header's + #error; fail by name at generation instead.""" + _set_flags("-DVTABLES_IN_BANANA") + with pytest.raises(EsphomeError, match="Unknown VTABLES_IN_.*BANANA"): + _resolve_build_config(_flag_defines(set())) + + +def test_vtables_conflicting_raises() -> None: + _set_flags("-DVTABLES_IN_DRAM", "-DVTABLES_IN_IRAM") + with pytest.raises(EsphomeError, match="Conflicting VTABLES_IN_"): + _resolve_build_config(_flag_defines(set())) def test_project_flags_empty_lib_flags_warn(