Refactor source path checks and library ignore logic #294
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactor source path handling and improve library ignore entry retrieval.
WalkthroughUpdates in builder/frameworks/espidf.py adjust compile_source_files to skip .rule and dummy_src.c files and to avoid reassigning src_path inside a branch. get_lib_ignore_components now conditionally calls a public get_lib_ignore_entries if available, otherwise falls back to _get_lib_ignore_entries. Minor whitespace cleanup in get_python_exe. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant B as Build System
participant F as compile_source_files
participant FS as FileSystem
B->>F: Iterate source files
F->>FS: Read next source path
alt path endswith ".rule" OR basename == "dummy_src.c"
F-->>B: Skip adding compile action
else path needs resolution
F->>FS: Resolve path using pre-evaluated src_path
F-->>B: Add compile action
end
sequenceDiagram
autonumber
participant C as get_lib_ignore_components
participant H as LibraryIgnoreHandler
C->>H: Check for get_lib_ignore_entries()
alt public method exists and callable
C->>H: get_lib_ignore_entries()
H-->>C: ignore entries
else
C->>H: _get_lib_ignore_entries()
H-->>C: ignore entries
end
C-->>C: Return collected entries
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
|
@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: 0
🧹 Nitpick comments (2)
builder/frameworks/espidf.py (2)
1026-1031: Nit: make the .rule and dummy file checks robust (case-insensitive, Path usage).Prevents surprises on case-insensitive filesystems and keeps style consistent with Path usage in this function.
- src_path = source["path"] - if src_path.endswith(".rule"): + src_path = source["path"] + if src_path.lower().endswith(".rule"): continue - # Always skip dummy_src.c to avoid duplicate build actions - if os.path.basename(src_path) == "dummy_src.c": + # Always skip dummy_src.c to avoid duplicate build actions + if Path(src_path).name.lower() == "dummy_src.c": continue
1136-1144: Library ignore retrieval: use public+legacy fallback, normalize output, and catch AttributeError.
- Verified: class LibraryIgnoreHandler and _get_lib_ignore_entries exist in builder/frameworks/component_manager.py (class ≈ line 531, _get_lib_ignore_entries ≈ line 605).
- Apply: getattr for "get_lib_ignore_entries" and "_get_lib_ignore_entries", call whichever is callable (call get_entries() — not get_entries()()), return a deduped/sanitized sorted list of non-empty strings (e.g. sorted({e.strip() for e in lib_ignore_entries if isinstance(e, str) and e.strip()})), and include AttributeError in the except clause.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/frameworks/espidf.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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.
⏰ 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). (20)
- GitHub Check: build (macos-15, examples/espidf-ulp-lp)
- GitHub Check: build (macos-15, examples/espidf-http-request)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (macos-15, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor source path handling and improve library ignore entry retrieval.
Description:
**Fix compile issues with some idf managed components
Checklist:
Summary by CodeRabbit
Bug Fixes
Chores