mirror of
https://github.com/esphome/esphome.git
synced 2026-08-31 01:56:01 +00:00
Address review: route project -L/-l to the link, fail on unresolvable libraries, remove dead ninja constants, tighten diagnostics
This commit is contained in:
@@ -18,7 +18,7 @@ import logging
|
||||
from pathlib import Path
|
||||
import shlex
|
||||
|
||||
from esphome.core import CORE, Library
|
||||
from esphome.core import CORE, EsphomeError, Library
|
||||
from esphome.espidf.extra_script import apply_extra_script
|
||||
from esphome.platformio.library import (
|
||||
DEFAULT_BUILD_INCLUDE_DIR,
|
||||
@@ -100,6 +100,12 @@ def _library_info(name: str, read_path: Path, data: dict) -> ArduinoLibrary:
|
||||
for d in [include_dir, src_dir, *include_flags]:
|
||||
if (path := (read_path / d)).is_dir():
|
||||
lib.include_dirs.append(path.resolve())
|
||||
elif d in include_flags:
|
||||
# The includeDir/srcDir defaults are probes; an explicit -I that
|
||||
# does not resolve is a manifest or packaging error worth naming
|
||||
_LOGGER.warning(
|
||||
"Library %s declares include dir %s which does not exist", name, d
|
||||
)
|
||||
|
||||
lib.sources = sorted(
|
||||
path.resolve()
|
||||
@@ -134,12 +140,11 @@ def resolve_libraries(framework_path: Path) -> list[ArduinoLibrary]:
|
||||
elif (framework_path / "libraries" / library.name).is_dir():
|
||||
bundled.append(_bundled_library(framework_path, library.name))
|
||||
else:
|
||||
# Likely a typo or a library the build genuinely needs; a debug
|
||||
# log here would surface only as a wall of include errors later.
|
||||
_LOGGER.warning(
|
||||
"Library %s is not bundled with the Arduino framework and has "
|
||||
"no version or repository to download it from; skipping",
|
||||
library.name,
|
||||
# PlatformIO fails on an unresolvable lib_deps entry too; building
|
||||
# without it would only surface as unrelated include/link errors.
|
||||
raise EsphomeError(
|
||||
f"Library {library.name} is not bundled with the Arduino "
|
||||
"framework and has no version or repository to download it from"
|
||||
)
|
||||
|
||||
converted: list[ArduinoLibrary] = []
|
||||
|
||||
@@ -56,13 +56,6 @@ ESPHOME_ARDUINO8266_FRAMEWORK_MIRRORS = str_to_lst_of_str(
|
||||
ESPHOME_ARDUINO8266_TOOLCHAIN_MIRRORS = str_to_lst_of_str(
|
||||
os.environ.get("ESPHOME_ARDUINO8266_TOOLCHAIN_MIRRORS", "")
|
||||
)
|
||||
# Default ninja source; downloads from it verify against _NINJA_SHA256.
|
||||
_NINJA_URL = (
|
||||
"https://github.com/ninja-build/ninja/releases/download/v{VERSION}/{ARCHIVE}"
|
||||
)
|
||||
ESPHOME_ARDUINO8266_NINJA_MIRRORS = str_to_lst_of_str(
|
||||
os.environ.get("ESPHOME_ARDUINO8266_NINJA_MIRRORS", "")
|
||||
)
|
||||
|
||||
|
||||
def get_arduino8266_tools_path() -> Path:
|
||||
@@ -143,6 +136,9 @@ def _registry_download(
|
||||
continue
|
||||
for file in ver.get("files", []):
|
||||
systems = file.get("system") or "*"
|
||||
# A bare string would make ``in`` a substring test
|
||||
if isinstance(systems, str) and systems != "*":
|
||||
systems = [systems]
|
||||
if systems == "*" or system in systems:
|
||||
return (
|
||||
file["download_url"],
|
||||
@@ -240,20 +236,27 @@ def get_build_env(toolchain_path: Path) -> dict[str, str]:
|
||||
def ccache_path() -> str | None:
|
||||
"""The ccache binary to prefix compiles with, or None when disabled.
|
||||
|
||||
Same opt-out convention as the PlatformIO path: on by default when the
|
||||
binary is on PATH, ``ESPHOME_CCACHE_ENABLE=0`` disables it.
|
||||
Same convention as the PlatformIO path: on by default when the binary is
|
||||
on PATH, ``ESPHOME_CCACHE_ENABLE=0`` disables it, and an explicit ``=1``
|
||||
warns when no binary is found and skips the runnability probe.
|
||||
"""
|
||||
from esphome.platformio.toolchain import _ccache_runs, _strip_win_long_path_prefix
|
||||
|
||||
if "ESPHOME_CCACHE_ENABLE" in os.environ and not get_bool_env(
|
||||
"ESPHOME_CCACHE_ENABLE"
|
||||
):
|
||||
explicit = "ESPHOME_CCACHE_ENABLE" in os.environ
|
||||
if explicit and not get_bool_env("ESPHOME_CCACHE_ENABLE"):
|
||||
return None
|
||||
ccache = shutil.which("ccache")
|
||||
if ccache is None:
|
||||
if explicit:
|
||||
_LOGGER.warning(
|
||||
"ESPHOME_CCACHE_ENABLE is set but no ccache binary is on PATH; "
|
||||
"compiling without ccache"
|
||||
)
|
||||
return None
|
||||
ccache = _strip_win_long_path_prefix(ccache)
|
||||
return ccache if _ccache_runs(ccache) else None
|
||||
if not explicit and not _ccache_runs(ccache):
|
||||
return None
|
||||
return ccache
|
||||
|
||||
|
||||
def ccache_env() -> dict[str, str]:
|
||||
|
||||
@@ -144,8 +144,13 @@ def _print_size_summary(build_dir: Path, toolchain_path: Path) -> None:
|
||||
try:
|
||||
sections[parts[0]] = int(parts[1])
|
||||
except ValueError:
|
||||
# An omitted section would silently skew the reported totals
|
||||
_LOGGER.warning("Unparsable size output for section %s", parts[0])
|
||||
# A confident total built on a dropped section would feed a
|
||||
# wrong number to CI's memory-impact metric; skip the summary.
|
||||
_LOGGER.warning(
|
||||
"Unparsable size output for section %s; skipping the size summary",
|
||||
parts[0],
|
||||
)
|
||||
return
|
||||
ram = sum(sections.get(s, 0) for s in _RAM_SECTIONS)
|
||||
flash = sum(sections.get(s, 0) for s in _FLASH_SECTIONS)
|
||||
print(f"RAM: {format_bar(ram, _MAX_RAM_SIZE)}")
|
||||
|
||||
@@ -183,6 +183,7 @@ def _resolve_build_config(defines: dict[str, str]) -> _BuildConfig:
|
||||
for name, define in _NONOSDK_VERSIONS:
|
||||
if f"PIO_FRAMEWORK_ARDUINO_ESPRESSIF_{name}" in defines:
|
||||
nonosdk = define
|
||||
break
|
||||
|
||||
tcp_mss, features, ipv6, lwip_lib = _LWIP_DEFAULT
|
||||
for knob, variant in _LWIP_VARIANTS:
|
||||
@@ -291,18 +292,30 @@ def _defines_flags(
|
||||
]
|
||||
|
||||
|
||||
def _project_flags() -> tuple[list[str], list[str]]:
|
||||
"""Split the ESPHome build flags into (compile, linker) lists.
|
||||
def _project_flags() -> tuple[list[str], list[str], list[Path], list[str]]:
|
||||
"""Split the ESPHome build flags into compile, linker, -L, and -l lists.
|
||||
|
||||
Unlike ``framework_helpers.get_project_compile_flags`` this keeps every
|
||||
non-linker flag, matching how PlatformIO passes ``build_flags`` to the
|
||||
compiler verbatim, and applies ``build_unflags``.
|
||||
compiler verbatim, and applies ``build_unflags``. ``-L``/``-l`` are
|
||||
classified out so they reach the link line as they do under PlatformIO.
|
||||
"""
|
||||
unflags = set(CORE.build_unflags)
|
||||
flags = [f for f in sorted(CORE.build_flags) if f not in unflags]
|
||||
compile_flags = [f for f in flags if not f.startswith("-Wl,")]
|
||||
link_flags = [f for f in flags if f.startswith("-Wl,")]
|
||||
return compile_flags, link_flags
|
||||
compile_flags: list[str] = []
|
||||
link_flags: list[str] = []
|
||||
lib_dirs: list[Path] = []
|
||||
libs: list[str] = []
|
||||
for flag in flags:
|
||||
if flag.startswith("-Wl,"):
|
||||
link_flags.append(flag)
|
||||
elif flag.startswith("-L") and len(flag) > 2:
|
||||
lib_dirs.append(Path(flag[2:]))
|
||||
elif flag.startswith("-l") and len(flag) > 2:
|
||||
libs.append(flag[2:])
|
||||
else:
|
||||
compile_flags.append(flag)
|
||||
return compile_flags, link_flags, lib_dirs, libs
|
||||
|
||||
|
||||
def _collect_sources(root: Path, exclude: set[str] = frozenset()) -> list[Path]:
|
||||
@@ -444,7 +457,12 @@ def write_project(paths: dict[str, Path]) -> bool:
|
||||
for lib in libraries:
|
||||
include_dirs += lib.include_dirs
|
||||
|
||||
project_compile_flags, project_link_flags = _project_flags()
|
||||
(
|
||||
project_compile_flags,
|
||||
project_link_flags,
|
||||
project_lib_dirs,
|
||||
project_libs,
|
||||
) = _project_flags()
|
||||
defines = _defines_flags(
|
||||
config, esp8266_data[KEY_FLASH_MODE], board, board_build["defines"]
|
||||
)
|
||||
@@ -470,12 +488,14 @@ def write_project(paths: dict[str, Path]) -> bool:
|
||||
link_flags += ["-T", flash_ld]
|
||||
|
||||
lib_dirs = [Path("ld"), sdk / "lib", sdk / "ld", sdk / "lib" / config.nonosdk]
|
||||
lib_dirs += project_lib_dirs
|
||||
for lib in libraries:
|
||||
lib_dirs += lib.link_dirs
|
||||
system_libs = (
|
||||
_SYSTEM_LIBS_PRE_LWIP
|
||||
+ [config.lwip_lib]
|
||||
+ _SYSTEM_LIBS_POST_LWIP
|
||||
+ project_libs
|
||||
+ [lib_name for lib in libraries for lib_name in lib.link_libs]
|
||||
+ ["stdc++-exc" if config.exceptions else "stdc++", "m", "c", "gcc"]
|
||||
)
|
||||
|
||||
@@ -222,6 +222,8 @@ def test_write_project_link_line_and_exclusions(tmp_path: Path) -> None:
|
||||
"-Wl,--wrap=millis",
|
||||
"-Wl,--wrap=printf",
|
||||
"-Wno-nonnull-compare",
|
||||
"-L/opt/blobs",
|
||||
"-luser_blob",
|
||||
)
|
||||
content = _write_ninja(paths)
|
||||
|
||||
@@ -247,11 +249,19 @@ def test_write_project_link_line_and_exclusions(tmp_path: Path) -> None:
|
||||
assert "-T eagle.flash.4m.ld" in content
|
||||
# scanf float disabled: the forced-link flag must not appear
|
||||
assert "_scanf_float" not in content
|
||||
# -L/-l from esphome build_flags reach the link line, not the compiles
|
||||
assert '-L"/opt/blobs"' in content
|
||||
assert "-luser_blob" in content
|
||||
assert "cflags" not in [
|
||||
line.split(" = ")[0].strip()
|
||||
for line in content.splitlines()
|
||||
if "user_blob" in line
|
||||
]
|
||||
# System libraries with the selected lwIP variant, in the builder's order
|
||||
assert (
|
||||
"-lhal -lphy -lpp -lnet80211 -llwip2-1460 -lwpa -lcrypto -lmain -lwps "
|
||||
"-lbearssl -lespnow -lsmartconfig -lairkiss -lwpa2 -lstdc++ -lm -lc -lgcc"
|
||||
in content
|
||||
"-lbearssl -lespnow -lsmartconfig -lairkiss -lwpa2 -luser_blob "
|
||||
"-lstdc++ -lm -lc -lgcc" in content
|
||||
)
|
||||
# Core exclusions: native OTA backend and waveform stubs
|
||||
assert "Updater.cpp" not in content
|
||||
@@ -511,3 +521,12 @@ def test_write_project_missing_framework_dir_raises(tmp_path: Path) -> None:
|
||||
_set_flags()
|
||||
with pytest.raises(EsphomeError, match="incomplete.*lwip2"):
|
||||
_write_ninja(paths)
|
||||
|
||||
|
||||
def test_build_config_nonosdk_precedence() -> None:
|
||||
"""With two SDK knobs set, the first table entry wins (documented order)."""
|
||||
_set_flags(
|
||||
"-DPIO_FRAMEWORK_ARDUINO_ESPRESSIF_SDK305",
|
||||
"-DPIO_FRAMEWORK_ARDUINO_ESPRESSIF_SDK221",
|
||||
)
|
||||
assert _resolve_build_config(_flag_defines()).nonosdk == "NONOSDK221"
|
||||
|
||||
@@ -93,15 +93,21 @@ def test_library_info_no_src_dir(tmp_path: Path) -> None:
|
||||
assert lib.include_dirs == [read_path.resolve()]
|
||||
|
||||
|
||||
def test_resolve_libraries_bundled_and_unknown(
|
||||
tmp_path: Path, caplog: pytest.LogCaptureFixture
|
||||
) -> None:
|
||||
def test_resolve_libraries_bundled(tmp_path: Path) -> None:
|
||||
framework = _make_framework(tmp_path)
|
||||
_add_library("ESP8266WiFi", None)
|
||||
_add_library("Typoo", None) # unknown bare name: skipped with a warning
|
||||
libs = component.resolve_libraries(framework)
|
||||
assert [lib.name for lib in libs] == ["ESP8266WiFi"]
|
||||
assert "Typoo" in caplog.text
|
||||
|
||||
|
||||
def test_resolve_libraries_unknown_bare_name_raises(tmp_path: Path) -> None:
|
||||
"""An unresolvable library fails the build by name, as PlatformIO does."""
|
||||
from esphome.core import EsphomeError
|
||||
|
||||
framework = _make_framework(tmp_path)
|
||||
_add_library("Typoo", None)
|
||||
with pytest.raises(EsphomeError, match="Typoo"):
|
||||
component.resolve_libraries(framework)
|
||||
|
||||
|
||||
def _converted(name: str, source_dir: Path, data: dict) -> ConvertedLibrary:
|
||||
@@ -205,3 +211,13 @@ def test_library_info_trailing_bare_flag_warns(
|
||||
assert lib.flags == ["-DA=1"]
|
||||
assert lib.link_libs == []
|
||||
assert "Ignoring trailing '-l'" in caplog.text
|
||||
|
||||
|
||||
def test_library_info_missing_explicit_include_warns(
|
||||
tmp_path: Path, caplog: pytest.LogCaptureFixture
|
||||
) -> None:
|
||||
read_path = tmp_path / "lib"
|
||||
(read_path / "src").mkdir(parents=True)
|
||||
lib = component._library_info("x", read_path, {"build": {"flags": ["-Inope"]}})
|
||||
assert lib.include_dirs == [(read_path / "src").resolve()]
|
||||
assert "include dir nope which does not exist" in caplog.text
|
||||
|
||||
@@ -87,6 +87,21 @@ def test_registry_download_matches_system() -> None:
|
||||
)
|
||||
|
||||
|
||||
def test_registry_download_bare_string_system() -> None:
|
||||
"""A bare-string system tag is an exact match, not a substring test."""
|
||||
resp = _registry_response(
|
||||
[
|
||||
{"system": "linux_x86", "download_url": "http://x/x86"},
|
||||
{"system": "linux_x86_64", "download_url": "http://x/x86_64"},
|
||||
]
|
||||
)
|
||||
with (
|
||||
patch("requests.get", return_value=resp),
|
||||
patch.object(framework, "_pio_system", return_value="linux_x86_64"),
|
||||
):
|
||||
assert framework._registry_download("pkg", "1.0.0")[0] == "http://x/x86_64"
|
||||
|
||||
|
||||
def test_registry_download_wildcard_system() -> None:
|
||||
resp = _registry_response([{"system": "*", "download_url": "http://x/any"}])
|
||||
with patch("requests.get", return_value=resp):
|
||||
@@ -243,6 +258,25 @@ def test_ccache_path_ok(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
assert framework.ccache_path() == "/usr/bin/ccache"
|
||||
|
||||
|
||||
def test_ccache_path_explicit_missing_binary_warns(
|
||||
monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture
|
||||
) -> None:
|
||||
monkeypatch.setenv("ESPHOME_CCACHE_ENABLE", "1")
|
||||
with patch("shutil.which", return_value=None):
|
||||
assert framework.ccache_path() is None
|
||||
assert "no ccache binary is on PATH" in caplog.text
|
||||
|
||||
|
||||
def test_ccache_path_explicit_skips_probe(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""An explicit opt-in trusts the binary without the runnability probe."""
|
||||
monkeypatch.setenv("ESPHOME_CCACHE_ENABLE", "1")
|
||||
with (
|
||||
patch("shutil.which", return_value="/usr/bin/ccache"),
|
||||
patch("esphome.platformio.toolchain._ccache_runs", side_effect=AssertionError),
|
||||
):
|
||||
assert framework.ccache_path() == "/usr/bin/ccache"
|
||||
|
||||
|
||||
def test_ccache_env(tmp_path: Path) -> None:
|
||||
with patch.object(framework, "ccache_path", return_value=None):
|
||||
assert framework.ccache_env() == {}
|
||||
|
||||
@@ -27,7 +27,6 @@ section size addr
|
||||
.text1 27489 1074790896
|
||||
.rodata 2588 1073645504
|
||||
.bss 26504 1073648096
|
||||
.comment abc 0
|
||||
Total 401861
|
||||
"""
|
||||
|
||||
@@ -222,3 +221,20 @@ def test_run_compile_skips_compdb_when_ninja_unchanged(tmp_path: Path) -> None:
|
||||
# Present compile DB + unchanged build.ninja: skipped
|
||||
(build_dir / "compile_commands.json").write_text("[]")
|
||||
run(regenerate_expected=False)
|
||||
|
||||
|
||||
def test_print_size_summary_unparsable_section_skips_summary(
|
||||
tmp_path: Path,
|
||||
capsys: pytest.CaptureFixture[str],
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""A section that fails to parse must not produce a confident wrong total."""
|
||||
bad = _SIZE_OUTPUT + ".broken abc 0\n"
|
||||
with patch.object(
|
||||
toolchain.subprocess,
|
||||
"run",
|
||||
return_value=MagicMock(returncode=0, stdout=bad),
|
||||
):
|
||||
toolchain._print_size_summary(tmp_path, tmp_path / "toolchain")
|
||||
assert capsys.readouterr().out == ""
|
||||
assert "Unparsable size output" in caplog.text
|
||||
|
||||
Reference in New Issue
Block a user