diff --git a/.github/workflows/ci-docker.yml b/.github/workflows/ci-docker.yml index 2784f13511..5bcdb22cb8 100644 --- a/.github/workflows/ci-docker.yml +++ b/.github/workflows/ci-docker.yml @@ -219,5 +219,6 @@ jobs: run: | docker run --rm \ -v "${{ github.workspace }}/docker/test_configs:/config" \ + -e ESPHOME_LDGEN_STRICT=1 \ "ghcr.io/esphome/esphome-amd64:${{ needs.check-docker.outputs.tag }}" \ compile "${{ matrix.id }}.yaml" diff --git a/esphome/build_gen/espidf.py b/esphome/build_gen/espidf.py index 9f48013a4e..124fa3ddb1 100644 --- a/esphome/build_gen/espidf.py +++ b/esphome/build_gen/espidf.py @@ -20,7 +20,7 @@ from esphome.framework_helpers import ( get_project_cxx_compile_flags, get_project_link_flags, ) -from esphome.helpers import mkdir_p, write_file_if_changed +from esphome.helpers import get_bool_env, mkdir_p, write_file_if_changed _LOGGER = logging.getLogger(__name__) @@ -35,6 +35,46 @@ 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 so app-only edits skip the +# sections.ld regeneration. Safe: no mapping fragment references it +# (run_compile re-checks each build). Filters only the top-level call; +# 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 " + "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") + 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) + if(esphome_ldgen_before EQUAL esphome_ldgen_after) + message(@SEVERITY@ "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() +else() + message(@MISSING@ "ESPHome ldgen override target not found; " + "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``. @@ -125,6 +165,22 @@ def get_project_cmakelists( else "" ) + # Stops the ~3s sections.ld regeneration on app-only edits; see + # _LDGEN_OVERRIDE. ESPHOME_LDGEN_FULL_DEPS=1 restores stock behavior; + # ESPHOME_LDGEN_STRICT=1 (CI) fails the configure when an IDF bump + # 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") + 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 # set when project.cmake seeds the component list, and on minimal @@ -202,6 +258,8 @@ set(EXTRA_COMPONENT_DIRS ${{CMAKE_SOURCE_DIR}}/src) include($ENV{{IDF_PATH}}/tools/cmake/project.cmake) +{ldgen_override} + {cpp_standard_options} {cxx_compile_options} @@ -214,6 +272,8 @@ include($ENV{{IDF_PATH}}/tools/cmake/project.cmake) project({CORE.name}) +{ldgen_override_check} + # Emit per-memory-type JSON size data for ESPHome to read post-build. # json2 stays small; raw dumps every symbol (~2s on a large map) and # this command runs inside the link edge, blocking everything downstream. diff --git a/esphome/espidf/toolchain.py b/esphome/espidf/toolchain.py index 4243eff0ba..d73dbf6b7e 100644 --- a/esphome/espidf/toolchain.py +++ b/esphome/espidf/toolchain.py @@ -1,6 +1,7 @@ """ESP-IDF direct build API for ESPHome.""" from dataclasses import dataclass, field +import fnmatch import hashlib import json import logging @@ -25,7 +26,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__) @@ -647,6 +648,65 @@ def _patch_memory_segments(): _LOGGER.warning("Could not patch memory segments in %s", memory_ld) +_LDGEN_FRAGMENTS_RE = re.compile(r'--fragments-list\s+"([^"]+)"') +_LDGEN_ARCHIVE_RE = re.compile(r"^\s*archive:\s*(\S+)", re.MULTILINE) + + +def _fragment_maps_app_archive(text: str) -> bool: + """True when an archive: spec selects libsrc.a, the archive of the src + component excluded as idf::src/__idf_src in build_gen/espidf.py. + + The bare * is IDF's stock catch-all; its archive-level entries resolve + in the linker against all link inputs, so it stays safe when the + archive is excluded from ldgen's own inputs. + """ + return any( + value != "*" and fnmatch.fnmatch("libsrc.a", value) + for value in _LDGEN_ARCHIVE_RE.findall(text) + ) + + +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: + """Belt for the ldgen exclusion (see build_gen/espidf.py): warn if any + 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: + ninja_text = build_ninja.read_text(encoding="utf-8", errors="replace") + except OSError as 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 _fragment_maps_app_archive(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: """Compile the ESP-IDF project. @@ -677,6 +737,9 @@ def run_compile(config, verbose: bool) -> int: else: _LOGGER.info("Build configuration is up to date") + 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/build_gen/test_espidf.py b/tests/unit_tests/build_gen/test_espidf.py index 4a84284796..7d53ee1e5e 100644 --- a/tests/unit_tests/build_gen/test_espidf.py +++ b/tests/unit_tests/build_gen/test_espidf.py @@ -163,6 +163,53 @@ def test_has_discovered_components_after_configure(tmp_path: Path) -> None: assert has_discovered_components() +@pytest.mark.parametrize("minimal", [False, True]) +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) + monkeypatch.delenv("ESPHOME_LDGEN_STRICT", raising=False) + content = _render(minimal=minimal) + assert "REMOVE_ITEM ${out_list_var} idf::src __idf_src" in content + # Quoted so spaced elements survive and an empty list stays defined + assert 'set(${out_list_var} "${${out_list_var}}" PARENT_SCOPE)' 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( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """ESPHOME_LDGEN_STRICT turns both degradation paths into hard errors so + CI fails right away when an IDF bump breaks the override.""" + monkeypatch.delenv("ESPHOME_LDGEN_FULL_DEPS", raising=False) + monkeypatch.setenv("ESPHOME_LDGEN_STRICT", "1") + 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 + + +def test_get_project_cmakelists_ldgen_full_deps_escape_hatch( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """ESPHOME_LDGEN_FULL_DEPS restores stock ldgen behavior.""" + monkeypatch.setenv("ESPHOME_LDGEN_FULL_DEPS", "true") + content = _render() + assert "__ldgen_get_lib_deps_of_target" not in content + assert "esphome_ldgen_armed" not in content + + def test_get_project_cmakelists_size_command_uses_json2() -> None: """The POST_BUILD size command uses the cheap json2 format, with --ng only on the 1.x tool bundled with IDF < 6.""" diff --git a/tests/unit_tests/test_espidf_toolchain.py b/tests/unit_tests/test_espidf_toolchain.py index c012785e89..1b40460b91 100644 --- a/tests/unit_tests/test_espidf_toolchain.py +++ b/tests/unit_tests/test_espidf_toolchain.py @@ -630,6 +630,154 @@ 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) + 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, 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]\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: + """Under ESPHOME_LDGEN_STRICT a mapped app archive fails the build.""" + monkeypatch.setenv("ESPHOME_LDGEN_STRICT", "1") + _setup_build(setup_core) + frag = tmp_path / "linker.lf" + frag.write_text("[mapping:evil]\narchive: libsrc.a\n") + _write_fragments_build_ninja(tmp_path, [frag]) + with pytest.raises(EsphomeError, match="maps the app archive"): + toolchain._warn_if_app_archive_mapped() + + +def test_warn_if_app_archive_mapped_glob( + setup_core: Path, tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A glob archive spec that selects the app archive is also flagged.""" + _setup_build(setup_core) + frag = tmp_path / "linker.lf" + frag.write_text("[mapping:evil]\narchive: lib*\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, including IDF's stock archive: * catch-all, + produce no warning.""" + _setup_build(setup_core) + frag = tmp_path / "linker.lf" + frag.write_text( + "[mapping:freertos]\narchive: libfreertos.a\n[mapping:default]\narchive: *\n" + ) + _write_fragments_build_ninja(tmp_path, [frag]) + toolchain._warn_if_app_archive_mapped() + assert "maps the app archive" not in caplog.text + + +def test_warn_if_app_archive_mapped_missing_fragment( + setup_core: Path, tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """An unreadable fragment file is non-fatal.""" + _setup_build(setup_core) + _write_fragments_build_ninja(tmp_path, [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_warn_if_app_archive_mapped_no_fragments_list(setup_core: Path) -> None: + """A build.ninja without a fragments-list argument is a quiet no-op.""" + _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") + 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) + + with ( + _up_to_date_compile(), + patch.object(toolchain, "_warn_if_app_archive_mapped") as mock_check, + ): + assert toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) == 0 + + mock_check.assert_called_once() + + +def test_run_compile_full_deps_skips_fragment_check( + setup_core: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ESPHOME_LDGEN_FULL_DEPS disables the fragment belt with the override.""" + monkeypatch.setenv("ESPHOME_LDGEN_FULL_DEPS", "1") + _setup_build(setup_core) + + with ( + _up_to_date_compile(), + patch.object(toolchain, "_warn_if_app_archive_mapped") as mock_check, + ): + assert toolchain.run_compile({CONF_ESPHOME: {}}, verbose=False) == 0 + + mock_check.assert_not_called() + + @pytest.mark.parametrize("limit", [1, None]) def test_run_compile_passes_compile_process_limit( setup_core: Path, limit: int | None