IEP-1395: Removing system env vars from IDE while executing scripts #1110
Conversation
WalkthroughThe pull request introduces logging enhancements and environment variable management modifications in the Espressif IDF UI plugin. The changes focus on improving logging in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (1)
Line range hint
407-427: Consider extracting PATH environment variable handling into a helper method.The logic for checking and handling PATH/Path environment variables is duplicated in
addPathToEnvironmentPathandloadIdfPathWithSystemPathmethods. Consider extracting this into a helper method to improve maintainability.Here's a suggested refactor:
+ private String getSystemPath(Map<String, String> environment) { + String pathVar = "PATH"; + String pathEntry = environment.get(pathVar); + if (pathEntry == null) { + pathVar = "Path"; + pathEntry = environment.get(pathVar); + if (pathEntry == null) { + Logger.log(new Exception("No PATH found in the system environment variables")); + } + } + return pathEntry; + } + + private void updateSystemPath(Map<String, String> environment, String newPath) { + if (environment.containsKey("PATH")) { + environment.put("PATH", newPath); + } else { + environment.put("Path", newPath); + } + } private void addPathToEnvironmentPath(Map<String, String> environment, String gitExecutablePath) { IPath gitPath = new org.eclipse.core.runtime.Path(gitExecutablePath); if (gitPath.toFile().exists()) { String gitDir = gitPath.removeLastSegments(1).toOSString(); - String path1 = environment.get("PATH"); - String path2 = environment.get("Path"); - if (!StringUtil.isEmpty(path1) && !path1.contains(gitDir)) { - path1 = gitDir.concat(";").concat(path1); - environment.put("PATH", path1); - } else if (!StringUtil.isEmpty(path2) && !path2.contains(gitDir)) { - path2 = gitDir.concat(";").concat(path2); - environment.put("Path", path2); - } + String path = getSystemPath(environment); + if (!StringUtil.isEmpty(path) && !path.contains(gitDir)) { + updateSystemPath(environment, gitDir.concat(";").concat(path)); + } } }Also applies to: 436-458
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java (1)
78-79: LGTM! Consider adding null check and log level.The added logging enhances visibility into the tools export process, which aligns well with debugging environment variable issues. Consider these minor improvements:
- Logger.log(status.getMessage()); + if (status.getMessage() != null) { + Logger.log(Logger.INFO, status.getMessage()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (2)
407-407: LGTM!The added newline improves code readability.
352-355: Verify the removal of IDF_PATH environment variable.While removing IDE-specific environment variables aligns with the bug fix, the removal of
IDF_PATHneeds verification as it's essential for ESP-IDF tools to locate the framework.Run this script to check if
IDF_PATHis properly set elsewhere:✅ Verification successful
IDF_PATH removal is safe and intentional
The removal of
IDF_PATHfrom the execution environment is part of a deliberate cleanup pattern. The variable is preserved in the build environment andidfToolSet, and is only temporarily removed to ensure a clean execution environment for IDF Python commands.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if IDF_PATH is set in other locations # Look for IDF_PATH assignments in Java files rg -A 3 "IDF_PATH|idfPath" --type java # Look for environment variable setup in Python scripts fd -e py -x rg -A 3 "IDF_PATH|idf_path" {}Length of output: 66522
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java (1)
78-79: Verify error handling scenarios.Let's verify that error scenarios are properly logged when environment variables cause issues.
✅ Verification successful
Error handling is properly implemented.
The code demonstrates comprehensive error handling around environment variables and other operations:
- Status severity checks before proceeding with operations
- Proper error logging and user feedback through console streams
- Early returns to prevent cascading issues
- Exception handling with appropriate logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns around environment variables rg -A 5 "environment|env" --type java # Look for existing error logging patterns ast-grep --pattern 'Logger.log($err)'Length of output: 71426
Script:
#!/bin/bash # Get the complete file content to understand the context cat bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java # Look for error handling patterns in the same file rg -A 5 "catch|error|exception|fail" bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java # Find the Status class usage ast-grep --pattern 'new Status($$$)'Length of output: 26568
| environment.remove(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH); | ||
| environment.remove(IDFEnvironmentVariables.ESP_IDF_VERSION); | ||
| environment.remove(IDFEnvironmentVariables.IDF_PATH); | ||
| environment.remove(IDFEnvironmentVariables.OPENOCD_SCRIPTS); |
There was a problem hiding this comment.
Hi @alirana01,
Do you know why these variables are not replaced with the same ones from the idfToolSet (which, as I understand, should have proper environment variables)?
There was a problem hiding this comment.
So basically the environment here is taken from System.getEnv() so it loads the system env. The function where the change was done is actually before these variables are even part of the idfToolSet. This function make sures that the correct variables from the idf export script are loaded into the idfToolSet basically it sits before that. Which is the reason we need to make sure that any of these variables from System environment are not loaded here
Description
The original bug reported here #1105
Fixes # (IEP-1395)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please try to put the system env vars in windows as mentioned on original reported issues and then try to install tools to test if they are getting installed with correct path in the IDE.
Test Configuration:
Checklist
Summary by CodeRabbit
Logging Improvements
Environment Configuration