Refactor tool installation by removing timeout and retries #276
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 WalkthroughRemoved timeout and retry mechanisms from tool installation in platform.py, simplified subprocess invocation for idf_tools install, and updated method signatures (_handle_existing_tool, install_tool). Error handling now returns False directly on failures. Associated docstrings were updated, and global constants for timeout and retry limits were deleted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant P as Espressif32Platform
participant S as idf_tools (subprocess)
C->>P: install_tool(tool_name)
activate P
P->>P: _handle_existing_tool(tool_name, paths)
alt Tool not present or needs install
P->>S: _run_idf_tools_install()
activate S
S-->>P: Exit code (success/failure)
deactivate S
alt Success
P-->>C: True
else Failure
note right of P: No retries or timeouts<br/>Directly return False
P-->>C: False
end
else Already OK
P-->>C: True
end
deactivate P
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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: 0
🧹 Nitpick comments (2)
platform.py (2)
398-428: Consider adding user feedback for long-running operations.While removing the timeout allows installations to complete on slow networks, users might benefit from knowing that the installation is still in progress. Since stdout/stderr are suppressed, the process could appear frozen.
Consider adding a log message before the potentially long-running subprocess call:
def _run_idf_tools_install(self, tools_json_path: str, idf_tools_path: str) -> bool: """Execute idf_tools.py install command.""" cmd = [ python_exe, idf_tools_path, "--quiet", "--non-interactive", "--tools-json", tools_json_path, "install" ] try: + logger.info(f"Installing tools via idf_tools.py (this may take several minutes)...") result = subprocess.run( cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False )
398-428: Document the rationale for removing timeouts.Based on the retrieved learnings, there's a history of intentional timeout values in related files. It would be helpful to document why this specific case differs.
Update the docstring to explain the design decision:
def _run_idf_tools_install(self, tools_json_path: str, idf_tools_path: str) -> bool: - """Execute idf_tools.py install command.""" + """Execute idf_tools.py install command. + + Note: No timeout is set to allow installations to complete on slow networks. + The tool-esp_install handles its own retry logic internally. + """
📜 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 (1)
platform.py(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:141-161
Timestamp: 2025-08-10T20:18:20.698Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, the subprocess timeouts (30s for pip install, 90s for listing, 600s for package installation) are intentionally kept at their current values. When timeouts occur, it's typically due to server-side connection aborts rather than slow networks, so increasing timeout values doesn't resolve the issue.
📚 Learning: 2025-08-10T20:18:20.698Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:141-161
Timestamp: 2025-08-10T20:18:20.698Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, the subprocess timeouts (30s for pip install, 90s for listing, 600s for package installation) are intentionally kept at their current values. When timeouts occur, it's typically due to server-side connection aborts rather than slow networks, so increasing timeout values doesn't resolve the issue.
Applied to files:
platform.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:
platform.py
📚 Learning: 2025-07-29T11:35:26.182Z
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.
Applied to files:
platform.py
⏰ 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). (15)
- GitHub Check: build (macos-15, examples/arduino-matter-light)
- GitHub Check: build (macos-15, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- 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)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
🔇 Additional comments (3)
platform.py (3)
398-428: LGTM! Simplified subprocess invocation improves reliability.The removal of timeout and retry mechanisms from
_run_idf_tools_installaligns with the PR objective of preferring a hang over interrupted installs for slow connections. This allows the underlying tool-esp_install to handle its own retry logic, which is a cleaner separation of concerns.
464-481: Method signature simplified correctly.The removal of the
retry_countparameter frominstall_toolis consistent with the PR's goal of eliminating retry logic. The method now returnsFalsedirectly on failures, making error handling more straightforward.
505-521: Consistent parameter removal in_handle_existing_tool.The removal of
retry_countfrom both the method signature and the recursive call toinstall_toolmaintains consistency throughout the refactoring. The simplified flow makes the code more maintainable.
1 participant
Description:
fix install issues with slow internet and computers. Better prefer hang before Interrupt of install.
Let tool-esp_install handle the retry attempts.
Checklist:
Summary by CodeRabbit
Refactor
Documentation