Move penv setup out of main.py / invert mcu detection logic #255
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 WalkthroughThis update centralizes Python virtual environment and dependency setup into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Main as builder/main.py
participant PEnv as builder/penv_setup.py
participant System as System/Python
Main->>PEnv: setup_python_environment(env, platform, platformio_dir)
PEnv->>System: Check Python version (>=3.10)
PEnv->>System: Create virtual environment if missing
PEnv->>System: Install dependencies via uv (if needed)
PEnv->>System: Install esptool from local package
PEnv-->>Main: Return penv Python executable and esptool path
Main->>Main: Continue build with environment set up
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 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
🧹 Nitpick comments (1)
builder/penv_setup.py (1)
104-109: Consider improving URL version extraction robustness.The regex pattern for extracting version from URLs could be made more robust to handle edge cases like pre-release versions or different URL formats.
Consider using a more comprehensive pattern or a URL parsing library:
- m = re.search(r'/v?(\d+\.\d+\.\d+(?:\.\d+)?)(?:\.(?:zip|tar\.gz|tar\.bz2))?$', spec) + # Handle more version formats including pre-releases + m = re.search(r'/v?(\d+\.\d+\.\d+(?:[.-]\w+)?(?:\.\d+)?)(?:\.(?:zip|tar\.gz|tar\.bz2))?$', spec)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/frameworks/_embed_files.py(1 hunks)builder/frameworks/ulp.py(1 hunks)builder/main.py(4 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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.
Applied to files:
builder/frameworks/ulp.pybuilder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/main.pybuilder/penv_setup.py
🪛 Ruff (0.12.2)
builder/frameworks/ulp.py
40-40: Undefined name idf_variant
(F821)
⏰ 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). (19)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (macos-15, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/tasmota)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp)
- GitHub Check: build (ubuntu-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (ubuntu-latest, examples/espidf-http-request)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- 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)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (5)
builder/frameworks/ulp.py (1)
39-42: LGTM! MCU detection logic correctly inverted.The condition now explicitly checks for Xtensa-based variants (esp32, esp32s2, esp32s3) to select the appropriate toolchain, which aligns with the similar changes in other build files.
builder/frameworks/_embed_files.py (1)
112-121: MCU detection logic correctly inverted for objcopy toolchain selection.The three conditions consistently check for non-Xtensa variants to select RISC-V toolchain parameters, maintaining alignment with the overall refactoring pattern.
builder/main.py (2)
35-35: Clean extraction of Python environment setup.The complex virtual environment and dependency management logic has been successfully consolidated into the
penv_setupmodule, improving code organization and maintainability.Also applies to: 45-46
421-422: MCU detection logic correctly inverted for toolchain selection.Both the toolchain architecture and GDB tool selection now consistently check for non-Xtensa variants, aligning with the changes in other build files.
Also applies to: 449-453
builder/penv_setup.py (1)
294-344: Well-structured main setup function.The function properly orchestrates the environment setup with good error handling, version checks, and clear return values. The extraction of this logic from main.py significantly improves code organization.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
builder/main.py (1)
596-607: CLI args appended twice to esp-idf-size command.
cli_argsare extended intocmdin Lines 601-603 and again in Lines 605-607. This duplicates args.Apply either of the following (remove one of the extensions). Example: keep the later block and drop the earlier:
- if "--" in sys.argv: - dash_index = sys.argv.index("--") - if dash_index + 1 < len(sys.argv): - cli_args = sys.argv[dash_index + 1:] - cmd.extend(cli_args) + if "--" in sys.argv: + dash_index = sys.argv.index("--") + if dash_index + 1 < len(sys.argv): + cli_args = sys.argv[dash_index + 1:]
♻️ Duplicate comments (1)
builder/penv_setup.py (1)
265-275: Optional: avoid f-string interpolation into Python snippet.Passing
esptool_repo_pathvia argv (json) avoids quoting edge cases and is easier to read. Prior discussion concluded inputs are trusted, so this is optional here.- result = subprocess.run( - [python_exe, "-c", - "import esptool; " - "import os; " - f"expected_path = os.path.normpath(r'{esptool_repo_path}'); " - "actual_path = os.path.normpath(os.path.dirname(esptool.__file__)); " - "print('MATCH' if expected_path in actual_path else 'MISMATCH')"], + import json + result = subprocess.run( + [python_exe, "-c", + "import esptool, os, sys, json; " + "expected_path = os.path.normpath(json.loads(sys.argv[1])); " + "actual_path = os.path.normpath(os.path.dirname(esptool.__file__)); " + "print('MATCH' if expected_path in actual_path else 'MISMATCH')", + json.dumps(esptool_repo_path)], capture_output=True, text=True, timeout=5 )
🧹 Nitpick comments (4)
builder/penv_setup.py (4)
280-281: Tighten exception handling for subprocess.run.
subprocess.rundoesn't raiseCalledProcessErrorunlesscheck=Trueis used. Either addcheck=Trueor drop that exception branch.- except (subprocess.CalledProcessError, subprocess.TimeoutExpired, FileNotFoundError): + except (subprocess.TimeoutExpired, FileNotFoundError): pass
139-144: Increase install timeouts for first-time setups.30s is often insufficient for cold installs (e.g., cryptography wheels, slow CI mirrors). Bump to a more forgiving window.
- timeout=30 # 30 second timeout + timeout=180 # allow slower mirrors/cold caches ... - timeout=30 # 30 second timeout + timeout=120 # listing should be quick but allow headroom ... - timeout=30 # 30 second timeout for package installation + timeout=300 # first-time installs can take longerAlso applies to: 175-176, 223-224
69-83: Consider avoiding adding the entire penv dir to sys.path.Adding the venv root via
site.addsitedir(penv_dir)is unusual and can shadow modules. Adding only site-packages is typically sufficient.- # Add penv_dir to module search path - site.addsitedir(penv_dir) + # Only add site-packages; avoid injecting venv root itself
19-19: Dependency availability at import time.
semantic_versionis imported at module load before penv is prepared. PlatformIO’s host Python typically includes it, but if this module is reused outside PIO, consider lazy importing inside the function that needs it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/frameworks/_embed_files.py(1 hunks)builder/frameworks/ulp.py(1 hunks)builder/main.py(4 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
📚 Learning: 2025-08-10T16:59:15.510Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.510Z
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:
builder/frameworks/ulp.pybuilder/main.pybuilder/penv_setup.py
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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.
Applied to files:
builder/frameworks/ulp.pybuilder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/main.pybuilder/penv_setup.py
🧬 Code Graph Analysis (1)
builder/main.py (1)
builder/penv_setup.py (1)
setup_python_environment(294-344)
🪛 Ruff (0.12.2)
builder/frameworks/ulp.py
40-40: Undefined name idf_variant
(F821)
🔇 Additional comments (4)
builder/main.py (2)
421-423: MCU detection logic inversion looks consistent.Switching to “not in Xtensa set => RISC-V” simplifies maintenance as new RISC-V targets appear. GDB package selection aligns with the new condition.
Please confirm boards outside ("esp32","esp32s2","esp32s3") are all RISC-V in your supported matrix; unknown MCUs will default to RISC-V with this logic.
Also applies to: 447-455
45-47: Good refactor: centralized penv setup.Replacing inline penv logic with
setup_python_environmentreduces complexity in main and standardizes tool setup.builder/frameworks/ulp.py (1)
38-42: Toolchain selection inversion aligns with main.py.Selecting Xtensa tools only for ("esp32","esp32s2","esp32s3") and RISC-V otherwise matches the unified rule elsewhere.
Static analysis may flag
idf_variantas undefined; it’s provided viaSCons.Script.Importat Line 24 and is valid at runtime.builder/frameworks/_embed_files.py (1)
112-121: Objcopy tool/arch inversion is consistent.RISC-V chosen when MCU not in ("esp32","esp32s2","esp32s3") for executable, output target, and binary arch. Matches the new global rule.
Also applies to: 118-121
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
builder/frameworks/ulp.py (1)
96-99: Bug: include lists concatenated without a separator; paths will be mangled
comp_includes = comp_includes + plain_includesproduces e.g."incA;incBincC;incD". Join after combining lists instead.- comp_includes = ";".join(get_component_includes(target_config)) - plain_includes = ";".join(app_includes["plain_includes"]) - comp_includes = comp_includes + plain_includes + comp_includes_list = list(get_component_includes(target_config)) + comp_includes_list.extend(app_includes["plain_includes"]) + comp_includes = ";".join(comp_includes_list)
♻️ Duplicate comments (2)
builder/penv_setup.py (2)
265-276: Addressed: subprocess path passing avoids injection and quoting pitfallsSwitching to
sys.argv[1]and realpath/normcase comparisons is correct and safe. This resolves the earlier injection concern.
283-284: Addressed: exact match removes false positive from "MISMATCH" containing "MATCH"Comparing
result.stdout.strip() == "MATCH"fixes the earlier bug.
🧹 Nitpick comments (7)
builder/penv_setup.py (6)
69-83: Nit: unnecessaryaddsitedir(penv_dir); site-packages is sufficientAdding the venv root to sys.path isn’t required and can slightly slow path scanning. Consider limiting to site-packages.
- # Add penv_dir to module search path - site.addsitedir(penv_dir) - # Add site-packages directory
118-158: UV bootstrap fallback is reasonable; consider a slightly longer timeout30s can be tight on slow mirrors/CI. Bumping to 120s reduces flaky failures without harming fast paths.
- timeout=30 # 30 second timeout + timeout=120
218-241: Good: all subprocess args are passed as list; errors surfaced; minor: increase install timeoutDependency installs can often exceed 30s (wheels build/large deps). Consider 180s.
- timeout=30 # 30 second timeout for package installation + timeout=180
259-262: Silent exit on missingtool-esptoolpyobscures root cause; add an error messageEmit a helpful error to stderr before exiting.
- if not esptool_repo_path or not os.path.isdir(esptool_repo_path)): - sys.exit(1) + if not esptool_repo_path or not os.path.isdir(esptool_repo_path): + sys.stderr.write( + f"Error: 'tool-esptoolpy' package directory not found: {esptool_repo_path!r}\n" + ) + sys.exit(1)
289-297: Enhance error reporting for esptool reinstall failuresSurface stderr to aid triage.
- except subprocess.CalledProcessError: - sys.exit(1) + except subprocess.CalledProcessError as e: + sys.stderr.write( + f"Error: Failed to install esptool from {esptool_repo_path} (exit {e.returncode})\n" + ) + sys.exit(1)
315-323: Confirm intentional Python >= 3.10 requirement (potentially breaking change)Bumping the host interpreter requirement to 3.10 may break existing users (PlatformIO host Python is often <3.10). If this is intended, document it prominently in release notes. Otherwise consider relaxing to 3.8/3.9.
Would you like a patch that relaxes this to 3.8+ while still supporting your dependency set?
builder/main.py (1)
588-629: UnreachableImportErrorhandler;firmware_metricsuses subprocess, not importSince
esp_idf_sizeis executed viasubprocess.run,ImportErrorin this context will never be raised. Rely on return code handling and remove the deadexcept ImportError.- except ImportError: - print("Error: esp-idf-size module not found.") - print("Install with: pip install esp-idf-size") except FileNotFoundError: print("Error: Python executable not found.") print("Check your Python installation.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/frameworks/_embed_files.py(1 hunks)builder/frameworks/ulp.py(1 hunks)builder/main.py(5 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
📚 Learning: 2025-08-10T16:59:15.510Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.510Z
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:
builder/penv_setup.pybuilder/main.pybuilder/frameworks/ulp.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/penv_setup.pybuilder/main.py
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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.
Applied to files:
builder/penv_setup.pybuilder/main.pybuilder/frameworks/ulp.py
🧬 Code Graph Analysis (1)
builder/main.py (1)
builder/penv_setup.py (1)
setup_python_environment(300-350)
🪛 Ruff (0.12.2)
builder/frameworks/ulp.py
40-40: Undefined name idf_variant
(F821)
🔇 Additional comments (12)
builder/penv_setup.py (4)
43-50: LGTM: cross-platform executable path resolutionClean handling of Windows vs POSIX Scripts/bin and .exe suffix is correct.
53-67: LGTM: venv creation with SCons action and basic sanity checkCreation via
"$PYTHONEXE" -m venv --clearand asserting presence ofpipbinary is sound in this context.
169-176: LGTM: enumerate installed packages via uv pip list with JSONGood use of
--format=jsonand lowercasing names; errors downgraded to warnings are appropriate here.
339-350: LGTM: esptool and uv executables derived from penv; deps installed before esptool; clear exit on failureOverall flow is correct and consistent with prior learnings about passing the venv Python explicitly in subprocess calls.
builder/frameworks/ulp.py (2)
39-42: MCU toolchain selection inversion looks correctSelecting Xtensa toolchain only for ("esp32","esp32s2","esp32s3") and RISC‑V otherwise aligns with the refactor across the codebase.
40-40: Ruff F821 “idf_variant” is a false positive (SCons injects it via Import)
idf_variantis provided byImport("env sdk_config project_config app_includes idf_variant"). Safe to ignore this linter warning here.If you want to appease static analysis locally, add a type comment near the Import line:
idf_variant: str # type: ignore[F821] (injected by SCons)builder/frameworks/_embed_files.py (1)
112-121: LGTM: objcopy triplet selection matches inverted MCU condition
- RISC‑V: riscv32-esp-elf-objcopy, elf32-littleriscv, "riscv".
- Xtensa: xtensa-{mcu}-elf-objcopy, elf32-xtensa-le, "xtensa".
Consistent with toolchain packages.
builder/main.py (5)
35-47: LGTM: centralized penv setup viasetup_python_environmentClean separation of concerns and correct handling of the returned Python/esptool paths.
420-423: LGTM: toolchain arch inversion consistent with other modulesRISC‑V default unless MCU is in the Xtensa trio.
447-459: GDB package selection aligns with toolchain arch; path construction looks correctBuilds
<pkg>/bin/<triplet>-elf-gdbappropriately for both Xtensa and RISC‑V.
428-433: Correct handling of spaces in esptool path for$OBJCOPYPre-quoting only when needed avoids nested quoting issues in later command strings.
724-775: LGTM: esptool upload commands—moved quoting to the variable; flags and offsets preserved
UPLOADCMDuses pre-quoted$UPLOADERwhen needed; flags mirror the ELF2IMAGE path. FS upload path also updated accordingly.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
builder/penv_setup.py (1)
271-285: Path verification and exact match handling look goodPassing
esptool_repo_pathvia argv and checkingstdout.strip() == "MATCH"avoids the earlier false-positive and injection concerns in this controlled environment.Also applies to: 291-295
🧹 Nitpick comments (5)
builder/penv_setup.py (2)
326-334: Python >= 3.10 requirement: confirm intended support floor and update docsRaising the minimum host Python to 3.10 is a user-facing breaking change (e.g., Ubuntu 20.04 ships 3.8). Please confirm project policy and update docs/release notes accordingly or consider relaxing to 3.8/3.9 if feasible.
If you decide to relax the requirement:
- if sys.version_info < (3, 10): + if sys.version_info < (3, 8): sys.stderr.write( - f"Error: Python 3.10 or higher is required. " + f"Error: Python 3.8 or higher is required. "Alternatively, check only for
venvavailability:- if sys.version_info < (3, 10): + try: + import venv # noqa + except Exception: sys.stderr.write( - f"Error: Python 3.10 or higher is required. " + "Error: A Python version with the 'venv' module is required.\n"Would you like me to draft a README/release-note snippet for this?
345-346: Use explicit error instead of assert for user-facing failure
assertmay be stripped with -O and isn’t user-friendly. Replace with an explicit check and message.- assert os.path.isfile(penv_python), f"Python executable not found: {penv_python}" + if not os.path.isfile(penv_python): + sys.stderr.write(f"Error: Python executable not found: {penv_python}\n") + sys.exit(1)builder/frameworks/ulp.py (1)
39-42: Xtensa allowlist inversion looks correct; consider centralizing chip listsSelecting Xtensa toolchain only for ("esp32","esp32s2","esp32s3") aligns with the PR’s inversion. To avoid drift across files, centralize this allowlist in one module (e.g., a helper in builder/) and import it where needed.
builder/frameworks/_embed_files.py (1)
112-121: Minor readability: compute architecture flag onceYou repeat the same condition 3x. Precompute a boolean to reduce duplication.
-mcu = board.get("build.mcu", "esp32") +mcuv = board.get("build.mcu", "esp32") +is_riscv = mcuv not in ("esp32","esp32s2","esp32s3") @@ - "riscv32-esp-elf-objcopy" - if mcu not in ("esp32","esp32s2","esp32s3") - else "xtensa-%s-elf-objcopy" % mcu, + "riscv32-esp-elf-objcopy" if is_riscv else f"xtensa-{mcuv}-elf-objcopy", @@ - "elf32-littleriscv" if mcu not in ("esp32","esp32s2","esp32s3") else "elf32-xtensa-le", + "elf32-littleriscv" if is_riscv else "elf32-xtensa-le", @@ - "riscv" if mcu not in ("esp32","esp32s2","esp32s3") else "xtensa", + "riscv" if is_riscv else "xtensa",Also applies to: 118-121
builder/main.py (1)
588-619: esp-idf-size invocation is robust; consider surfacing stderr on nonzero exit (optional)Using
PYTHON_EXE -m esp_idf_sizeis correct. As an optional improvement, print stderr when the command fails to aid troubleshooting.- result = subprocess.run(cmd, check=False, capture_output=False, env=os.environ) + result = subprocess.run(cmd, check=False, capture_output=True, text=True, env=os.environ) if result.returncode != 0: print(f"Warning: esp-idf-size exited with code {result.returncode}") + if result.stderr: + print(result.stderr.strip())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/frameworks/_embed_files.py(1 hunks)builder/frameworks/ulp.py(1 hunks)builder/main.py(6 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
📚 Learning: 2025-08-10T16:59:15.510Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.510Z
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:
builder/frameworks/ulp.pybuilder/main.pybuilder/penv_setup.py
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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.
Applied to files:
builder/frameworks/ulp.pybuilder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T19:13:25.569Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.569Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T19:12:17.965Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:19-19
Timestamp: 2025-08-10T19:12:17.965Z
Learning: In PlatformIO ESP32 platform builds, `semantic_version` is a dependency provided by PlatformIO itself and is guaranteed to be available when the build scripts run. The module is already imported at the top level in existing files like `builder/frameworks/espidf.py`, so lazy-loading it is unnecessary.
Applied to files:
builder/penv_setup.py
🧬 Code Graph Analysis (1)
builder/main.py (1)
builder/penv_setup.py (1)
setup_python_environment(311-363)
🪛 Ruff (0.12.2)
builder/frameworks/ulp.py
40-40: Undefined name idf_variant
(F821)
⏰ 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 (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (ubuntu-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (ubuntu-latest, examples/espidf-coap-server)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (6)
builder/penv_setup.py (2)
19-19: Top-level import of semantic_version is acceptable in this contextPlatformIO provides
semantic_versionas a transitive dependency. Keeping the top-level import is fine here and aligns with existing builder modules.
31-45: Do not add semantic_version to python_deps (transitive via PlatformIO)
semantic_versionis installed via the pinned PlatformIO core package. No need to duplicate it inpython_deps.builder/main.py (4)
35-47: Good refactor: penv setup is cleanly encapsulatedImporting and delegating penv/deps to
setup_python_environmentsimplifiesmain.pyand reduces duplication. The returnedPYTHON_EXEusage in subprocesses is correct per prior best practices.
420-423: MCU inversion consistent with other filesUsing RISC‑V when MCU is not in ("esp32","esp32s2","esp32s3") keeps Xtensa handling explicit and matches changes in _embed_files.py and ulp.py.
Also applies to: 447-455
429-433: Quoting esptool path via OBJCOPY is correct and cross-platform friendlyConditionally quoting
$OBJCOPYavoids issues with spaces in paths, and using it for both elf2image and erase aligns with existing conventions.Also applies to: 435-463
771-772: UPLOADCMD change (uploadfs): unquoted $UPLOADER is fine due to pre-quotingSince
$UPLOADERnow contains quotes when needed (path with spaces), removing quotes here avoids double-quoting pitfalls.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
builder/frameworks/ulp.py (2)
44-49: Guard PATH entries and prefer penv Python dir over where_is_program('python')Avoid adding empty/None entries to PATH and prefer the already-prepared penv Python for consistency with the refactor. This also avoids a potential TypeError if where_is_program("python") returns None.
- toolchain_path_ulp = platform.get_package_dir( - "toolchain-esp32ulp" - if sdk_config.get("ULP_COPROC_TYPE_FSM", False) - else "" - ) + toolchain_path_ulp = platform.get_package_dir("toolchain-esp32ulp") \ + if sdk_config.get("ULP_COPROC_TYPE_FSM", False) else None @@ - additional_packages = [ - toolchain_path, - toolchain_path_ulp, - platform.get_package_dir("tool-ninja"), - os.path.join(platform.get_package_dir("tool-cmake"), "bin"), - os.path.dirname(where_is_program("python")), - ] - - for package in additional_packages: - ulp_env.PrependENVPath("PATH", package) + python_dir = os.path.dirname(ulp_env.subst("$PYTHONEXE")) or "" + additional_packages = ( + toolchain_path, + toolchain_path_ulp, + platform.get_package_dir("tool-ninja"), + os.path.join(platform.get_package_dir("tool-cmake"), "bin"), + python_dir, + ) + for package in additional_packages: + if package: + ulp_env.PrependENVPath("PATH", package)Also applies to: 58-60
96-99: Fix include concatenation: currently missing the delimiter
comp_includes = comp_includes + plain_includesconcatenates two strings without a semicolon. This will merge the last and first include paths together.- comp_includes = ";".join(get_component_includes(target_config)) - plain_includes = ";".join(app_includes["plain_includes"]) - comp_includes = comp_includes + plain_includes + comp_includes_list = get_component_includes(target_config) + plain_includes_list = app_includes["plain_includes"] + comp_includes = ";".join(comp_includes_list + plain_includes_list)
♻️ Duplicate comments (1)
builder/penv_setup.py (1)
19-19: Top-levelsemantic_versionimport is acceptable in this environmentPer prior discussion, PlatformIO guarantees this dependency; lazy-loading not required.
🧹 Nitpick comments (5)
builder/frameworks/ulp.py (2)
38-42: Toolchain selection inversion aligns with Xtensa/RISC-V split — LGTMCondition correctly restricts xtensa toolchain to esp32/esp32s2/esp32s3, defaulting others to riscv32. Nit: add spaces after commas for readability.
- if idf_variant in ("esp32","esp32s2","esp32s3") + if idf_variant in ("esp32", "esp32s2", "esp32s3")
88-95: Style: preferif not ...over explicit== FalsePythonic and clearer.
- if lp_core_ulp_enabled == False: + if not lp_core_ulp_enabled:builder/frameworks/_embed_files.py (2)
24-26: Centralized Xtensa detection — LGTMConsistent
is_xtensacheck improves readability and aligns with main.py/ulp.py. Nit: spacing.-is_xtensa = mcu in ("esp32","esp32s2","esp32s3") +is_xtensa = mcu in ("esp32", "esp32s2", "esp32s3")
106-123: Objcopy selection inversion is correct and completeSwitching on
is_xtensacleanly adjusts executable name, output format, and architecture flags. Minor readability nit: prefer f-strings for the xtensa objcopy name.- else "xtensa-%s-elf-objcopy" % mcu, + else f"xtensa-{mcu}-elf-objcopy",builder/penv_setup.py (1)
74-88: Optional: avoid adding penv root to sys.pathAdding site-packages is needed; adding penv_dir itself is not. Avoids polluting sys.path with a directory that isn't a module root.
- # Add penv_dir to module search path - site.addsitedir(penv_dir) - # Add site-packages directory
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/frameworks/_embed_files.py(2 hunks)builder/frameworks/ulp.py(1 hunks)builder/main.py(9 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
📚 Learning: 2025-08-10T16:59:15.510Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.510Z
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:
builder/frameworks/ulp.pybuilder/main.pybuilder/penv_setup.py
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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.
Applied to files:
builder/frameworks/ulp.pybuilder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T19:13:25.569Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.569Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T20:18:20.676Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:141-161
Timestamp: 2025-08-10T20:18:20.676Z
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:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T19:12:17.965Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:19-19
Timestamp: 2025-08-10T19:12:17.965Z
Learning: In PlatformIO ESP32 platform builds, `semantic_version` is a dependency provided by PlatformIO itself and is guaranteed to be available when the build scripts run. The module is already imported at the top level in existing files like `builder/frameworks/espidf.py`, so lazy-loading it is unnecessary.
Applied to files:
builder/penv_setup.py
🧬 Code Graph Analysis (1)
builder/main.py (1)
builder/penv_setup.py (1)
setup_python_environment(311-361)
🪛 Ruff (0.12.2)
builder/frameworks/_embed_files.py
107-107: Undefined name env
(F821)
110-110: Undefined name env
(F821)
builder/frameworks/ulp.py
40-40: Undefined name idf_variant
(F821)
⏰ 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). (19)
- GitHub Check: build (macos-15, examples/espidf-ulp-lp)
- GitHub Check: build (macos-15, examples/espidf-storage-sdcard)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-exceptions)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- 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 (13)
builder/frameworks/ulp.py (1)
40-40: Ruff F821 false positive:idf_variantis imported by SCons
idf_variantis provided byImport("env sdk_config project_config app_includes idf_variant")on Line 24. Safe to ignore the linter warning in this SCons context.builder/frameworks/_embed_files.py (1)
107-110: Ruff F821 false positive:envis defined via SCons Import
Import("env")on Line 21 providesenv. The linter doesn't understand SCons’ dynamic import.builder/main.py (5)
35-47: Great refactor: penv setup extracted and single entrypoint — LGTMImporting
setup_python_environmentand wiring returned paths keeps main lean and consistent. No public API changes, aligns with PR goals.
417-424: Architecture detection viais_xtensais consistent with other filesClear separation between Xtensa and RISC-V; toolchain_arch fallback is correct.
447-456: GDB tool package selection now matches MCU arch — LGTMCorrectly selects xtensa vs riscv GDB and composes binary name from toolchain_arch.
561-616: Firmware metrics cleanup — LGTMRemoves duplication, uses penv Python, handles args sanely, and preserves diagnostics. Nice.
430-434: Quoting strategy for$OBJCOPYis robustPre-quoting the uploader path only when it contains spaces removes unnecessary quotes while ensuring correctness.
- Verified via
rgthatOBJCOPYis defined only as the upload tool (esptool) and never used elsewhere to invoke binutils’objcopy.- No occurrences of
OBJCOPYfound outside of this context (e.g. lines 455–459, 465–469, 511–515).All checks pass—no further changes required.
builder/penv_setup.py (6)
48-56: Executable path helper — LGTMCorrectly handles Scripts/bin and .exe suffix on Windows.
58-72: Venv creation is clear; minor note on invoking PythonUsing
"$PYTHONEXE" -m venvis fine in PIO builds. Assert on pip existence is a good sanity check.
90-121: Dependency resolution logic — LGTM
- Handles URL-pinned platformio with semantic version check.
- Case-insensitive matching and semver spec for others.
123-148: uv-first install flow with bounded timeouts — LGTMChecks uv availability, installs when missing, lists installed pkgs, computes missing/upgrades, and installs with uv. Timeouts match earlier design decisions.
Also applies to: 164-247
264-292: Esptool install from package path with robust verification — LGTM
- Path comes from PlatformIO package dir (trusted).
- Avoids false positive by comparing exact
MATCHstring and using realpaths with normcase and startswith.- Editing mode install ensures reusability.
Also applies to: 297-309
326-361: Entry-point API is cohesive and safeEnforces Python >= 3.10, wires env.PYTHONEXE, installs deps and esptool, returns both paths. Clear failure modes via stderr + exit.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
builder/penv_setup.py (1)
74-88: Python path setup: consider skipping addsitedir(penv_dir)Minor: addsitedir(penv_dir) is typically unnecessary; only site-packages matters. Safe to keep, but you could omit it to reduce sys.path noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/frameworks/_embed_files.py(2 hunks)builder/frameworks/ulp.py(2 hunks)builder/main.py(9 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/main.pybuilder/penv_setup.pybuilder/frameworks/ulp.py
📚 Learning: 2025-08-10T19:13:25.569Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.569Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T16:59:15.510Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.510Z
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:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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.
Applied to files:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T20:18:20.676Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:141-161
Timestamp: 2025-08-10T20:18:20.676Z
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:
builder/main.pybuilder/penv_setup.py
📚 Learning: 2025-08-10T19:12:17.965Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:19-19
Timestamp: 2025-08-10T19:12:17.965Z
Learning: In PlatformIO ESP32 platform builds, `semantic_version` is a dependency provided by PlatformIO itself and is guaranteed to be available when the build scripts run. The module is already imported at the top level in existing files like `builder/frameworks/espidf.py`, so lazy-loading it is unnecessary.
Applied to files:
builder/penv_setup.py
🧬 Code Graph Analysis (1)
builder/main.py (1)
builder/penv_setup.py (1)
setup_python_environment(311-361)
🪛 Ruff (0.12.2)
builder/frameworks/_embed_files.py
107-107: Undefined name env
(F821)
110-110: Undefined name env
(F821)
builder/frameworks/ulp.py
34-34: Undefined name idf_variant
(F821)
47-47: Undefined name sdk_config
(F821)
92-92: Undefined name idf_variant
(F821)
99-99: Undefined name app_includes
(F821)
⏰ 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). (19)
- GitHub Check: build (macos-15, examples/espidf-ulp)
- GitHub Check: build (macos-15, examples/espidf-arduino-littlefs)
- GitHub Check: build (macos-15, examples/espidf-blink)
- GitHub Check: build (macos-15, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (macos-15, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
🔇 Additional comments (21)
builder/frameworks/_embed_files.py (2)
24-26: Unify MCU detection with is_xtensa looks goodThe boolean flag simplifies logic across the file and aligns with other builders. No functional concerns.
114-122: Objcopy/toolchain target selection is correct
- Executable names: xtensa-{mcu}-elf-objcopy for esp32/esp32s2/esp32s3 and riscv32-esp-elf-objcopy for RISC‑V look right.
- Targets/arch: elf32-xtensa-le vs elf32-littleriscv and “xtensa” vs “riscv” are appropriate.
No action required.
builder/frameworks/ulp.py (2)
34-34: Consistent Xtensa/RISC‑V detectionUsing is_xtensa keeps this builder consistent with main.py and _embed_files.py. Good change.
98-101: Include path concatenation fix is cleanerJoining component and plain includes before “;” join avoids malformed paths. Looks good.
builder/main.py (8)
45-47: Centralizing Python env setup is a solid refactorSingle call to setup_python_environment greatly reduces complexity and keeps the builder lean. Returning both PYTHON_EXE and esptool path is practical.
417-424: MCU arch inversion reads clearer and reduces duplicationis_xtensa plus a single negation for RISC‑V is consistent with other files. Toolchain arch resolution is correct for esp32/esp32s2/esp32s3 vs RISC‑V parts.
430-434: Safe quoting for paths with spacesPre-quoting uploader_path only when needed avoids double-quoting and makes command construction robust. Good detail.
457-468: OBJCOPY repurposed to esptool path: acceptable hereIn this build system OBJCOPY is used to run esptool (elf2image/erase). The rename to uploader_path improves readability; assignment is correct. ERASEFLAGS quoting for $UPLOAD_PORT prevents argument splitting issues.
585-616: Firmware metrics: correct use of venv PythonUsing PYTHON_EXE explicitly (rather than relying on PATH) matches platform conventions. Error handling and optional verbosity are reasonable.
719-741: Esptool upload commands: quoting and flags look right
- UPLOADER now uses the pre-quoted uploader_path, and $UPLOADCMD dropped extra quotes — avoids double-quoting.
- Keeping "$UPLOAD_PORT" quoted is correct.
Looks good.Also applies to: 745-769
784-795: DFU upload: keep $SOURCE quotedRetaining quotes around "$SOURCE" is needed for paths with spaces. Change is correct.
35-36: Import path verified:penv_setup.pyresides alongsidemain.pyand the import works as expected under SCons. No fallback or changes needed.builder/penv_setup.py (9)
31-45: Dependency pinning looks intentional and sufficient
- platformio pinned to v6.1.18 via URL; semantic_version comes transitively from platformio-core (per prior context), so no need to list it explicitly.
- Specs for other deps are reasonable.
No change requested.
58-72: venv creation via SCons action is fine; assert ensures integrityCreating the venv with "$PYTHONEXE" and asserting the pip binary exists is a good safeguard. No issues spotted.
102-121: Version comparison logic is solid; URL version pin handled correctly
- Using PLATFORMIO_URL_VERSION_RE + pepver_to_semver avoids false positives.
- SimpleSpec matching for other deps is appropriate.
Looks good.
130-163: UV bootstrap flow is reasonableDetect uv first, then install via pip if missing. Timeouts/readability are fine; messages are actionable.
172-203: Package listing via uv pip list (JSON) is correctParsing into a lowercase-name dict with pepver_to_semver normalization is the right approach. Good warnings on failures.
264-293: Path verification for esptool source is robust
- Using sys.argv to pass the path avoids interpolation issues.
- Comparing realpath with startswith on normalized case is sufficient for editable installs.
Good guard; early return when MATCH is printed is precise.
297-307: Editable install via uv pip is appropriateForce-reinstall editable from tool-esptoolpy dir ensures alignment with the packaged source. Error handling is clear.
335-361: End-to-end env setup is clean and idempotent
- Version gate (>=3.10), venv setup, env.Replace(PYTHONEXE), site paths, deps install, and esptool install are well sequenced.
- Returning both penv python and esptool path matches main.py expectations.
LGTM.
223-231: Ignore timeout increase—30s is intentional
The 30-second timeout forsubprocess.run()in builder/penv_setup.py (lines 223–231) matches our established defaults (30 s for pip installs, 90 s for listings, 600 s for full package installations). These values are deliberate to fail fast on server-side aborts rather than slow networks. No change needed.Likely an incorrect or invalid review comment.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Refactor penv setup out of main.py
Checklist:
Summary by CodeRabbit
Here are the updated release notes based on the provided changes: