Fix up usermod libArchive settings #4669
Conversation
The ConfigureProjectLibBuilder process will flush and reload the library settings from the on-disk manifests if any new library is installed at that stage. This has the side effect of reverting the libArchive setting applied to usermods which was performed prior to that call. Apply the setting afterwards, instead. Fixes wled#4597
|
""" WalkthroughThe script Changes
Assessment against linked issues
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pio-scripts/load_usermods.py (3)
10-14: Minor cleanup: remove redundantf-string and avoid shadowingThe
fprefix is unnecessary because no interpolation occurs, and re-using the loop variable namefforPathobjects may confuse future readers.- all_usermods = [f for f in usermod_dir.iterdir() if f.is_dir() and f.joinpath('library.json').exists()] - env.GetProjectConfig().set(f"env:usermods", 'custom_usermods', " ".join([f.name for f in all_usermods])) + all_usermods = [ + mod_path + for mod_path in usermod_dir.iterdir() + if mod_path.is_dir() and mod_path.joinpath("library.json").exists() + ] + env.GetProjectConfig().set("env:usermods", "custom_usermods", + " ".join([mod_path.name for mod_path in all_usermods]))🧰 Tools
🪛 Ruff (0.8.2)
11-11: Undefined name
env(F821)
14-14: Undefined name
env(F821)
14-14: f-string without any placeholders
Remove extraneous
fprefix(F541)
16-33: Consolidate repeated path checks infind_usermodThe current implementation repeats almost identical logic three times. A data-driven approach is easier to maintain and extend.
-def find_usermod(mod: str) -> Path: - """Locate this library in the usermods folder. - We do this to avoid needing to rename a bunch of folders; - this could be removed later - """ - # Check name match - mp = usermod_dir / mod - if mp.exists(): - return mp - mp = usermod_dir / f"{mod}_v2" - if mp.exists(): - return mp - mp = usermod_dir / f"usermod_v2_{mod}" - if mp.exists(): - return mp - raise RuntimeError(f"Couldn't locate module {mod} in usermods directory!") +def find_usermod(mod: str) -> Path: + """ + Locate a library folder inside *usermods* using a few common naming + conventions. + """ + candidates = [ + mod, + f"{mod}_v2", + f"usermod_v2_{mod}", + ] + for candidate in candidates: + mp = usermod_dir / candidate + if mp.exists(): + return mp + raise RuntimeError(f"Couldn't locate module {mod!r} in usermods directory")
34-38: Improveis_wled_modulerobustness
- Case-insensitive prefix check avoids missing modules named
WLED-XYZ.- Explicit equality check catches the rare case where
dep.src_diris exactlyusermod_dir.- Using
os.fspathensures comparison between the same path types.-from pathlib import Path # For OS-agnostic path manipulation +from pathlib import Path +import os @@ -def is_wled_module(dep: LibBuilderBase) -> bool: - """Returns true if the specified library is a wled module - """ - return usermod_dir in Path(dep.src_dir).parents or str(dep.name).startswith("wled-") +def is_wled_module(dep: LibBuilderBase) -> bool: + """ + Return *True* if the dependency resides inside *usermods* **or** + its declared name starts with ``wled-`` (case-insensitive). + """ + src_path = Path(dep.src_dir).resolve() + return ( + src_path == usermod_dir.resolve() + or usermod_dir.resolve() in src_path.parents + or str(dep.name).lower().startswith("wled-") + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/load_usermods.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pio-scripts/load_usermods.py
8-8: Undefined name env
(F821)
11-11: Undefined name env
(F821)
14-14: Undefined name env
(F821)
14-14: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
🔇 Additional comments (1)
pio-scripts/load_usermods.py (1)
5-5:❓ Verification inconclusive
Verify availability of
LibBuilderBasein the current PlatformIO release
platformio.builder.tools.piolib.LibBuilderBaseis an internal class whose import path has changed in the past (e.g. 6.1 → 6.1.6). A mismatched version silently breaks the build at import-time.To confirm that the symbol exists in the CI environment, you can grep the installed PlatformIO package:
If this fails, fall back to
platformio.builder.tools.piolib_builder(the new location in ≥6.1.17).
🏁 Script executed:
#!/bin/bash python - <<'PY' import importlib, inspect, sys try: m = importlib.import_module("platformio.builder.tools.piolib") print("LibBuilderBase found in", m.__file__) except ModuleNotFoundError as e: sys.exit("❌ LibBuilderBase not found: " + str(e)) PYLength of output: 68
Verify availability of
LibBuilderBaseimport in your CI environmentThe sandbox test did not have the
platformiopackage installed, so we couldn’t confirm whether
platformio.builder.tools.piolib.LibBuilderBaseexists in your CI. Please verify on your CI runners by executing:platformio --version python3 - <<'PY' import importlib, sys for module in ( "platformio.builder.tools.piolib", "platformio.builder.tools.piolib_builder" ): try: m = importlib.import_module(module) print(f"✅ LibBuilderBase found in {module} → {m.__file__}") sys.exit(0) except ImportError: continue sys.exit("❌ LibBuilderBase not found in either location") PYIf the only available location is
piolib_builder, update your import to fallback gracefully:-from platformio.builder.tools.piolib import LibBuilderBase +try: + from platformio.builder.tools.piolib import LibBuilderBase +except ImportError: + from platformio.builder.tools.piolib_builder import LibBuilderBase
pio-scripts/load_usermods.py
Outdated
| for dep in result.depbuilders: | ||
| if is_wled_module(dep): | ||
| # Add the wled folder to the include path | ||
| dep.env.PrependUnique(CPPPATH=wled_dir) | ||
| # Add WLED's own dependencies | ||
| for dir in extra_include_dirs: | ||
| dep.env.PrependUnique(CPPPATH=dir) | ||
| # Enforce that libArchive is not set; we must link them directly to the executable | ||
| if dep.lib_archive: | ||
| build = dep._manifest.get("build", {}) | ||
| build["libArchive"] = False | ||
| dep._manifest["build"] = build |
There was a problem hiding this comment.
Ensure path types match & persist libArchive override
PrependUnique treats Path and str as distinct objects; duplicates slip through and
some SCons versions can’t handle Path. Also update the in-memory flag so later logic
(e.g. verbose prints) remains accurate.
- # Add the wled folder to the include path
- dep.env.PrependUnique(CPPPATH=wled_dir)
+ # Add the WLED root folder to the include path
+ dep.env.PrependUnique(CPPPATH=str(wled_dir))
@@
- dep.env.PrependUnique(CPPPATH=dir)
+ dep.env.PrependUnique(CPPPATH=str(dir))
@@
- if dep.lib_archive:
+ if dep.lib_archive:
build = dep._manifest.get("build", {})
build["libArchive"] = False
dep._manifest["build"] = build
+ # Keep the runtime flag in sync with the manifest change
+ dep.lib_archive = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for dep in result.depbuilders: | |
| if is_wled_module(dep): | |
| # Add the wled folder to the include path | |
| dep.env.PrependUnique(CPPPATH=wled_dir) | |
| # Add WLED's own dependencies | |
| for dir in extra_include_dirs: | |
| dep.env.PrependUnique(CPPPATH=dir) | |
| # Enforce that libArchive is not set; we must link them directly to the executable | |
| if dep.lib_archive: | |
| build = dep._manifest.get("build", {}) | |
| build["libArchive"] = False | |
| dep._manifest["build"] = build | |
| for dep in result.depbuilders: | |
| if is_wled_module(dep): | |
| # Add the WLED root folder to the include path | |
| dep.env.PrependUnique(CPPPATH=str(wled_dir)) | |
| # Add WLED's own dependencies | |
| for dir in extra_include_dirs: | |
| dep.env.PrependUnique(CPPPATH=str(dir)) | |
| # Enforce that libArchive is not set; we must link them directly to the executable | |
| if dep.lib_archive: | |
| build = dep._manifest.get("build", {}) | |
| build["libArchive"] = False | |
| dep._manifest["build"] = build | |
| # Keep the runtime flag in sync with the manifest change | |
| dep.lib_archive = False |
There was a problem hiding this comment.
The point about PrependUnique is well taken, I'll make a correction there.
dep.lib_archive is a read-only @property derived from the contents of dep._manifest; it cannot be directly set.
Also, it's poor practice to put unrelated review items in a single comment. If you find yourself using 'also' to begin a sentence, that's strong indication that it would be best to make a second comment instead.
There was a problem hiding this comment.
@willmmiles Thank you for the feedback! I appreciate you correcting me on dep.lib_archive - I should have verified it was a writable property before suggesting the direct assignment.
I'll also take your advice about separating unrelated review items into different comments in future reviews. Using "also" is indeed a good indicator that comments should be split.
🐰✏️ Rabbit taking notes thoughtfully
✏️ Learnings added
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: In PlatformIO's LibBuilderBase class, `lib_archive` is a read-only @property derived from the contents of the `_manifest` dictionary and cannot be directly set.
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: When using SCons' PrependUnique method with Path objects, convert them to strings first using str() to avoid type mismatches, as PrependUnique treats Path and str as distinct objects.
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: When reviewing code, separate unrelated items into different comments rather than combining them. If using "also" to begin a sentence in a review comment, that's a strong indication it should be a separate comment.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Ensure that entries put in CPPPATH are always strings so SCons can correctlly deduplicate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pio-scripts/load_usermods.py (1)
14-14: Minor: Remove unnecessary f-string prefixThe f-string prefix is unnecessary when there are no placeholders in the string.
- env.GetProjectConfig().set(f"env:usermods", 'custom_usermods', " ".join([f.name for f in all_usermods])) + env.GetProjectConfig().set("env:usermods", 'custom_usermods', " ".join([f.name for f in all_usermods]))🧰 Tools
🪛 Ruff (0.8.2)
14-14: Undefined name
env(F821)
14-14: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/load_usermods.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
pio-scripts/load_usermods.py (1)
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: When using SCons' PrependUnique method with Path objects, convert them to strings first using str() to avoid type mismatches, as PrependUnique treats Path and str as distinct objects.
🪛 Ruff (0.8.2)
pio-scripts/load_usermods.py
8-8: Undefined name env
(F821)
11-11: Undefined name env
(F821)
14-14: Undefined name env
(F821)
14-14: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (5)
pio-scripts/load_usermods.py (5)
5-5: Good addition of LibBuilderBase importThis import is appropriately added for the new type annotation in the is_wled_module function, keeping dependencies organized at the top of the file.
10-13: Good refactoring of all_usermods initializationMoving the initialization of
all_usermodsinto the conditional block is a good practice as it limits the variable scope to where it's actually used.🧰 Tools
🪛 Ruff (0.8.2)
11-11: Undefined name
env(F821)
16-17: Improved type safety with return annotationAdding the return type annotation
-> Pathto the find_usermod function improves code clarity and enables better type checking.
34-37: Good utility function abstractionThe new
is_wled_modulefunction effectively centralizes the logic for identifying WLED modules, making the code more maintainable and easier to understand.
108-119: Proper fix for the libArchive setting issueThis implementation correctly addresses the issue described in #4597 by ensuring the libArchive setting is applied after the ConfigureProjectLibBuilder process completes, and by properly converting Path objects to strings as needed.
The ConfigureProjectLibBuilder process will flush and reload the library settings from the on-disk manifests if any new library is added to the build at that stage. This reverts the libArchive setting change performed prior to that call. Apply the setting afterwards, instead.
Fixes #4597
Summary by CodeRabbit