Fix: include file contents in custom_sdkconfig hash computation#441
Closed
antoinecellerier wants to merge 1 commit into
Closed
Fix: include file contents in custom_sdkconfig hash computation#441antoinecellerier wants to merge 1 commit into
antoinecellerier wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe changes enhance custom sdkconfig configuration handling by resolving Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Not correct. The file stores a hash to detect changes in setup. Deleting forces a compile of the Arduino libs. In short the AI does not get how HybridCompile is working The file could be deleted if detected there is no custom_sdkconfig set at all. Not needed since it gets overwritten when outdated. If this is a result of your WiP Arduino ULP HybridCompile. This part is completely wrong there. |
Author
|
Interesting. Could this then be a case of the hash not properly accounting for configuration impacting dependencies? If so I can try looking in that direction. Apologies if the questions are a bit naive. To your question regarding ULP, this should be independent but will re-validate thoroughly to ensure it really is. I was hitting data+bss limits trying to enable Wifi DPP on the project ... so ended-up needing to disable other components like Bluetooth. DPP ended up being a dead end but I was thinking solving being able to disable Bluetooth would be useful to others too. |
When custom_sdkconfig uses a file:// reference (e.g.
custom_sdkconfig = file://sdkconfig.custom), the hash used to detect
configuration changes is computed from the option text
("file://sdkconfig.custom") rather than the file's actual contents.
Editing the referenced file doesn't change the hash, so
matching_custom_sdkconfig() returns True and stale recompiled libs
are silently reused — even if the file now disables a major feature
like Bluetooth.
Fix: resolve file:// references before hash computation in both
arduino.py (where the hash is checked) and espidf.py (where it is
written). Both sides now hash the file contents + inline options +
MCU name, so editing the file triggers lib recompilation via the
existing Reinstall path (which also runs safe_remove_sdkconfig_files
to clean stale per-env sdkconfigs).
Inline custom_sdkconfig values (without file://) are unaffected.
Tested on dfrobot_firebeetle2_esp32e with file://sdkconfig.custom:
Before fix (stock pioarduino):
Build 1: custom_sdkconfig = file://sdkconfig.custom
(CONFIG_ESP_WIFI_STATIC_RX_BUFFER_NUM=4)
→ lib recompile, hash=3d58653b, RAM=45104,
BTDM_RESERVE_DRAM=0xdb5c (55KB BT reservation)
Edit: add "# CONFIG_BT_ENABLED is not set" to file
Build 2: NO recompile (7s cached), hash unchanged,
BT still reserved — file change silently ignored
After fix:
Build 1: same → lib recompile, hash=5c9e7d85, RAM=45064
Edit: add "# CONFIG_BT_ENABLED is not set" to file
Build 2: hash mismatch detected → Reinstall + recompile,
"Replace: CONFIG_BT_ENABLED=y with: # not set",
BTDM_RESERVE_DRAM=0, RAM=44160 (-944 bytes)
Also guards PathCache.sdk_dir against framework_lib_dir being None
(crashes with TypeError on Python 3.13 when packages are missing).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
antoinecellerier
force-pushed
the
fix/stale-sdkconfig-cleanup
branch
from
a1578f1 to
bbc82a1
Compare
antoinecellerier
changed the title
Author
|
I've reworked this to fix the actual root cause instead. The issue: when custom_sdkconfig = file://sdkconfig.custom, the hash in matching_custom_sdkconfig() is computed from the literal string "file://sdkconfig.custom" rather than the file's contents. Editing the file doesn't change the hash, so the existing change detection never fires and stale libs are silently reused. The fix resolves file:// references before hashing in both arduino.py and espidf.py, so that a file content change produces a different hash → triggers the existing Reinstall path (which already calls safe_remove_sdkconfig_files()). No files are deleted outside the normal flow. The file:// path matters because it's the only way to pass Kconfig disable syntax like # CONFIG_BT_ENABLED is not set — the # is stripped by the INI parser when used inline. Tested end-to-end: editing sdkconfig.custom to disable BT now correctly triggers recompilation and removes the 55KB BTDM DRAM reservation. I've also updated the PR title & description to match. |
|
Well, it is possible to disable Bluetooth, in the platformio.ini file. Needs quoting. Anyways i will review the PR |
|
@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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 782-792: The combined-config hashing currently uses resolved
content from load_custom_sdkconfig_file() for all schemes (file/http/https), but
arduino.py's matching_custom_sdkconfig() still hashes the literal URL, causing
cache misses for URL-backed configs; to fix, change the logic that builds
custom_sdk_config_flags (the custom_sdk_config_flags variable in
builder/frameworks/espidf.py where raw =
env.GetProjectOption("custom_sdkconfig") and resolved =
load_custom_sdkconfig_file()) so it only expands/resolves and concatenates
file:// entries (local file content) into the checksum input, leaving http://
and https:// entries as their original literal strings, or alternatively update
matching_custom_sdkconfig() in builder/frameworks/arduino.py to resolve URLs the
same way—pick one approach and implement it so both sides use the same
representation for URL-backed custom_sdkconfig entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7198aee8-d921-4421-aeeb-2ee32db06fa8
📒 Files selected for processing (2)
builder/frameworks/arduino.pybuilder/frameworks/espidf.py
Comment on lines
+782
to
+792
| # Write final configuration file with checksum. | ||
| # Include resolved file content (not just the raw "file://..." reference) | ||
| # so that editing the referenced file changes the hash and triggers | ||
| # recompilation. Combine file content + inline options to match what | ||
| # build_idf_config_flags() produces. | ||
| custom_sdk_config_flags = "" | ||
| if config.has_option("env:" + env["PIOENV"], "custom_sdkconfig"): | ||
| custom_sdk_config_flags = env.GetProjectOption("custom_sdkconfig").rstrip("\n") + "\n" | ||
|
|
||
| raw = env.GetProjectOption("custom_sdkconfig") | ||
| resolved = load_custom_sdkconfig_file() | ||
| # Combine resolved file content with raw inline options (both are applied) | ||
| custom_sdk_config_flags = ((resolved + "\n") if resolved else "") + raw.rstrip("\n") + "\n" |
There was a problem hiding this comment.
Keep this new hash expansion scoped to file://, or align arduino.py with it.
load_custom_sdkconfig_file() also resolves http:// / https:// entries, but builder/frameworks/arduino.py::matching_custom_sdkconfig() still hashes the literal URL string. That means URL-backed custom_sdkconfig values will now miss the cached hash every run and keep forcing reinstall/recompile. If this PR is only fixing file://, narrow the checksum input here to local-file entries; otherwise both files should share the same resolver.
💡 Minimal fix
if config.has_option("env:" + env["PIOENV"], "custom_sdkconfig"):
raw = env.GetProjectOption("custom_sdkconfig")
- resolved = load_custom_sdkconfig_file()
+ resolved = ""
+ for line in raw.splitlines():
+ line = line.strip()
+ if not line.startswith("file://"):
+ continue
+ file_ref = line[7:]
+ file_path = file_ref if os.path.isabs(file_ref) else str(Path(PROJECT_DIR) / file_ref)
+ if os.path.exists(file_path):
+ try:
+ with open(file_path, "r") as f:
+ resolved = f.read()
+ except IOError:
+ pass
+ break
# Combine resolved file content with raw inline options (both are applied)
custom_sdk_config_flags = ((resolved + "\n") if resolved else "") + raw.rstrip("\n") + "\n"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Write final configuration file with checksum. | |
| # Include resolved file content (not just the raw "file://..." reference) | |
| # so that editing the referenced file changes the hash and triggers | |
| # recompilation. Combine file content + inline options to match what | |
| # build_idf_config_flags() produces. | |
| custom_sdk_config_flags = "" | |
| if config.has_option("env:" + env["PIOENV"], "custom_sdkconfig"): | |
| custom_sdk_config_flags = env.GetProjectOption("custom_sdkconfig").rstrip("\n") + "\n" | |
| raw = env.GetProjectOption("custom_sdkconfig") | |
| resolved = load_custom_sdkconfig_file() | |
| # Combine resolved file content with raw inline options (both are applied) | |
| custom_sdk_config_flags = ((resolved + "\n") if resolved else "") + raw.rstrip("\n") + "\n" | |
| # Write final configuration file with checksum. | |
| # Include resolved file content (not just the raw "file://..." reference) | |
| # so that editing the referenced file changes the hash and triggers | |
| # recompilation. Combine file content + inline options to match what | |
| # build_idf_config_flags() produces. | |
| custom_sdk_config_flags = "" | |
| if config.has_option("env:" + env["PIOENV"], "custom_sdkconfig"): | |
| raw = env.GetProjectOption("custom_sdkconfig") | |
| resolved = "" | |
| for line in raw.splitlines(): | |
| line = line.strip() | |
| if not line.startswith("file://"): | |
| continue | |
| file_ref = line[7:] | |
| file_path = file_ref if os.path.isabs(file_ref) else str(Path(PROJECT_DIR) / file_ref) | |
| if os.path.exists(file_path): | |
| try: | |
| with open(file_path, "r") as f: | |
| resolved = f.read() | |
| except IOError: | |
| pass | |
| break | |
| # Combine resolved file content with raw inline options (both are applied) | |
| custom_sdk_config_flags = ((resolved + "\n") if resolved else "") + raw.rstrip("\n") + "\n" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builder/frameworks/espidf.py` around lines 782 - 792, The combined-config
hashing currently uses resolved content from load_custom_sdkconfig_file() for
all schemes (file/http/https), but arduino.py's matching_custom_sdkconfig()
still hashes the literal URL, causing cache misses for URL-backed configs; to
fix, change the logic that builds custom_sdk_config_flags (the
custom_sdk_config_flags variable in builder/frameworks/espidf.py where raw =
env.GetProjectOption("custom_sdkconfig") and resolved =
load_custom_sdkconfig_file()) so it only expands/resolves and concatenates
file:// entries (local file content) into the checksum input, leaving http://
and https:// entries as their original literal strings, or alternatively update
matching_custom_sdkconfig() in builder/frameworks/arduino.py to resolve URLs the
same way—pick one approach and implement it so both sides use the same
representation for URL-backed custom_sdkconfig entries.
|
@coderabbitai Make a suggestion in a extra PR. Do not check the complete file for changes. Thats expensive regarding performance. Store the file time and invalidate the hash if the file time has changed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15 🏁 Script executed: Length of output: 167 🏁 Script executed: Length of output: 10581 🏁 Script executed: Length of output: 12042
|
PR #441 (content hash) |
mtime approach | |
|---|---|---|
| Cost | Reads entire file | Single stat() call |
| Miss change? | Never (hash reflects exact content) | Only if mtime is not updated (e.g. cp --preserve=timestamps) |
| False positive rebuild? | No | Yes if file is touch-ed without content change |
| Pattern already in codebase | No | Yes (espidf.py lines 872–878) |
The mtime approach is the right trade-off for this use case — users editing sdkconfig.custom will always get a new mtime, and false-positive rebuilds (from a plain touch) are harmless.
Would you like me to open a new PR with these changes?
🧠 Learnings used
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 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
Repo: pioarduino/platform-espressif32 PR: 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.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 432
File: builder/frameworks/espidf.py:1167-1179
Timestamp: 2026-03-15T15:31:38.160Z
Learning: In pioarduino/platform-espressif32, response file paths (`file` tokens) emitted by the ESP-IDF/CMake codemodel are always absolute paths. There is no need to resolve relative paths when handling response file expansion in `get_app_flags` or `prepare_build_envs` in `builder/frameworks/espidf.py`. The existing `os.path.isfile(resp_path)` check is sufficient.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 285
File: platform.py:670-683
Timestamp: 2025-09-09T18:07:14.583Z
Learning: In the pioarduino/platform-espressif32 project, when tools.json exists in mklittlefs tool directories, it triggers a reinstall via install_tool(). This is intentional behavior to convert tools installed via idf_tools.py method into the platform's tool management system, rather than a redundant check that should be avoided.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2026-03-07T22:12:49.994Z
Learning: In monitor/filter_exception_decoder.py for pioarduino/platform-espressif32: the `is_return_addr` / `-1` decrement in `_resolve_address` and `_prefetch_addresses` was intentionally removed in PR 419. ESP-IDF's panic handler already provides call-site adjusted addresses; decrementing again produces wrong function/line results. On RISC-V with compressed instructions, subtracting 1 byte can land mid-instruction. Stack trace words should never be blindly decremented — `PcAddressMatcher` ELF section filtering is the correct mechanism. Cache keys must use the original address string (not the decremented lookup) for prefetch/resolve consistency.
|
@coderabbitai Open a new PR with the mtime aproach. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 12469
Task: New PR — use mtime instead of file content for
|
|
@antoinecellerier Please switch to the mtime approach. The AI has already posted the probably working code diffs. |
Author
|
@Jason2866 I'd thought about that as well but that would lead to different hashes for no-op changes, so likely more rebuilds than necessary right? Implementation would be simpler though. |
|
The probably very seldom wrong rebuilds are way more acceptable than the performance hit by scanning ALWAYS the sdkconfig file by every compile run. In other words i will accept only the mtime approach which is a bug fix! |
4 tasks
|
Thx for the bug finding. Closing with implementing in #446 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Problem
When
custom_sdkconfiguses afile://reference:custom_sdkconfig = file://sdkconfig.customthe hash used to detect configuration changes is computed from the literal text "file://sdkconfig.custom" rather than the file's actual contents. Editing the referenced file doesn't hange the hash, so matching_custom_sdkconfig() returns True and stale recompiled libs are silently reused — even if the file now disables a major feature like Bluetooth.
This is the only way to pass Kconfig disable syntax (# CONFIG_BT_ENABLED is not set) since # lines in platformio.ini are stripped by the INI parser.
Fix
Resolve file:// references before hash computation in both arduino.py (where the hash is checked via matching_custom_sdkconfig()) and espidf.py (where it is written via write_sdkconfig_file()). Both sides now hash the resolved file contents + inline options + MCU name, so editing the file triggers lib recompilation via the existing Reinstall path (which also runs safe_remove_sdkconfig_files() to clean stale per-env sdkconfigs).
Inline custom_sdkconfig values (without file://) are unaffected.
Also guards PathCache.sdk_dir against framework_lib_dir being None (crashes with TypeError on Python 3.13 when framework packages are not yet installed). I'll admit I'm not sure why this kept happening in testing.
Repro
platformio.ini:
sdkconfig.custom (initial):
CONFIG_ESP_WIFI_STATIC_RX_BUFFER_NUM=4Now edit sdkconfig.custom to add # CONFIG_BT_ENABLED is not set:
With this fix, Build 2 detects the hash mismatch, triggers Reinstall + recompile, and properly disables BT (BTDM_RESERVE_DRAM drops from 0xdb5c to 0).
Use of AI
This PR was generated with the assistance of Claude Code (claude-opus-4-6). Do let me know if it still looks completely wrong.
Summary by CodeRabbit
Bug Fixes