Version check for tool-esp_install (renamed tl-install) and auto install version listed in platform.json #237
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 WalkthroughThe changes rename the "tl-install" package to "tool-esp_install" in the platform configuration, update its version and URL, and introduce comprehensive version checking and installation logic for this tool within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Espressif32Platform
participant PackageManager
User->>Espressif32Platform: Initialize platform / configure installer
Espressif32Platform->>Espressif32Platform: _check_tl_install_version()
alt tool-esp_install not present or version mismatch
Espressif32Platform->>Espressif32Platform: _install_tl_install(version)
Espressif32Platform->>PackageManager: Install tool-esp_install
PackageManager-->>Espressif32Platform: Installation result
end
Espressif32Platform->>User: Proceed with installer configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platform.json(1 hunks)platform.py(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
platform.py (1)
Learnt from: Jason2866
PR: #225
File: builder/main.py:99-108
Timestamp: 2025-07-13T20:03:29.695Z
Learning: In the ESP32 platform builder, the subprocess call to get site-packages directory in setup_python_paths() function is intentionally left without error handling because if this basic Python operation fails, it indicates a fundamental system issue that would cause the entire build process to fail anyway.
⏰ 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). (18)
- GitHub Check: build (macos-15, examples/espidf-ulp-riscv)
- GitHub Check: build (macos-15, examples/espidf-ulp)
- GitHub Check: build (macos-15, examples/espidf-http-request)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-zigbee-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (8)
platform.json (1)
98-104: LGTM! Package rename follows established naming conventions.The rename from
"tl-install"to"tool-esp_install"aligns with the platform's naming pattern where tool packages use the"tool-"prefix. The addition of"package-version"field and updated URL are consistent with other tool packages in the configuration.platform.py (7)
36-36: Good use of constant for package name.Defining
tl_install_nameas a constant improves maintainability and reduces the risk of typos when referencing the package name throughout the code.
213-231: Version comparison logic is appropriate.The method correctly handles both direct version strings and URL-based versions, extracting the semantic version for comparison. The string equality check is sufficient for semantic version comparison.
232-254: Well-implemented version extraction logic.The method robustly handles both URL-based and direct version strings. The regex pattern correctly extracts semantic versions, and the fallback ensures the method doesn't fail even with unexpected formats.
324-325: Consistent use of constant.Good update to use the
tl_install_nameconstant instead of the hardcoded string, maintaining consistency with the package rename.
565-582: Improved installer configuration with version checking.The addition of version checking before proceeding with the installer configuration is a good improvement. This ensures the correct version is installed before use.
However, note the continued pattern of modifying the package's optional status at line 579, which may cause inconsistency with the platform configuration.
616-616: Fixed error message formatting.Good catch on fixing the extra space in the error message.
286-286: No action needed forpm.install(version)usage
Thepm.installmethod accepts either a plain version (using the instance’s predefined package name) or a full URL, matching its use elsewhere in the codebase (e.g. installing viatl_pathin platform.py and framework URLs in arduino.py). This call is correct as is.
Jason2866
changed the title
we dont need and it makes issues
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platform.json(1 hunks)platform.py(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the `tool-esp_install` package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting `self.packages[tl_install_name]["optional"] = True` after version verification is the intended design pattern.
platform.json (1)
Learnt from: Jason2866
PR: #237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the tool-esp_install package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting self.packages[tl_install_name]["optional"] = True after version verification is the intended design pattern.
platform.py (2)
Learnt from: Jason2866
PR: #237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the tool-esp_install package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting self.packages[tl_install_name]["optional"] = True after version verification is the intended design pattern.
Learnt from: Jason2866
PR: #225
File: builder/main.py:99-108
Timestamp: 2025-07-13T20:03:29.695Z
Learning: In the ESP32 platform builder, the subprocess call to get site-packages directory in setup_python_paths() function is intentionally left without error handling because if this basic Python operation fails, it indicates a fundamental system issue that would cause the entire build process to fail anyway.
🪛 Ruff (0.12.2)
platform.py
315-315: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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-arduino-C6-ULP-blink)
- GitHub Check: build (macos-15, examples/espidf-storage-sdcard)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- 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-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (7)
platform.json (1)
98-104: Package rename and version specification look good!The rename from "tl-install" to "tool-esp_install" is properly implemented with the addition of the
package-versionfield for version checking. The non-optional status aligns with the intended design where the tool is mandatory during initial installation.platform.py (6)
36-36: New constant and helper functions are well-implemented!The
tl_install_nameconstant and the new helper functionssafe_remove_fileandsafe_copy_directoryfollow the established patterns in the codebase with proper error handling through thesafe_file_operationdecorator.Also applies to: 113-120, 154-161
181-230: Version checking logic is comprehensive and well-structured!The
_check_tl_install_versionmethod properly handles all cases: missing installation, version mismatch, and correct version. The transition to optional status after verification aligns with the documented design pattern.
231-272: Version comparison and extraction logic is robust!The methods handle both URL-based and direct version formats effectively. The regex pattern
r'v(\d+\.\d+\.\d+)'correctly extracts semantic versions from URLs.
348-351: Good use of the constant instead of hardcoded string!The update to use
tl_install_nameinstead of hardcoded "tl-install" improves maintainability.
589-612: Installer configuration properly integrates version checking!The
_configure_installermethod correctly:
- Performs version checking/installation first
- Handles legacy tl-install cleanup
- Uses the new constant throughout
- Provides appropriate error handling and logging
646-646: Good catch on the logging message fix!The error message now correctly indicates it's about reading package data.
Description:
Related issue (if applicable): fixes #
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores