Claude 53 #156
Claude 53 #156
Conversation
so C6 gets optimized even with a small sketch, but not all others MCUs
WalkthroughThis set of changes introduces extensive refactoring and enhancements across the Arduino and ESP-IDF framework build scripts, the main build entry point, and the Espressif32 platform configuration. The updates focus on secure file operations, robust dependency management, improved path handling (especially for Windows), modular tool installation, and enhanced build target flexibility for firmware metrics analysis. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuildSystem
participant ArduinoFramework
participant PathCache
participant Logger
User->>BuildSystem: Start build (Windows)
BuildSystem->>ArduinoFramework: Initiate build process
ArduinoFramework->>PathCache: Resolve framework and SDK paths
ArduinoFramework->>Logger: Log build and deletion actions
ArduinoFramework->>ArduinoFramework: Validate and securely delete files/dirs
ArduinoFramework->>ArduinoFramework: Install Python dependencies
ArduinoFramework->>ArduinoFramework: Analyze include paths
ArduinoFramework->>ArduinoFramework: If path too long, apply path shortening
ArduinoFramework-->>BuildSystem: Build complete
sequenceDiagram
participant User
participant BuildSystem
participant MainScript
participant MetricsTool
User->>BuildSystem: Run `metrics` or `metrics-only` target
BuildSystem->>MainScript: Invoke firmware_metrics
MainScript->>MetricsTool: Run esp-idf-size with extra CLI/config args
MetricsTool-->>MainScript: Return result or error
MainScript-->>User: Display metrics or error message
Possibly related PRs
Poem
✨ Finishing Touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
builder/frameworks/espidf.py (1)
2155-2157: Consider a more robust approach for silencing actionsWhile the current hack works, it completely suppresses all output including potential errors. Consider using SCons's built-in verbosity controls or at least ensure error messages are still visible.
- silent_action = env.Action(idf_lib_copy) - silent_action.strfunction = lambda target, source, env: '' # hack to silence scons command output - env.AddPostAction("checkprogsize", silent_action) + # Use VerboseAction with conditional output based on verbosity level + action = env.Action(idf_lib_copy) + if not int(ARGUMENTS.get("PIOVERBOSE", 0)): + action.strfunction = lambda target, source, env: '' + env.AddPostAction("checkprogsize", action)This approach would allow verbose output when needed for debugging while keeping builds quiet by default.
Also applies to: 2174-2176
platform.py (3)
263-273: Simplify control flow by removing unnecessary elseThe
elseblock is unnecessary after thereturnstatement.def _handle_existing_tool(self, tool_name: str, paths: Dict[str, str], retry_count: int) -> bool: """Handle already installed tools""" if self._check_tool_version(tool_name): # Version matches, use tool self.packages[tool_name]["version"] = paths['tool_path'] self.packages[tool_name]["optional"] = False logger.debug(f"Tool {tool_name} found with correct version") return True - else: - # Wrong version, reinstall - logger.info(f"Reinstalling {tool_name} due to version mismatch") - safe_remove_directory(paths['tool_path']) - return self.install_tool(tool_name, retry_count + 1) + # Wrong version, reinstall + logger.info(f"Reinstalling {tool_name} due to version mismatch") + safe_remove_directory(paths['tool_path']) + return self.install_tool(tool_name, retry_count + 1)
552-564: Simplify control flow in _get_openocd_interfaceThe elif/else chain can be simplified since each branch returns.
def _get_openocd_interface(self, link: str, board) -> str: """Determine OpenOCD interface for debug link""" if link in ("jlink", "cmsis-dap"): return link - elif link in ("esp-prog", "ftdi"): + if link in ("esp-prog", "ftdi"): if board.id == "esp32-s2-kaluga-1": return "ftdi/esp32s2_kaluga_v1" - else: - return "ftdi/esp32_devkitj_v1" - elif link == "esp-bridge": + return "ftdi/esp32_devkitj_v1" + if link == "esp-bridge": return "esp_usb_bridge" - elif link == "esp-builtin": + if link == "esp-builtin": return "esp_usb_jtag" - else: - return f"ftdi/{link}" + return f"ftdi/{link}"
451-453: Consider more granular exception handlingWhile not re-raising exceptions maintains compatibility, it might hide critical configuration failures. Consider differentiating between recoverable and critical errors.
except Exception as e: logger.error(f"Error in package configuration: {type(e).__name__}: {e}") - # Don't re-raise to maintain compatibility + # Re-raise critical errors that would prevent build + if isinstance(e, (ToolInstallationError, OSError, IOError)): + raise + # Log but continue for other errors to maintain compatibility + logger.warning("Continuing with partial configuration due to non-critical error")builder/frameworks/arduino.py (4)
287-296: Remove unnecessary else blockThe
elseblock is unnecessary after thereturnstatement.if safe_delete_directory(FRAMEWORK_DIR): #print("Framework successfully removed") return True - else: - print("Error removing framework") - return False + print("Error removing framework") + return False else: logging.error(f"PlatformIO path validation failed: {FRAMEWORK_DIR}") return False
401-403: Consider using a more modern hash functionWhile MD5 is not used for security here, it's still deprecated. Consider using SHA256 or xxhash for better performance.
def get_MD5_hash(phrase): - return hashlib.md5(phrase.encode('utf-8')).hexdigest()[:16] + return hashlib.sha256(phrase.encode('utf-8')).hexdigest()[:16]
514-566: Consider refactoring to reduce complexityThis function has too many local variables (19) which makes it hard to read and maintain.
Consider extracting the path shortening logic into a separate helper function:
def calculate_shortened_paths(includes, framework_sdk_dir): """Calculate shortened include paths for framework folders""" shortened = [] generic = [] saved_chars = 0 for inc in includes: if is_framework_subfolder(inc): relative_path = fs.to_unix_path(relpath(inc, framework_sdk_dir)) shortened_path = "-iwithprefix/" + relative_path shortened.append(shortened_path) original_chars = len(f"-I{inc}") new_chars = len(shortened_path) saved_chars += max(0, original_chars - new_chars) else: generic.append(inc) return shortened, generic, saved_chars
641-641: Remove trailing space- build_script_path = join(FRAMEWORK_DIR, "tools", "pioarduino-build.py") + build_script_path = join(FRAMEWORK_DIR, "tools", "pioarduino-build.py")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/frameworks/arduino.py(1 hunks)builder/frameworks/espidf.py(4 hunks)builder/main.py(2 hunks)platform.py(6 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
platform.py
[refactor] 263-273: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 552-564: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 555-558: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
builder/frameworks/arduino.py
[refactor] 287-292: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 514-514: Too many local variables (19/15)
(R0914)
🔇 Additional comments (6)
builder/frameworks/espidf.py (1)
1783-1787: Good: Unconditional map file generationMaking the map file generation unconditional ensures it's always available for firmware metrics analysis, which aligns well with the enhanced metrics functionality.
builder/main.py (3)
350-368: Well-structured enhancement to firmware metricsThe improved map file detection logic and terminal encoding check are good additions. The function now properly handles cases where the map file might be in different locations.
384-387: Good CLI argument handling implementationThe use of
sys.argvwith--separator and proper bounds checking is a clean approach for passing arguments to esp-idf-size.
629-648: Excellent addition of custom metrics targetsThe dual-target approach provides flexibility:
metrics: Ensures fresh build before analysismetrics-only: Quick re-analysis without rebuildThe clear descriptions help users understand the CLI argument support.
platform.py (1)
88-118: Excellent implementation of safe file operationsThe decorator pattern for filesystem operations provides robust error handling and logging. The
safe_copy_filefunction's automatic parent directory creation is particularly helpful.builder/frameworks/arduino.py (1)
279-296: Fix variable order dependency issueThe function uses
FRAMEWORK_DIRwhich is defined later on line 341. This will cause aNameErrorat runtime.Move this function definition after line 341 where
FRAMEWORK_DIRis defined, or pass it as a parameter:-def safe_framework_cleanup(): +def safe_framework_cleanup(framework_dir): """Secure cleanup of Arduino Framework""" # Secure deletion of framework directories - if exists(FRAMEWORK_DIR): - logging.info(f"Attempting to validate framework path: {FRAMEWORK_DIR}") + if exists(framework_dir): + logging.info(f"Attempting to validate framework path: {framework_dir}") # Use specialized PlatformIO path validation - if validate_platformio_path(FRAMEWORK_DIR): + if validate_platformio_path(framework_dir): #print("*** Secure framework cleanup ***") - logging.info(f"Framework path validated successfully: {FRAMEWORK_DIR}") + logging.info(f"Framework path validated successfully: {framework_dir}") - if safe_delete_directory(FRAMEWORK_DIR): + if safe_delete_directory(framework_dir): #print("Framework successfully removed") return True else: print("Error removing framework") return False else: - logging.error(f"PlatformIO path validation failed: {FRAMEWORK_DIR}") + logging.error(f"PlatformIO path validation failed: {framework_dir}") return False return TrueLikely an incorrect or invalid review comment.
| except Exception: | ||
| print("Warning! Couldn't extract the list of installed Python packages.") | ||
| return {} | ||
| for p in packages: | ||
| result[p["name"]] = pepver_to_semver(p["version"]) | ||
|
|
||
|
|
There was a problem hiding this comment.
Improve exception handling specificity
The bare except clause could hide important errors. Consider catching specific exceptions to better handle different failure scenarios.
- except Exception:
+ except (subprocess.CalledProcessError, json.JSONDecodeError, OSError) as e:
- print("Warning! Couldn't extract the list of installed Python packages.")
+ print(f"Warning! Couldn't extract the list of installed Python packages: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception: | |
| print("Warning! Couldn't extract the list of installed Python packages.") | |
| return {} | |
| for p in packages: | |
| result[p["name"]] = pepver_to_semver(p["version"]) | |
| except (subprocess.CalledProcessError, json.JSONDecodeError, OSError) as e: | |
| print(f"Warning! Couldn't extract the list of installed Python packages: {e}") |
🤖 Prompt for AI Agents
In builder/frameworks/espidf.py around lines 92 to 94, replace the bare except
clause with specific exception types that might occur during the package
extraction process, such as ImportError or subprocess.CalledProcessError. This
will prevent hiding unexpected errors and allow for more precise error handling
and debugging.
| if flag_custom_sdkconfig == True and flag_any_custom_sdkconfig == False: | ||
|
|
||
| # Secure framework cleanup | ||
| if safe_framework_cleanup(): |
There was a problem hiding this comment.
Update function call to match new signature
Based on the earlier suggested fix for safe_framework_cleanup, this call needs to pass the framework directory as a parameter.
- if safe_framework_cleanup():
+ if safe_framework_cleanup(FRAMEWORK_DIR):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if safe_framework_cleanup(): | |
| if safe_framework_cleanup(FRAMEWORK_DIR): |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py at line 615, the call to
safe_framework_cleanup() must be updated to pass the framework directory
argument as per the new function signature. Modify the call to include the
appropriate framework directory variable so it matches the updated definition.
| # Framework reinstallation if required - IMPROVED WITH SECURE DELETION | ||
| if check_reinstall_frwrk(): | ||
| # Secure removal of SDKConfig files | ||
| safe_remove_sdkconfig_files() |
There was a problem hiding this comment.
Update function call with required parameters
Based on the earlier suggested fix for safe_remove_sdkconfig_files, this call needs to pass the required parameters.
- safe_remove_sdkconfig_files()
+ safe_remove_sdkconfig_files(config, project_dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| safe_remove_sdkconfig_files() | |
| safe_remove_sdkconfig_files(config, project_dir) |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py at line 610, the call to
safe_remove_sdkconfig_files is missing required parameters. Update this function
call to include all necessary arguments as defined in the updated function
signature to ensure it operates correctly.
| def safe_remove_sdkconfig_files(): | ||
| """Secure removal of SDKConfig files""" | ||
| envs = [section.replace("env:", "") for section in config.sections() if section.startswith("env:")] | ||
| for env_name in envs: | ||
| file_path = join(project_dir, f"sdkconfig.{env_name}") | ||
| if exists(file_path): | ||
| safe_delete_file(file_path) | ||
|
|
There was a problem hiding this comment.
Fix undefined variables in function
This function uses config and project_dir which are defined later on lines 310 and 316. This will cause a NameError at runtime.
Either move this function after the variable definitions or pass them as parameters:
-def safe_remove_sdkconfig_files():
+def safe_remove_sdkconfig_files(config, project_dir):
"""Secure removal of SDKConfig files"""
envs = [section.replace("env:", "") for section in config.sections() if section.startswith("env:")]
for env_name in envs:
file_path = join(project_dir, f"sdkconfig.{env_name}")
if exists(file_path):
safe_delete_file(file_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def safe_remove_sdkconfig_files(): | |
| """Secure removal of SDKConfig files""" | |
| envs = [section.replace("env:", "") for section in config.sections() if section.startswith("env:")] | |
| for env_name in envs: | |
| file_path = join(project_dir, f"sdkconfig.{env_name}") | |
| if exists(file_path): | |
| safe_delete_file(file_path) | |
| def safe_remove_sdkconfig_files(config, project_dir): | |
| """Secure removal of SDKConfig files""" | |
| envs = [section.replace("env:", "") for section in config.sections() if section.startswith("env:")] | |
| for env_name in envs: | |
| file_path = join(project_dir, f"sdkconfig.{env_name}") | |
| if exists(file_path): | |
| safe_delete_file(file_path) |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py around lines 298 to 305, the function
safe_remove_sdkconfig_files uses the variables config and project_dir before
they are defined, causing a NameError. To fix this, either move the function
definition to after lines 310 and 316 where these variables are defined, or
modify the function to accept config and project_dir as parameters and pass them
in when calling the function.
| if len(str(env.GetProjectOption("build_unflags"))) == 2: # No valid env, needs init | ||
| env['BUILD_UNFLAGS'] = {} | ||
| build_unflags = " ".join(env['BUILD_UNFLAGS']) | ||
| build_unflags = build_unflags + " -mdisable-hardware-atomics -ustart_app_other_cores" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve build_unflags validity check
Checking if the string representation has length 2 is fragile and unclear. This appears to be checking for an empty dictionary/list representation like "{}".
Use a more explicit check:
- if len(str(env.GetProjectOption("build_unflags"))) == 2: # No valid env, needs init
+ build_unflags_value = env.GetProjectOption("build_unflags")
+ if not build_unflags_value or build_unflags_value == "{}": # No valid env, needs init
env['BUILD_UNFLAGS'] = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(str(env.GetProjectOption("build_unflags"))) == 2: # No valid env, needs init | |
| env['BUILD_UNFLAGS'] = {} | |
| build_unflags = " ".join(env['BUILD_UNFLAGS']) | |
| build_unflags = build_unflags + " -mdisable-hardware-atomics -ustart_app_other_cores" | |
| build_unflags_value = env.GetProjectOption("build_unflags") | |
| if not build_unflags_value or build_unflags_value == "{}": # No valid env, needs init | |
| env['BUILD_UNFLAGS'] = {} |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py around lines 354 to 356, the current check for
build_unflags validity uses the length of the string representation, which is
fragile and unclear. Replace this with an explicit check to see if
env.GetProjectOption("build_unflags") returns an empty dictionary or list by
directly comparing it to {} or [] or by checking its truthiness. This makes the
intent clearer and the check more robust.
| def setup_logging(): | ||
| """Setup logging with optional file output""" | ||
| handlers = [logging.StreamHandler()] | ||
|
|
||
| # Only add file handler if writable and not disabled | ||
| log_file = os.environ.get('ARDUINO_FRAMEWORK_LOG_FILE') | ||
| if log_file: | ||
| with suppress(OSError, PermissionError): | ||
| handlers.append(logging.FileHandler(log_file)) | ||
|
|
||
| logging.basicConfig( | ||
| level=logging.INFO, | ||
| format='%(asctime)s - %(levelname)s - %(message)s', | ||
| handlers=handlers | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add path validation for log file
The logging setup should validate and create the directory for the log file if it doesn't exist.
def setup_logging():
"""Setup logging with optional file output"""
handlers = [logging.StreamHandler()]
# Only add file handler if writable and not disabled
log_file = os.environ.get('ARDUINO_FRAMEWORK_LOG_FILE')
if log_file:
with suppress(OSError, PermissionError):
+ # Ensure log directory exists
+ log_dir = os.path.dirname(log_file)
+ if log_dir and not os.path.exists(log_dir):
+ os.makedirs(log_dir, exist_ok=True)
handlers.append(logging.FileHandler(log_file))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def setup_logging(): | |
| """Setup logging with optional file output""" | |
| handlers = [logging.StreamHandler()] | |
| # Only add file handler if writable and not disabled | |
| log_file = os.environ.get('ARDUINO_FRAMEWORK_LOG_FILE') | |
| if log_file: | |
| with suppress(OSError, PermissionError): | |
| handlers.append(logging.FileHandler(log_file)) | |
| logging.basicConfig( | |
| level=logging.INFO, | |
| format='%(asctime)s - %(levelname)s - %(message)s', | |
| handlers=handlers | |
| ) | |
| def setup_logging(): | |
| """Setup logging with optional file output""" | |
| handlers = [logging.StreamHandler()] | |
| # Only add file handler if writable and not disabled | |
| log_file = os.environ.get('ARDUINO_FRAMEWORK_LOG_FILE') | |
| if log_file: | |
| with suppress(OSError, PermissionError): | |
| # Ensure log directory exists | |
| log_dir = os.path.dirname(log_file) | |
| if log_dir and not os.path.exists(log_dir): | |
| os.makedirs(log_dir, exist_ok=True) | |
| handlers.append(logging.FileHandler(log_file)) | |
| logging.basicConfig( | |
| level=logging.INFO, | |
| format='%(asctime)s - %(levelname)s - %(message)s', | |
| handlers=handlers | |
| ) |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py around lines 57 to 72, the setup_logging
function adds a file handler without validating the log file path. To fix this,
before appending the FileHandler, check if the directory for the log file exists
and create it if it does not. Use os.path.dirname to get the directory and
os.makedirs with exist_ok=True to create it safely. This ensures the log file
can be created without errors due to missing directories.
| def safe_delete_file(file_path: Union[str, Path], | ||
| force: bool = False) -> bool: | ||
| """ | ||
| Secure file deletion | ||
|
|
||
| Args: | ||
| file_path: Path to file to be deleted | ||
| force: Forces deletion even for write-protected files | ||
|
|
||
| Returns: | ||
| bool: True if successfully deleted | ||
| """ | ||
| file_path = Path(file_path) | ||
|
|
||
| try: | ||
| # Check existence | ||
| if not file_path.exists(): | ||
| logging.warning(f"File does not exist: {file_path}") | ||
| return False | ||
|
|
||
| # Remove write protection if necessary | ||
| if force and not os.access(file_path, os.W_OK): | ||
| file_path.chmod(0o666) | ||
|
|
||
| # Delete file | ||
| file_path.unlink() | ||
| logging.info(f"File deleted: {file_path}") | ||
| return True | ||
|
|
||
| except PermissionError: | ||
| logging.error(f"No permission to delete: {file_path}") | ||
| return False | ||
| except Exception as e: | ||
| logging.error(f"Error deleting {file_path}: {e}") | ||
| return False | ||
|
|
There was a problem hiding this comment.
Add path validation before deletion
The safe_delete_file function doesn't validate the path before deletion, which could lead to accidental deletion of important files.
Add validation before deletion:
def safe_delete_file(file_path: Union[str, Path],
force: bool = False) -> bool:
"""
Secure file deletion
Args:
file_path: Path to file to be deleted
force: Forces deletion even for write-protected files
Returns:
bool: True if successfully deleted
"""
file_path = Path(file_path)
+ # Validate path before deletion
+ if not validate_platformio_path(file_path):
+ logging.error(f"Path validation failed: {file_path}")
+ return False
+
try:
# Check existence
if not file_path.exists():
logging.warning(f"File does not exist: {file_path}")
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def safe_delete_file(file_path: Union[str, Path], | |
| force: bool = False) -> bool: | |
| """ | |
| Secure file deletion | |
| Args: | |
| file_path: Path to file to be deleted | |
| force: Forces deletion even for write-protected files | |
| Returns: | |
| bool: True if successfully deleted | |
| """ | |
| file_path = Path(file_path) | |
| try: | |
| # Check existence | |
| if not file_path.exists(): | |
| logging.warning(f"File does not exist: {file_path}") | |
| return False | |
| # Remove write protection if necessary | |
| if force and not os.access(file_path, os.W_OK): | |
| file_path.chmod(0o666) | |
| # Delete file | |
| file_path.unlink() | |
| logging.info(f"File deleted: {file_path}") | |
| return True | |
| except PermissionError: | |
| logging.error(f"No permission to delete: {file_path}") | |
| return False | |
| except Exception as e: | |
| logging.error(f"Error deleting {file_path}: {e}") | |
| return False | |
| def safe_delete_file(file_path: Union[str, Path], | |
| force: bool = False) -> bool: | |
| """ | |
| Secure file deletion | |
| Args: | |
| file_path: Path to file to be deleted | |
| force: Forces deletion even for write-protected files | |
| Returns: | |
| bool: True if successfully deleted | |
| """ | |
| file_path = Path(file_path) | |
| # Validate path before deletion | |
| if not validate_platformio_path(file_path): | |
| logging.error(f"Path validation failed: {file_path}") | |
| return False | |
| try: | |
| # Check existence | |
| if not file_path.exists(): | |
| logging.warning(f"File does not exist: {file_path}") | |
| return False | |
| # Remove write protection if necessary | |
| if force and not os.access(file_path, os.W_OK): | |
| file_path.chmod(0o666) | |
| # Delete file | |
| file_path.unlink() | |
| logging.info(f"File deleted: {file_path}") | |
| return True | |
| except PermissionError: | |
| logging.error(f"No permission to delete: {file_path}") | |
| return False | |
| except Exception as e: | |
| logging.error(f"Error deleting {file_path}: {e}") | |
| return False |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py around lines 143 to 178, the safe_delete_file
function lacks validation of the file path before deletion, risking accidental
removal of important files. Add a validation step before attempting deletion to
ensure the file_path is within an allowed directory or matches expected
patterns. Implement checks to prevent deletion of critical system or application
files by verifying the path against a whitelist or base directory constraint,
and only proceed with deletion if the validation passes.
| def install_python_deps(): | ||
| def _get_installed_pip_packages(): | ||
| result = {} | ||
| packages = {} | ||
| pip_output = subprocess.check_output( | ||
| [ | ||
| env.subst("$PYTHONEXE"), | ||
| "-m", | ||
| "pip", | ||
| "list", | ||
| "--format=json", | ||
| "--disable-pip-version-check", | ||
| ] | ||
| ) | ||
| try: | ||
| pip_output = subprocess.check_output([ | ||
| env.subst("$PYTHONEXE"), | ||
| "-m", "pip", "list", "--format=json", "--disable-pip-version-check" | ||
| ]) | ||
| packages = json.loads(pip_output) | ||
| except: | ||
| for p in packages: | ||
| result[p["name"]] = pepver_to_semver(p["version"]) | ||
| except Exception: | ||
| print("Warning! Couldn't extract the list of installed Python packages.") | ||
| return {} | ||
| for p in packages: | ||
| result[p["name"]] = pepver_to_semver(p["version"]) | ||
|
|
||
|
|
||
| return result | ||
|
|
||
| deps = { | ||
| "wheel": ">=0.35.1", | ||
| "rich-click": ">=1.8.6", | ||
| "zopfli": ">=0.2.2", | ||
| "esp-idf-size": ">=1.6.1" | ||
| } | ||
|
|
||
| installed_packages = _get_installed_pip_packages() | ||
| packages_to_install = [] | ||
| for package, spec in deps.items(): | ||
| if package not in installed_packages: | ||
| packages_to_install.append(package) | ||
| else: | ||
| version_spec = semantic_version.Spec(spec) | ||
| if not version_spec.match(installed_packages[package]): | ||
| packages_to_install.append(package) | ||
| packages_to_install = list(get_packages_to_install(python_deps, installed_packages)) | ||
|
|
||
| if packages_to_install: | ||
| packages_str = " ".join(f'"{p}{python_deps[p]}"' for p in packages_to_install) | ||
| env.Execute( | ||
| env.VerboseAction( | ||
| ( | ||
| '"$PYTHONEXE" -m pip install -U -q -q -q ' | ||
| + " ".join( | ||
| [ | ||
| '"%s%s"' % (p, deps[p]) | ||
| for p in packages_to_install | ||
| ] | ||
| ) | ||
| ), | ||
| f'"$PYTHONEXE" -m pip install -U -q -q -q {packages_str}', | ||
| "Installing Arduino Python dependencies", | ||
| ) | ||
| ) | ||
| return | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling specificity
The broad except Exception clause could hide important errors like network issues or permission problems.
Be more specific about expected exceptions:
- except Exception:
+ except (subprocess.CalledProcessError, json.JSONDecodeError, KeyError) as e:
print("Warning! Couldn't extract the list of installed Python packages.")
+ if env.get("VERBOSE"):
+ print(f"Error details: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def install_python_deps(): | |
| def _get_installed_pip_packages(): | |
| result = {} | |
| packages = {} | |
| pip_output = subprocess.check_output( | |
| [ | |
| env.subst("$PYTHONEXE"), | |
| "-m", | |
| "pip", | |
| "list", | |
| "--format=json", | |
| "--disable-pip-version-check", | |
| ] | |
| ) | |
| try: | |
| pip_output = subprocess.check_output([ | |
| env.subst("$PYTHONEXE"), | |
| "-m", "pip", "list", "--format=json", "--disable-pip-version-check" | |
| ]) | |
| packages = json.loads(pip_output) | |
| except: | |
| for p in packages: | |
| result[p["name"]] = pepver_to_semver(p["version"]) | |
| except Exception: | |
| print("Warning! Couldn't extract the list of installed Python packages.") | |
| return {} | |
| for p in packages: | |
| result[p["name"]] = pepver_to_semver(p["version"]) | |
| return result | |
| deps = { | |
| "wheel": ">=0.35.1", | |
| "rich-click": ">=1.8.6", | |
| "zopfli": ">=0.2.2", | |
| "esp-idf-size": ">=1.6.1" | |
| } | |
| installed_packages = _get_installed_pip_packages() | |
| packages_to_install = [] | |
| for package, spec in deps.items(): | |
| if package not in installed_packages: | |
| packages_to_install.append(package) | |
| else: | |
| version_spec = semantic_version.Spec(spec) | |
| if not version_spec.match(installed_packages[package]): | |
| packages_to_install.append(package) | |
| packages_to_install = list(get_packages_to_install(python_deps, installed_packages)) | |
| if packages_to_install: | |
| packages_str = " ".join(f'"{p}{python_deps[p]}"' for p in packages_to_install) | |
| env.Execute( | |
| env.VerboseAction( | |
| ( | |
| '"$PYTHONEXE" -m pip install -U -q -q -q ' | |
| + " ".join( | |
| [ | |
| '"%s%s"' % (p, deps[p]) | |
| for p in packages_to_install | |
| ] | |
| ) | |
| ), | |
| f'"$PYTHONEXE" -m pip install -U -q -q -q {packages_str}', | |
| "Installing Arduino Python dependencies", | |
| ) | |
| ) | |
| return | |
| def install_python_deps(): | |
| def _get_installed_pip_packages(): | |
| result = {} | |
| try: | |
| pip_output = subprocess.check_output([ | |
| env.subst("$PYTHONEXE"), | |
| "-m", "pip", "list", "--format=json", "--disable-pip-version-check" | |
| ]) | |
| packages = json.loads(pip_output) | |
| for p in packages: | |
| result[p["name"]] = pepver_to_semver(p["version"]) | |
| except (subprocess.CalledProcessError, json.JSONDecodeError, KeyError) as e: | |
| print("Warning! Couldn't extract the list of installed Python packages.") | |
| if env.get("VERBOSE"): | |
| print(f"Error details: {e}") | |
| return result | |
| installed_packages = _get_installed_pip_packages() | |
| packages_to_install = list(get_packages_to_install(python_deps, installed_packages)) | |
| if packages_to_install: | |
| packages_str = " ".join(f'"{p}{python_deps[p]}"' for p in packages_to_install) | |
| env.Execute( | |
| env.VerboseAction( | |
| f'"$PYTHONEXE" -m pip install -U -q -q -q {packages_str}', | |
| "Installing Arduino Python dependencies", | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py around lines 371 to 398, the current code uses
a broad except Exception clause when trying to get the list of installed pip
packages, which can mask important errors. Replace the broad except with more
specific exceptions such as subprocess.CalledProcessError, json.JSONDecodeError,
and possibly OSError to handle expected failure modes explicitly. This will
improve error handling clarity and avoid hiding critical issues.
| # Cache class for frequently used paths | ||
| class PathCache: | ||
| def __init__(self, platform, mcu): | ||
| self.platform = platform | ||
| self.mcu = mcu | ||
| self._framework_dir = None | ||
| self._sdk_dir = None | ||
|
|
||
| @property | ||
| def framework_dir(self): | ||
| if self._framework_dir is None: | ||
| self._framework_dir = self.platform.get_package_dir("framework-arduinoespressif32") | ||
| return self._framework_dir | ||
|
|
||
| @property | ||
| def sdk_dir(self): | ||
| if self._sdk_dir is None: | ||
| self._sdk_dir = fs.to_unix_path( | ||
| join(self.framework_dir, "tools", "esp32-arduino-libs", self.mcu, "include") | ||
| ) | ||
| return self._sdk_dir | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to PathCache properties
The framework_dir property doesn't handle the case where platform.get_package_dir might return None or raise an exception.
Add error handling:
@property
def framework_dir(self):
if self._framework_dir is None:
- self._framework_dir = self.platform.get_package_dir("framework-arduinoespressif32")
+ try:
+ framework_dir = self.platform.get_package_dir("framework-arduinoespressif32")
+ if not framework_dir:
+ raise ValueError("Framework directory not found")
+ self._framework_dir = framework_dir
+ except Exception as e:
+ logging.error(f"Failed to get framework directory: {e}")
+ raise
return self._framework_dir📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Cache class for frequently used paths | |
| class PathCache: | |
| def __init__(self, platform, mcu): | |
| self.platform = platform | |
| self.mcu = mcu | |
| self._framework_dir = None | |
| self._sdk_dir = None | |
| @property | |
| def framework_dir(self): | |
| if self._framework_dir is None: | |
| self._framework_dir = self.platform.get_package_dir("framework-arduinoespressif32") | |
| return self._framework_dir | |
| @property | |
| def sdk_dir(self): | |
| if self._sdk_dir is None: | |
| self._sdk_dir = fs.to_unix_path( | |
| join(self.framework_dir, "tools", "esp32-arduino-libs", self.mcu, "include") | |
| ) | |
| return self._sdk_dir | |
| # Cache class for frequently used paths | |
| class PathCache: | |
| def __init__(self, platform, mcu): | |
| self.platform = platform | |
| self.mcu = mcu | |
| self._framework_dir = None | |
| self._sdk_dir = None | |
| @property | |
| def framework_dir(self): | |
| if self._framework_dir is None: | |
| try: | |
| framework_dir = self.platform.get_package_dir("framework-arduinoespressif32") | |
| if not framework_dir: | |
| raise ValueError("Framework directory not found") | |
| self._framework_dir = framework_dir | |
| except Exception as e: | |
| logging.error(f"Failed to get framework directory: {e}") | |
| raise | |
| return self._framework_dir | |
| @property | |
| def sdk_dir(self): | |
| if self._sdk_dir is None: | |
| self._sdk_dir = fs.to_unix_path( | |
| join(self.framework_dir, "tools", "esp32-arduino-libs", self.mcu, "include") | |
| ) | |
| return self._sdk_dir |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py around lines 91 to 112, the framework_dir
property does not handle cases where platform.get_package_dir returns None or
raises an exception. Modify the framework_dir property to catch exceptions from
get_package_dir and check if the result is None; if so, raise a clear error
indicating the framework directory could not be found. This ensures that
consumers of PathCache get immediate feedback on missing or inaccessible
framework directories.
| def validate_platformio_path(path: Union[str, Path]) -> bool: | ||
| """ | ||
| Special validation for PlatformIO package paths | ||
| """ | ||
| path = Path(path).resolve() | ||
| path_str = str(path) | ||
|
|
||
| # Must be within .platformio directory structure | ||
| if ".platformio" not in path_str: | ||
| return False | ||
|
|
||
| # Must be a packages directory | ||
| if "packages" not in path_str: | ||
| return False | ||
|
|
||
| # Must be framework-related | ||
| framework_indicators = [ | ||
| "framework-arduinoespressif32", | ||
| "esp32-arduino-libs", | ||
| ".platformio/packages", | ||
| "packages/framework-arduinoespressif32" | ||
| ] | ||
|
|
||
| if not any(indicator in path_str for indicator in framework_indicators): | ||
| return False | ||
|
|
||
| # Must not be a critical system path | ||
| critical_paths = ["/usr", "/bin", "/sbin", "/etc", "/boot"] | ||
| return not any(critical in path_str for critical in critical_paths) | ||
|
|
There was a problem hiding this comment.
Strengthen path validation logic
The current string-based validation could be bypassed with path traversal. For example, a path like /etc/../.platformio/packages/framework-arduinoespressif32 would pass validation but could be dangerous.
Use resolved paths and proper containment checks:
def validate_platformio_path(path: Union[str, Path]) -> bool:
"""
Special validation for PlatformIO package paths
"""
path = Path(path).resolve()
path_str = str(path)
- # Must be within .platformio directory structure
- if ".platformio" not in path_str:
- return False
+ # Get the user's .platformio directory
+ platformio_dir = Path.home() / ".platformio"
+ if not platformio_dir.exists():
+ return False
- # Must be a packages directory
- if "packages" not in path_str:
- return False
+ # Check if path is within .platformio directory
+ try:
+ path.relative_to(platformio_dir)
+ except ValueError:
+ return False
# Must be framework-related
framework_indicators = [
"framework-arduinoespressif32",
"esp32-arduino-libs",
- ".platformio/packages",
- "packages/framework-arduinoespressif32"
]
if not any(indicator in path_str for indicator in framework_indicators):
return False
- # Must not be a critical system path
- critical_paths = ["/usr", "/bin", "/sbin", "/etc", "/boot"]
- return not any(critical in path_str for critical in critical_paths)
+ return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate_platformio_path(path: Union[str, Path]) -> bool: | |
| """ | |
| Special validation for PlatformIO package paths | |
| """ | |
| path = Path(path).resolve() | |
| path_str = str(path) | |
| # Must be within .platformio directory structure | |
| if ".platformio" not in path_str: | |
| return False | |
| # Must be a packages directory | |
| if "packages" not in path_str: | |
| return False | |
| # Must be framework-related | |
| framework_indicators = [ | |
| "framework-arduinoespressif32", | |
| "esp32-arduino-libs", | |
| ".platformio/packages", | |
| "packages/framework-arduinoespressif32" | |
| ] | |
| if not any(indicator in path_str for indicator in framework_indicators): | |
| return False | |
| # Must not be a critical system path | |
| critical_paths = ["/usr", "/bin", "/sbin", "/etc", "/boot"] | |
| return not any(critical in path_str for critical in critical_paths) | |
| def validate_platformio_path(path: Union[str, Path]) -> bool: | |
| """ | |
| Special validation for PlatformIO package paths | |
| """ | |
| path = Path(path).resolve() | |
| path_str = str(path) | |
| # Get the user's .platformio directory | |
| platformio_dir = Path.home() / ".platformio" | |
| if not platformio_dir.exists(): | |
| return False | |
| # Check if path is within .platformio directory | |
| try: | |
| path.relative_to(platformio_dir) | |
| except ValueError: | |
| return False | |
| # Must be framework-related | |
| framework_indicators = [ | |
| "framework-arduinoespressif32", | |
| "esp32-arduino-libs", | |
| ] | |
| if not any(indicator in path_str for indicator in framework_indicators): | |
| return False | |
| return True |
🤖 Prompt for AI Agents
In builder/frameworks/arduino.py around lines 198 to 227, the current path
validation relies on substring checks which can be bypassed using path traversal
sequences. To fix this, use resolved absolute paths and verify containment by
checking if the path is a subdirectory of the expected
.platformio/packages/framework directory structure using proper path operations
instead of string matching. Also, explicitly check that the path does not
resolve to any critical system directories by comparing resolved paths rather
than substrings.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor