From 4a815946e1a192c308bf0213b31078e3263b5ddb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 11:36:06 -0500 Subject: [PATCH] Close the remaining silent degradation paths --- esphome/build_gen/espidf.py | 24 +++++++-- esphome/espidf/toolchain.py | 50 ++++++++++++------- tests/unit_tests/build_gen/test_espidf.py | 8 ++- tests/unit_tests/test_espidf_toolchain.py | 59 ++++++++++++++++++++--- 4 files changed, 112 insertions(+), 29 deletions(-) diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index b87a60e2f4..87aa1e0690 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -39,6 +39,7 @@ idf_build_set_property(CXX_COMPILE_OPTIONS "${{esphome_cxx_compile_options}}")"" # the prior definition stays reachable with an underscore prefix. _LDGEN_OVERRIDE = """\ if(COMMAND __ldgen_get_lib_deps_of_target) + set_property(GLOBAL PROPERTY ESPHOME_LDGEN_ARMED 1) function(__ldgen_get_lib_deps_of_target target out_list_var) if(NOT COMMAND ___ldgen_get_lib_deps_of_target) message(FATAL_ERROR "ESPHome ldgen override lost the original " @@ -46,6 +47,7 @@ if(COMMAND __ldgen_get_lib_deps_of_target) endif() ___ldgen_get_lib_deps_of_target(${target} ${out_list_var}) if(out_list_var STREQUAL "ldgen_libraries") + set_property(GLOBAL PROPERTY ESPHOME_LDGEN_FILTERED 1) list(LENGTH ${out_list_var} esphome_ldgen_before) list(REMOVE_ITEM ${out_list_var} idf::src __idf_src) list(LENGTH ${out_list_var} esphome_ldgen_after) @@ -61,6 +63,16 @@ else() "app edits will regenerate sections.ld.") endif()""" +# Runs after project() so the walk has happened; catches the remaining +# silent path where the top-level out-var was renamed. +_LDGEN_OVERRIDE_CHECK = """\ +get_property(esphome_ldgen_armed GLOBAL PROPERTY ESPHOME_LDGEN_ARMED) +get_property(esphome_ldgen_filtered GLOBAL PROPERTY ESPHOME_LDGEN_FILTERED) +if(esphome_ldgen_armed AND NOT esphome_ldgen_filtered) + message(@SEVERITY@ "ESPHome ldgen override never filtered the app " + "archive; app edits will regenerate sections.ld.") +endif()""" + def get_available_components() -> list[str] | None: """List the built-in ESP-IDF components from ``project_description.json``. @@ -156,11 +168,15 @@ def get_project_cmakelists( # breaks the override instead of degrading to stock deps. if get_bool_env("ESPHOME_LDGEN_FULL_DEPS"): ldgen_override = "" + ldgen_override_check = "" else: strict = get_bool_env("ESPHOME_LDGEN_STRICT") - ldgen_override = _LDGEN_OVERRIDE.replace( - "@SEVERITY@", "FATAL_ERROR" if strict else "WARNING" - ).replace("@MISSING@", "FATAL_ERROR" if strict else "STATUS") + severity = "FATAL_ERROR" if strict else "WARNING" + missing = "FATAL_ERROR" if strict else "STATUS" + ldgen_override = _LDGEN_OVERRIDE.replace("@SEVERITY@", severity).replace( + "@MISSING@", missing + ) + ldgen_override_check = _LDGEN_OVERRIDE_CHECK.replace("@SEVERITY@", severity) # CMake variables registered via cg.add_cmake_arg(). Emitted before # include(project.cmake) so values like EXCLUDE_COMPONENTS are already @@ -253,6 +269,8 @@ include($ENV{{IDF_PATH}}/tools/cmake/project.cmake) project({CORE.name}) +{ldgen_override_check} + # Emit raw JSON size data for ESPHome to read post-build. add_custom_command( TARGET ${{CMAKE_PROJECT_NAME}}.elf POST_BUILD diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 23e1d454ba..3906c469bf 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -480,7 +480,14 @@ def _patch_memory_segments(): _LDGEN_FRAGMENTS_RE = re.compile(r'--fragments-list\s+"([^"]+)"') -_APP_ARCHIVE_MAPPED_RE = re.compile(r"^archive:\s*libsrc\.a\s*$", re.MULTILINE) +_APP_ARCHIVE_MAPPED_RE = re.compile(r"^\s*archive:\s*libsrc\.a\b", re.MULTILINE) + + +def _ldgen_check_skip(msg: str, strict: bool) -> None: + """A skipped fragment check is debug for users, fatal under strict.""" + if strict: + raise EsphomeError(f"ldgen fragment check: {msg} (ESPHOME_LDGEN_STRICT)") + _LOGGER.debug("Skipping ldgen fragment check: %s", msg) def _warn_if_app_archive_mapped() -> None: @@ -488,26 +495,33 @@ def _warn_if_app_archive_mapped() -> None: linker fragment names the app archive, since ldgen would silently skip remapping it rather than fail. """ + strict = get_bool_env("ESPHOME_LDGEN_STRICT") build_ninja = CORE.relative_build_path("build", "build.ninja") try: - match = _LDGEN_FRAGMENTS_RE.search(build_ninja.read_text(encoding="utf-8")) - if match is None: - return - for fragment in match.group(1).split(";"): - if _APP_ARCHIVE_MAPPED_RE.search( - Path(fragment).read_text(encoding="utf-8") - ): - msg = ( - f"Linker fragment {fragment} maps the app archive; its " - "entries may be skipped. Set ESPHOME_LDGEN_FULL_DEPS=1 " - "and rebuild." - ) - if get_bool_env("ESPHOME_LDGEN_STRICT"): - raise EsphomeError(msg) - _LOGGER.warning("%s", msg) - return + ninja_text = build_ninja.read_text(encoding="utf-8", errors="replace") except OSError as e: - _LOGGER.debug("Skipping ldgen fragment check: %s", e) + _ldgen_check_skip(f"could not read {build_ninja}: {e}", strict) + return + match = _LDGEN_FRAGMENTS_RE.search(ninja_text) + if match is None: + _ldgen_check_skip(f"no --fragments-list in {build_ninja}", strict) + return + for fragment in match.group(1).split(";"): + try: + text = Path(fragment).read_text(encoding="utf-8", errors="replace") + except OSError as e: + _ldgen_check_skip(f"could not read {fragment}: {e}", strict) + continue + if _APP_ARCHIVE_MAPPED_RE.search(text): + msg = ( + f"Linker fragment {fragment} maps the app archive; its " + "entries may be skipped. Set ESPHOME_LDGEN_FULL_DEPS=1 " + "and rebuild." + ) + if strict: + raise EsphomeError(msg) + _LOGGER.warning("%s", msg) + return def run_compile(config, verbose: bool) -> int: diff --git a/tests/unit_tests/build_gen/test_espidf.py b/tests/unit_tests/build_gen/test_espidf.py index 952c8323b9..4adb3d7884 100644 --- a/tests/unit_tests/build_gen/test_espidf.py +++ b/tests/unit_tests/build_gen/test_espidf.py @@ -175,9 +175,12 @@ def test_get_project_cmakelists_emits_ldgen_override( assert "REMOVE_ITEM ${out_list_var} idf::src __idf_src" in content assert 'message(WARNING "ESPHome ldgen app archive exclusion' in content assert 'message(STATUS "ESPHome ldgen override target not found' in content + assert 'message(WARNING "ESPHome ldgen override never filtered' in content assert content.index("tools/cmake/project.cmake") < content.index( "function(__ldgen_get_lib_deps_of_target" ) + # The never-filtered check must run after project() has walked the deps + assert content.index("project(test)") < content.index("esphome_ldgen_armed GLOBAL") def test_get_project_cmakelists_ldgen_strict_fails_closed( @@ -190,6 +193,7 @@ def test_get_project_cmakelists_ldgen_strict_fails_closed( content = _render() assert 'message(FATAL_ERROR "ESPHome ldgen app archive exclusion' in content assert 'message(FATAL_ERROR "ESPHome ldgen override target not found' in content + assert 'message(FATAL_ERROR "ESPHome ldgen override never filtered' in content assert "@SEVERITY@" not in content assert "@MISSING@" not in content @@ -199,7 +203,9 @@ def test_get_project_cmakelists_ldgen_full_deps_escape_hatch( ) -> None: """ESPHOME_LDGEN_FULL_DEPS restores stock ldgen behavior.""" monkeypatch.setenv("ESPHOME_LDGEN_FULL_DEPS", "true") - assert "__ldgen_get_lib_deps_of_target" not in _render() + content = _render() + assert "__ldgen_get_lib_deps_of_target" not in content + assert "esphome_ldgen_armed" not in content def test_get_project_cmakelists_uses_supplied_builtin_components() -> None: diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index 0a24062217..66bcf43450 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -623,6 +623,13 @@ def test_component_cache_ignores_corrupt_file(setup_core: Path, tmp_path: Path) assert toolchain.load_cached_builtin_components() is None +@pytest.fixture(autouse=True) +def _clear_ldgen_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Isolate tests from ambient ldgen escape hatch and strict knobs.""" + monkeypatch.delenv("ESPHOME_LDGEN_STRICT", raising=False) + monkeypatch.delenv("ESPHOME_LDGEN_FULL_DEPS", raising=False) + + def _write_fragments_build_ninja(tmp_path: Path, fragments: list[Path]) -> None: build_dir = CORE.relative_build_path("build") build_dir.mkdir(parents=True, exist_ok=True) @@ -633,21 +640,43 @@ def _write_fragments_build_ninja(tmp_path: Path, fragments: list[Path]) -> None: def test_warn_if_app_archive_mapped_warns( - setup_core: Path, - tmp_path: Path, - caplog: pytest.LogCaptureFixture, - monkeypatch: pytest.MonkeyPatch, + setup_core: Path, tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """A fragment naming the app archive triggers the loud warning.""" - monkeypatch.delenv("ESPHOME_LDGEN_STRICT", raising=False) + """A fragment naming the app archive, even with trailing text or leading + whitespace, triggers the loud warning.""" _setup_build(setup_core) frag = tmp_path / "linker.lf" - frag.write_text("[mapping:evil]\narchive: libsrc.a\nentries:\n * (noflash)\n") + frag.write_text("[mapping:evil]\n archive: libsrc.a # app\nentries:\n") _write_fragments_build_ninja(tmp_path, [frag]) toolchain._warn_if_app_archive_mapped() assert "maps the app archive" in caplog.text +def test_warn_if_app_archive_mapped_scans_past_unreadable( + setup_core: Path, tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """An unreadable fragment doesn't stop later fragments being checked.""" + _setup_build(setup_core) + frag = tmp_path / "linker.lf" + frag.write_text("[mapping:evil]\narchive: libsrc.a\n") + _write_fragments_build_ninja(tmp_path, [tmp_path / "missing.lf", frag]) + toolchain._warn_if_app_archive_mapped() + assert "maps the app archive" in caplog.text + + +def test_warn_if_app_archive_mapped_strict_no_fragments_list( + setup_core: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Under strict, a build.ninja the check can't parse fails the build.""" + monkeypatch.setenv("ESPHOME_LDGEN_STRICT", "1") + _setup_build(setup_core) + build_dir = CORE.relative_build_path("build") + build_dir.mkdir(parents=True, exist_ok=True) + (build_dir / "build.ninja").write_text("rule CXX\n command = gcc\n") + with pytest.raises(EsphomeError, match="no --fragments-list"): + toolchain._warn_if_app_archive_mapped() + + def test_warn_if_app_archive_mapped_strict_raises( setup_core: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -698,6 +727,22 @@ def test_warn_if_app_archive_mapped_no_fragments_list(setup_core: Path) -> None: toolchain._warn_if_app_archive_mapped() +def test_run_compile_runs_fragment_check(setup_core: Path) -> None: + """The fragment belt runs by default on every compile.""" + _setup_build(setup_core) + config = {CONF_ESPHOME: {}} + + with ( + patch.object(toolchain, "need_reconfigure", return_value=False), + patch.object(toolchain, "run_idf_py", return_value=0), + patch.object(toolchain, "print_summary"), + patch.object(toolchain, "_warn_if_app_archive_mapped") as mock_check, + ): + assert toolchain.run_compile(config, verbose=False) == 0 + + mock_check.assert_called_once() + + def test_run_compile_full_deps_skips_fragment_check( setup_core: Path, monkeypatch: pytest.MonkeyPatch ) -> None: