Enhance tool installation with retries and timeouts #275
Conversation
WalkthroughAdds a retry-based IDF Tools installation flow with dynamic timeouts, updates tool status/path/version checks, adjusts tl-install compatibility, expands MCU/toolchain and dynamic debug/upload options, tweaks installer configuration and logging, and updates exported method signatures; all changes are contained within Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as Caller
participant P as Espressif32Platform
participant S as _check_tool_status
participant G as _get_tool_paths
participant V as _check_tool_version
participant I as _install_with_idf_tools
participant R as _run_idf_tools_install
participant PM as PlatformIO PM
U->>P: install_tool(tool_name)
P->>S: query state
S-->>P: has_idf_tools / has_tools_json / has_piopm / tool_exists
P->>G: resolve & cache paths
G-->>P: paths (incl. package_path, idf_tools_path, piopm_path)
P->>V: check required vs installed version
alt Version mismatch or not installed
P->>I: _install_with_idf_tools(tool_name, paths, retry_count=0)
loop up to RETRY_LIMIT (3 attempts)
I->>R: run idf_tools.py install (timeout grows per attempt)
alt Install succeeded
R-->>I: success
I->>PM: install package via PlatformIO
PM-->>I: done
I-->>P: success
else Install failed
R-->>I: failure
I-->I: increment retry_count, increase timeout
end
end
alt All attempts failed
I-->>P: failure
end
else Up-to-date
P-->>U: no-op
end
sequenceDiagram
autonumber
participant P as Espressif32Platform
participant H as _handle_existing_tool
participant I as install_tool (re-entry)
P->>H: _handle_existing_tool(tool_name, paths, retry_count)
alt Version mismatch
H->>I: install_tool(tool_name, retry_count+1)
I-->>H: result
else Versions aligned
H-->>P: keep existing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform.py (1)
489-512: Retry policy split across two places; consolidate and use the passed retry_count.install_tool enforces RETRY_LIMIT, while _install_with_idf_tools has its own max_retries=3. Keep a single source of truth.
- if retry_count >= RETRY_LIMIT: + if retry_count >= RETRY_LIMIT: logger.error( f"Installation of {tool_name} failed after {RETRY_LIMIT} attempts" ) return FalseAnd see follow-up change in _install_with_idf_tools to use RETRY_LIMIT and honor retry_count as the starting attempt.
🧹 Nitpick comments (7)
platform.py (7)
274-296: Version extraction is brittle for pre-releases/alt tag formats.support vX.Y, vX.Y.Z, and pre-releases (e.g., v5.1.0-rc1).
Apply:
- if version_string.startswith(('http://', 'https://')): + if version_string.startswith(('http://', 'https://')): # Extract version from URL like: .../v5.1.0/esp_install-v5.1.0.zip - import re - version_match = re.search(r'v(\d+\.\d+\.\d+)', version_string) + import re + version_match = re.search(r'v(\d+\.\d+(?:\.\d+)?(?:[-._][A-Za-z0-9]+)?)', version_string) if version_match: return version_match.group(1) # Returns "5.1.0" else: # Fallback: Use entire URL return version_string
297-348: Preserve stack traces during failures.Use logger.exception in the except block to aid diagnosis.
- except Exception as e: - logger.error(f"Error installing {tl_install_name}: {e}") + except Exception: + logger.exception(f"Error installing {tl_install_name}") return False
349-377: Also log exception details when cleanup fails.Keeps diagnostics when fs operations error.
- except OSError as e: - logger.error(f"Error cleaning up versioned directories for {tool_name}: {e}") + except OSError: + logger.exception(f"Error cleaning up versioned directories for {tool_name}")
539-543: Consider marking package as optional after successful installation or setting explicit version spec.Depending on PlatformIO semantics, you may want to set
self.packages[tool_name]["optional"] = Falseonly untilpm.install(tl_path)succeeds, then flip to True or set"version" = tl_pathfor clarity. Optional.
404-453: Optional: add small randomized backoff between attempts.Helps avoid thundering herd against the same mirrors.
+ import random, time @@ - else: + else: logger.error(f"All {max_retries} installation attempts failed for {tool_name}") - return False + # jittered sleep before next attempt + if attempt < max_retries - 1: + time.sleep(1.0 + random.random() * 2.0) + return False
103-107: Tiny nit: redundant join.
IDF_TOOLS_PATH = PROJECT_CORE_DIRis sufficient.-IDF_TOOLS_PATH=os.path.join(PROJECT_CORE_DIR) +IDF_TOOLS_PATH = PROJECT_CORE_DIR
404-453: Cap idf_tools install timeout and surface error output
Exponential backoff using SUBPROCESS_TIMEOUT·3ⁿ can lead to 45–135 min waits; bound it via anESP32_INSTALL_MAX_TIMEOUTenv var (default 1200 s) and switch to a 2ⁿ backoff. Pipe stderr instead of silencing it, then log the last ~20 lines on non-zero exit; uselogger.exceptionfor unexpected errors.
📜 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(13 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-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
📚 Learning: 2025-08-20T14:13:13.842Z
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.
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
🪛 Ruff (0.12.2)
platform.py
252-252: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
376-376: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
432-432: subprocess call: check for execution of untrusted input
(S603)
445-445: Consider moving this statement to an else block
(TRY300)
448-448: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
451-451: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
513-513: Unused method argument: retry_count
(ARG002)
⏰ 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). (4)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (1)
platform.py (1)
205-254: tl-install version check matches intended lifecycle (mandatory → optional).Setting tool-esp_install to optional after verifying version aligns with the established pattern for this repo. LGTM.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform.py (1)
404-453: Cap and unify idf_tools.py timeout backoff; include exception stacktraces; clarify success path
- Bound
current_timeoutbyESP32_INSTALL_MAX_TIMEOUT(default 1200s) and use2**retry_countto match the caller’s backoff.- Replace
logger.errorwithlogger.exceptionin both exception handlers to capture stack traces.- Wrap the success log in an
elsebranch after checkingresult.returncode == 0.- S603 is a false positive here (shell=False and controlled cmd).
def _run_idf_tools_install(self, tools_json_path: str, idf_tools_path: str, retry_count: int = 0) -> bool: @@ - # Dynamic timeout based on retry count (multiply by factor 3 for each retry) - current_timeout = SUBPROCESS_TIMEOUT * (3 ** retry_count) + # Exponential backoff capped by ESP32_INSTALL_MAX_TIMEOUT (env var) + max_timeout = int(os.environ.get("ESP32_INSTALL_MAX_TIMEOUT", "1200")) + current_timeout = min(max_timeout, SUBPROCESS_TIMEOUT * (2 ** retry_count)) @@ - if result.returncode != 0: - logger.error(f"idf_tools.py installation failed (attempt {retry_count + 1})") - return False - - logger.debug("idf_tools.py executed successfully") - return True + if result.returncode != 0: + logger.error(f"idf_tools.py installation failed (attempt {retry_count + 1})") + return False + else: + logger.debug("idf_tools.py executed successfully") + return True @@ - except subprocess.TimeoutExpired: - logger.error(f"Timeout in idf_tools.py after {current_timeout}s (attempt {retry_count + 1})") + except subprocess.TimeoutExpired: + logger.exception(f"Timeout in idf_tools.py after {current_timeout}s (attempt {retry_count + 1})") return False - except (subprocess.SubprocessError, OSError) as e: - logger.error(f"Error in idf_tools.py (attempt {retry_count + 1}): {e}") + except (subprocess.SubprocessError, OSError) as e: + logger.exception(f"Error in idf_tools.py (attempt {retry_count + 1}): {e}") return False
🧹 Nitpick comments (5)
platform.py (5)
251-253: Prefer logging.exception for richer context on JSON read failures.Switching to exception preserves stacktrace for tricky package.json issues.
- except (json.JSONDecodeError, FileNotFoundError) as e: - logger.error(f"Error reading package data for {tl_install_name}: {e}") + except (json.JSONDecodeError, FileNotFoundError) as e: + logger.exception(f"Error reading package data for {tl_install_name}: {e}") return self._install_tl_install(required_version)
349-377: Emit stacktrace when cleanup of versioned dirs fails.Use logging.exception to aid diagnosing permission/path issues.
- except OSError as e: - logger.error(f"Error cleaning up versioned directories for {tool_name}: {e}") + except OSError as e: + logger.exception(f"Error cleaning up versioned directories for {tool_name}: {e}")
378-393: Name nit: clarify idf_tools_path is a script path.Consider renaming dict key to idf_tools_script to avoid confusion with a directory path.
526-556: Message timeout should reflect actual next attempt timeout.After unifying base/cap, next_timeout calc here should match _run_idf_tools_install; otherwise logs mislead.
- next_timeout = min(max_timeout, SUBPROCESS_TIMEOUT * (2 ** ((attempt - start) + 1))) + next_timeout = min(max_timeout, SUBPROCESS_TIMEOUT * (2 ** ((attempt - start) + 1))) # Keep this in sync with _run_idf_tools_install()
726-737: Avoid redundant install_tool() calls for mklittlefs(3/4).Both the “exists” checks trigger install, causing duplicate work. Collapse to if/elif or remove the second checks.
- if not os.path.exists(mklittlefs_dir): - self.install_tool("tool-mklittlefs") - if os.path.exists(os.path.join(mklittlefs_dir, "tools.json")): - self.install_tool("tool-mklittlefs") + if not os.path.exists(mklittlefs_dir) or os.path.exists(os.path.join(mklittlefs_dir, "tools.json")): + self.install_tool("tool-mklittlefs") @@ - if not os.path.exists(mklittlefs4_dir): - self.install_tool("tool-mklittlefs4") - if os.path.exists(os.path.join(mklittlefs4_dir, "tools.json")): - self.install_tool("tool-mklittlefs4") + if not os.path.exists(mklittlefs4_dir) or os.path.exists(os.path.join(mklittlefs4_dir, "tools.json")): + self.install_tool("tool-mklittlefs4")
📜 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(14 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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-20T14:13:13.842Z
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.
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
🪛 Ruff (0.12.2)
platform.py
252-252: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
376-376: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
432-432: subprocess call: check for execution of untrusted input
(S603)
445-445: Consider moving this statement to an else block
(TRY300)
448-448: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
451-451: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ 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-http-request)
- GitHub Check: build (macos-15, examples/espidf-blink)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (macos-15, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (4)
platform.py (4)
105-106: IDF_TOOLS_PATH override matches maintainer preference.Setting IDF_TOOLS_PATH to core_dir aligns with past guidance for consistent tool management.
205-246: tl-install version check flow looks correct.
- Reads required version, compares via URL-aware parser.
- On match, marks package optional per intended pattern.
No functional concerns here.
489-496: Retry guard is fine.Early exit when retry_count >= RETRY_LIMIT is clear and prevents runaway loops.
513-557: Fix file:// URI construction for cross-platform (Windows) and align backoff with _run().
- f"file://{path}" is not a valid URI on Windows (needs file:///C:/...). Use Path(...).as_uri().
- Keep the backoff base consistent with _run_idf_tools_install (after unification).
- tl_path = f"file://{os.path.join(IDF_TOOLS_PATH, 'tools', tool_name)}" - pm.install(tl_path) + from pathlib import Path + tl_dir = Path(os.path.join(IDF_TOOLS_PATH, "tools", tool_name)) + tl_path = tl_dir.as_uri() + pm.install(tl_path)⛔ Skipped due to learnings
Learnt from: Jason2866 PR: pioarduino/platform-espressif32#263 File: platform.py:492-492 Timestamp: 2025-08-20T14:13:54.005Z Learning: In PlatformIO projects, `file://` is a special PlatformIO syntax for path references, not standard file URIs. The construction `f"file://{os.path.join(path, subpath)}"` is correct PlatformIO syntax and should not be replaced with pathlib.Path.as_uri().
|
Closing in favour of #276 |
1 participant
Description:
fix download and install issue with slow Internet connection
Checklist:
Summary by CodeRabbit
New Features
Improvements