From 417fdd9dc76ef0a5a29e151cbf32a43e10d068c1 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 5 Oct 2026 10:53:35 -0500 Subject: [PATCH] [nrf52] Share the ccache between devices (#20028) --- esphome/components/nrf52/__init__.py | 19 +++-- esphome/components/nrf52/framework.py | 78 ++++++++++++++++--- tests/unit_tests/components/nrf52/test_pch.py | 75 ++++++++++++++---- tests/unit_tests/test_nrf52_framework.py | 49 +++++++++++- 4 files changed, 183 insertions(+), 38 deletions(-) diff --git a/esphome/components/nrf52/__init__.py b/esphome/components/nrf52/__init__.py index 1e841fffcf..86d485777c 100644 --- a/esphome/components/nrf52/__init__.py +++ b/esphome/components/nrf52/__init__.py @@ -27,6 +27,7 @@ from esphome.components.zephyr.const import ( CONF_CDC_ACM, KEY_BOARD, KEY_BOOTLOADER, + KEY_SYSBUILD, KEY_ZEPHYR, CdcAcm, ) @@ -77,6 +78,7 @@ from .framework import ( get_build_paths, setup_platformio_python_env, toolchain_tool, + wanted_west_projects, ) # force import gpio to register pin schema @@ -822,19 +824,22 @@ _PCH_SUM_PATH = "CMakeFiles/app.dir/cmake_pch.hxx.gch.sum" def _write_pch_checksum(build_dir: Path, source_dir: Path) -> None: - """Write the checksum ccache reads in place of the .gch. The app binary - dir only exists after the first configure; sysbuild nests it.""" - app_dir = build_dir / "zephyr" - if not (app_dir / "CMakeCache.txt").is_file(): - app_dir = build_dir - if not (app_dir / "CMakeCache.txt").is_file(): - return + """Write the checksum ccache reads in place of the .gch; before the + first build too, or its compiles hash the path laden .gch instead. + The app image dir follows the SDK version, like get_elf_path; + 2.9.2+ always wraps the build in sysbuild.""" + app_dir = build_dir + if CORE.data[KEY_CORE][KEY_FRAMEWORK_VERSION] >= cv.Version(2, 9, 2): + app_dir = build_dir / "zephyr" checksum = pch.pch_checksum( CORE.relative_src_path(), pch.PCH_DEFAULT_HEADERS, ( str(CORE.data[KEY_CORE][KEY_FRAMEWORK_VERSION]), zephyr_data()[KEY_BOARD], + # Kconfig inputs that reach autoconf.h without a .conf line + ",".join(sorted(wanted_west_projects())), + str(zephyr_data().get(KEY_SYSBUILD)), # What the Zephyr configuration is generated from *( path.read_text(encoding="utf-8") diff --git a/esphome/components/nrf52/framework.py b/esphome/components/nrf52/framework.py index e6ccde6a2d..00796562f4 100644 --- a/esphome/components/nrf52/framework.py +++ b/esphome/components/nrf52/framework.py @@ -225,6 +225,19 @@ def get_build_env(ccache: str | None) -> dict: env.setdefault("CCACHE_DISABLE", "1") else: env.update(ccache_env(ccache, SDK_NRF_TOOLS_CACHE)) + # Drop only the per build map entry (posix, CMake's spelling); + # its from side covers no compiled sources. A spaced path cannot + # survive ccache's space split list, so it stays hashed. + source_dir = CORE.relative_build_path("zephyr").as_posix() + if any(ch.isspace() for ch in source_dir): + _LOGGER.debug( + "Whitespace in %s; the per build map stays hashed", source_dir + ) + else: + device_map = f"-fmacro-prefix-map={source_dir}=CMAKE_SOURCE_DIR" + env["CCACHE_IGNOREOPTIONS"] = ( + f"{env.get('CCACHE_IGNOREOPTIONS', '')} {device_map}".strip() + ) return env @@ -301,21 +314,62 @@ def setup_platformio_python_env() -> None: _prepend_env_path("PATH", str(env_python_path.parent)) +def _patch_framework_file(path: Path, old: str, new: str) -> bool: + """Replace ``old`` with ``new`` in a framework script, atomically and + keeping the file mode (helpers.write_file would flatten it to 0o644). + Returns False when nothing matched.""" + import tempfile + + content = path.read_text(encoding="utf-8") + patched = content.replace(old, new) + if patched == content: + return False + # Unique sibling tmp: the install lock is best effort, so two builds + # may patch at once and a shared tmp name could rename a half + # written file into place. + fd, tmp_name = tempfile.mkstemp(dir=path.parent, suffix=".tmp") + tmp = Path(tmp_name) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(patched) + shutil.copymode(path, tmp) + tmp.replace(path) + except BaseException: + tmp.unlink(missing_ok=True) + raise + return True + + def _patch_uf2conv_escape_sequences(framework_path: Path) -> None: # SDK v2.6.1 ships uf2conv.py with '\s+' — an unrecognised escape that # Python 3.12+ flags with SyntaxWarning (a future version will reject it). uf2conv = framework_path / "zephyr" / "scripts" / "build" / "uf2conv.py" - if not uf2conv.exists(): + if uf2conv.exists(): + _patch_framework_file( + uf2conv, "re.split('\\s+', line)", "re.split('\\\\s+', line)" + ) + + +def _patch_gen_defines_dts_path(framework_path: Path) -> None: + # The absolute zephyr.dts.pre path in the header's top comment is + # its only per device byte and blocks ccache sharing; emit the + # basename. Upstream candidate. + gen_defines = framework_path / "zephyr" / "scripts" / "dts" / "gen_defines.py" + if not gen_defines.exists(): return - content = uf2conv.read_text(encoding="utf-8") - patched = content.replace("re.split('\\s+', line)", "re.split('\\\\s+', line)") - if patched == content: + if _patch_framework_file( + gen_defines, " {edt.dts_path}", " {os.path.basename(edt.dts_path)}" + ): return - # Write atomically so a concurrent build never sees a truncated file - tmp = uf2conv.with_suffix(".py.tmp") - tmp.write_text(patched, encoding="utf-8") - shutil.copymode(uf2conv, tmp) - tmp.replace(uf2conv) + if "{os.path.basename(edt.dts_path)}" not in gen_defines.read_text( + encoding="utf-8" + ): + # Upstream reformatted the comment; sharing silently degrading + # would be invisible, so say it out loud. + _LOGGER.warning( + "gen_defines.py no longer matches; the devicetree header " + "stays per device and ccache sharing between devices degrades" + ) # West projects every build needs; components add others with include_west_project() @@ -349,7 +403,7 @@ def bluetooth_west_projects() -> tuple[str, ...]: return ("tinycrypt",) -def _wanted_west_projects() -> set[str]: +def wanted_west_projects() -> set[str]: projects = set(_get_data().west_projects) # Zephyr 4.1 moved the Cortex-M core headers to cmsis_6 if CORE.data[KEY_CORE][KEY_FRAMEWORK_VERSION] >= cv.Version(3, 1, 0): @@ -587,7 +641,7 @@ def _check_and_install(version: str) -> None: framework_path = _get_framework_path(version) sentinel = framework_path / ".ready" zephyr_reqs = framework_path / "zephyr" / "scripts" / "requirements.txt" - projects = _wanted_west_projects() + projects = wanted_west_projects() if not sentinel.exists() or not zephyr_reqs.exists(): _install_framework(env_python_path, framework_path, version, projects) framework_ver = CORE.data[KEY_CORE][KEY_FRAMEWORK_VERSION] @@ -596,6 +650,8 @@ def _check_and_install(version: str) -> None: sentinel.touch() else: _fetch_missing_west_projects(env_python_path, framework_path, version, projects) + # Every run: existing installs need it too, and it is a no-op once applied + _patch_gen_defines_dts_path(framework_path) zephyr_sentinel = python_env_path / ".zephyr_reqs_ready" if ( diff --git a/tests/unit_tests/components/nrf52/test_pch.py b/tests/unit_tests/components/nrf52/test_pch.py index 41635c3580..7eb2fe10f3 100644 --- a/tests/unit_tests/components/nrf52/test_pch.py +++ b/tests/unit_tests/components/nrf52/test_pch.py @@ -5,9 +5,11 @@ from unittest.mock import Mock, patch import pytest +from esphome.build_helpers import pch from esphome.components import nrf52 from esphome.components.nrf52 import framework -from esphome.components.zephyr.const import KEY_BOARD +from esphome.components.nrf52.toolchain import get_elf_path +from esphome.components.zephyr.const import KEY_BOARD, KEY_SYSBUILD import esphome.config_validation as cv from esphome.const import KEY_CORE, KEY_FRAMEWORK_VERSION, Toolchain from esphome.core import CORE, EsphomeError @@ -55,8 +57,6 @@ def test_cmake_lists_pch_block_disabled(tmp_path: Path) -> None: def test_the_zephyr_compiler_decides_on_windows( windows_gcc_rule: None, version: tuple[int, ...], on: bool ) -> None: - from esphome.build_helpers import pch - # platformdirs would pick its Windows backend from the patched sys.platform with ( patch.object(nrf52, "toolchain_tool", lambda name: Path(f"/sdk/{name}.exe")), @@ -66,8 +66,16 @@ def test_the_zephyr_compiler_decides_on_windows( assert asked.call_args.args[0] == (Path("/sdk/g++.exe"),) -def _write_checksum(tmp_path: Path, app: str, conf: str = "CONFIG_X=y\n") -> Path: +def _write_checksum( + tmp_path: Path, + app: str, + conf: str = "CONFIG_X=y\n", + configured: bool = True, + sysbuild: bool = False, + version: cv.Version | None = None, +) -> Path: """Write the checksum for a build dir whose app image sits in ``app``.""" + version = version or cv.Version(2, 9, 2) CORE.build_path = tmp_path header = tmp_path / "src" / "esphome" / "core" / "pch_prefix.h" header.parent.mkdir(parents=True, exist_ok=True) @@ -77,33 +85,65 @@ def _write_checksum(tmp_path: Path, app: str, conf: str = "CONFIG_X=y\n") -> Pat (source_dir / "prj.conf").write_text(conf) (source_dir / "CMakeLists.txt").write_text("not part of the checksum\n") build_dir = tmp_path / ".pioenvs" / "livingroom" - (build_dir / app).mkdir(parents=True, exist_ok=True) - (build_dir / app / "CMakeCache.txt").write_text("") + if configured: + (build_dir / app).mkdir(parents=True, exist_ok=True) + (build_dir / app / "CMakeCache.txt").write_text("") with ( - patch.dict(CORE.data, {KEY_CORE: {KEY_FRAMEWORK_VERSION: "2.9.2"}}), - patch.object(nrf52, "zephyr_data", return_value={KEY_BOARD: "board"}), + patch.dict(CORE.data, {KEY_CORE: {KEY_FRAMEWORK_VERSION: version}}), + patch.object( + nrf52, + "zephyr_data", + return_value={KEY_BOARD: "board", KEY_SYSBUILD: sysbuild}, + ), ): nrf52._write_pch_checksum(build_dir, source_dir) return build_dir / app / SUM -@pytest.mark.parametrize("app", ["zephyr", "."]) -def test_pch_checksum_is_written_next_to_the_gch(tmp_path: Path, app: str) -> None: - """Sysbuild nests the app image; without it the build dir is the app.""" - sum_path = _write_checksum(tmp_path, app) +@pytest.mark.parametrize( + ("app", "version"), [("zephyr", cv.Version(2, 9, 2)), (".", cv.Version(2, 9, 1))] +) +def test_pch_checksum_is_written_next_to_the_gch( + tmp_path: Path, app: str, version: cv.Version +) -> None: + """The SDK version decides the layout, like get_elf_path.""" + sum_path = _write_checksum(tmp_path, app, version=version) assert len(sum_path.read_text().strip()) == 64 +def test_pch_checksum_tracks_the_kconfig_side_inputs(tmp_path: Path) -> None: + """West projects and the sysbuild flag reach autoconf without a .conf + line; the sum must move with them or stale objects get served.""" + first = _write_checksum(tmp_path, "zephyr").read_text() + with patch.object(nrf52, "wanted_west_projects", return_value={"extra"}): + second = _write_checksum(tmp_path, "zephyr").read_text() + assert first != second + third = _write_checksum(tmp_path, "zephyr", sysbuild=True).read_text() + assert first != third + + def test_pch_checksum_tracks_the_zephyr_configuration(tmp_path: Path) -> None: first = _write_checksum(tmp_path, "zephyr").read_text() assert _write_checksum(tmp_path, "zephyr", "CONFIG_X=n\n").read_text() != first -def test_pch_checksum_waits_for_the_first_configure(tmp_path: Path) -> None: - CORE.build_path = tmp_path - build_dir = tmp_path / ".pioenvs" / "livingroom" - nrf52._write_pch_checksum(build_dir, tmp_path / "zephyr") - assert not build_dir.exists() +@pytest.mark.parametrize("version", [cv.Version(2, 9, 2), cv.Version(2, 9, 1)]) +def test_pch_checksum_lands_where_the_build_writes_the_image( + tmp_path: Path, version: cv.Version +) -> None: + """One layout rule: the sum must sit in get_elf_path's app dir, or a + layout drift silently costs the first build's sharing.""" + app = "zephyr" if version >= cv.Version(2, 9, 2) else "." + sum_path = _write_checksum(tmp_path, app, version=version) + CORE.name = "livingroom" + with patch.dict(CORE.data, {KEY_CORE: {KEY_FRAMEWORK_VERSION: version}}): + expected = get_elf_path().parent.parent / SUM + assert sum_path.resolve() == expected.resolve() + + +def test_pch_checksum_written_before_the_first_configure(tmp_path: Path) -> None: + """The first build's compiles hash the sum in place of the .gch.""" + assert _write_checksum(tmp_path, "zephyr", configured=False).is_file() def _fake_build_env(ccache: str | None) -> dict[str, str]: @@ -130,6 +170,7 @@ def run_cmd(tmp_path: Path) -> Mock: CORE.name = "livingroom" CORE.toolchain = Toolchain.SDK_NRF CORE.data[KEY_CORE] = {KEY_FRAMEWORK_VERSION: cv.Version(3, 2, 0)} + (tmp_path / "build" / "zephyr").mkdir(parents=True) with ( patch.dict("os.environ", {}, clear=True), patch.object(nrf52, "check_and_install"), diff --git a/tests/unit_tests/test_nrf52_framework.py b/tests/unit_tests/test_nrf52_framework.py index d9cf2d37b3..522061dec2 100644 --- a/tests/unit_tests/test_nrf52_framework.py +++ b/tests/unit_tests/test_nrf52_framework.py @@ -11,7 +11,7 @@ from unittest.mock import ANY, call, patch import platformdirs import pytest -from esphome.components.nrf52 import _resolve_toolchain +from esphome.components.nrf52 import _resolve_toolchain, framework from esphome.components.nrf52.framework import ( _PLATFORMIO_PENV_REQUIREMENTS, _REQUIREMENTS, @@ -22,12 +22,12 @@ from esphome.components.nrf52.framework import ( _get_toolchain_platform_info, _install_toolchain, _needs_venv_rebuild, - _wanted_west_projects, check_and_install, get_build_env, get_sdk_nrf_tools_path, include_west_project, setup_platformio_python_env, + wanted_west_projects, ) from esphome.components.zephyr.const import KEY_SYSBUILD, KEY_ZEPHYR import esphome.config_validation as cv @@ -539,7 +539,7 @@ class TestCheckAndInstall: """Zephyr 4.1 moved the Cortex-M core headers to the cmsis_6 module.""" CORE.data[KEY_CORE] = {KEY_FRAMEWORK_VERSION: Version.parse(sdk_version)} - assert ("cmsis_6" in _wanted_west_projects()) is has_cmsis_6 + assert ("cmsis_6" in wanted_west_projects()) is has_cmsis_6 def test_default_projects_never_read_the_stamp( self, @@ -1121,6 +1121,22 @@ def test_get_build_env_with_ccache( assert env["CCACHE_DEPEND"] == "1" assert env["CCACHE_BASEDIR"] == str((tmp_path / "build").resolve()) assert "CCACHE_DISABLE" not in env + # Only the per build map entry leaves the hash; user maps stay in + assert env["CCACHE_IGNOREOPTIONS"] == ( + f"-fmacro-prefix-map={(tmp_path / 'build' / 'zephyr').as_posix()}" + "=CMAKE_SOURCE_DIR" + ) + + +def test_get_build_env_skips_the_map_entry_on_whitespace( + nrf52_dirs: SimpleNamespace, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The ignore list splits on spaces; a spaced path cannot be + expressed, so the entry is left hashed rather than emitted broken.""" + monkeypatch.delenv("CCACHE_IGNOREOPTIONS", raising=False) + CORE.build_path = tmp_path / "with space" / "build" + env = get_build_env("/usr/bin/ccache") + assert "CCACHE_IGNOREOPTIONS" not in env def test_get_build_env_sdk_3_4_0_uses_toolchain_root( @@ -1250,3 +1266,30 @@ def test_resolve_toolchain_rejects_unsupported() -> None: CORE.toolchain = Toolchain.ARDUINO with pytest.raises(cv.Invalid, match="Unsupported toolchain 'arduino'"): _resolve_toolchain({}) + + +def test_patch_gen_defines_relativizes_the_dts_path(tmp_path: Path) -> None: + """The absolute dts.pre path is the only per device byte in the + devicetree header; the patch makes gen_defines emit the basename.""" + gen = tmp_path / "zephyr" / "scripts" / "dts" / "gen_defines.py" + gen.parent.mkdir(parents=True) + gen.write_text("s = f'DTS input file:\\n {edt.dts_path}\\n'\n") + framework._patch_gen_defines_dts_path(tmp_path) + assert "{os.path.basename(edt.dts_path)}" in gen.read_text() + assert not list(gen.parent.glob("*.tmp")) # no leftovers + before = gen.read_text() + framework._patch_gen_defines_dts_path(tmp_path) # idempotent + assert gen.read_text() == before + framework._patch_gen_defines_dts_path(tmp_path / "absent") # tolerant + + +def test_patch_gen_defines_warns_when_the_anchor_is_gone( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A reformatted upstream must not silently cost the sharing.""" + gen = tmp_path / "zephyr" / "scripts" / "dts" / "gen_defines.py" + gen.parent.mkdir(parents=True) + gen.write_text("s = 'something else entirely'\n") + with caplog.at_level("WARNING"): + framework._patch_gen_defines_dts_path(tmp_path) + assert "gen_defines.py no longer matches" in caplog.text