Refactor: using Path instead of join #281
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughModernize many builder modules to use pathlib.Path, harden Arduino/ESP-IDF path and tool handling, introduce safe move/backup utilities and lib-ignore protections (TinyUSB, BT/BLE, DSP), add a BackupManager, expose get_lib_ignore_components API, and update example envs to expand lib_ignore and component removals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant PIO as PlatformIO
participant Builder as Builder (espidf / arduino / ulp)
participant CM as ComponentManager / BackupManager
participant FS as Filesystem
participant CMake as CMake / tool-cmake
Dev->>PIO: request build
PIO->>Builder: init (Path-based resolution)
Builder->>FS: resolve framework/tool/library paths (Path())
Builder->>CM: get_lib_ignore_components()
CM-->>Builder: lib-ignore list + protected components (tinyusb, BT/BLE, DSP)
alt backup required
Builder->>CM: BackupManager.backup_pioarduino_build_py()
CM-->>FS: create backup if MCU libs dir exists
end
alt any missing tools/invalid paths
Builder-->>PIO: emit descriptive error & abort
else
Builder->>CMake: invoke via CMAKE_DIR / CMK_TOOL
CMake-->>Builder: build artifacts
Builder->>FS: safe file replace/move via _replace_move
alt restore after failure
CM->>FS: BackupManager.restore_pioarduino_build_py()
end
Builder-->>PIO: build complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
builder/frameworks/component_manager.py (1)
569-591: Consider promoting critical_components to a module-level setMinor: defining critical_components once (as a set) avoids rebuild per call and speeds membership checks.
- critical_components = [ + critical_components = { 'lwip', 'freertos', 'esp_system', 'esp_common', 'driver', 'nvs_flash', 'spi_flash', 'esp_timer', 'esp_event', - 'log', - 'arduino_tinyusb', - 'tinyusb' - ] + 'log', + 'arduino_tinyusb', + 'tinyusb', + } ... - if entry not in critical_components: + if entry not in critical_components: filtered_entries.append(entry)builder/frameworks/espidf.py (2)
2073-2082: Guard optional library moves to avoid FileNotFoundErrorIf an archive isn’t present (packaging change), _replace_move will raise. Check existence first.
- _replace_move(str(Path(lib_dst) / "libspi_flash.a"), str(Path(mem_var) / "libspi_flash.a")) + src_spi = str(Path(lib_dst) / "libspi_flash.a") + if os.path.exists(src_spi): + _replace_move(src_spi, str(Path(mem_var) / "libspi_flash.a")) _replace_move(str(Path(env_build) / "memory.ld"), str(Path(ld_dst) / "memory.ld")) if mcu == "esp32s3": - _replace_move(str(Path(lib_dst) / "libesp_psram.a"), str(Path(mem_var) / "libesp_psram.a")) - _replace_move(str(Path(lib_dst) / "libesp_system.a"), str(Path(mem_var) / "libesp_system.a")) - _replace_move(str(Path(lib_dst) / "libfreertos.a"), str(Path(mem_var) / "libfreertos.a")) - _replace_move(str(Path(lib_dst) / "libbootloader_support.a"), str(Path(mem_var) / "libbootloader_support.a")) - _replace_move(str(Path(lib_dst) / "libesp_hw_support.a"), str(Path(mem_var) / "libesp_hw_support.a")) - _replace_move(str(Path(lib_dst) / "libesp_lcd.a"), str(Path(mem_var) / "libesp_lcd.a")) + for lib in ( + "libesp_psram.a","libesp_system.a","libfreertos.a", + "libbootloader_support.a","libesp_hw_support.a","libesp_lcd.a" + ): + src = str(Path(lib_dst) / lib) + if os.path.exists(src): + _replace_move(src, str(Path(mem_var) / lib))
554-554: Tiny nit: list concatenation style (RUF005)Use list unpack for readability.
- idf_env["PATH"] = os.pathsep.join(additional_packages + [idf_env["PATH"]]) + idf_env["PATH"] = os.pathsep.join([*additional_packages, idf_env["PATH"]])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
builder/frameworks/component_manager.py(3 hunks)builder/frameworks/espidf.py(61 hunks)examples/arduino-blink/platformio.ini(7 hunks)examples/arduino-rmt-blink/platformio.ini(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#199
File: examples/arduino-blink/platformio.ini:81-83
Timestamp: 2025-06-26T09:56:30.658Z
Learning: In examples/arduino-blink/platformio.ini, the different lib_ignore configurations across environments (some including wifi, some not, different syntax styles) are intentional test cases, not inconsistencies that need to be fixed.
Applied to files:
examples/arduino-rmt-blink/platformio.iniexamples/arduino-blink/platformio.ini
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/espidf.pyexamples/arduino-blink/platformio.ini
📚 Learning: 2025-08-08T09:55:50.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:55:50.842Z
Learning: In pioarduino/platform-espressif32 (builder/frameworks/espidf.py, Python), for get_app_partition_offset: preferred behavior is to probe app/ota_0 first, then app/factory; avoid using the {"name":"boot"} sentinel; make probes non-fatal (don’t exit on missing partitions), optionally fall back to calling parttool.py with --partition-boot-default, then default to 0x10000.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:50:01.969Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:50:01.969Z
Learning: Preference in pioarduino/platform-espressif32 (builder/frameworks/espidf.py): Avoid using {"name": "boot"} as a sentinel when resolving the default app partition offset. Instead, call parttool.py with --partition-boot-default directly (and optionally fall back to factory/ota_0) to prevent confusion with non-existent CSV names.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:44:18.366Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:44:18.366Z
Learning: In builder/frameworks/espidf.py, get_partition_info treats pt_params["name"] == "boot" as a sentinel to pass --partition-boot-default to parttool.py; "boot" is not expected to be a CSV partition name.
Applied to files:
builder/frameworks/espidf.py
🪛 Ruff (0.12.2)
builder/frameworks/espidf.py
554-554: Consider [*additional_packages, idf_env["PATH"]] instead of concatenation
Replace with [*additional_packages, idf_env["PATH"]]
(RUF005)
🔇 Additional comments (5)
examples/arduino-rmt-blink/platformio.ini (1)
5-8: lib_ignore expansions look consistent and safe across boardsAdding ble and keeping wifi/spiffs/NetworkClientSecure ignored aligns with minimal RMT examples and avoids unnecessary pulls. No action needed.
Also applies to: 16-19, 27-30, 38-41, 49-52
builder/frameworks/component_manager.py (3)
580-583: Protecting TinyUSB from lib_ignore is correctIncluding arduino_tinyusb and tinyusb in critical_components prevents accidental breakage of USB stacks. Good call.
813-817: Map usb/tinyusb aliases to arduino_tinyusb include pathThis closes common alias gaps and keeps include removal logic consistent. Looks good.
844-847: Cache init via None-sentinel is preferable to hasattrThis avoids false negatives and is clearer. Good change.
examples/arduino-blink/platformio.ini (1)
15-22: lib_ignore/Matter/RainMaker updates and component tweaks look intentionalChanges mirror other examples and the component manager’s new protections. The added espressif/cmake_utilities (0.*) in C2 is reasonable. No issues spotted.
Also applies to: 44-50, 71-78, 96-103, 131-138, 150-156, 168-175
Refactor paths to use pathlib for better path handling.
Added functionality to retrieve components to ignore based on the lib_ignore project option using the component_manager. Updated error messages for better clarity.
Jason2866
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
builder/main.py (1)
787-797: Syntax error and quoting issue in DFU uploader block.
There’s an extra closing parenthesis after the str(...) call, and $UPLOADER isn’t quoted in UPLOADCMD (breaks when path has spaces).Apply:
- env.Replace( - UPLOADER=str( - Path(platform.get_package_dir("tool-dfuutil-arduino")).resolve() / "dfu-util") - ), + env.Replace( + UPLOADER=str(Path(platform.get_package_dir("tool-dfuutil-arduino")).resolve() / "dfu-util"), UPLOADERFLAGS=[ "-d", ",".join(["%s:%s" % (hwid[0], hwid[1]) for hwid in hwids]), "-Q", "-D", ], - UPLOADCMD='$UPLOADER $UPLOADERFLAGS "$SOURCE"', + UPLOADCMD='"$UPLOADER" $UPLOADERFLAGS "$SOURCE"', )
♻️ Duplicate comments (4)
builder/frameworks/espidf.py (4)
1418-1433: Quote CMAKE and all -D path args in preprocess_linker_fileAvoids failures with spaces and improves robustness.
- CMAKE_DIR, - "-DCC=%s" % str(Path(TOOLCHAIN_DIR) / "bin" / "$CC"), - "-DSOURCE=$SOURCE", - "-DTARGET=$TARGET", - "-DCONFIG_DIR=%s" % str(Path(BUILD_DIR) / "config"), - "-DLD_DIR=%s" % str(Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld"), + f'"{CMAKE_DIR}"', + f'-DCC="{str(Path(TOOLCHAIN_DIR) / "bin" / "$CC")}"', + '-DSOURCE="$SOURCE"', + '-DTARGET="$TARGET"', + f'-DCONFIG_DIR="{str(Path(BUILD_DIR) / "config")}"', + f'-DLD_DIR="{str(Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld")}"', "-P", - str(Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake"), + f'"{str(Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake")}"',
482-487: Windows: use cmake.exe; set CMAKE_DIR to the executable namePrevents failures on Windows.
-CMK_TOOL = platform.get_package_dir("tool-cmake") +CMK_TOOL = platform.get_package_dir("tool-cmake") if not CMK_TOOL or not os.path.isdir(CMK_TOOL): sys.stderr.write(f"Error: Missing CMake package directory '{CMK_TOOL}'\n") env.Exit(1) -CMAKE_DIR = str(Path(CMK_TOOL) / "bin" / "cmake") +_CMAKE_EXE = "cmake.exe" if IS_WINDOWS else "cmake" +CMAKE_DIR = str(Path(CMK_TOOL) / "bin" / _CMAKE_EXE)
551-563: PATH: add cmake’s directory, not the binary pathAppending a file path to PATH is ineffective.
additional_packages = [ str(Path(TOOLCHAIN_DIR) / "bin"), NINJA_DIR, - CMAKE_DIR, + os.path.dirname(CMAKE_DIR), os.path.dirname(get_python_exe()), ]
1476-1482: Likewise quote -D args and script path for bundle embedPrevents breakage when paths contain spaces.
- f'"{CMAKE_DIR}"', - "-DDATA_FILE=" + bundle_path, - "-DSOURCE_FILE=%s.S" % bundle_path, - "-DFILE_TYPE=BINARY", - "-P", - str(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake"), + f'"{CMAKE_DIR}"', + f'-DDATA_FILE="{bundle_path}"', + f'-DSOURCE_FILE="{bundle_path}.S"', + "-DFILE_TYPE=BINARY", + "-P", + f'"{str(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake")}"',
🧹 Nitpick comments (12)
builder/penv_setup.py (1)
101-106: uv resolution fallback aligns with project preferenceDeriving uv next to PYTHONEXE and falling back to PATH (“uv”) matches the previously stated preference to trust system executables. Consider logging the fallback path when used for easier troubleshooting.
- if not os.path.isfile(uv_cmd): - uv_cmd = "uv" + if not os.path.isfile(uv_cmd): + uv_cmd = "uv" + print("Info: using system 'uv' from PATH")examples/arduino-blink/platformio.ini (1)
16-22: Expanded lib_ignore sets (ble, Matter, ESP RainMaker)Looks intentional for coverage of different dependency scenarios. Please confirm “ESP RainMaker” matches the exact library name you intend to ignore in these envs; otherwise builds may not filter it.
Also applies to: 44-52, 71-78, 96-103, 131-138, 150-156, 168-175
builder/frameworks/arduino.py (1)
902-902: Silence linter: unused lambda args in strfunctionRuff flags ARG005 for target/source/env. Use a star‑arg lambda.
- # silence scons command output - silent_action.strfunction = lambda target, source, env: '' + # silence scons command output + silent_action.strfunction = lambda *_: ''builder/frameworks/ulp.py (2)
185-187: Ensure SCons variable expansion for Depends/Requires targetUsing "$BUILD_DIR" inside a raw string can bypass env substitution in some contexts. Safer to pre‑expand with ulp_env.subst.
-ulp_env.Depends(str(Path("$BUILD_DIR") / "${PROGNAME}.elf"), ulp_assembly) -ulp_env.Requires(str(Path("$BUILD_DIR") / "${PROGNAME}.elf"), ulp_assembly) +_prog_elf = ulp_env.subst(str(Path("$BUILD_DIR") / "${PROGNAME}.elf")) +ulp_env.Depends(_prog_elf, ulp_assembly) +ulp_env.Requires(_prog_elf, ulp_assembly)
31-31: Quiet false‑positive “undefined name” from linters (SCons Import)If Ruff runs on this file, consider TYPE_CHECKING hints to appease F821.
from SCons.Script import Import +from typing import TYPE_CHECKING, Any + if TYPE_CHECKING: + project_config: dict[str, Any] + idf_variant: strAlso applies to: 110-110
builder/frameworks/_embed_files.py (1)
95-101: Optional: silence linters about env being “undefined” in SCons filesStatic analyzers may flag env; add TYPE_CHECKING stub.
Outside this hunk:
from typing import TYPE_CHECKING if TYPE_CHECKING: from SCons.Script import Environment env: "Environment" # type: ignorebuilder/frameworks/component_manager.py (2)
678-689: Minor perf: prefer os.scandir for library discoverySlightly faster and returns dir entries with is_dir().
- for entry in os.listdir(arduino_libs_dir): - lib_path = str(Path(arduino_libs_dir) / entry) - if os.path.isdir(lib_path): + for de in os.scandir(arduino_libs_dir): + if de.is_dir(): + entry = de.name + lib_path = de.path
936-948: Regex pass is broad; precompile per-lib to reduce backtrackingPrecompile patterns and use non-greedy anchors to avoid accidental multi-line removals.
builder/frameworks/espidf.py (1)
1118-1131: Narrow exception handling and structure try/except/elseAvoid blind catch and satisfy linter TRY300.
- try: - # Create a LibraryIgnoreHandler instance to process lib_ignore - config = _component_manager.ComponentManagerConfig(env) - logger = _component_manager.ComponentLogger() - lib_handler = _component_manager.LibraryIgnoreHandler(config, logger) - - # Get the processed lib_ignore entries (already converted to component names) - lib_ignore_entries = lib_handler._get_lib_ignore_entries() - - return lib_ignore_entries - except Exception as e: - print(f"[ESP-IDF] Warning: Could not process lib_ignore: {e}") - return [] + try: + config = _component_manager.ComponentManagerConfig(env) + logger = _component_manager.ComponentLogger() + lib_handler = _component_manager.LibraryIgnoreHandler(config, logger) + lib_ignore_entries = lib_handler._get_lib_ignore_entries() + except Exception as e: + print(f"[ESP-IDF] Warning: Could not process lib_ignore: {e}") + return [] + else: + return lib_ignore_entriesbuilder/main.py (3)
468-470: Dedicated ERASETOOL/ERASECMD improves clarity and decouples erase from other tools.
Consider renaming ERASETOOL to ESPTOOL for semantic clarity, since it’s now used beyond erase (e.g., elf2image). Optional.
622-624: Augment install hint to include pip fallback (keeps uv first as preferred).
Based on your past preference to trust PATH-resolved tools like uv, keep uv primary but add a pip fallback in the message for users without uv.Apply:
- print(f'Make sure esp-idf-size is installed: uv pip install --python "{PYTHON_EXE}" esp-idf-size') + print(f'Make sure esp-idf-size is installed: uv pip install --python "{PYTHON_EXE}" esp-idf-size (or: "{PYTHON_EXE}" -m pip install esp-idf-size)')
731-733: Minor nit: stray blank line in UPLOADERFLAGS list.
No functional impact; remove if you prefer tighter formatting."--after", - board.get("upload.after_reset", "hard-reset"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
builder/frameworks/_embed_files.py(7 hunks)builder/frameworks/arduino.py(7 hunks)builder/frameworks/component_manager.py(18 hunks)builder/frameworks/espidf.py(63 hunks)builder/frameworks/ulp.py(11 hunks)builder/main.py(10 hunks)builder/penv_setup.py(5 hunks)examples/arduino-blink/platformio.ini(7 hunks)examples/arduino-rmt-blink/platformio.ini(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/penv_setup.pybuilder/frameworks/espidf.py
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/espidf.pyexamples/arduino-blink/platformio.ini
📚 Learning: 2025-08-08T09:55:50.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:55:50.842Z
Learning: In pioarduino/platform-espressif32 (builder/frameworks/espidf.py, Python), for get_app_partition_offset: preferred behavior is to probe app/ota_0 first, then app/factory; avoid using the {"name":"boot"} sentinel; make probes non-fatal (don’t exit on missing partitions), optionally fall back to calling parttool.py with --partition-boot-default, then default to 0x10000.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:50:01.969Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:50:01.969Z
Learning: Preference in pioarduino/platform-espressif32 (builder/frameworks/espidf.py): Avoid using {"name": "boot"} as a sentinel when resolving the default app partition offset. Instead, call parttool.py with --partition-boot-default directly (and optionally fall back to factory/ota_0) to prevent confusion with non-existent CSV names.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:44:18.366Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:44:18.366Z
Learning: In builder/frameworks/espidf.py, get_partition_info treats pt_params["name"] == "boot" as a sentinel to pass --partition-boot-default to parttool.py; "boot" is not expected to be a CSV partition name.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#199
File: examples/arduino-blink/platformio.ini:81-83
Timestamp: 2025-06-26T09:56:30.658Z
Learning: In examples/arduino-blink/platformio.ini, the different lib_ignore configurations across environments (some including wifi, some not, different syntax styles) are intentional test cases, not inconsistencies that need to be fixed.
Applied to files:
examples/arduino-blink/platformio.iniexamples/arduino-rmt-blink/platformio.ini
🪛 Ruff (0.12.2)
builder/frameworks/ulp.py
31-31: Undefined name project_config
(F821)
110-110: Undefined name idf_variant
(F821)
builder/frameworks/_embed_files.py
95-95: Undefined name env
(F821)
96-96: Undefined name env
(F821)
98-98: Undefined name env
(F821)
99-99: Undefined name env
(F821)
100-100: Undefined name env
(F821)
100-100: Undefined name env
(F821)
103-103: Unused function argument: target
(ARG001)
103-103: Unused function argument: env
(ARG001)
137-137: Undefined name env
(F821)
142-142: Undefined name env
(F821)
builder/frameworks/espidf.py
564-564: Consider [*additional_packages, idf_env["PATH"]] instead of concatenation
Replace with [*additional_packages, idf_env["PATH"]]
(RUF005)
1127-1127: Consider moving this statement to an else block
(TRY300)
1128-1128: Do not catch blind exception: Exception
(BLE001)
builder/frameworks/arduino.py
902-902: Unused lambda argument: target
(ARG005)
902-902: Unused lambda argument: source
(ARG005)
902-902: Unused lambda argument: env
(ARG005)
🔇 Additional comments (18)
examples/arduino-rmt-blink/platformio.ini (1)
5-6: Confirm intent of expanded ignores (ble + wifi) across envsThese additions look fine; just confirm they’re intentional for this example since they disable BLE/WiFi builds on S2/S3/C3/C5/C6. If that’s the goal (pure RMT demo), all good.
Also applies to: 16-17, 27-28, 38-41, 49-50
builder/penv_setup.py (3)
81-81: Pathlib return for executables — LGTMReturning str(Path(...)) is correct for downstream consumers.
145-147: Cross‑platform site‑packages detection — LGTMWindows/posix branches are correct; casting to str avoids surprises with subprocess/env.
427-427: Using Path for penv_dir — LGTMConsistent with the PR’s goal and safe.
builder/frameworks/arduino.py (3)
38-39: Use platformio.compat.IS_WINDOWS instead of ad‑hoc detection — LGTM
72-76: esp32c3 include‑path threshold bumped to 32000 — sanity checkValue is still under the 32767 ceiling; just confirm no regressions on typical toolchains/paths for C3.
313-314: Pathlib migration in SDK/config/build paths — LGTMConversions to str(Path(...)) before external use are correct and consistent.
Also applies to: 509-512, 569-569, 909-910
builder/frameworks/ulp.py (1)
17-18: Pathlib migration throughout ULP builder — LGTMStringifying Path objects before passing to tools keeps behavior stable and cross‑platform.
Also applies to: 31-31, 55-55, 64-66, 81-81, 101-106, 109-120, 131-136, 141-146, 155-158, 166-172, 175-178
builder/frameworks/_embed_files.py (1)
37-60: Pathlib migration here looks goodUsing Path("$PROJECT_DIR")/… then env.subst later is consistent and safe.
builder/frameworks/component_manager.py (1)
824-828: TinyUSB include mapping additions look correctMapping usb/tinyusb to arduino_tinyusb prevents accidental removal.
builder/frameworks/espidf.py (1)
137-144: Dynamic import pathing for component_manager is fineLoading via platform.get_dir()/builder/frameworks/... is consistent with repo layout.
builder/main.py (7)
22-22: Pathlib import aligns with the refactor goal — good move.
576-585: Robust map-file resolution using Path and fallback – LGTM.
Nice fallback to project dir with clear error messaging.
632-638: nobuild targets: Path-based targets are correct and portable.
646-647: DataToBin target path via Path is correct; suffix handling remains with the Builder — good.
651-653: ElfToBin invocation with Path target aligns with the refactor; dependency on checkprogsize retained.
709-713: ESP OTA: UPLOADER path resolution via Path.resolve() and quoting of PYTHON_EXE — solid.
514-532: No lingering binutils objcopy dependency in elf2image conversion
All remainingOBJCOPYreferences are either the env var inbuilder/main.py(pointing at the uploader tool) or the toolchain’s*-elf-objcopycalls inframeworks/_embed_files.py(for embedding binary data)—neither impacts the new esptool-based image conversion.
Removed unnecessary newline in upload command.
to reduce compile time
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
builder/penv_setup.py (2)
198-258: Blocking issue: uv-in-penv assumption breaks installs; use external uv or pip fallbackThe logic assumes a uv binary inside penv, then tries to “install uv” via pip. Astral’s uv isn’t a pip package that yields a uv executable in the venv, so subsequent calls like [penv_uv_executable, "pip", ...] can fail, blocking dependency installation and esptool setup.
Recommend: resolve a working uv command (external uv preferred per project learnings), otherwise fall back to pip for both listing and installing. Do not try to “pip install uv”.
Apply minimal surgical changes:
@@ - penv_uv_executable = get_executable_path(penv_dir, "uv") - - # Check if uv is available in the penv - uv_in_penv_available = False - try: - result = subprocess.run( - [penv_uv_executable, "--version"], - capture_output=True, - text=True, - timeout=10 - ) - uv_in_penv_available = result.returncode == 0 - except (FileNotFoundError, subprocess.TimeoutExpired): - uv_in_penv_available = False - - # Install uv into penv if not available - if not uv_in_penv_available: - if external_uv_executable: - # Use external uv to install uv into the penv - try: - subprocess.check_call( - [external_uv_executable, "pip", "install", "uv>=0.1.0", f"--python={python_exe}", "--quiet"], - stdout=subprocess.DEVNULL, - stderr=subprocess.STDOUT, - timeout=120 - ) - except subprocess.CalledProcessError as e: - print(f"Error: uv installation failed with exit code {e.returncode}") - return False - except subprocess.TimeoutExpired: - print("Error: uv installation timed out") - return False - except FileNotFoundError: - print("Error: External uv executable not found") - return False - except Exception as e: - print(f"Error installing uv package manager into penv: {e}") - return False - else: - # No external uv available, use pip to install uv into penv - try: - subprocess.check_call( - [python_exe, "-m", "pip", "install", "uv>=0.1.0", "--quiet"], - stdout=subprocess.DEVNULL, - stderr=subprocess.STDOUT, - timeout=120 - ) - except subprocess.CalledProcessError as e: - print(f"Error: uv installation via pip failed with exit code {e.returncode}") - return False - except subprocess.TimeoutExpired: - print("Error: uv installation via pip timed out") - return False - except FileNotFoundError: - print("Error: Python executable not found") - return False - except Exception as e: - print(f"Error installing uv package manager via pip: {e}") - return False + penv_uv_executable = get_executable_path(penv_dir, "uv") + # Resolve a usable uv command: external uv (preferred), then penv uv, then PATH + uv_cmd = None + for cand in filter(None, [external_uv_executable, penv_uv_executable, "uv"]): + try: + subprocess.run( + [cand, "--version"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + timeout=5, + check=True, + ) + uv_cmd = cand + break + except Exception: + continue @@ - def _get_installed_uv_packages(): + def _get_installed_packages(): @@ - cmd = [penv_uv_executable, "pip", "list", f"--python={python_exe}", "--format=json"] + cmd = ( + [uv_cmd, "pip", "list", f"--python={python_exe}", "--format=json"] + if uv_cmd + else [python_exe, "-m", "pip", "list", "--format=json"] + ) @@ - installed_packages = _get_installed_uv_packages() + installed_packages = _get_installed_packages() @@ - cmd = [ - penv_uv_executable, "pip", "install", - f"--python={python_exe}", - "--quiet", "--upgrade" - ] + packages_list + cmd = ( + [uv_cmd, "pip", "install", f"--python={python_exe}", "--quiet", "--upgrade"] + packages_list + if uv_cmd + else [python_exe, "-m", "pip", "install", "--quiet", "--upgrade"] + packages_list + )This keeps external uv as the first choice (aligned with project preference) while ensuring builds proceed even when uv isn’t present in the venv.
Also applies to: 268-299, 303-339
389-395: esptool install: add robust fallback to pip when uv is unavailableCurrently this unconditionally calls a uv executable path which may not exist. Safe fallback to pip avoids blocking builds.
- try: - subprocess.check_call([ - uv_executable, "pip", "install", "--quiet", "--force-reinstall", - f"--python={python_exe}", - "-e", esptool_repo_path - ], timeout=60) + try: + if uv_executable and os.path.isfile(uv_executable): + cmd = [uv_executable, "pip", "install", "--quiet", "--force-reinstall", + f"--python={python_exe}", "-e", esptool_repo_path] + else: + cmd = [python_exe, "-m", "pip", "install", "--quiet", "--force-reinstall", + "-e", esptool_repo_path] + subprocess.check_call(cmd, timeout=60)
♻️ Duplicate comments (5)
builder/frameworks/_embed_files.py (1)
79-84: Fix crash on zero-length files in prepare_file (repeat)Still seeks -1 from end; zero-length files will raise. Prior comment remains valid.
with open(filepath, "rb+") as fp: - fp.seek(-1, SEEK_END) - if fp.read(1) != b"\0": - fp.seek(0, SEEK_CUR) - fp.write(b"\0") + fp.seek(0, SEEK_END) + if fp.tell() == 0: + fp.write(b"\0") + return + fp.seek(-1, SEEK_END) + if fp.read(1) != b"\0": + fp.write(b"\0")builder/frameworks/espidf.py (4)
482-487: Windows: use cmake.exe on Windows, cmake otherwiseWithout the “.exe”, subprocess calls will fail on Windows.
-CMAKE_DIR = str(Path(CMK_TOOL) / "bin" / "cmake") +_CMAKE_EXE = "cmake.exe" if IS_WINDOWS else "cmake" +CMAKE_DIR = str(Path(CMK_TOOL) / "bin" / _CMAKE_EXE)
557-565: Add cmake’s directory to PATH, not the binary itselfAppending the file path to PATH is ineffective.
- additional_packages = [ - str(Path(TOOLCHAIN_DIR) / "bin"), - NINJA_DIR, - CMAKE_DIR, - os.path.dirname(get_python_exe()), - ] + additional_packages = [ + str(Path(TOOLCHAIN_DIR) / "bin"), + NINJA_DIR, + os.path.dirname(CMAKE_DIR), + os.path.dirname(get_python_exe()), + ]
1437-1445: Quote all -D path arguments in string-based cmake actionPrevents breakage when paths contain spaces.
- f'"{CMAKE_DIR}"', - "-DCC=%s" % str(Path(TOOLCHAIN_DIR) / "bin" / "$CC"), + f'"{CMAKE_DIR}"', + f'-DCC="{str(Path(TOOLCHAIN_DIR) / "bin" / "$CC")}"', "-DSOURCE=$SOURCE", "-DTARGET=$TARGET", - "-DCONFIG_DIR=%s" % str(Path(BUILD_DIR) / "config"), - "-DLD_DIR=%s" % str(Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld"), + f'-DCONFIG_DIR="{str(Path(BUILD_DIR) / "config")}"', + f'-DLD_DIR="{str(Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld")}"', "-P", - str(Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake"), + f'"{str(Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake")}"',
1487-1493: Likewise quote -D args in data_file_embed_asm invocationAvoids failures with spaces in BUILD_DIR or home path.
- f'"{CMAKE_DIR}"', - "-DDATA_FILE=" + bundle_path, - "-DSOURCE_FILE=%s.S" % bundle_path, + f'"{CMAKE_DIR}"', + f'-DDATA_FILE="{bundle_path}"', + f'-DSOURCE_FILE="{bundle_path}.S"', "-DFILE_TYPE=BINARY", "-P", - str(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake"), + f'"{str(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake")}"',
🧹 Nitpick comments (14)
builder/penv_setup.py (2)
443-445: Pass a working uv to esptool installerPrefer the external uv used to create the venv; fall back to penv uv if present, otherwise just "uv" (PATH). This prevents None/invalid command usage.
- esptool_binary_path = get_executable_path(penv_dir, "esptool") - uv_executable = get_executable_path(penv_dir, "uv") + esptool_binary_path = get_executable_path(penv_dir, "esptool") + penv_uv = get_executable_path(penv_dir, "uv") + uv_executable = used_uv_executable or (penv_uv if os.path.isfile(penv_uv) else "uv") @@ - install_esptool(env, platform, penv_python, uv_executable) + install_esptool(env, platform, penv_python, uv_executable)Also applies to: 455-456
28-36: Minor: duplicate Python version checksTop-level and in-function checks duplicate. Keep one (prefer top-level) to avoid drift.
Also applies to: 418-426
builder/frameworks/_embed_files.py (1)
103-106: Nit: silence unused args for SCons emitterRename to underscores to satisfy linters without changing behavior.
-def transform_to_asm(target, source, env): +def transform_to_asm(_target, source, _env):builder/frameworks/arduino.py (1)
902-902: Nit: underscore unused lambda args to appease linters- silent_action.strfunction = lambda target, source, env: '' + silent_action.strfunction = lambda _t, _s, _e: ''builder/frameworks/component_manager.py (3)
161-164: Avoid potentially misleading backup log_backup_pioarduino_build_py() can no-op (e.g., missing libs dir), yet we always log “Created”. Make the message generic to reflect intent without implying success.
- self._backup_pioarduino_build_py() - self.logger.log_change("Created backup of build file") + self._backup_pioarduino_build_py() + self.logger.log_change("Attempted to create backup of build file")
470-498: Regex-based CPPPATH pruning is broad; scope it to reduce accidental deletionsPatterns match any line containing include/ and may catch unrelated strings. Consider anchoring around the CPPPATH assignment or a known sentinel to narrow impact. The 50% file-size check helps, but scoping reduces churn and review noise. I can propose an AST-based edit if you want higher precision.
1047-1071: BackupManager uses libs path without the early-return guard; ensure consistencyHere we build paths from arduino_libs_mcu without a truthiness check, unlike the other backup helpers. If arduino_libs_mcu were empty, build_py_path becomes a relative “pioarduino-build.py”. If that’s intentional per prior preference, please confirm call sites guarantee a valid libs dir before invoking BackupManager. I can help unify behavior across the three backup helpers if desired.
builder/frameworks/espidf.py (1)
2222-2226: Hardcoded “variants/tasmota” base for extra images—confirm intent or make configurableIf this is meant only for Tasmota boards, fine. If broader, read the base dir from a board option with a sensible default.
- extra_img_dir = Path(env.subst("$PROJECT_DIR")) / "variants" / "tasmota" + extra_img_dir = Path(env.subst("$PROJECT_DIR")) / board.get( + "upload.arduino.extra_images_dir", "variants/tasmota" + )builder/main.py (6)
22-22: Pathlib import is a good move; consider completing the migration.You still import os.path helpers (isfile, join). For consistency and to avoid mixing paradigms, consider migrating remaining join/isfile usages in this file to Path as follow-ups.
467-470: Rename ERASETOOL to ESPTOOL for clarity (it’s now used beyond erase).This var backs both erase and elf2image. A neutral name avoids confusion and improves maintainability.
Apply:
- ERASETOOL=uploader_path, - ERASECMD='$ERASETOOL $ERASEFLAGS erase-flash', + ESPTOOL=uploader_path, + ERASECMD='$ESPTOOL $ERASEFLAGS erase-flash',Follow-up: update other references in this file to $ESPTOOL (see ElfToBin suggestion below).
514-527: Use the unified $ESPTOOL var in ElfToBin (pairs with the rename above).Keeps the tool reference consistent across erase and elf2image.
- "$ERASETOOL", + "$ESPTOOL",
576-582: Path-based map-file resolution looks good; small readability tweak.Avoid recreating Path from str multiple times; keep a Path throughout and stringify only at subprocess call.
- map_file = str(Path(env.subst("$BUILD_DIR")) / (env.subst("$PROGNAME") + ".map")) - if not Path(map_file).is_file(): - # map file can be in project dir - map_file = str(Path(get_project_dir()) / (env.subst("$PROGNAME") + ".map")) - if not Path(map_file).is_file(): + progname = env.subst("$PROGNAME") + map_file = Path(env.subst("$BUILD_DIR")) / f"{progname}.map" + if not map_file.is_file(): + # map file can be in project dir + map_file = Path(get_project_dir()) / f"{progname}.map" + if not map_file.is_file():And later when passing to subprocess (outside this hunk):
# replace cmd.append(map_file) # with cmd.append(str(map_file))
632-638: Variable expansion inside Path strings: verify Windows behavior.Using Path("$BUILD_DIR") yields backslashes on Windows (e.g., "$BUILD_DIR\..."). SCons typically resolves these fine, but please sanity-check that “pio run -t nobuild” paths expand correctly on Windows shells.
786-789: dfu-util path via Path.resolve(): OK; consider .with_suffix on Windows only if needed.If any Windows users hit resolution issues, consider appending “.exe” conditionally; otherwise this is consistent with PlatformIO’s typical handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
builder/frameworks/_embed_files.py(7 hunks)builder/frameworks/arduino.py(7 hunks)builder/frameworks/component_manager.py(18 hunks)builder/frameworks/espidf.py(65 hunks)builder/frameworks/ulp.py(11 hunks)builder/main.py(9 hunks)builder/penv_setup.py(5 hunks)examples/arduino-blink/platformio.ini(4 hunks)examples/arduino-rmt-blink/platformio.ini(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/penv_setup.pybuilder/frameworks/espidf.py
📚 Learning: 2025-09-04T15:27:18.112Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
Applied to files:
builder/frameworks/component_manager.pybuilder/frameworks/_embed_files.py
📚 Learning: 2025-08-26T14:52:29.044Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Applied to files:
builder/frameworks/component_manager.pybuilder/frameworks/_embed_files.py
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/component_manager.pyexamples/arduino-blink/platformio.inibuilder/frameworks/espidf.pybuilder/frameworks/_embed_files.py
📚 Learning: 2025-08-26T14:53:03.852Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:192-194
Timestamp: 2025-08-26T14:53:03.852Z
Learning: Jason2866 prefers to trust the system for path handling in penv_setup.py and doesn't want additional path validation for security concerns around penv directory derivation, indicating confidence in the controlled environment and internal path generation.
Applied to files:
builder/frameworks/component_manager.pybuilder/frameworks/_embed_files.py
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#199
File: examples/arduino-blink/platformio.ini:81-83
Timestamp: 2025-06-26T09:56:30.658Z
Learning: In examples/arduino-blink/platformio.ini, the different lib_ignore configurations across environments (some including wifi, some not, different syntax styles) are intentional test cases, not inconsistencies that need to be fixed.
Applied to files:
examples/arduino-rmt-blink/platformio.iniexamples/arduino-blink/platformio.ini
📚 Learning: 2025-08-08T09:55:50.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:55:50.842Z
Learning: In pioarduino/platform-espressif32 (builder/frameworks/espidf.py, Python), for get_app_partition_offset: preferred behavior is to probe app/ota_0 first, then app/factory; avoid using the {"name":"boot"} sentinel; make probes non-fatal (don’t exit on missing partitions), optionally fall back to calling parttool.py with --partition-boot-default, then default to 0x10000.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:50:01.969Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:50:01.969Z
Learning: Preference in pioarduino/platform-espressif32 (builder/frameworks/espidf.py): Avoid using {"name": "boot"} as a sentinel when resolving the default app partition offset. Instead, call parttool.py with --partition-boot-default directly (and optionally fall back to factory/ota_0) to prevent confusion with non-existent CSV names.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:44:18.366Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:44:18.366Z
Learning: In builder/frameworks/espidf.py, get_partition_info treats pt_params["name"] == "boot" as a sentinel to pass --partition-boot-default to parttool.py; "boot" is not expected to be a CSV partition name.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-10T16:59:15.533Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Applied to files:
builder/frameworks/_embed_files.py
🪛 Ruff (0.12.2)
builder/frameworks/arduino.py
902-902: Unused lambda argument: target
(ARG005)
902-902: Unused lambda argument: source
(ARG005)
902-902: Unused lambda argument: env
(ARG005)
builder/frameworks/ulp.py
31-31: Undefined name project_config
(F821)
110-110: Undefined name idf_variant
(F821)
builder/frameworks/espidf.py
1135-1135: Consider moving this statement to an else block
(TRY300)
builder/frameworks/_embed_files.py
95-95: Undefined name env
(F821)
96-96: Undefined name env
(F821)
98-98: Undefined name env
(F821)
99-99: Undefined name env
(F821)
100-100: Undefined name env
(F821)
100-100: Undefined name env
(F821)
103-103: Unused function argument: target
(ARG001)
103-103: Unused function argument: env
(ARG001)
137-137: Undefined name env
(F821)
142-142: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-zigbee-light)
🔇 Additional comments (21)
examples/arduino-rmt-blink/platformio.ini (1)
5-11: LGTM: lib_ignore expanded and normalized across envsMulti-line style and added entries (ble, wifi, ESP RainMaker, ESP Insights, Zigbee, etc.) look consistent with the broader PR. If any name mismatches the actual Arduino lib names, PlatformIO will silently not ignore—worth a quick spot-check in frameworks-libs.
Also applies to: 19-25, 33-39, 47-53, 61-67
builder/penv_setup.py (3)
81-81: Pathlib return: correct and cross-platformReturning str(Path(...)/Scripts|bin/) is the right normalization for SCons/subprocess.
101-106: uv resolution: good derivation + PATH fallbackDeriving uv beside PYTHONEXE and falling back to "uv" matches the project preference to trust system executables.
145-147: site-packages path calc: correct for venvs on Win/LinuxThe pythonX.Y segment on non-Windows is right; stringifying Path before site.addsitedir avoids surprises.
builder/frameworks/ulp.py (2)
31-31: ULP_BUILD_DIR Pathlib conversion: goodStringified Path for SCons is correct; keeps semantics, improves portability.
100-106: CMake/tool/script paths: consistent Path usageDirect Path-based construction without forcing .exe and without extra quoting is correct here given exec_command/VerboseAction usage and prior project guidance.
Also applies to: 119-120, 141-146, 166-172, 175-177
builder/frameworks/_embed_files.py (1)
37-38: Pathlib migration: consistent and safeAll touched joins moved to Path with str(...) at call sites; matches the PR’s objective and SCons expectations.
Also applies to: 59-60, 95-101, 104-105, 137-143, 165-170
builder/frameworks/arduino.py (4)
39-39: Importing IS_WINDOWS from platformio.compat: correctRemoves local duplication; aligns with other modules.
72-80: esp32c3 threshold bumped to 32000: OKMatches aggressive defaults used elsewhere; keep an eye on command-line limits on older Windows.
312-314: sdk_dir Path join: goodfs.to_unix_path(str(Path(...))) ensures consistent flags downstream.
569-569: Pathlib normalization: looks goodexists(str(Path(...))) and Path-built build script path are correct.
Also applies to: 600-600, 908-909
examples/arduino-blink/platformio.ini (2)
16-41: Lib exclusions and component removals look consistent with the new lib_ignore flowExpanded lib_ignore and custom_component_remove entries align with the new ComponentManager logic and the prior intent to prune optional subsystems in examples. No functional concerns from my side.
Also applies to: 48-73, 80-104, 110-147, 154-178, 184-207, 214-238
72-74: Confirm availability of espressif/cmake_utilities 0. for C2*Sanity-check that this component exists in the ESP Component Registry and is compatible with your pinned IDF range to avoid resolution failures during dependency lock.
builder/frameworks/component_manager.py (4)
53-57: Path-based Arduino libs MCU resolution: LGTMClean Path usage with safe fallback when the package is missing.
235-249: idf_component.yml discovery/creation: LGTMFramework-first lookup with project fallback and default generation is robust.
447-461: Guard against empty include base when removing directoriesinclude_path uses self.config.arduino_libs_mcu. If that’s empty, removal targets a relative include/… path. Given past preference to proceed without added guards, please confirm arduino_libs_mcu is always non-empty before _cleanup_removed_components runs.
678-682: Arduino core library scan: LGTMEarly exit when framework dir is unavailable and normalized Path assembly are correct.
builder/frameworks/espidf.py (2)
57-63: Dynamic import of component_manager via Path: LGTMLocalizes the dependency without altering sys.path permanently.
90-96: Framework dir validation and Path normalization: LGTMEarly exit with a clear error message is good here.
builder/main.py (2)
646-652: Good: consistent Path usage for DataToBin/ElfToBin targets.This avoids OS-specific concatenation issues and improves readability.
709-712: espota.py path via Path.resolve() is solid; quoting is correct.UPLOADER is unquoted, but you wrap it in quotes in UPLOADCMD, so spaces in paths are handled.
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
builder/frameworks/component_manager.py (1)
457-461: Guard against accidental deletion outside Arduino libs treeWhen arduino_libs_mcu is empty, include_path becomes a relative path like "include/", risking unintended deletions. Add a minimal anchor check before rmtree.
- include_path = str(Path(self.config.arduino_libs_mcu) / "include" / component) + if not self.config.arduino_libs_mcu: + return + include_path = str(Path(self.config.arduino_libs_mcu) / "include" / component)builder/frameworks/ulp.py (1)
140-161: Windows breakage and quoting: cmake path and args need fixes
- On Windows, cmake is “cmake.exe”; current path omits the extension.
- VerboseAction executes a shell string; unquoted paths break when dirs contain spaces.
- cmd = ( - str(Path(platform.get_package_dir("tool-cmake")) / "bin" / "cmake"), - "--build", - ULP_BUILD_DIR, - "--target", - "build", - ) + cmake_exe = str(Path(platform.get_package_dir("tool-cmake")) / "bin" / + ("cmake.exe" if "windows" in get_systype() else "cmake")) + cmd = (cmake_exe, "--build", ULP_BUILD_DIR, "--target", "build") @@ - return ulp_binary_env.Command( + cmd_str = " ".join([f'"{cmd[0]}"', "--build", f'"{ULP_BUILD_DIR}"', "--target", "build"]) + return ulp_binary_env.Command( [ str(Path(ULP_BUILD_DIR) / "ulp_main.h"), str(Path(ULP_BUILD_DIR) / "ulp_main.ld"), str(Path(ULP_BUILD_DIR) / "ulp_main.bin"), ], None, - ulp_binary_env.VerboseAction(" ".join(cmd), "Generating ULP project files $TARGETS"), + ulp_binary_env.VerboseAction(cmd_str, "Generating ULP project files $TARGETS"), )
♻️ Duplicate comments (6)
builder/frameworks/_embed_files.py (1)
79-84: Fix zero-length file crash in prepare_filefp.seek(-1, SEEK_END) raises on empty files. Guard by checking size first.
with open(filepath, "rb+") as fp: - fp.seek(-1, SEEK_END) - if fp.read(1) != b"\0": - fp.seek(0, SEEK_CUR) - fp.write(b"\0") + fp.seek(0, SEEK_END) + if fp.tell() == 0: + fp.write(b"\0") + return + fp.seek(-1, SEEK_END) + if fp.read(1) != b"\0": + fp.write(b"\0")builder/frameworks/espidf.py (5)
1437-1445: Quote -D path args in string-based cmake actionPrevents breakage when paths contain spaces.
- f'"{CMAKE_DIR}"', - "-DCC=%s" % str(Path(TOOLCHAIN_DIR) / "bin" / "$CC"), + f'"{CMAKE_DIR}"', + f'-DCC="{str(Path(TOOLCHAIN_DIR) / "bin" / "$CC")}"', "-DSOURCE=$SOURCE", "-DTARGET=$TARGET", - "-DCONFIG_DIR=%s" % str(Path(BUILD_DIR) / "config"), - "-DLD_DIR=%s" % str(Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld"), + f'-DCONFIG_DIR="{str(Path(BUILD_DIR) / "config")}"', + f'-DLD_DIR="{str(Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld")}"', "-P", - str(Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake"), + f'"{str(Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake")}"',
1487-1493: Likewise quote -D args in data_file_embed_asm invocationEnsures stability with spaces in paths.
- f'"{CMAKE_DIR}"', - "-DDATA_FILE=" + bundle_path, - "-DSOURCE_FILE=%s.S" % bundle_path, + f'"{CMAKE_DIR}"', + f'-DDATA_FILE="{bundle_path}"', + f'-DSOURCE_FILE="{bundle_path}.S"', "-DFILE_TYPE=BINARY", "-P", - str(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake"), + f'"{str(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake")}"',
482-487: Windows: cmake path missing “.exe”Use platform-aware filename when constructing CMAKE_DIR.
-CMAKE_DIR = str(Path(CMK_TOOL) / "bin" / "cmake") +_CMAKE_EXE = "cmake.exe" if IS_WINDOWS else "cmake" +CMAKE_DIR = str(Path(CMK_TOOL) / "bin" / _CMAKE_EXE)
551-565: PATH pollution: add cmake’s directory, not the binaryPrepend the directory containing cmake to PATH.
- additional_packages = [ - str(Path(TOOLCHAIN_DIR) / "bin"), - NINJA_DIR, - CMAKE_DIR, - os.path.dirname(get_python_exe()), - ] + additional_packages = [ + str(Path(TOOLCHAIN_DIR) / "bin"), + NINJA_DIR, + os.path.dirname(CMAKE_DIR), + os.path.dirname(get_python_exe()), + ]
1121-1139: Avoid private method; also apply BT/BLE protectionStop calling a private helper and mirror Arduino-side BT/BLE safety so “bt” isn’t ignored when NimBLE/BLE is in lib_deps.
def get_lib_ignore_components(): @@ - # Create a LibraryIgnoreHandler instance to process lib_ignore - config = _component_manager.ComponentManagerConfig(env) - logger = _component_manager.ComponentLogger() - lib_handler = _component_manager.LibraryIgnoreHandler(config, logger) - - # Get the processed lib_ignore entries (already converted to component names) - lib_ignore_entries = lib_handler._get_lib_ignore_entries() - - return lib_ignore_entries + config = _component_manager.ComponentManagerConfig(env) + logger = _component_manager.ComponentLogger() + lib_handler = _component_manager.LibraryIgnoreHandler(config, logger) + lib_ignore_entries = lib_handler._get_lib_ignore_entries() + try: + if lib_handler._has_bt_ble_dependencies(): + lib_ignore_entries = [e for e in lib_ignore_entries if e.lower() != "bt"] + except Exception: + pass + return lib_ignore_entries
🧹 Nitpick comments (5)
builder/frameworks/arduino.py (1)
902-903: Silence action: avoid unused-arg warningsUse a varargs lambda to silence Ruff ARG005.
-# silence scons command output -silent_action.strfunction = lambda target, source, env: '' +# silence scons command output +silent_action.strfunction = lambda *_: ''builder/frameworks/_embed_files.py (1)
137-143: Quote -D/-P argument values to survive spacesKeep the cmake path as-is (per prior feedback), but quote DATA_FILE/SOURCE_FILE/FILE_TYPE values and the -P script path.
- str(Path(env.PioPlatform().get_package_dir("tool-cmake") or "") / "bin" / "cmake"), - "-DDATA_FILE=$SOURCE", - "-DSOURCE_FILE=$TARGET", - "-DFILE_TYPE=$FILE_TYPE", - "-P", - str(Path(env.PioPlatform().get_package_dir("framework-espidf") or "") / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake"), + str(Path(env.PioPlatform().get_package_dir("tool-cmake") or "") / "bin" / "cmake"), + '-DDATA_FILE="$SOURCE"', + '-DSOURCE_FILE="$TARGET"', + '-DFILE_TYPE="$FILE_TYPE"', + "-P", + f'"{str(Path(env.PioPlatform().get_package_dir("framework-espidf") or "") / "tools" / "cmake" / "scripts" / "data_file_embed_asm.cmake")}"',builder/frameworks/component_manager.py (1)
52-57: Prefer caching Path alongside str to cut repeated conversionsStore a Path for arduino_libs_mcu and keep the string for API parity. Reduces repeated Path(...) calls throughout.
- self.arduino_libs_mcu = ( - str(Path(ald) / self.mcu) if ald else "" - ) + self.arduino_libs_mcu_path = (Path(ald) / self.mcu) if ald else None + self.arduino_libs_mcu = str(self.arduino_libs_mcu_path) if self.arduino_libs_mcu_path else ""builder/main.py (2)
576-582: Make map file discovery more robust and error message clearer.Prefer a single candidates list and pick the first existing; print all attempted paths if none found.
- map_file = str(Path(env.subst("$BUILD_DIR")) / (env.subst("$PROGNAME") + ".map")) - if not Path(map_file).is_file(): - # map file can be in project dir - map_file = str(Path(get_project_dir()) / (env.subst("$PROGNAME") + ".map")) - - if not Path(map_file).is_file(): - print(f"Error: Map file not found: {map_file}") + candidates = [ + Path(env.subst("$BUILD_DIR")) / f'{env.subst("$PROGNAME")}.map', + Path(get_project_dir()) / f'{env.subst("$PROGNAME")}.map', + ] + map_path = next((p for p in candidates if p.is_file()), None) + if not map_path: + print("Error: Map file not found in any of: " + ", ".join(map(str, candidates))) print("Make sure the project is built first with 'pio run'") return + map_file = str(map_path)
786-788: Handle dfu-util executable name on Windows.Add .exe on Windows to avoid lookup failures.
- UPLOADER=str( - Path(platform.get_package_dir("tool-dfuutil-arduino")).resolve() / "dfu-util" - ), + UPLOADER=str( + (Path(platform.get_package_dir("tool-dfuutil-arduino") or "").resolve() + / ("dfu-util.exe" if IS_WINDOWS else "dfu-util")) + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
builder/frameworks/_embed_files.py(7 hunks)builder/frameworks/arduino.py(7 hunks)builder/frameworks/component_manager.py(18 hunks)builder/frameworks/espidf.py(65 hunks)builder/frameworks/ulp.py(11 hunks)builder/main.py(9 hunks)builder/penv_setup.py(5 hunks)examples/arduino-blink/platformio.ini(4 hunks)examples/arduino-rmt-blink/platformio.ini(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/penv_setup.pybuilder/frameworks/espidf.py
📚 Learning: 2025-08-26T14:52:29.044Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Applied to files:
builder/frameworks/_embed_files.pybuilder/frameworks/component_manager.py
📚 Learning: 2025-08-26T14:53:03.852Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:192-194
Timestamp: 2025-08-26T14:53:03.852Z
Learning: Jason2866 prefers to trust the system for path handling in penv_setup.py and doesn't want additional path validation for security concerns around penv directory derivation, indicating confidence in the controlled environment and internal path generation.
Applied to files:
builder/frameworks/_embed_files.pybuilder/frameworks/component_manager.py
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/_embed_files.pybuilder/frameworks/espidf.pybuilder/frameworks/component_manager.pyexamples/arduino-blink/platformio.ini
📚 Learning: 2025-09-04T15:27:18.112Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
Applied to files:
builder/frameworks/_embed_files.pybuilder/frameworks/component_manager.py
📚 Learning: 2025-08-10T16:59:15.533Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Applied to files:
builder/frameworks/_embed_files.py
📚 Learning: 2025-08-08T09:55:50.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:55:50.842Z
Learning: In pioarduino/platform-espressif32 (builder/frameworks/espidf.py, Python), for get_app_partition_offset: preferred behavior is to probe app/ota_0 first, then app/factory; avoid using the {"name":"boot"} sentinel; make probes non-fatal (don’t exit on missing partitions), optionally fall back to calling parttool.py with --partition-boot-default, then default to 0x10000.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:50:01.969Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:50:01.969Z
Learning: Preference in pioarduino/platform-espressif32 (builder/frameworks/espidf.py): Avoid using {"name": "boot"} as a sentinel when resolving the default app partition offset. Instead, call parttool.py with --partition-boot-default directly (and optionally fall back to factory/ota_0) to prevent confusion with non-existent CSV names.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:44:18.366Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:44:18.366Z
Learning: In builder/frameworks/espidf.py, get_partition_info treats pt_params["name"] == "boot" as a sentinel to pass --partition-boot-default to parttool.py; "boot" is not expected to be a CSV partition name.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#199
File: examples/arduino-blink/platformio.ini:81-83
Timestamp: 2025-06-26T09:56:30.658Z
Learning: In examples/arduino-blink/platformio.ini, the different lib_ignore configurations across environments (some including wifi, some not, different syntax styles) are intentional test cases, not inconsistencies that need to be fixed.
Applied to files:
examples/arduino-blink/platformio.iniexamples/arduino-rmt-blink/platformio.ini
🪛 Ruff (0.12.2)
builder/frameworks/_embed_files.py
95-95: Undefined name env
(F821)
96-96: Undefined name env
(F821)
98-98: Undefined name env
(F821)
99-99: Undefined name env
(F821)
100-100: Undefined name env
(F821)
100-100: Undefined name env
(F821)
103-103: Unused function argument: target
(ARG001)
103-103: Unused function argument: env
(ARG001)
137-137: Undefined name env
(F821)
142-142: Undefined name env
(F821)
builder/frameworks/arduino.py
902-902: Unused lambda argument: target
(ARG005)
902-902: Unused lambda argument: source
(ARG005)
902-902: Unused lambda argument: env
(ARG005)
builder/frameworks/espidf.py
1135-1135: Consider moving this statement to an else block
(TRY300)
builder/frameworks/ulp.py
31-31: Undefined name project_config
(F821)
110-110: Undefined name idf_variant
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (19)
examples/arduino-rmt-blink/platformio.ini (1)
5-11: Lib ignore expansion looks good for this exampleThe added ble/RainMaker/Insights/Zigbee ignores are consistent with a minimal RMT blink and reduce needless dependency processing.
Also applies to: 19-25, 33-39, 47-53, 61-67
builder/penv_setup.py (2)
81-81: Pathlib refactor: LGTMUsing Path with explicit str() at the call sites improves cross-platform stability and SCons interop.
Also applies to: 101-101, 145-147, 427-427
45-58: No changes needed for version floors Specified minimums for rich (>=14.0.0), cryptography (>=45.0.3), and certifi (>=2025.8.3) are at or below the current PyPI releases (rich 14.1.0, cryptography 45.0.7, certifi 2025.8.3), so they won’t break fresh installs.builder/frameworks/arduino.py (1)
313-314: Path conversions and script path usage: LGTMConsistent Path->str joins for SDK/config/build script paths are correct and safer on Windows.
Also applies to: 509-510, 569-569, 599-599, 908-909
examples/arduino-blink/platformio.ini (1)
16-22: Example config pruning: LGTM (intentional variability acknowledged)The added ble/Matter/RainMaker ignores and broader custom_component_remove align with this example’s scope. Leaving per-env differences as-is matches prior intent for test coverage.
Also applies to: 23-41, 48-54, 55-73, 81-87, 88-104, 110-117, 130-147, 155-161, 162-178, 184-190, 191-207, 214-220, 221-238
builder/frameworks/component_manager.py (7)
235-249: Component YAML discovery logic looks solidFramework-first, then project fallback, and default creation with backup is clean and consistent.
425-427: Confirm early-return policy when arduino_libs_mcu is unsetYou return early if arduino_libs_mcu is falsy. Noting the maintainer’s stated preference in this repo to avoid guards in backup flows and “let it proceed” when the dir may be missing. Please confirm this intentional divergence here.
Using retrieved learnings from prior PRs for this project. Would you like me to align backup behavior across all three backup helpers?
578-591: Good safety list for critical componentsProtecting lwIP, FreeRTOS, core ESP, and TinyUSB from lib_ignore is appropriate.
675-682: LGTM: robust Arduino core libraries discoveryUsing resolved framework dir and defensive fallbacks is correct.
855-858: Nice fix on cache initializationSwitching to “is None” avoids hasattr pitfalls.
999-1015: Backup behavior differs across classes—unify?Here you guard on arduino_libs_mcu; BackupManager.backup_pioarduino_build_py doesn’t. Consider a single policy across all three to reduce surprises (either always guard or never guard).
I can prep a small refactor to route all backups through BackupManager for consistency.
1047-1055: BackupManager implementation looks fineMCU-suffixed backups with copy2 and restore flow are clear.
Also applies to: 1069-1074
builder/main.py (7)
22-22: Pathlib import looks good.Adopts pathlib without disrupting legacy os.path usage.
514-527: ElfToBin now decoupled from OBJCOPY via ERASETOOL — OK.This makes the elf2image driver explicit and consistent with erase.
624-624: Helpful guidance on installing esp-idf-size — OK.
646-647: FS image target path via Path — OK.
651-652: Bin target path via Path — OK.
709-712: espota path resolution via Path.resolve and quoted PYTHON_EXE — OK.
632-638: Ensure BUILD_DIR expansion and default APP offset for nobuild
- Use
env.subst("$BUILD_DIR")(or wrapPath()inenv.subst) to guarantee proper Node resolution on Windows and when runningpio run -t nobuild -t upload.- Add a fallback for
ESP32_APP_OFFSETif unset (e.g. default to"0x10000"before partition parsing) to avoid emittingwrite-flash None ….
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
builder/penv_setup.py (1)
389-401: Offline build can fail if uv isn’t present in penv (no fallback to pip)When no internet and penv was created without uv, installing esptool via uv raises FileNotFoundError. Provide a pip fallback and catch FileNotFoundError.
Apply:
@@ - try: - subprocess.check_call([ - uv_executable, "pip", "install", "--quiet", "--force-reinstall", - f"--python={python_exe}", - "-e", esptool_repo_path - ], timeout=60) + try: + cmd = ( + [uv_executable, "pip", "install", "--quiet", "--force-reinstall", + f"--python={python_exe}", "-e", esptool_repo_path] + if os.path.isfile(uv_executable) + else [python_exe, "-m", "pip", "install", "--quiet", + "--force-reinstall", "-e", esptool_repo_path] + ) + subprocess.check_call(cmd, timeout=120) - except subprocess.CalledProcessError as e: + except (subprocess.CalledProcessError, FileNotFoundError) as e: sys.stderr.write( - f"Error: Failed to install esptool from {esptool_repo_path} (exit {e.returncode})\n" + f"Error: Failed to install esptool from {esptool_repo_path} ({e})\n" ) sys.exit(1)builder/frameworks/component_manager.py (3)
447-461: Prevent accidental deletion when Arduino libs path is missingIf
arduino_libs_mcuis empty,include_pathbecomes a relativeinclude/<component>path and could delete a user/project include directory. Add a guard and return early when the MCU libs root is unavailable.def _remove_include_directory(self, component: str) -> None: """ Remove include directory for a specific component. @@ - include_path = str(Path(self.config.arduino_libs_mcu) / "include" / component) + if not self.config.arduino_libs_mcu: + return + include_path = str(Path(self.config.arduino_libs_mcu) / "include" / component) if os.path.exists(include_path): shutil.rmtree(include_path)
470-498: Guard edits when Arduino libs path is missingSame risk applies here: constructing
build_py_pathwith an empty base yields a relative path and may mangle unintended files. Guard and return.def _remove_cpppath_entries(self) -> None: @@ - build_py_path = str(Path(self.config.arduino_libs_mcu) / "pioarduino-build.py") + if not self.config.arduino_libs_mcu: + return + build_py_path = str(Path(self.config.arduino_libs_mcu) / "pioarduino-build.py")
896-975: Also guard lib_ignore edits without Arduino libs path
_remove_ignored_lib_includesshould no-op ifarduino_libs_mcuis unset to avoid touching unrelated files.def _remove_ignored_lib_includes(self) -> None: @@ - build_py_path = str(Path(self.config.arduino_libs_mcu) / "pioarduino-build.py") + if not self.config.arduino_libs_mcu: + self.logger.log_change("Build file root not available; skipping lib_ignore processing") + return + build_py_path = str(Path(self.config.arduino_libs_mcu) / "pioarduino-build.py")builder/main.py (1)
786-796: Potential failure on Windows: quote dfu-util pathIf the package directory is under a user path with spaces, dfu-util won’t launch. Quote $UPLOADER in the command.
Apply:
- UPLOADCMD='$UPLOADER $UPLOADERFLAGS "$SOURCE"', + UPLOADCMD='"$UPLOADER" $UPLOADERFLAGS "$SOURCE"',Optional: align with the esptool approach by pre-quoting UPLOADER only when the path contains spaces, or always quote at call sites consistently across all upload/erase commands.
♻️ Duplicate comments (2)
builder/frameworks/_embed_files.py (1)
37-37: Pathlib + quoting: looks solid; zero-length file fix is correct
- Path joins replaced with Path are correct; quoting CMake/script paths is safe and cross-platform without adding .exe (matches project preference).
- prepare_file now handles empty files without seek(-1) crash. This addresses the earlier bot comment.
Also applies to: 57-61, 81-89, 100-106, 109-111, 142-148, 170-171
builder/frameworks/espidf.py (1)
551-565: Don’t add the cmake binary to PATH; add its bin directoryAppending
CMAKE_DIR(a file path) to PATH is ineffective. Add the directory instead to ensure subordinate tools findcmakeif needed.def populate_idf_env_vars(idf_env): @@ - additional_packages = [ - str(Path(TOOLCHAIN_DIR) / "bin"), - NINJA_DIR, - CMAKE_DIR, - os.path.dirname(get_python_exe()), - ] + additional_packages = [ + str(Path(TOOLCHAIN_DIR) / "bin"), + NINJA_DIR, + str(Path(CMK_TOOL) / "bin"), + os.path.dirname(get_python_exe()), + ] - idf_env["PATH"] = os.pathsep.join([*additional_packages, idf_env["PATH"]]) + idf_env["PATH"] = os.pathsep.join([*additional_packages, idf_env["PATH"]])
🧹 Nitpick comments (10)
builder/frameworks/arduino.py (1)
902-902: Silence linter on strfunction lambda without changing behaviorRename unused args to underscores to satisfy Ruff ARG005.
- # silence scons command output - silent_action.strfunction = lambda target, source, env: '' + # silence scons command output + silent_action.strfunction = lambda _target, _source, _env: ''builder/frameworks/_embed_files.py (1)
108-111: Minor: silence unused-arg lint in emitterRename unused args to underscores; behavior unchanged.
-def transform_to_asm(target, source, env): +def transform_to_asm(_target, source, _env): files = [str(Path("$BUILD_DIR") / (s.name + ".S")) for s in source] return files, sourcebuilder/frameworks/component_manager.py (3)
482-496: Tighten regex to only touch CPPPATH entriesSome patterns are broad (e.g., matching any line containing “/name/”), which risks removing unrelated lines. Anchor to the known CPPPATH structures to reduce false positives.
- patterns = [ - rf'.*join\([^,]*,\s*"include",\s*"{re.escape(lib_name)}"[^)]*\),?\n', - rf'.*"include/{re.escape(lib_name)}"[^,\n]*,?\n', - rf'.*"[^"]*include[^"]*{re.escape(lib_name)}[^"]*"[^,\n]*,?\n', - rf'.*"[^"]*/{re.escape(lib_name)}/include[^"]*"[^,\n]*,?\n', - rf'.*"[^"]*{re.escape(lib_name)}[^"]*include[^"]*"[^,\n]*,?\n', - rf'.*join\([^)]*"include"[^)]*"{re.escape(lib_name)}"[^)]*\),?\n', - rf'.*"{re.escape(lib_name)}/include"[^,\n]*,?\n', - rf'\s*"[^"]*[\\/]{re.escape(lib_name)}[\\/][^"]*",?\n', - # pathlib-style: Path(...)/"include"/"<name>" - rf'.*Path\([^)]*\)\s*/\s*"include"\s*/\s*"{re.escape(lib_name)}"[^,\n]*,?\n', - rf'.*Path\([^)]*{re.escape(lib_name)}[^)]*\)\s*/\s*"include"[^,\n]*,?\n' - ] + patterns = [ + # env.AppendUnique(CPPPATH=[..., join(...,"include","<name>"), ...]) + rf'.*CPPPATH=.*join\([^,]*,\s*"include",\s*"{re.escape(lib_name)}"[^)]*\),?\n', + # "include/<name>" entries inside CPPPATH lists + rf'.*CPPPATH=.*"[^"]*include/{re.escape(lib_name)}"[^,\n]*,?\n', + # pathlib-style: Path(...)/"include"/"<name>" inside CPPPATH lists + rf'.*CPPPATH=.*Path\([^)]*\)\s*/\s*"include"\s*/\s*"{re.escape(lib_name)}"[^,\n]*,?\n', + ]If you prefer breadth, keep the current list but gate replacements to lines containing “CPPPATH=”.
262-265: Preserve file metadata when creating .orig backupsUse
copy2to keep timestamps/metadata. This helps future diffing and restores.- if not os.path.exists(backup_path): - shutil.copy(file_path, backup_path) + if not os.path.exists(backup_path): + shutil.copy2(file_path, backup_path)
422-430: Align backup guard policy across classesHandlers guard on missing
arduino_libs_mcu, butBackupManagerdoesn’t. Pick one approach for consistency. Given prior project preference to “not silently skip backups,” consider removing the guards in the handlers; otherwise, add the same guard toBackupManager.Option A (remove guards in handlers):
def _backup_pioarduino_build_py(self) -> None: @@ - if not self.config.arduino_libs_mcu: - returndef _backup_pioarduino_build_py(self) -> None: @@ - if not self.config.arduino_libs_mcu: - returnOption B (add guards to BackupManager):
def backup_pioarduino_build_py(self) -> None: @@ + if not self.config.arduino_libs_mcu: + return @@ def restore_pioarduino_build_py(self, target=None, source=None, env=None) -> None: @@ + if not self.config.arduino_libs_mcu: + returnAlso applies to: 998-1013, 1047-1054, 1069-1071
builder/frameworks/espidf.py (2)
482-487: Optional: name clarity — CMAKE_DIR is an executable path
CMAKE_DIRholds the executable, not a directory. Consider renaming toCMAKE_EXEfor readability.-CMAKE_DIR = str(Path(CMK_TOOL) / "bin" / "cmake") +CMAKE_EXE = str(Path(CMK_TOOL) / "bin" / "cmake")Follow-up: replace usages accordingly.
1121-1139: Avoid relying on a private helper
get_lib_ignore_components()callsLibraryIgnoreHandler._get_lib_ignore_entries(). Expose a small public accessor on the handler (e.g.,get_processed_entries()) to decouple from internals.Proposed minimal change in component_manager.py:
class LibraryIgnoreHandler: @@ def handle_lib_ignore(self) -> None: ... + def get_processed_entries(self) -> List[str]: + return self._get_lib_ignore_entries()Then here:
- lib_ignore_entries = lib_handler._get_lib_ignore_entries() + lib_ignore_entries = lib_handler.get_processed_entries()I can open a follow-up PR if you want.
builder/main.py (3)
468-470: esptool erase subcommand with hyphen is correct; consider quoting consistencyerase-flash is the modern syntax. One note: since ERASETOOL may be pre-quoted, keep usages consistent across commands to avoid double- or under-quoting on different OSes.
514-527: Quote targets/sources to be robust against spacesIf BUILD_DIR or PROGNAME contains spaces (common on Windows), the elf2image call can break. Wrap $TARGET and $SOURCES.
Apply:
- "-o", - "$TARGET", + "-o", + "\"$TARGET\"", - "$SOURCES", + "\"$SOURCES\"",
576-584: Use Path end-to-end, convert to str only when invoking the toolSaves repeated parsing and keeps intent clear.
Apply:
- map_file = str(Path(env.subst("$BUILD_DIR")) / (env.subst("$PROGNAME") + ".map")) - if not Path(map_file).is_file(): + map_file = Path(env.subst("$BUILD_DIR")) / (env.subst("$PROGNAME") + ".map") + if not map_file.is_file(): # map file can be in project dir - map_file = str(Path(get_project_dir()) / (env.subst("$PROGNAME") + ".map")) - if not Path(map_file).is_file(): - print(f"Error: Map file not found: {map_file}") + map_file = Path(get_project_dir()) / (env.subst("$PROGNAME") + ".map") + if not map_file.is_file(): + print(f"Error: Map file not found: {map_file.as_posix()}") print("Make sure the project is built first with 'pio run'") return @@ - cmd.append(map_file) + cmd.append(str(map_file))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
builder/frameworks/_embed_files.py(7 hunks)builder/frameworks/arduino.py(7 hunks)builder/frameworks/component_manager.py(18 hunks)builder/frameworks/espidf.py(65 hunks)builder/frameworks/ulp.py(11 hunks)builder/main.py(9 hunks)builder/penv_setup.py(5 hunks)examples/arduino-blink/platformio.ini(4 hunks)examples/arduino-rmt-blink/platformio.ini(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
📚 Learning: 2025-09-05T11:58:49.667Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/ulp.py:165-178
Timestamp: 2025-09-05T11:58:49.667Z
Learning: In the pioarduino/platform-espressif32 project, cmake executable paths in build scripts (like ulp.py) are provided as paths to the build engine (SCons/PlatformIO) rather than direct executable calls. The build engine handles cross-platform executable resolution automatically, so there's no need to add Windows .exe suffixes or special quoting - the current approach with str(Path(...) / "bin" / "cmake") works correctly across platforms.
Applied to files:
builder/frameworks/ulp.pybuilder/frameworks/_embed_files.pybuilder/frameworks/espidf.pybuilder/frameworks/arduino.pybuilder/frameworks/component_manager.py
📚 Learning: 2025-08-26T14:52:29.044Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Applied to files:
builder/frameworks/ulp.pybuilder/frameworks/_embed_files.pybuilder/frameworks/espidf.pybuilder/frameworks/component_manager.pybuilder/main.py
📚 Learning: 2025-08-26T14:53:03.852Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:192-194
Timestamp: 2025-08-26T14:53:03.852Z
Learning: Jason2866 prefers to trust the system for path handling in penv_setup.py and doesn't want additional path validation for security concerns around penv directory derivation, indicating confidence in the controlled environment and internal path generation.
Applied to files:
builder/frameworks/ulp.pybuilder/frameworks/_embed_files.pybuilder/frameworks/component_manager.py
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/ulp.pybuilder/frameworks/_embed_files.pybuilder/frameworks/espidf.pybuilder/frameworks/component_manager.pyexamples/arduino-blink/platformio.ini
📚 Learning: 2025-09-04T15:27:18.112Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
Applied to files:
builder/frameworks/ulp.pybuilder/frameworks/_embed_files.pybuilder/frameworks/espidf.pybuilder/frameworks/component_manager.py
📚 Learning: 2025-08-10T16:59:15.533Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Applied to files:
builder/frameworks/_embed_files.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/penv_setup.pybuilder/frameworks/espidf.py
📚 Learning: 2025-09-05T12:20:25.769Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/espidf.py:1121-1139
Timestamp: 2025-09-05T12:20:25.769Z
Learning: In the pioarduino/platform-espressif32 project, the LibraryIgnoreHandler._get_lib_ignore_entries() private method already includes BT/BLE protection logic internally, so there's no need to add additional BT/BLE dependency checking when using this method in get_lib_ignore_components().
Applied to files:
builder/frameworks/espidf.pyexamples/arduino-rmt-blink/platformio.iniexamples/arduino-blink/platformio.ini
📚 Learning: 2025-08-08T09:55:50.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:55:50.842Z
Learning: In pioarduino/platform-espressif32 (builder/frameworks/espidf.py, Python), for get_app_partition_offset: preferred behavior is to probe app/ota_0 first, then app/factory; avoid using the {"name":"boot"} sentinel; make probes non-fatal (don’t exit on missing partitions), optionally fall back to calling parttool.py with --partition-boot-default, then default to 0x10000.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:50:01.969Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:50:01.969Z
Learning: Preference in pioarduino/platform-espressif32 (builder/frameworks/espidf.py): Avoid using {"name": "boot"} as a sentinel when resolving the default app partition offset. Instead, call parttool.py with --partition-boot-default directly (and optionally fall back to factory/ota_0) to prevent confusion with non-existent CSV names.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-08T09:44:18.366Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:44:18.366Z
Learning: In builder/frameworks/espidf.py, get_partition_info treats pt_params["name"] == "boot" as a sentinel to pass --partition-boot-default to parttool.py; "boot" is not expected to be a CSV partition name.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#199
File: examples/arduino-blink/platformio.ini:81-83
Timestamp: 2025-06-26T09:56:30.658Z
Learning: In examples/arduino-blink/platformio.ini, the different lib_ignore configurations across environments (some including wifi, some not, different syntax styles) are intentional test cases, not inconsistencies that need to be fixed.
Applied to files:
examples/arduino-rmt-blink/platformio.iniexamples/arduino-blink/platformio.ini
📚 Learning: 2025-09-05T11:57:05.734Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/main.py:468-470
Timestamp: 2025-09-05T11:57:05.734Z
Learning: The latest versions of esptool have standardized all commands to use dashes (-) instead of underscores (_). For example, erase_flash is now erase-flash, flash_id is now flash-id, etc. The old underscore format still works but shows deprecation warnings. This change was implemented to improve consistency across esptool commands.
Applied to files:
builder/main.py
🪛 Ruff (0.12.2)
builder/frameworks/ulp.py
31-31: Undefined name project_config
(F821)
110-110: Undefined name idf_variant
(F821)
builder/frameworks/_embed_files.py
100-100: Undefined name env
(F821)
101-101: Undefined name env
(F821)
103-103: Undefined name env
(F821)
104-104: Undefined name env
(F821)
105-105: Undefined name env
(F821)
105-105: Undefined name env
(F821)
108-108: Unused function argument: target
(ARG001)
108-108: Unused function argument: env
(ARG001)
142-142: Undefined name env
(F821)
147-147: Undefined name env
(F821)
builder/frameworks/espidf.py
1135-1135: Consider moving this statement to an else block
(TRY300)
1438-1438: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1441-1441: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1442-1442: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1444-1444: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1492-1492: Use explicit conversion flag
Replace with conversion flag
(RUF010)
builder/frameworks/arduino.py
902-902: Unused lambda argument: target
(ARG005)
902-902: Unused lambda argument: source
(ARG005)
902-902: Unused lambda argument: env
(ARG005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest, examples/arduino-blink)
🔇 Additional comments (13)
examples/arduino-rmt-blink/platformio.ini (1)
5-11: Lib ignore expansions look goodThe added items (ble, wifi, ESP RainMaker/Insights, Zigbee) align with the broader lib-ignore hardening and the example-suite’s intentional variability. No issues from my side.
Also applies to: 19-25, 33-39, 47-53, 61-67
builder/penv_setup.py (1)
23-23: Pathlib migration: LGTMSwitching to Path for executable/site-packages/penv paths is correct and keeps behavior intact across platforms.
Also applies to: 81-81, 101-101, 145-147, 427-427
builder/frameworks/arduino.py (2)
76-76: ESP32-C3 threshold tweak acknowledgedDefault threshold to 32000 for esp32c3 is within your max guardrails and consistent with aggressive defaults.
314-314: Pathlib refactor: SDK/config/build-script pathsConversions to Path (sdk_dir, sdkconfig paths, build script) are correct and keep SCons interactions stable.
Also applies to: 569-569, 600-600, 909-909
examples/arduino-blink/platformio.ini (1)
16-16: Expanded lib_ignore and component curation: OKThe added ignore and component-remove entries follow the project’s example-suite strategy and align with recent lib-ignore protections. No action needed.
Also applies to: 20-22, 23-41, 48-49, 51-54, 55-56, 61-73, 73-73, 81-82, 84-87, 89-104, 110-117, 130-147, 155-156, 158-161, 163-177, 184-190, 192-207, 214-215, 217-220, 223-237
builder/frameworks/ulp.py (2)
31-32: Path migration LGTM
ULP_BUILD_DIRviaPathis clear and robust. Good use of.replace("__idf_", "").
50-55: Correct PATH setup for CMakeAdding
.../tool-cmake/bin(a directory) instead of a binary path prevents PATH pollution issues. This matches best practices.builder/frameworks/espidf.py (2)
1700-1712: Quoting handled well in linker script generationGood move to quote paths within the string action; avoids space-related breakage.
2080-2165: Safe move/copy helper looks solid
_replace_movewith parent creation andos.replacefallback is a good cross-OS approach; nice cleanup handling and clear logging.builder/main.py (4)
22-22: Pathlib import: good step toward cleaner path handlingNo issues; aligns with the PR goal.
623-623: Helpful install hintMessage is clear and aligns with the project’s preference to rely on PATH tools like uv.
632-638: Build outputs: Path refactor looks goodConsistent Path usage for target_elf/target_firm and builder inputs improves readability and reduces path bugs.
Also applies to: 646-647, 651-652
709-712: espota path resolution and quoting are correctResolved Path plus quoting in UPLOADCMD is robust.
Description:
Related issue (if applicable): fixes #
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores