prepare IDF 6: add changed linker script handling #302
Conversation
Refactor ESP-IDF version handling and linker script preprocessing.
WalkthroughAdds version resolution via get_framework_version(), sets ESP_IDF_VERSION env/defines, normalizes version components, and revises linker script preprocessing with a new API supporting IDF 5.x and 6.x flows. Updates bootloader/main paths to preprocess .ld/.ld.in, restructure linker flags, and propagate version defines. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Build Env
participant ESP as espidf.py
participant Meta as Package Metadata
participant FS as Filesystem
participant CMake as CMake Scripts
Note over ESP: Resolve ESP-IDF version
Env->>ESP: Init build
ESP->>Meta: Read package version
alt Found in metadata
Meta-->>ESP: Version string
else Not in metadata
ESP->>FS: Read framework cmake version file
FS-->>ESP: Raw version
end
ESP->>ESP: Normalize/coerce (major/minor/patch, rc)
ESP-->>Env: Set ESP_IDF_VERSION + defines
sequenceDiagram
autonumber
participant Build as Build Step
participant ESP as preprocess_linker_file()
participant FS as Filesystem
participant CMake as CMake (IDF5/IDF6)
Build->>ESP: src_ld, target_ld, config_dir, extra_includes
ESP->>ESP: Detect IDF major version
alt IDF 6.x path
ESP->>ESP: Build include paths & CFLAGS
ESP->>CMake: linker_script_preprocessor.cmake
CMake-->>FS: Generated target_ld
else IDF 5.x path
ESP->>CMake: linker_script_generator.cmake
CMake-->>FS: Generated target_ld
end
ESP-->>Build: target_ld for linker (-T)
Note over Build: Used by bootloader and main project
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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 (1)
builder/frameworks/espidf.py (1)
1532-1598: Consider using f-string conversion flags for consistency.The implementation correctly handles both IDF 5.x and 6.x preprocessing paths. However, consider using explicit conversion flags in f-strings for consistency with PEP 498.
Apply this diff to improve f-string usage:
" ".join([ f'"{CMAKE_DIR}"', - f'-DCC="{fs.to_unix_path(str(Path(TOOLCHAIN_DIR) / "bin" / "$CC"))}"', + f'-DCC="{fs.to_unix_path(Path(TOOLCHAIN_DIR) / "bin" / "$CC")!s}"', f'-DSOURCE="{src_ld_script}"', f'-DTARGET="{target_ld_script}"', f'-DCFLAGS="{cflags_value}"', "-P", - f'"{fs.to_unix_path(str(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "linker_script_preprocessor.cmake"))}"', + f'"{fs.to_unix_path(Path(FRAMEWORK_DIR) / "tools" / "cmake" / "linker_script_preprocessor.cmake")!s}"', ]), "Generating LD script $TARGET", ), ) else: # IDF 5.x: Use legacy linker_script_generator.cmake method return env.Command( target_ld_script, src_ld_script, env.VerboseAction( " ".join([ f'"{CMAKE_DIR}"', - f'-DCC="{str(Path(TOOLCHAIN_DIR) / "bin" / "$CC")}"', + f'-DCC="{Path(TOOLCHAIN_DIR) / "bin" / "$CC"!s}"', "-DSOURCE=$SOURCE", "-DTARGET=$TARGET", f'-DCONFIG_DIR="{config_dir}"', - f'-DLD_DIR="{str(Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld")}"', + f'-DLD_DIR="{Path(FRAMEWORK_DIR) / "components" / "esp_system" / "ld"!s}"', "-P", - f'"{str(Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake")}"', + f'"{Path("$BUILD_DIR") / "esp-idf" / "esp_system" / "ld" / "linker_script_generator.cmake"!s}"', ]), "Generating LD script $TARGET", ), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/frameworks/espidf.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
builder/frameworks/espidf.py
136-136: Consider moving this statement to an else block
(TRY300)
1588-1588: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1592-1592: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1594-1594: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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-blink)
- GitHub Check: build (macos-15, examples/espidf-arduino-littlefs)
- GitHub Check: build (macos-15, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp)
- GitHub Check: build (windows-latest, examples/tasmota)
- 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/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
🔇 Additional comments (6)
builder/frameworks/espidf.py (6)
142-147: LGTM!The framework version environment variable setup correctly extracts the major and minor version components and handles the edge case where the minor version might be missing.
1232-1234: LGTM!Adding ESP-IDF version defines to the bootloader CMake configuration ensures version information is available during bootloader compilation, which is essential for version-aware build logic.
990-996: LGTM!The version-aware linker script preprocessing for the main project correctly applies preprocessing only for IDF versions > 5.2, using appropriate defaults for the main project context.
1839-1847: LGTM!The main linker script preprocessing follows the same version-aware pattern as the project LD script generation, ensuring consistent handling across the build system.
1275-1352: Complex linker script preprocessing logic – warn on silent fallback and verify upstream files
- Emit a warning when neither an original nor a template
.ldscript is found, to avoid silently passing through an unprocessed file.- Manually verify in your ESP-IDF installation (e.g.
$IDF_PATH/components/bootloader/subproject/main/ld/<variant>) that each supported variant has the required.ldor.ld.inscripts.
108-140: Verify regex fallback handles edge cases.The version extraction logic is well-structured. However, the regex fallback at line 138 assumes a version string will always match the pattern
(\d+)\.(\d+)\.(\d+). Ifsemantic_version.Version.coerce()fails with aValueErrororTypeError, but the version string doesn't match the regex (e.g., "5.3", "6.0-dev"), this will return"0.0.0".Consider testing with various version formats to ensure the fallback logic handles all expected cases:
* prepare IDF 6: add changed linker script handling (#302) Refactor ESP-IDF version handling and linker script preprocessing. * use boards.json settings for HybridCompile Refactor ESP-IDF configuration handling to support board-specific configurations and improve flash/PSRAM settings management. * Settings for HybridCompile are choosen now from boards.json Removed now unnecessary custom SDK configuration options for ESP32S3. * Fix esp_idf_size command arguments Remove unnecessary '--ng' argument from esp_idf_size command * Update esp-idf-size package version to 2.0.0
Description:
Refactor ESP-IDF version handling and new linker script preprocessing for IDF 6
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes