From cb85c8d5a47eb72449fae2e7d1d8c3e67f3c9fa8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 10:32:33 -0500 Subject: [PATCH] Replace the framework patch with a CMake function override --- esphome/build_gen/espidf.py | 45 +++++++++--- esphome/espidf/framework.py | 54 -------------- tests/unit_tests/build_gen/test_espidf.py | 19 +++-- tests/unit_tests/test_espidf_framework.py | 90 ----------------------- 4 files changed, 48 insertions(+), 160 deletions(-) diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index 65a392d45f..5a23fc78eb 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -33,6 +33,36 @@ list(FILTER esphome_cxx_compile_options EXCLUDE REGEX "^-std=") list(APPEND esphome_cxx_compile_options "-std={standard}") idf_build_set_property(CXX_COMPILE_OPTIONS "${{esphome_cxx_compile_options}}")""" +# Drops the app archive from ldgen's inputs (the sections.ld DEPENDS and the +# ldgen_libraries file) by overriding the IDF helper that collects them, so +# app-only edits skip the sections.ld regeneration and ldgen never opens the +# archive. Safe because no linker mapping fragment references the app archive +# (run_compile re-checks that each build); the override filters only the top +# level call, after the recursive walk finished, so the walker's cycle guard +# is untouched. CMake keeps the prior definition reachable with an underscore +# prefix; a future IDF that renames the helper skips the override and keeps +# stock behavior. Emitted after include(project.cmake), before project(). +_LDGEN_OVERRIDE = """\ +if(COMMAND __ldgen_get_lib_deps_of_target) + 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 " + "implementation; set ESPHOME_LDGEN_FULL_DEPS=1 and rebuild.") + endif() + ___ldgen_get_lib_deps_of_target(${target} ${out_list_var}) + if(out_list_var STREQUAL "ldgen_libraries") + 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) + if(esphome_ldgen_before EQUAL esphome_ldgen_after) + message(WARNING "ESPHome ldgen app archive exclusion matched " + "nothing; app edits will regenerate sections.ld.") + endif() + endif() + set(${out_list_var} ${${out_list_var}} PARENT_SCOPE) + endfunction() +endif()""" + def get_available_components() -> list[str] | None: """List the built-in ESP-IDF components from ``project_description.json``. @@ -122,14 +152,9 @@ def get_project_cmakelists( else "" ) - # Honored by the ESPHome-patched ldgen.cmake, a no-op on a stock IDF; - # stops the ~3s sections.ld regeneration on app-only edits. - # ESPHOME_LDGEN_FULL_DEPS=1 restores stock dependencies. - ldgen_dep_exclude = ( - "set(ESPHOME_LDGEN_DEP_EXCLUDE idf::src __idf_src)" - if not get_bool_env("ESPHOME_LDGEN_FULL_DEPS") - else "" - ) + # Stops the ~3s sections.ld regeneration on app-only edits; see + # _LDGEN_OVERRIDE. ESPHOME_LDGEN_FULL_DEPS=1 restores stock behavior. + ldgen_override = "" if get_bool_env("ESPHOME_LDGEN_FULL_DEPS") else _LDGEN_OVERRIDE # CMake variables registered via cg.add_cmake_arg(). Emitted before # include(project.cmake) so values like EXCLUDE_COMPONENTS are already @@ -204,12 +229,12 @@ set(CMAKE_NINJA_FORCE_RESPONSE_FILE 1) set(IDF_TARGET {idf_target}) set(EXTRA_COMPONENT_DIRS ${{CMAKE_SOURCE_DIR}}/src) -{ldgen_dep_exclude} - {cmake_args} include($ENV{{IDF_PATH}}/tools/cmake/project.cmake) +{ldgen_override} + {cpp_standard_options} {cxx_compile_options} diff --git a/esphome/espidf/framework.py b/esphome/espidf/framework.py index 87834fb7bd..6c2a285360 100644 --- a/esphome/espidf/framework.py +++ b/esphome/espidf/framework.py @@ -686,57 +686,6 @@ def _patch_tools_json_demote_unused_tools(framework_path: Path) -> None: ) -_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's inputs. - # ldgen only reads archives named in mapping fragments' archive: lines; - # 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() -""" - - -def _patch_ldgen_cmake(framework_path: Path) -> None: - """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 - are unaffected. An unexpected ldgen.cmake keeps stock behavior. - """ - ldgen_cmake = framework_path / "tools" / "cmake" / "ldgen.cmake" - if not ldgen_cmake.is_file(): - return - try: - content = ldgen_cmake.read_text(encoding="utf-8") - except OSError as e: - _LOGGER.warning( - "Could not apply the ldgen dependency patch to %s (%s); skipping.", - ldgen_cmake, - e, - ) - return - if "ESPHOME_LDGEN_DEP_EXCLUDE" in content: - return - 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).", - ldgen_cmake, - ) - return - write_file_if_changed( - 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) - - def _prefetch_idf_tool_archives( framework_path: Path, targets_str: str, @@ -967,9 +916,6 @@ def _check_esphome_idf_framework_install( # check recovers on the next build. _patch_tools_json_demote_unused_tools(framework_path) - # Applied every invocation so pre-patch trees heal without a clean. - _patch_ldgen_cmake(framework_path) - # 3. Check if the framework tools are the same and correctly installed stored_stamp = None if fresh_extract else _read_stamp(env_stamp_file) install = fresh_extract diff --git a/tests/unit_tests/build_gen/test_espidf.py b/tests/unit_tests/build_gen/test_espidf.py index a39d7add56..e7a4980a10 100644 --- a/tests/unit_tests/build_gen/test_espidf.py +++ b/tests/unit_tests/build_gen/test_espidf.py @@ -9,6 +9,7 @@ from unittest.mock import patch import pytest +from esphome.build_gen import espidf as build_gen_espidf from esphome.components.esp32 import ( KEY_COMPONENTS, KEY_ESP32, @@ -164,19 +165,25 @@ def test_has_discovered_components_after_configure(tmp_path: Path) -> None: @pytest.mark.parametrize("minimal", [False, True]) -def test_get_project_cmakelists_sets_ldgen_dep_exclude(minimal: bool) -> None: - """Both renders exclude the app archive from ldgen's DEPENDS.""" - assert "set(ESPHOME_LDGEN_DEP_EXCLUDE idf::src __idf_src)" in _render( - minimal=minimal +def test_get_project_cmakelists_emits_ldgen_override( + minimal: bool, monkeypatch: pytest.MonkeyPatch +) -> None: + """Both renders override the ldgen dep walker to drop the app archive, + after include(project.cmake) which defines the original.""" + monkeypatch.delenv("ESPHOME_LDGEN_FULL_DEPS", raising=False) + content = _render(minimal=minimal) + assert build_gen_espidf._LDGEN_OVERRIDE in content + assert content.index("tools/cmake/project.cmake") < content.index( + "function(__ldgen_get_lib_deps_of_target" ) def test_get_project_cmakelists_ldgen_full_deps_escape_hatch( monkeypatch: pytest.MonkeyPatch, ) -> None: - """ESPHOME_LDGEN_FULL_DEPS restores stock ldgen dependencies.""" + """ESPHOME_LDGEN_FULL_DEPS restores stock ldgen behavior.""" monkeypatch.setenv("ESPHOME_LDGEN_FULL_DEPS", "true") - assert "ESPHOME_LDGEN_DEP_EXCLUDE" not in _render() + assert "__ldgen_get_lib_deps_of_target" not in _render() def test_get_project_cmakelists_uses_supplied_builtin_components() -> None: diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index 3665d138fa..afa4433aa1 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -20,7 +20,6 @@ from unittest.mock import MagicMock, patch import pytest from esphome.espidf.framework import ( - _LDGEN_DEP_FILTER, ESPHOME_STAMP_FILE, STAMP_SCHEMA_VERSION, _ccache_env, @@ -34,7 +33,6 @@ from esphome.espidf.framework import ( _get_python_env_path, _get_python_version, _parse_git_source, - _patch_ldgen_cmake, _patch_tools_json_demote_unused_tools, _patch_tools_json_for_linux_arm64, _prefetch_idf_tool_archives, @@ -403,7 +401,6 @@ def espidf_mocks(setup_core: Path): patch("esphome.espidf.framework._write_idf_version_txt"), patch("esphome.espidf.framework._patch_tools_json_for_linux_arm64"), patch("esphome.espidf.framework._patch_tools_json_demote_unused_tools"), - patch("esphome.espidf.framework._patch_ldgen_cmake"), patch("esphome.espidf.framework._prefetch_idf_tool_archives"), patch("esphome.espidf.framework._write_stamp"), patch("esphome.espidf.framework._check_stamp", return_value=True), @@ -867,93 +864,6 @@ def test_patch_tools_json_already_patched_is_noop(tmp_path: Path) -> None: assert tools_json.read_text(encoding="utf-8") == before -# --------------------------------------------------------------------------- -# _patch_ldgen_cmake -# --------------------------------------------------------------------------- - - -_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" - DEPENDS ${template} ${ldgen_fragment_files} ${ldgen_deps} ${SDKCONFIG} - VERBATIM - ) -endfunction() -""" - - -def _write_ldgen_cmake(framework_path: Path, content: str) -> Path: - cmake_dir = framework_path / "tools" / "cmake" - cmake_dir.mkdir(parents=True, exist_ok=True) - ldgen_cmake = cmake_dir / "ldgen.cmake" - ldgen_cmake.write_text(content, encoding="utf-8") - return ldgen_cmake - - -def test_patch_ldgen_cmake_inserts_guarded_filter(tmp_path: Path) -> None: - ldgen_cmake = _write_ldgen_cmake(tmp_path, _LDGEN_CMAKE_STOCK) - _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 - - -def test_patch_ldgen_cmake_is_idempotent(tmp_path: Path) -> None: - ldgen_cmake = _write_ldgen_cmake(tmp_path, _LDGEN_CMAKE_STOCK) - _patch_ldgen_cmake(tmp_path) - patched = ldgen_cmake.read_text(encoding="utf-8") - _patch_ldgen_cmake(tmp_path) - assert ldgen_cmake.read_text(encoding="utf-8") == patched - - -def test_patch_ldgen_cmake_missing_file_is_noop(tmp_path: Path) -> None: - _patch_ldgen_cmake(tmp_path) # no tools/cmake/ldgen.cmake present - - -def test_patch_ldgen_cmake_unreadable_file_warns_and_skips( - tmp_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - _write_ldgen_cmake(tmp_path, _LDGEN_CMAKE_STOCK) - with patch.object(Path, "read_text", side_effect=OSError("boom")): - _patch_ldgen_cmake(tmp_path) - assert "Could not apply the ldgen dependency patch" in caplog.text - - -@pytest.mark.parametrize( - "content", - [ - pytest.param( - _LDGEN_CMAKE_STOCK.replace("${ldgen_deps}", "${other_deps}"), - id="no_ldgen_deps", - ), - pytest.param( - _LDGEN_CMAKE_STOCK - + '\n list(JOIN ldgen_libraries_expr "\\n" ldgen_libraries_str)\n', - id="duplicate_join", - ), - ], -) -def test_patch_ldgen_cmake_unexpected_layout_skips( - content: str, tmp_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - """A future IDF that restructures ldgen.cmake keeps stock behavior.""" - ldgen_cmake = _write_ldgen_cmake(tmp_path, content) - _patch_ldgen_cmake(tmp_path) - assert ldgen_cmake.read_text(encoding="utf-8") == content - assert "does not match the expected layout" in caplog.text - - # --------------------------------------------------------------------------- # _prefetch_idf_tool_archives # ---------------------------------------------------------------------------