From c579a2792f79bd4c0aa4b8c1aafc16c5a22fda72 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 20 Aug 2026 04:23:14 -0500 Subject: [PATCH] Address review: quote ninja command paths, refuse unverified downloads, extend lib_ignore and build_unflags parity --- esphome/arduino8266/component.py | 25 ++++++++-- esphome/arduino8266/framework.py | 14 ++++-- esphome/build_gen/arduino8266.py | 24 ++++----- .../unit_tests/build_gen/test_arduino8266.py | 10 +++- .../unit_tests/test_arduino8266_component.py | 49 +++++++++++++++++++ .../unit_tests/test_arduino8266_framework.py | 31 ++++++++++-- 6 files changed, 128 insertions(+), 25 deletions(-) diff --git a/esphome/arduino8266/component.py b/esphome/arduino8266/component.py index 8fee3d05f8..73121bee87 100644 --- a/esphome/arduino8266/component.py +++ b/esphome/arduino8266/component.py @@ -64,6 +64,11 @@ def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: 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(): + # A silently empty source set would surface as link errors instead + _LOGGER.warning( + "Library %s declares srcDir %s which does not exist", name, src_dir + ) src_filter = ensure_list(build.get("srcFilter", DEFAULT_BUILD_SRC_FILTER)) # PlatformIO shell-lexes each build.flags entry @@ -100,9 +105,9 @@ def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary: for d in [include_dir, src_dir, *include_flags]: if (path := (read_path / d)).is_dir(): lib.include_dirs.append(path.resolve()) - elif d in include_flags: - # The includeDir/srcDir defaults are probes; an explicit -I that - # does not resolve is a manifest or packaging error worth naming + elif d in include_flags or (d == include_dir and "includeDir" in build): + # The includeDir/srcDir defaults are probes; an explicitly + # declared path that does not resolve is a manifest error _LOGGER.warning( "Library %s declares include dir %s which does not exist", name, d ) @@ -127,7 +132,15 @@ def resolve_libraries(framework_path: Path) -> list[ArduinoLibrary]: """Resolve every ``cg.add_library()`` entry into an :class:`ArduinoLibrary`.""" bundled: list[ArduinoLibrary] = [] external: list[Library] = [] + # PlatformIO's lib_ignore covers framework-bundled libraries too; the + # shared converter only filters the registry/git ones. + lib_ignore = { + ignore.split("/")[-1].lower() + for ignore in CORE.platformio_options.get("lib_ignore", []) + } for library in CORE.platformio_libraries.values(): + if library.name and library.name.split("/")[-1].lower() in lib_ignore: + continue # A version pin means a registry package ("pngle@1.1.0"), never a # framework-bundled library. if ( @@ -161,9 +174,15 @@ def resolve_libraries(framework_path: Path) -> list[ArduinoLibrary]: or not (framework_path / "libraries" / name).is_dir() ): continue + if name.lower() in lib_ignore: + continue try: check_library_data(dep, ESP8266_PLATFORM, "arduino") except InvalidLibrary as err: + # check_library_data's only raise is the platform filter, and + # rejecting another platform's dependency of a cross-platform + # manifest is routine (every ESPAsyncWebServer build hits it); + # a warning here would be noise, and the reason is in the log. _LOGGER.debug("Skipping bundled dependency %s: %s", name, err) continue bundled_names.add(name) diff --git a/esphome/arduino8266/framework.py b/esphome/arduino8266/framework.py index b1295f9469..e75ef892f3 100644 --- a/esphome/arduino8266/framework.py +++ b/esphome/arduino8266/framework.py @@ -139,11 +139,15 @@ def _registry_download( # ensure_list: a bare string would make ``in`` a substring test systems = ensure_list(file.get("system") or "*") if "*" in systems or system in systems: - return ( - file["download_url"], - (file.get("checksum") or {}).get("sha256"), - file.get("size"), - ) + sha256 = (file.get("checksum") or {}).get("sha256") + if not sha256: + # Never extract an unverified archive; the registry + # publishes a checksum for every package file. + raise EsphomeError( + f"The package registry returned no sha256 for " + f"{package} {version}; refusing the unverified download" + ) + return (file["download_url"], sha256, file.get("size")) raise EsphomeError(f"No {package} {version} build for this platform ({system})") raise EsphomeError(f"{package} {version} not found in the package registry") diff --git a/esphome/build_gen/arduino8266.py b/esphome/build_gen/arduino8266.py index cfcc643e32..65e184e127 100644 --- a/esphome/build_gen/arduino8266.py +++ b/esphome/build_gen/arduino8266.py @@ -489,14 +489,16 @@ def write_project(paths: dict[str, Path]) -> bool: ) asflags = _ASFLAGS + defines + includes + project_compile_flags - # build_unflags applies to the framework flag sets too, as it does under - # PlatformIO (a silently ignored ``build_unflags: -Os`` would diverge). - if unflags := set(CORE.build_unflags): + # build_unflags applies to the framework flag sets too (compile and link), + # as under PlatformIO (a silently ignored ``build_unflags: -Os`` would + # diverge between the toolchains). + unflags = set(CORE.build_unflags) + if unflags: cflags = [f for f in cflags if f not in unflags] cxxflags = [f for f in cxxflags if f not in unflags] asflags = [f for f in asflags if f not in unflags] - link_flags = list(_LINKFLAGS) + link_flags = [f for f in _LINKFLAGS if f not in unflags] if esp8266_data.get(KEY_SCANF_FLOAT): link_flags += ["-u", "_scanf_float"] link_flags += project_link_flags @@ -530,35 +532,35 @@ def write_project(paths: dict[str, Path]) -> bool: f"ccache = {_q(ccache) if ccache else ''}", "", "rule cc", - " command = $ccache $cc -MMD -MF $out.d $cflags $flags -c $in -o $out", + ' command = $ccache $cc -MMD -MF "$out.d" $cflags $flags -c "$in" -o "$out"', " depfile = $out.d", " deps = gcc", " description = CC $out", "rule cxx", - " command = $ccache $cxx -MMD -MF $out.d $cxxflags $flags -c $in -o $out", + ' command = $ccache $cxx -MMD -MF "$out.d" $cxxflags $flags -c "$in" -o "$out"', " depfile = $out.d", " deps = gcc", " description = CXX $out", "rule asm", - " command = $ccache $cc -MMD -MF $out.d -x assembler-with-cpp $asflags $flags -c $in -o $out", + ' command = $ccache $cc -MMD -MF "$out.d" -x assembler-with-cpp $asflags $flags -c "$in" -o "$out"', " depfile = $out.d", " deps = gcc", " description = AS $out", "rule ar", - f" command = $python $buildtool ar {_q(toolchain_bin / 'xtensa-lx106-elf-ar')} $out $out.rsp", + f' command = $python $buildtool ar {_q(toolchain_bin / "xtensa-lx106-elf-ar")} "$out" "$out.rsp"', " rspfile = $out.rsp", " rspfile_content = $in_newline", " description = AR $out", "rule link", - " command = $cxx -o $out $linkflags @$out.rsp $libdirflags -Wl,--start-group $archives $libflags -Wl,--end-group", + ' command = $cxx -o "$out" $linkflags @"$out.rsp" $libdirflags -Wl,--start-group $archives $libflags -Wl,--end-group', " rspfile = $out.rsp", " rspfile_content = $in_newline", " description = LINK $out", "rule elf2bin", - f" command = $python {_q(framework / 'tools' / 'elf2bin.py')} --eboot {_q(framework / 'bootloaders' / 'eboot' / 'eboot.elf')} --app $in --flash_mode {esp8266_data[KEY_FLASH_MODE]} --flash_freq 40 --flash_size {_flash_size_str(flash_ld_name)} --path {_q(toolchain_bin)} --out $out", + f' command = $python {_q(framework / "tools" / "elf2bin.py")} --eboot {_q(framework / "bootloaders" / "eboot" / "eboot.elf")} --app "$in" --flash_mode {esp8266_data[KEY_FLASH_MODE]} --flash_freq 40 --flash_size {_flash_size_str(flash_ld_name)} --path {_q(toolchain_bin)} --out "$out"', " description = BIN $out", "rule copy", - " command = $python $buildtool copy $in $out", + ' command = $python $buildtool copy "$in" "$out"', " description = COPY $out", "", f"cflags = {' '.join(cflags)}", diff --git a/tests/unit_tests/build_gen/test_arduino8266.py b/tests/unit_tests/build_gen/test_arduino8266.py index d9844baf4a..1d51830c9b 100644 --- a/tests/unit_tests/build_gen/test_arduino8266.py +++ b/tests/unit_tests/build_gen/test_arduino8266.py @@ -249,6 +249,9 @@ def test_write_project_link_line_and_exclusions(tmp_path: Path) -> None: assert "-T eagle.flash.4m.ld" in content # scanf float disabled: the forced-link flag must not appear assert "_scanf_float" not in content + # Compile inputs and outputs are quoted (space-containing cache paths) + assert '-c "$in" -o "$out"' in content + assert '--app "$in"' in content # -L/-l from esphome build_flags reach the link line, not the compiles assert '-L"/opt/blobs"' in content assert "-luser_blob" in content @@ -545,8 +548,11 @@ def test_write_project_build_unflags_apply_to_framework_flags(tmp_path: Path) -> """build_unflags removes flags from the framework sets, as PlatformIO does.""" paths = _make_framework(tmp_path) _set_flags() - CORE.build_unflags = {"-fipa-pta"} + CORE.build_unflags = {"-fipa-pta", "-Wl,--gc-sections"} content = _write_ninja(paths) for line in content.splitlines(): - if line.split(" = ")[0] in ("cflags", "cxxflags", "asflags"): + key = line.split(" = ")[0] + if key in ("cflags", "cxxflags", "asflags"): assert "-fipa-pta" not in line + if key == "linkflags": + assert "-Wl,--gc-sections" not in line diff --git a/tests/unit_tests/test_arduino8266_component.py b/tests/unit_tests/test_arduino8266_component.py index d27da30847..28ceb5a4c3 100644 --- a/tests/unit_tests/test_arduino8266_component.py +++ b/tests/unit_tests/test_arduino8266_component.py @@ -222,3 +222,52 @@ def test_library_info_missing_explicit_include_warns( 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 + + +def test_library_info_missing_declared_dirs_warn( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Explicitly declared srcDir/includeDir that do not exist warn by name.""" + read_path = tmp_path / "lib" + read_path.mkdir() + component._library_info( + "x", read_path, {"build": {"srcDir": "nosrc", "includeDir": "noinc"}} + ) + assert "srcDir nosrc which does not exist" in caplog.text + assert "include dir noinc which does not exist" in caplog.text + + +def test_resolve_libraries_lib_ignore_covers_bundled(tmp_path: Path) -> None: + """lib_ignore applies to framework-bundled libraries, as under PlatformIO.""" + framework = _make_framework(tmp_path) + _add_library("ESP8266WiFi", None) + _add_library("Wire", None) + CORE.platformio_options = {"lib_ignore": ["Wire"]} + libs = component.resolve_libraries(framework) + assert [lib.name for lib in libs] == ["ESP8266WiFi"] + + +def test_resolve_libraries_lib_ignore_covers_bundled_dependencies( + tmp_path: Path, +) -> None: + framework = _make_framework(tmp_path) + _add_library("Some/External", "1.0.0") + CORE.platformio_options = {"lib_ignore": ["Wire"]} + + lib_dir = tmp_path / "converted" / "external" + lib_dir.mkdir(parents=True) + converted = _converted( + "some__External", lib_dir, {"dependencies": [{"name": "Wire"}]} + ) + + def fake_convert(libraries: list, backend: LibraryBackend) -> list: + backend.emit(converted) + return [converted] + + with ( + patch.object(component, "convert_libraries", side_effect=fake_convert), + patch.object(component, "apply_extra_script"), + ): + libs = component.resolve_libraries(framework) + + assert [lib.name for lib in libs] == ["some__External"] diff --git a/tests/unit_tests/test_arduino8266_framework.py b/tests/unit_tests/test_arduino8266_framework.py index 758ee3bf90..7d9d56796d 100644 --- a/tests/unit_tests/test_arduino8266_framework.py +++ b/tests/unit_tests/test_arduino8266_framework.py @@ -92,7 +92,11 @@ def test_registry_download_bare_string_system() -> None: resp = _registry_response( [ {"system": "linux_x86", "download_url": "http://x/x86"}, - {"system": "linux_x86_64", "download_url": "http://x/x86_64"}, + { + "system": "linux_x86_64", + "download_url": "http://x/x86_64", + "checksum": {"sha256": "abc"}, + }, ] ) with ( @@ -103,15 +107,34 @@ def test_registry_download_bare_string_system() -> None: def test_registry_download_wildcard_system() -> None: - resp = _registry_response([{"system": "*", "download_url": "http://x/any"}]) + resp = _registry_response( + [ + { + "system": "*", + "download_url": "http://x/any", + "checksum": {"sha256": "abc"}, + "size": 7, + } + ] + ) with patch("requests.get", return_value=resp): assert framework._registry_download("pkg", "1.0.0") == ( "http://x/any", - None, - None, + "abc", + 7, ) +def test_registry_download_missing_checksum_raises() -> None: + """An unverifiable archive is refused, never silently extracted.""" + resp = _registry_response([{"system": "*", "download_url": "http://x/any"}]) + with ( + patch("requests.get", return_value=resp), + pytest.raises(EsphomeError, match="no sha256"), + ): + framework._registry_download("pkg", "1.0.0") + + def test_registry_download_no_system_match() -> None: resp = _registry_response( [{"system": ["windows_amd64"], "download_url": "http://x/win"}]