fix(espidf): one include root per library — kill the header-shadow class
The merged component copied every library into ONE interleaved tree and
then exported EVERY directory that held a file. A library's private
internals therefore sat on the global -I and hijacked #includes issued by
other libraries, by the arduino-esp32 core and by ESP-IDF itself. Each
incident was patched by adding a name to _SHADOW_STD_HEADERS — chasing
symptoms, since the denylist grows with every library that ships a
generically-named internal header (three landed yesterday alone:
FastLED's platforms/stub/Arduino.h, its fl/stl/asio/http/http_parser.h,
and earlier LovyanGFX's lgfx/internal/limits.h).
Adopt arduino-cli's model instead — the resolver every other Velxio board
family already delegates to. It shares one global -I list too, but adds
exactly ONE directory per library: <lib>/src for the recursive layout,
<lib> for the flat legacy one, never a subdirectory; utility/ stays
private to its owner. arduino-esp32 does the same as an IDF component.
- Each library is copied into its own subtree of the component, so two
libraries shipping the same relative path no longer collapse into one
silently-picked copy.
- INCLUDE_DIRS is one root per library, in resolution order. Internals
stay copied and reachable through file-relative includes.
- Legacy-layout utility/ dirs go in PRIV_INCLUDE_DIRS.
- Root-level header collisions (the only shadowing still possible) are
logged the way the Arduino IDE reports them, instead of being silent.
- The transitive scan now walks only the library just copied.
One component is deliberate: a component per library would swap this
failure mode for REQUIRES-order shadowing (IDF exports component includes
as plain -I, never -isystem) plus IDF component-name collisions, and buys
no caching ccache does not already provide.
Verified against the real FastLED+U8g2 project WITH the two denylist
entries removed: green, byte-identical firmware size. The 3-library
SSD1306+GFX+BusIO corpus is green too. VELXIO_PER_LIB_ROOTS=0 restores
the legacy layout; the mode is folded into the build-variant hash so the
two can never share a configured build dir.
This commit is contained in:
David Montero Crespo
parent
2a4d19e9c2
commit
a7db031e73
|
|
@ -60,6 +60,50 @@ _USE_PERSISTENT_DIR = (
|
|||
)
|
||||
|
||||
|
||||
# ── Per-library include roots (Arduino-faithful layout) ──────────────────────
|
||||
# The merged `user_libs_all` component used to copy every library's files into
|
||||
# ONE interleaved tree and then put EVERY directory that held a file on the
|
||||
# component's public INCLUDE_DIRS. That is the root cause of the header-shadow
|
||||
# bug class: a library's private internals — FastLED's host-test
|
||||
# `src/platforms/stub/Arduino.h`, its `src/fl/stl/asio/http/http_parser.h`,
|
||||
# LovyanGFX's `src/lgfx/internal/limits.h` — landed on the global -I and
|
||||
# hijacked `#include`s issued by OTHER libraries, by the arduino-esp32 core and
|
||||
# by ESP-IDF itself. Every fix was another name in `_SHADOW_STD_HEADERS`, i.e.
|
||||
# chasing symptoms: the denylist grows with every library that ships a
|
||||
# generically-named internal header.
|
||||
#
|
||||
# arduino-cli — the resolver every other Velxio board family already delegates
|
||||
# to — shares one global -I list too, but adds exactly ONE directory per
|
||||
# library: `<lib>/src` for the recursive layout, `<lib>` for the legacy flat
|
||||
# one, never a subdirectory (its header index is non-recursive, so
|
||||
# `src/sub/foo.h` is only ever reachable as `<sub/foo.h>`). `utility/` stays
|
||||
# private to the library that owns it. arduino-esp32 does the same as an IDF
|
||||
# component: one public include dir per bundled library.
|
||||
#
|
||||
# So: keep the single component — a component per library would trade this bug
|
||||
# for REQUIRES-order shadowing (IDF exports component includes as plain -I,
|
||||
# never -isystem) plus IDF-component name collisions — but give each library
|
||||
# its own subtree and publish only its include root.
|
||||
#
|
||||
# Set VELXIO_PER_LIB_ROOTS=0 to fall back to the legacy interleaved layout
|
||||
# without rebuilding the image (rollback hatch).
|
||||
_PER_LIB_ROOTS = (
|
||||
os.environ.get('VELXIO_PER_LIB_ROOTS', '1')
|
||||
not in ('0', 'false', 'False', '')
|
||||
)
|
||||
|
||||
|
||||
def _sanitise_lib_dirname(name: str) -> str:
|
||||
"""Component-relative directory name for a library.
|
||||
|
||||
Cache folder names carry `@version-hash` suffixes (`fastled@3.10.5-017d…`)
|
||||
and user libs can be named anything; keep the characters that are safe in a
|
||||
path AND inside a quoted CMake string, fold the rest.
|
||||
"""
|
||||
safe = re.sub(r'[^A-Za-z0-9_.@-]', '_', name).strip('.')
|
||||
return safe or 'lib'
|
||||
|
||||
|
||||
def _idf_version_signature() -> str:
|
||||
"""Snapshot of the ESP-IDF + arduino-esp32 toolchain version. Used to
|
||||
invalidate persistent build dirs after an upstream submodule bump (the
|
||||
|
|
@ -1488,10 +1532,15 @@ class ESPIDFCompiler:
|
|||
arduino-esp32 libs and bundled esp32_libs are always allowed (they are
|
||||
platform-provided, not user installs).
|
||||
|
||||
All library files are copied flat into one directory, so every header is
|
||||
visible to every other header and source file without any cross-component
|
||||
REQUIRES propagation — which is unreliable in ESP-IDF 4.x for deeply
|
||||
nested transitive dependencies.
|
||||
Layout (see the _PER_LIB_ROOTS note at module scope): every library is
|
||||
copied into its OWN subtree of the component and exports exactly ONE
|
||||
include root — `<lib>/src`, or `<lib>` for the flat legacy layout, with
|
||||
`utility/` kept PRIVATE. That is arduino-cli's model, and the reason a
|
||||
library's private internals can no longer hijack another library's (or
|
||||
the core's, or ESP-IDF's) `#include`s. One component is deliberate: a
|
||||
component per library would swap this failure mode for REQUIRES-order
|
||||
shadowing plus IDF component-name collisions, and buys no caching that
|
||||
ccache does not already provide.
|
||||
|
||||
Search priority per header:
|
||||
1. arduino_libs (user-installed via Library Manager) → merge into component
|
||||
|
|
@ -1528,6 +1577,23 @@ class ESPIDFCompiler:
|
|||
header_to_comp: dict[str, str] = {}
|
||||
found_any = False
|
||||
|
||||
# Per-library bookkeeping for the Arduino-faithful layout
|
||||
# (see _PER_LIB_ROOTS):
|
||||
# lib_include_roots — ONE component-relative include root per library,
|
||||
# in resolution order (arduino-cli's -I order).
|
||||
# lib_priv_roots — legacy-layout `utility/` dirs. PRIVATE so they
|
||||
# reach the library sources that need them without
|
||||
# entering the sketch's namespace, exactly as
|
||||
# arduino-cli scopes them to that library's units.
|
||||
# root_header_owner — root header basename -> first library that
|
||||
# published it, for collision reporting.
|
||||
# lib_prefix_by_name— library dir name -> unique sanitised subdir.
|
||||
lib_include_roots: list[str] = []
|
||||
lib_priv_roots: list[str] = []
|
||||
root_header_owner: dict[str, str] = {}
|
||||
lib_prefix_by_name: dict[str, str] = {}
|
||||
used_prefixes: set[str] = set()
|
||||
|
||||
headers_to_resolve: list[str] = list(ext_headers)
|
||||
# Headers the SKETCH itself includes (vs transitive pulls found by
|
||||
# re-scanning copied library headers). The architecture guard is
|
||||
|
|
@ -1690,22 +1756,85 @@ class ESPIDFCompiler:
|
|||
# presumed to be buildable source.
|
||||
return True
|
||||
|
||||
# Destination subtree. Per-library roots keep each library in
|
||||
# its OWN directory, so two libraries shipping the same
|
||||
# relative path can no longer collapse into a single
|
||||
# silently-picked copy; legacy mode interleaves them under the
|
||||
# component root (first-resolved wins).
|
||||
first_copy = lib_dir_name not in lib_prefix_by_name
|
||||
if _PER_LIB_ROOTS:
|
||||
if first_copy:
|
||||
_prefix = _sanitise_lib_dirname(lib_dir_name)
|
||||
if _prefix in used_prefixes:
|
||||
_n = 2
|
||||
while f'{_prefix}_{_n}' in used_prefixes:
|
||||
_n += 1
|
||||
_prefix = f'{_prefix}_{_n}'
|
||||
used_prefixes.add(_prefix)
|
||||
lib_prefix_by_name[lib_dir_name] = _prefix
|
||||
lib_prefix = lib_prefix_by_name[lib_dir_name]
|
||||
dest_root = comp_dir / lib_prefix
|
||||
else:
|
||||
lib_prefix = ''
|
||||
lib_prefix_by_name.setdefault(lib_dir_name, '')
|
||||
dest_root = comp_dir
|
||||
|
||||
for f in lib_root.rglob('*'):
|
||||
if not f.is_file():
|
||||
continue
|
||||
rel_path = f.relative_to(lib_root)
|
||||
if not _should_include(rel_path):
|
||||
continue
|
||||
# Track file by its relative path to preserve structure
|
||||
file_key = str(rel_path).replace('\\', '/')
|
||||
# Track file by its COMPONENT-relative path: it preserves
|
||||
# structure, and in legacy mode it is also the cross-library
|
||||
# dedup key.
|
||||
rel_key = str(rel_path).replace('\\', '/')
|
||||
file_key = f'{lib_prefix}/{rel_key}' if lib_prefix else rel_key
|
||||
if file_key not in seen_names:
|
||||
dest = comp_dir / rel_path
|
||||
dest = dest_root / rel_path
|
||||
dest.parent.mkdir(parents=True, exist_ok=True)
|
||||
shutil.copy2(f, dest)
|
||||
seen_names.add(file_key)
|
||||
if f.suffix in ('.cpp', '.c') and file_key not in cpp_files:
|
||||
cpp_files.append(file_key)
|
||||
|
||||
if _PER_LIB_ROOTS and first_copy:
|
||||
# arduino-cli's `SourceDir`: `<lib>/src` for the recursive
|
||||
# layout, `<lib>` for the flat legacy one. Nothing else.
|
||||
lib_include_roots.append(
|
||||
f'{lib_prefix}/src' if has_src_layout else lib_prefix
|
||||
)
|
||||
if not has_src_layout and (lib_root / 'utility').is_dir():
|
||||
lib_priv_roots.append(f'{lib_prefix}/utility')
|
||||
# With one include root per library these root-level headers
|
||||
# are the ONLY ones that can still shadow anything, so they
|
||||
# must not be silent — the Arduino IDE reports the same
|
||||
# thing as "Multiple libraries were found for X.h".
|
||||
_src_dir = (lib_root / 'src') if has_src_layout else lib_root
|
||||
try:
|
||||
for _entry in sorted(_src_dir.iterdir()):
|
||||
if not _entry.is_file():
|
||||
continue
|
||||
if _entry.suffix.lower() not in ('.h', '.hpp', '.hh', '.hxx'):
|
||||
continue
|
||||
_owner = root_header_owner.setdefault(_entry.name, lib_dir_name)
|
||||
if _owner != lib_dir_name:
|
||||
logger.warning(
|
||||
f'[espidf] header collision: <{_entry.name}> is '
|
||||
f'published by both "{_owner}" and '
|
||||
f'"{lib_dir_name}" — "{_owner}" wins (its '
|
||||
f'include root comes first)'
|
||||
)
|
||||
if _entry.name.lower() in self._SHADOW_STD_HEADERS:
|
||||
logger.warning(
|
||||
f'[espidf] "{lib_dir_name}" publishes '
|
||||
f'{_entry.name} at its include root — that '
|
||||
f'shadows the toolchain/core header for every '
|
||||
f'library in this build'
|
||||
)
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
# Scan newly copied headers for transitive includes.
|
||||
# Use rglob so libs with `src/` layout (e.g. GxEPD2, ArduinoJson)
|
||||
# are scanned recursively — otherwise their headers live under
|
||||
|
|
@ -1713,7 +1842,10 @@ class ESPIDFCompiler:
|
|||
# ALL header extensions (.h/.hpp/.hh/.hxx/.inc): C++ libs like
|
||||
# M5Unified put the transitive `#include <M5GFX.h>` in a .hpp,
|
||||
# so a `*.h`-only scan silently drops that dependency.
|
||||
for lib_file in comp_dir.rglob('*'):
|
||||
# Per-library roots let this scan the library that was JUST
|
||||
# copied instead of re-walking every library merged so far —
|
||||
# same result (resolved_headers dedups), a fraction of the I/O.
|
||||
for lib_file in (dest_root if _PER_LIB_ROOTS else comp_dir).rglob('*'):
|
||||
if lib_file.suffix.lower() not in ('.h', '.hpp', '.hh', '.hxx', '.inc'):
|
||||
continue
|
||||
try:
|
||||
|
|
@ -1748,26 +1880,50 @@ class ESPIDFCompiler:
|
|||
# a stray `#include <limits.h>` deep in FreeRTOS resolves to the
|
||||
# library's copy and breaks the build. The files stay copied so the
|
||||
# owning library's file-relative includes still resolve.
|
||||
shadow_dirs: set[str] = set()
|
||||
for file_key in seen_names:
|
||||
if PurePosixPath(file_key).name.lower() in self._SHADOW_STD_HEADERS:
|
||||
parent = str(PurePosixPath(file_key).parent)
|
||||
if parent and parent != '.':
|
||||
shadow_dirs.add(parent)
|
||||
|
||||
include_dirs: set[str] = {'.'}
|
||||
for file_key in seen_names:
|
||||
parent = str(PurePosixPath(file_key).parent)
|
||||
if parent and parent != '.' and parent not in shadow_dirs:
|
||||
include_dirs.add(parent)
|
||||
|
||||
if shadow_dirs:
|
||||
logger.info(
|
||||
f'[espidf] kept {len(shadow_dirs)} dir(s) off -I to avoid '
|
||||
f'shadowing standard headers: {sorted(shadow_dirs)}'
|
||||
priv_include_dirs_line = ''
|
||||
if _PER_LIB_ROOTS:
|
||||
# ONE include root per library, in resolution order. This is the
|
||||
# whole fix: a library's internals stay reachable by its own sources
|
||||
# through file-relative includes, and by nobody else. No denylist
|
||||
# needed for subdirectories because no subdirectory is ever exported.
|
||||
include_dirs_line = 'INCLUDE_DIRS ' + ' '.join(
|
||||
f'"{d}"' for d in ['.'] + lib_include_roots
|
||||
)
|
||||
if lib_priv_roots:
|
||||
priv_include_dirs_line = 'PRIV_INCLUDE_DIRS ' + ' '.join(
|
||||
f'"{d}"' for d in lib_priv_roots
|
||||
)
|
||||
logger.info(
|
||||
f'[espidf] include roots ({len(lib_include_roots)} librar'
|
||||
f'{"y" if len(lib_include_roots) == 1 else "ies"}): '
|
||||
f'{lib_include_roots}'
|
||||
+ (f' | private: {lib_priv_roots}' if lib_priv_roots else '')
|
||||
)
|
||||
else:
|
||||
# Legacy interleaved layout: every directory holding a copied file
|
||||
# is exported, minus the hand-maintained shadow denylist.
|
||||
shadow_dirs: set[str] = set()
|
||||
for file_key in seen_names:
|
||||
if PurePosixPath(file_key).name.lower() in self._SHADOW_STD_HEADERS:
|
||||
parent = str(PurePosixPath(file_key).parent)
|
||||
if parent and parent != '.':
|
||||
shadow_dirs.add(parent)
|
||||
|
||||
include_dirs_line = 'INCLUDE_DIRS ' + ' '.join(f'"{d}"' for d in sorted(include_dirs))
|
||||
include_dirs: set[str] = {'.'}
|
||||
for file_key in seen_names:
|
||||
parent = str(PurePosixPath(file_key).parent)
|
||||
if parent and parent != '.' and parent not in shadow_dirs:
|
||||
include_dirs.add(parent)
|
||||
|
||||
if shadow_dirs:
|
||||
logger.info(
|
||||
f'[espidf] kept {len(shadow_dirs)} dir(s) off -I to avoid '
|
||||
f'shadowing standard headers: {sorted(shadow_dirs)}'
|
||||
)
|
||||
|
||||
include_dirs_line = 'INCLUDE_DIRS ' + ' '.join(
|
||||
f'"{d}"' for d in sorted(include_dirs)
|
||||
)
|
||||
|
||||
# LovyanGFX's Bus_EPD.cpp (e-paper bus, compiled unconditionally as part
|
||||
# of the M5GFX merge) includes <esp_lcd_panel_io.h>, which is provided by
|
||||
|
|
@ -1850,12 +2006,20 @@ class ESPIDFCompiler:
|
|||
' -Wno-error=uninitialized)\n'
|
||||
)
|
||||
|
||||
_priv_cmake_line = (
|
||||
f' {priv_include_dirs_line}\n' if priv_include_dirs_line else ''
|
||||
)
|
||||
cmake_content = (
|
||||
'# Auto-generated by Velxio — all user libraries merged into one component.\n'
|
||||
'# Directory structure preserved for libraries like ArduinoJson with src/ layout.\n'
|
||||
'# Auto-generated by Velxio — user libraries as one IDF component.\n'
|
||||
'# Each library keeps its own subtree and exports exactly ONE include\n'
|
||||
'# root (<lib>/src, or <lib> for the flat legacy layout), matching\n'
|
||||
"# arduino-cli's resolution semantics. Library internals stay\n"
|
||||
'# reachable through file-relative includes and never enter another\n'
|
||||
"# library's — or the core's — include path.\n"
|
||||
'idf_component_register(\n'
|
||||
f' {srcs_line}\n'
|
||||
f' {include_dirs_line}\n'
|
||||
f'{_priv_cmake_line}'
|
||||
f' REQUIRES {arduino_comp_name}{extra_requires}\n'
|
||||
')\n'
|
||||
f'{defs_block}'
|
||||
|
|
@ -3310,6 +3474,10 @@ class ESPIDFCompiler:
|
|||
# never share a configured build/ — the cmake cache and
|
||||
# every object file are tied to the IDF tree.
|
||||
+ f'|idf:{5 if use_idf5 else 4}|ard:{int(arduino_mode)}'
|
||||
# The user-libs layout changes every object path and the
|
||||
# component's include list, so the two modes must never
|
||||
# share a configured build/ (stale objects + cmake cache).
|
||||
+ f'|libroots:{int(_PER_LIB_ROOTS)}'
|
||||
+ _lang_token
|
||||
).encode()
|
||||
).hexdigest()[:12]
|
||||
|
|
|
|||
|
|
@ -35,6 +35,24 @@ def make_compiler() -> ESPIDFCompiler:
|
|||
return comp
|
||||
|
||||
|
||||
def merged_has(component_root: Path, *rel_suffixes: str) -> bool:
|
||||
"""True when every suffix exists somewhere under the merged component.
|
||||
|
||||
The component's internal layout is a build-system detail that changed with
|
||||
VELXIO_PER_LIB_ROOTS (each library now keeps its own subtree). These tests
|
||||
care that a file WAS merged, not where — the exported include roots are
|
||||
asserted directly in test_espidf_include_roots.py.
|
||||
"""
|
||||
found = {
|
||||
str(p.relative_to(component_root)).replace('\\', '/')
|
||||
for p in component_root.rglob('*') if p.is_file()
|
||||
}
|
||||
return all(
|
||||
any(f == suffix or f.endswith('/' + suffix) for f in found)
|
||||
for suffix in rel_suffixes
|
||||
)
|
||||
|
||||
|
||||
def make_library(libs_dir: Path, lib_name: str, headers: list[str],
|
||||
sources: list[str], use_src_subdir: bool = False) -> Path:
|
||||
lib_dir = libs_dir / lib_name
|
||||
|
|
@ -283,7 +301,7 @@ class TestResolveLibraryComponents(unittest.TestCase):
|
|||
"""Adafruit_GFX.h directly in sketch → copied into user_libs_all."""
|
||||
self._resolve(['Adafruit_GFX.h'])
|
||||
self.assertTrue(
|
||||
(self.user_libs / 'user_libs_all' / 'Adafruit_GFX.h').exists(),
|
||||
merged_has(self.user_libs / 'user_libs_all', 'Adafruit_GFX.h'),
|
||||
'Adafruit_GFX.h not copied into user_libs_all — will not be on include path',
|
||||
)
|
||||
|
||||
|
|
@ -291,25 +309,25 @@ class TestResolveLibraryComponents(unittest.TestCase):
|
|||
"""SSD1306 sketch: both GFX and SSD1306 files must be in user_libs_all."""
|
||||
self._resolve(['Wire.h', 'Adafruit_GFX.h', 'Adafruit_SSD1306.h'])
|
||||
all_dir = self.user_libs / 'user_libs_all'
|
||||
self.assertTrue((all_dir / 'Adafruit_GFX.h').exists(),
|
||||
self.assertTrue(merged_has(all_dir, 'Adafruit_GFX.h'),
|
||||
'Adafruit_GFX.h missing — SSD1306.cpp will fail to compile')
|
||||
self.assertTrue((all_dir / 'Adafruit_SSD1306.h').exists(),
|
||||
self.assertTrue(merged_has(all_dir, 'Adafruit_SSD1306.h'),
|
||||
'Adafruit_SSD1306.h missing')
|
||||
self.assertTrue((all_dir / 'Adafruit_GFX.cpp').exists())
|
||||
self.assertTrue((all_dir / 'Adafruit_SSD1306.cpp').exists())
|
||||
self.assertTrue(merged_has(all_dir, 'Adafruit_GFX.cpp'))
|
||||
self.assertTrue(merged_has(all_dir, 'Adafruit_SSD1306.cpp'))
|
||||
|
||||
def test_wire_files_not_in_merged_component(self):
|
||||
"""Wire is a core esp32 lib — must NOT be copied into user_libs_all."""
|
||||
self._resolve(['Wire.h', 'Adafruit_SSD1306.h'])
|
||||
all_dir = self.user_libs / 'user_libs_all'
|
||||
self.assertFalse((all_dir / 'Wire.h').exists(),
|
||||
self.assertFalse(merged_has(all_dir, 'Wire.h'),
|
||||
'Wire.h was incorrectly copied — it is already in arduino-esp32')
|
||||
|
||||
def test_transitive_gfx_discovered_via_ssd1306(self):
|
||||
"""When sketch only includes SSD1306, GFX is discovered transitively."""
|
||||
self._resolve(['Adafruit_SSD1306.h'])
|
||||
self.assertTrue(
|
||||
(self.user_libs / 'user_libs_all' / 'Adafruit_GFX.h').exists(),
|
||||
merged_has(self.user_libs / 'user_libs_all', 'Adafruit_GFX.h'),
|
||||
'Transitive Adafruit_GFX.h not discovered — SSD1306.h includes it',
|
||||
)
|
||||
|
||||
|
|
@ -560,15 +578,15 @@ class TestHeaderOnlyLibrary(unittest.TestCase):
|
|||
def test_header_only_library_still_exposes_its_header(self):
|
||||
cmake = self._cmake_for(['ArduinoJson.h'])
|
||||
self.assertIn('idf_component_register', cmake)
|
||||
# The library's src/ layout is preserved, so the header lands at
|
||||
# src/ArduinoJson.h and "src" has to be on the include path for the
|
||||
# The library's src/ layout is preserved, and its include ROOT (the
|
||||
# `src` dir, wherever the layout puts it) must be exported for the
|
||||
# sketch's #include <ArduinoJson.h> to resolve.
|
||||
merged = self.user_libs / 'user_libs_all'
|
||||
self.assertTrue(
|
||||
(merged / 'src' / 'ArduinoJson.h').exists(),
|
||||
merged_has(merged, 'src/ArduinoJson.h'),
|
||||
'ArduinoJson.h not copied — the sketch would fail on "No such file"',
|
||||
)
|
||||
self.assertIn('INCLUDE_DIRS "." "src"', cmake)
|
||||
self.assertRegex(cmake, r'INCLUDE_DIRS "\." "(?:[^"]+/)?src"')
|
||||
|
||||
def test_library_with_sources_keeps_the_progmem_definitions(self):
|
||||
cmake = self._cmake_for(['LiquidCrystal_I2C.h'])
|
||||
|
|
|
|||
|
|
@ -0,0 +1,332 @@
|
|||
"""
|
||||
Regression tests for the library include-root layout (VELXIO_PER_LIB_ROOTS).
|
||||
|
||||
Every case here is a real incident. The merged component used to copy all
|
||||
libraries into one interleaved tree and export EVERY directory that held a
|
||||
file, so a library's private internals sat on the global -I and hijacked
|
||||
`#include`s issued by other libraries, by the arduino-esp32 core and by
|
||||
ESP-IDF itself:
|
||||
|
||||
* FastLED ships a host-test stub `src/platforms/stub/Arduino.h`. With that
|
||||
directory exported, U8g2's `#include <Arduino.h>` resolved to the stub —
|
||||
'yield' was not declared, and fl::string polluted String overload
|
||||
resolution inside the core's WString.h.
|
||||
* FastLED ships `src/fl/stl/asio/http/http_parser.h`. Exported, it shadowed
|
||||
the IDF http_parser component header that esp_http_server.h includes, and
|
||||
every httpd_method_t use failed to compile.
|
||||
* LovyanGFX ships `src/lgfx/internal/limits.h`. Exported, FreeRTOS's
|
||||
portmacro.h picked it up inside an extern "C" block.
|
||||
|
||||
The fix is arduino-cli's model: one subtree per library, exactly one exported
|
||||
include root per library (`<lib>/src`, or `<lib>` for the flat legacy layout),
|
||||
`utility/` private. These tests assert the property — no directory other than a
|
||||
library's own include root is ever exported — rather than any single denylist
|
||||
entry, so a new library shipping a new generically-named internal cannot
|
||||
reintroduce the class.
|
||||
|
||||
Run from the repo root:
|
||||
python -m pytest test/backend/unit/test_espidf_include_roots.py -v
|
||||
"""
|
||||
|
||||
import re
|
||||
import shutil
|
||||
import sys
|
||||
import tempfile
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent.parent.parent / 'backend'))
|
||||
|
||||
from app.services import espidf_compiler as espidf_mod
|
||||
from app.services.espidf_compiler import ESPIDFCompiler
|
||||
|
||||
|
||||
def make_compiler() -> ESPIDFCompiler:
|
||||
comp = ESPIDFCompiler.__new__(ESPIDFCompiler)
|
||||
comp.idf_path = ''
|
||||
comp.arduino_path = ''
|
||||
comp.has_arduino = False
|
||||
comp.idf5_path = ''
|
||||
comp.arduino5_path = ''
|
||||
comp.has_arduino5 = False
|
||||
return comp
|
||||
|
||||
|
||||
def write(path: Path, text: str = '// x') -> None:
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(text, encoding='utf-8')
|
||||
|
||||
|
||||
class IncludeRootsBase(unittest.TestCase):
|
||||
"""Builds a libraries dir that mirrors the real offenders."""
|
||||
|
||||
def setUp(self):
|
||||
self.tmp = Path(tempfile.mkdtemp())
|
||||
self.arduino_libs = self.tmp / 'libraries'
|
||||
self.esp32_libs = self.tmp / 'esp32_libs'
|
||||
self.user_libs = self.tmp / 'user_libs'
|
||||
for d in (self.arduino_libs, self.esp32_libs, self.user_libs):
|
||||
d.mkdir(parents=True)
|
||||
self.comp = make_compiler()
|
||||
|
||||
# FastLED: recursive layout, ships an Arduino.h stub and an
|
||||
# http_parser.h deep inside src/, plus a unity build that reaches them
|
||||
# through file-relative includes.
|
||||
fl = self.arduino_libs / 'fastled@3.10.5-017d142ff4ff'
|
||||
write(fl / 'src' / 'FastLED.h', '#include "platforms/stub/Arduino.h"')
|
||||
write(fl / 'src' / 'FastLED.cpp')
|
||||
write(fl / 'src' / 'platforms' / 'stub' / 'Arduino.h', '// host-test stub')
|
||||
write(fl / 'src' / 'fl' / 'stl' / 'asio' / 'http' / 'http_parser.h')
|
||||
write(fl / 'src' / 'fl' / 'stl' / 'string.h')
|
||||
write(fl / 'library.properties', 'name=FastLED\narchitectures=*\n')
|
||||
|
||||
# U8g2: recursive layout, includes <Arduino.h> expecting the CORE one.
|
||||
u8 = self.arduino_libs / 'U8g2'
|
||||
write(u8 / 'src' / 'U8g2lib.h', '#include <Arduino.h>')
|
||||
write(u8 / 'src' / 'U8g2lib.cpp', '#include <Arduino.h>')
|
||||
# A benign internal subdirectory: not on any denylist, so it is the
|
||||
# clean way to show what each layout exports.
|
||||
write(u8 / 'src' / 'fonts' / 'Font5x7.h')
|
||||
write(u8 / 'library.properties', 'name=U8g2\narchitectures=*\n')
|
||||
|
||||
# LovyanGFX-style internal limits.h.
|
||||
lg = self.arduino_libs / 'M5GFX'
|
||||
write(lg / 'src' / 'M5GFX.h')
|
||||
write(lg / 'src' / 'M5GFX.cpp')
|
||||
write(lg / 'src' / 'lgfx' / 'internal' / 'limits.h')
|
||||
write(lg / 'library.properties', 'name=M5GFX\narchitectures=esp32\n')
|
||||
|
||||
# Legacy flat layout with a utility/ dir (Arduino keeps it private).
|
||||
legacy = self.arduino_libs / 'LegacyLib'
|
||||
write(legacy / 'LegacyLib.h')
|
||||
write(legacy / 'LegacyLib.cpp')
|
||||
write(legacy / 'utility' / 'twi.h')
|
||||
write(legacy / 'utility' / 'twi.c')
|
||||
|
||||
def tearDown(self):
|
||||
shutil.rmtree(self.tmp)
|
||||
|
||||
def resolve(self, headers):
|
||||
return self.comp._resolve_library_components(
|
||||
headers, self.arduino_libs, self.esp32_libs,
|
||||
'arduino-esp32', self.user_libs,
|
||||
)
|
||||
|
||||
def cmake_text(self) -> str:
|
||||
return (self.user_libs / 'user_libs_all' / 'CMakeLists.txt').read_text()
|
||||
|
||||
def exported_dirs(self) -> list[str]:
|
||||
"""Directories on the component's PUBLIC include path."""
|
||||
m = re.search(r'INCLUDE_DIRS ([^\n]*)', self.cmake_text())
|
||||
self.assertIsNotNone(m, 'component CMakeLists has no INCLUDE_DIRS')
|
||||
return re.findall(r'"([^"]+)"', m.group(1))
|
||||
|
||||
def private_dirs(self) -> list[str]:
|
||||
m = re.search(r'PRIV_INCLUDE_DIRS ([^\n]*)', self.cmake_text())
|
||||
return re.findall(r'"([^"]+)"', m.group(1)) if m else []
|
||||
|
||||
def merged_files(self) -> list[str]:
|
||||
root = self.user_libs / 'user_libs_all'
|
||||
return sorted(
|
||||
str(p.relative_to(root)).replace('\\', '/')
|
||||
for p in root.rglob('*') if p.is_file()
|
||||
)
|
||||
|
||||
|
||||
class TestPerLibraryIncludeRoots(IncludeRootsBase):
|
||||
"""Default mode: one subtree and one exported include root per library."""
|
||||
|
||||
def setUp(self):
|
||||
super().setUp()
|
||||
self._saved = espidf_mod._PER_LIB_ROOTS
|
||||
espidf_mod._PER_LIB_ROOTS = True
|
||||
|
||||
def tearDown(self):
|
||||
espidf_mod._PER_LIB_ROOTS = self._saved
|
||||
super().tearDown()
|
||||
|
||||
def test_only_library_roots_are_exported(self):
|
||||
"""THE property: nothing but '.' and one root per library is on -I."""
|
||||
self.resolve(['FastLED.h', 'U8g2lib.h', 'M5GFX.h'])
|
||||
dirs = self.exported_dirs()
|
||||
self.assertEqual(dirs[0], '.')
|
||||
roots = dirs[1:]
|
||||
self.assertEqual(len(roots), 3, f'expected one root per library, got {roots}')
|
||||
for root in roots:
|
||||
self.assertRegex(
|
||||
root, r'^[^/]+/src$',
|
||||
f'{root} is not a library include root — only <lib>/src may be exported',
|
||||
)
|
||||
|
||||
def test_fastled_stub_arduino_h_is_not_exported(self):
|
||||
"""The 2026-08-19 incident: U8g2's <Arduino.h> must reach the core."""
|
||||
self.resolve(['FastLED.h', 'U8g2lib.h'])
|
||||
for d in self.exported_dirs():
|
||||
self.assertFalse(
|
||||
d.endswith('platforms/stub'),
|
||||
f'{d} exported — FastLED\'s Arduino.h stub shadows the core again',
|
||||
)
|
||||
|
||||
def test_fastled_http_parser_dir_is_not_exported(self):
|
||||
"""esp_http_server.h's <http_parser.h> must reach the IDF component."""
|
||||
self.resolve(['FastLED.h'])
|
||||
for d in self.exported_dirs():
|
||||
self.assertFalse(
|
||||
d.endswith('fl/stl/asio/http'),
|
||||
f'{d} exported — FastLED\'s http_parser.h shadows the IDF header again',
|
||||
)
|
||||
|
||||
def test_lovyangfx_internal_limits_dir_is_not_exported(self):
|
||||
"""FreeRTOS portmacro.h's <limits.h> must reach the toolchain."""
|
||||
self.resolve(['M5GFX.h'])
|
||||
for d in self.exported_dirs():
|
||||
self.assertFalse(
|
||||
d.endswith('lgfx/internal'),
|
||||
f'{d} exported — LovyanGFX\'s limits.h shadows the toolchain again',
|
||||
)
|
||||
|
||||
def test_benign_internal_subdirectory_is_not_exported_either(self):
|
||||
"""Not a denylist: NO subdirectory is exported, harmless or not."""
|
||||
self.resolve(['U8g2lib.h'])
|
||||
self.assertFalse(
|
||||
any(d.endswith('/fonts') for d in self.exported_dirs()),
|
||||
'a library internal subdir reached -I — the class is back',
|
||||
)
|
||||
self.assertTrue(
|
||||
any(f.endswith('src/fonts/Font5x7.h') for f in self.merged_files()),
|
||||
'the header must still be copied for file-relative includes',
|
||||
)
|
||||
|
||||
def test_internals_are_still_copied(self):
|
||||
"""Exclusion is from -I only: file-relative includes must keep working."""
|
||||
files = self.merged_files()
|
||||
self.resolve(['FastLED.h'])
|
||||
files = self.merged_files()
|
||||
self.assertTrue(
|
||||
any(f.endswith('src/platforms/stub/Arduino.h') for f in files),
|
||||
'the stub was dropped — FastLED\'s own relative include breaks',
|
||||
)
|
||||
self.assertTrue(
|
||||
any(f.endswith('src/fl/stl/asio/http/http_parser.h') for f in files),
|
||||
)
|
||||
|
||||
def test_each_library_keeps_its_own_subtree(self):
|
||||
"""No interleaving: a library's files live under its own directory."""
|
||||
self.resolve(['FastLED.h', 'U8g2lib.h'])
|
||||
files = self.merged_files()
|
||||
fastled = [f for f in files if f.endswith('FastLED.h')]
|
||||
u8g2 = [f for f in files if f.endswith('U8g2lib.h')]
|
||||
self.assertTrue(fastled and u8g2)
|
||||
self.assertNotEqual(
|
||||
fastled[0].split('/')[0], u8g2[0].split('/')[0],
|
||||
'libraries share a subtree — same-path files can collapse silently',
|
||||
)
|
||||
|
||||
def test_same_relative_path_in_two_libraries_survives(self):
|
||||
"""Legacy dedup dropped one copy; per-library subtrees keep both."""
|
||||
a = self.arduino_libs / 'LibA'
|
||||
write(a / 'src' / 'LibA.h')
|
||||
write(a / 'src' / 'utility' / 'helper.h', '// A')
|
||||
b = self.arduino_libs / 'LibB'
|
||||
write(b / 'src' / 'LibB.h')
|
||||
write(b / 'src' / 'utility' / 'helper.h', '// B')
|
||||
self.resolve(['LibA.h', 'LibB.h'])
|
||||
helpers = [f for f in self.merged_files() if f.endswith('utility/helper.h')]
|
||||
self.assertEqual(len(helpers), 2, f'one copy was silently dropped: {helpers}')
|
||||
|
||||
def test_legacy_utility_dir_is_private_not_public(self):
|
||||
"""arduino-cli scopes utility/ to the owning library, never globally."""
|
||||
self.resolve(['LegacyLib.h'])
|
||||
self.assertFalse(
|
||||
any(d.endswith('/utility') for d in self.exported_dirs()),
|
||||
'utility/ is on the public include path — a generic utility/debug.h '
|
||||
'would shadow every other library\'s',
|
||||
)
|
||||
self.assertTrue(
|
||||
any(d.endswith('/utility') for d in self.private_dirs()),
|
||||
'utility/ must stay reachable privately for the library\'s own sources',
|
||||
)
|
||||
|
||||
def test_flat_library_exports_its_root(self):
|
||||
self.resolve(['LegacyLib.h'])
|
||||
roots = [d for d in self.exported_dirs() if d != '.']
|
||||
self.assertTrue(
|
||||
any(not r.endswith('/src') for r in roots),
|
||||
f'flat-layout library must export its own root, got {roots}',
|
||||
)
|
||||
|
||||
def test_sources_are_registered_with_library_prefixed_paths(self):
|
||||
self.resolve(['FastLED.h'])
|
||||
srcs = re.search(r'SRCS ([^\n]*)', self.cmake_text()).group(1)
|
||||
self.assertIn('/src/FastLED.cpp', srcs)
|
||||
|
||||
def test_versioned_cache_dirname_is_path_safe(self):
|
||||
"""Cache entries are `name@version-sha`; the subdir must stay valid."""
|
||||
self.resolve(['FastLED.h'])
|
||||
roots = [d for d in self.exported_dirs() if d != '.']
|
||||
self.assertTrue(roots)
|
||||
for r in roots:
|
||||
self.assertNotRegex(r, r'[<>:"|?*\\\\]', f'unsafe path component: {r}')
|
||||
|
||||
def test_transitive_dependency_still_resolves(self):
|
||||
"""Per-library scanning must not lose transitive pulls."""
|
||||
dep = self.arduino_libs / 'DepLib'
|
||||
write(dep / 'src' / 'DepLib.h')
|
||||
write(dep / 'src' / 'DepLib.cpp')
|
||||
top = self.arduino_libs / 'TopLib'
|
||||
write(top / 'src' / 'TopLib.h', '#include <DepLib.h>')
|
||||
write(top / 'src' / 'TopLib.cpp')
|
||||
self.resolve(['TopLib.h'])
|
||||
files = self.merged_files()
|
||||
self.assertTrue(any(f.endswith('DepLib.h') for f in files),
|
||||
'transitive dependency was not merged')
|
||||
|
||||
def test_still_one_component(self):
|
||||
names, header_to_comp = self.resolve(['FastLED.h'])
|
||||
self.assertEqual(names, ['user_libs_all'])
|
||||
self.assertEqual(header_to_comp['FastLED.h'], 'user_libs_all')
|
||||
|
||||
|
||||
class TestLegacyLayoutStillWorks(IncludeRootsBase):
|
||||
"""VELXIO_PER_LIB_ROOTS=0 must reproduce the old behaviour exactly."""
|
||||
|
||||
def setUp(self):
|
||||
super().setUp()
|
||||
self._saved = espidf_mod._PER_LIB_ROOTS
|
||||
espidf_mod._PER_LIB_ROOTS = False
|
||||
|
||||
def tearDown(self):
|
||||
espidf_mod._PER_LIB_ROOTS = self._saved
|
||||
super().tearDown()
|
||||
|
||||
def test_files_are_interleaved_at_component_root(self):
|
||||
self.resolve(['FastLED.h', 'U8g2lib.h'])
|
||||
files = self.merged_files()
|
||||
self.assertIn('src/FastLED.h', files)
|
||||
self.assertIn('src/U8g2lib.h', files)
|
||||
|
||||
def test_subdirectories_are_exported(self):
|
||||
"""The legacy mechanism: every dir holding a file is exported, and only
|
||||
the hand-maintained denylist keeps the dangerous ones out. A benign
|
||||
internal subdir therefore DOES reach -I here — which is exactly the
|
||||
design the per-library layout replaces."""
|
||||
self.resolve(['U8g2lib.h'])
|
||||
self.assertTrue(
|
||||
any(d.endswith('fonts') for d in self.exported_dirs()),
|
||||
'legacy mode should still export internal subdirectories',
|
||||
)
|
||||
|
||||
def test_denylisted_dirs_are_kept_off_the_include_path(self):
|
||||
"""The old safety net still works when the flag is off."""
|
||||
self.resolve(['FastLED.h'])
|
||||
for d in self.exported_dirs():
|
||||
self.assertFalse(d.endswith('platforms/stub'), d)
|
||||
self.assertFalse(d.endswith('fl/stl/asio/http'), d)
|
||||
|
||||
def test_no_private_include_dirs(self):
|
||||
self.resolve(['LegacyLib.h'])
|
||||
self.assertEqual(self.private_dirs(), [])
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
Loading…
Reference in New Issue