From c290a508f63d4d0dfac6b037b4e6ecdb484111a4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 10:24:26 -0500 Subject: [PATCH] Drop the app archive from ldgen_libraries too and add a fragment check --- esphome/espidf/framework.py | 20 +++++++----- esphome/espidf/toolchain.py | 35 +++++++++++++++++++- tests/unit_tests/test_espidf_framework.py | 12 +++++-- tests/unit_tests/test_espidf_toolchain.py | 39 +++++++++++++++++++++++ 4 files changed, 95 insertions(+), 11 deletions(-) diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index cca32b52fb..87834fb7bd 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -686,21 +686,24 @@ def _patch_tools_json_demote_unused_tools(framework_path: Path) -> None: ) -_LDGEN_COMMAND_ANCHOR = " add_custom_command(\n" +_LDGEN_JOIN_ANCHOR = ' list(JOIN ldgen_libraries_expr "\\n" ldgen_libraries_str)\n' _LDGEN_DEP_FILTER = """\ - # Patched by ESPHome: drop app-only archives from ldgen DEPENDS. + # Patched by ESPHome: drop app-only archives from ldgen's inputs. # ldgen only reads archives named in mapping fragments' archive: lines; - # the app component never appears there, so rebuilding it cannot change - # the generated linker script. Set by the ESPHome project CMakeLists. + # the app component never appears there, so it cannot change the output, + # and keeping it listed would race ldgen's objdump against the archive + # being written. Set by the ESPHome project CMakeLists. if(ESPHOME_LDGEN_DEP_EXCLUDE) list(REMOVE_ITEM ldgen_deps ${ESPHOME_LDGEN_DEP_EXCLUDE}) + foreach(esphome_ldgen_excl ${ESPHOME_LDGEN_DEP_EXCLUDE}) + list(REMOVE_ITEM ldgen_libraries_expr "$") + endforeach() endif() - add_custom_command( """ def _patch_ldgen_cmake(framework_path: Path) -> None: - """Let projects drop their app archive from the sections.ld DEPENDS. + """Let projects drop their app archive from ldgen's inputs. Guarded by ESPHOME_LDGEN_DEP_EXCLUDE, which only the ESPHome project CMakeLists sets, so stock IDF builds using the shared framework copy @@ -720,7 +723,7 @@ def _patch_ldgen_cmake(framework_path: Path) -> None: return if "ESPHOME_LDGEN_DEP_EXCLUDE" in content: return - if "${ldgen_deps}" not in content or content.count(_LDGEN_COMMAND_ANCHOR) != 1: + if "${ldgen_deps}" not in content or content.count(_LDGEN_JOIN_ANCHOR) != 1: _LOGGER.warning( "ldgen.cmake at %s does not match the expected layout; " "skipping the ldgen dependency patch (builds stay correct).", @@ -728,7 +731,8 @@ def _patch_ldgen_cmake(framework_path: Path) -> None: ) return write_file_if_changed( - ldgen_cmake, content.replace(_LDGEN_COMMAND_ANCHOR, _LDGEN_DEP_FILTER) + ldgen_cmake, + content.replace(_LDGEN_JOIN_ANCHOR, _LDGEN_DEP_FILTER + _LDGEN_JOIN_ANCHOR), ) _LOGGER.info("Patched %s to honor ESPHOME_LDGEN_DEP_EXCLUDE.", ldgen_cmake) diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 986f9dfb8b..fce99c2e24 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -24,7 +24,7 @@ from esphome.core import CORE, EsphomeError from esphome.espidf import variant_to_idf_target from esphome.espidf.framework import check_esp_idf_install, get_framework_env from esphome.espidf.size_summary import print_summary -from esphome.helpers import add_git_ceiling_directory, write_file +from esphome.helpers import add_git_ceiling_directory, get_bool_env, write_file _LOGGER = logging.getLogger(__name__) @@ -479,6 +479,36 @@ def _patch_memory_segments(): _LOGGER.warning("Could not patch memory segments in %s", memory_ld) +_LDGEN_FRAGMENTS_RE = re.compile(r'--fragments-list\s+"([^"]+)"') +_APP_ARCHIVE_MAPPED_RE = re.compile(r"^archive:\s*libsrc\.a\s*$", re.MULTILINE) + + +def _warn_if_app_archive_mapped() -> None: + """Belt for the ldgen dependency exclusion (see build_gen/espidf.py). + + The exclusion is safe only while no linker fragment names the app + archive; if one ever did, ldgen would silently skip remapping it, so + surface that loudly instead of relying on the invariant forever. + """ + 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") + ): + _LOGGER.warning( + "Linker fragment %s maps the app archive; its entries may be " + "skipped. Set ESPHOME_LDGEN_FULL_DEPS=1 and rebuild.", + fragment, + ) + return + except OSError as e: + _LOGGER.debug("Skipping ldgen fragment check: %s", e) + + def run_compile(config, verbose: bool) -> int: """Compile the ESP-IDF project. @@ -505,6 +535,9 @@ def run_compile(config, verbose: bool) -> int: if path.is_file(): os.utime(path) + if not get_bool_env("ESPHOME_LDGEN_FULL_DEPS"): + _warn_if_app_archive_mapped() + # In testing mode, generate the linker script first, patch DRAM/IRAM sizes, # then build. memory.ld is regenerated by ninja during the build phase, # so we must patch after it's generated but before linking (same timing diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index 3411eb6584..3665d138fa 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -876,6 +876,9 @@ _LDGEN_CMAKE_STOCK = """\ function(__ldgen_create_target exe_target) idf_build_get_property(python PYTHON) + list(JOIN ldgen_libraries_expr "\\n" ldgen_libraries_str) + file(WRITE ${build_dir}/ldgen_libraries.in "${ldgen_libraries_str}") + add_custom_command( OUTPUT ${output} COMMAND ${python} "${idf_path}/tools/ldgen/ldgen.py" @@ -899,6 +902,10 @@ def test_patch_ldgen_cmake_inserts_guarded_filter(tmp_path: Path) -> None: _patch_ldgen_cmake(tmp_path) content = ldgen_cmake.read_text(encoding="utf-8") assert _LDGEN_DEP_FILTER in content + # The libraries list must be filtered before it is serialized to disk + assert content.index("REMOVE_ITEM ldgen_libraries_expr") < content.index( + "list(JOIN ldgen_libraries_expr" + ) assert "DEPENDS ${template}" in content @@ -931,8 +938,9 @@ def test_patch_ldgen_cmake_unreadable_file_warns_and_skips( id="no_ldgen_deps", ), pytest.param( - _LDGEN_CMAKE_STOCK + "\n add_custom_command(\n OUTPUT x)\n", - id="duplicate_command", + _LDGEN_CMAKE_STOCK + + '\n list(JOIN ldgen_libraries_expr "\\n" ldgen_libraries_str)\n', + id="duplicate_join", ), ], ) diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index 9deb27d83c..622327bb3d 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -623,6 +623,45 @@ def test_component_cache_ignores_corrupt_file(setup_core: Path, tmp_path: Path) assert toolchain.load_cached_builtin_components() is None +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) + frag_list = ";".join(str(f) for f in fragments) + (build_dir / "build.ninja").write_text( + f' COMMAND = python ldgen.py --fragments-list "{frag_list}" --input x\n' + ) + + +def test_warn_if_app_archive_mapped_warns( + setup_core: Path, tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A fragment naming the app archive 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") + _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_clean( + setup_core: Path, tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Normal fragments produce no warning; missing files are non-fatal.""" + _setup_build(setup_core) + frag = tmp_path / "linker.lf" + frag.write_text("[mapping:freertos]\narchive: libfreertos.a\n") + _write_fragments_build_ninja(tmp_path, [frag, tmp_path / "missing.lf"]) + toolchain._warn_if_app_archive_mapped() + assert "maps the app archive" not in caplog.text + + +def test_warn_if_app_archive_mapped_no_build_ninja(setup_core: Path) -> None: + """No build.ninja yet is a quiet no-op.""" + _setup_build(setup_core) + toolchain._warn_if_app_archive_mapped() + + def test_run_compile_passes_compile_process_limit(setup_core: Path) -> None: """compile_process_limit is forwarded to run_idf_py as the job limit.""" _setup_build(setup_core)