IEP-1043: Allow the user to configure IDF_TOOLS_PATH #988
Conversation
|
Warning Rate limit exceeded@alirana01 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 56 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe primary change across the files involves replacing direct system environment variable access ( Changes
Sequence DiagramsUpdated Environment Variable ManagementsequenceDiagram
participant User
participant UI
participant ESPToolChainManager
participant IDFUtil
User->>UI: Trigger action requiring env variables
UI->>ESPToolChainManager: Request to run command
ESPToolChainManager->>IDFUtil: Call getSystemEnv()
IDFUtil-->>ESPToolChainManager: Return environment variables
ESPToolChainManager->>UI: Execute command with env variables
UI-->>User: Display results
Adding IDF Tools Path ConfigurationsequenceDiagram
participant User
participant UI
participant EspresssifPreferencesPage
participant IDFUtil
User->>UI: Open preferences
UI->>EspresssifPreferencesPage: Load preferences page
EspresssifPreferencesPage->>IDFUtil: Retrieve current IDF tools path
IDFUtil-->>EspresssifPreferencesPage: Return IDF tools path
User->>EspresssifPreferencesPage: Set new IDF tools path
EspresssifPreferencesPage->>IDFUtil: Save new IDF tools path
User->>UI: Confirm changes
UI-->>User: Display confirmation
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 as PR comments)
Additionally, you can add 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (2 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (6 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (1 hunks)
Files not summarized due to errors (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java: Error: Server error. Please try again later.
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java: Error: Server error. Please try again later.
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java: Error: Server error. Please try again later.
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties
Additional comments not posted (6)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1)
24-24: Added label for the ESP-IDF tools installation directory.This label will help users identify the new preference for setting the ESP-IDF tools path.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (1)
35-38: Added constants forIDF_TOOLS_PATH.These constants are well-defined and properly differentiate between Windows and other operating systems, which is crucial for path handling.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (3)
42-42: Added a new text field for IDF tools path.This addition is necessary to allow users to specify a custom path for IDF tools directly from the preferences page.
105-127: Setup of label and text field for IDF tools path.The label and text field are correctly configured and added to the GUI layout. The text field is properly initialized with the current or default settings value, ensuring a seamless user experience.
233-233: Proper handling of setting and defaulting the IDF tools path.The code correctly handles the persistence of the IDF tools path across sessions by setting the value in the preference store and initializing it with the default if necessary.
Also applies to: 257-257, 274-274
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (1)
484-487: Correctly set theIDF_TOOLS_PATHin the environment.The environment variable
IDF_TOOLS_PATHis correctly retrieved from the preferences and set for the process environment. This is crucial for ensuring that the tools use the correct directory.
|
@alirana01 hi ! My previous tools were installed in the /TESTTOOL/Espressif folder. I have set this folder as a user variable. Using this PR I have configured new IDF with next(default) tools folder: Once tools are installed, I do see it still execute python from |
| public static final String IDF_TOOLS_PATH = "IDF_TOOLS_PATH"; //$NON-NLS-1$ | ||
| public static final String IDF_TOOLS_PATH_DEFAULT = Platform.getOS().equals(Platform.OS_WIN32) | ||
| ? "%USER_PROFILE\\.espressif" //$NON-NLS-1$ | ||
| : "HOME/.espressif"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Should it be $HOME/.espressif? Is it working on macOS and linux?
There was a problem hiding this comment.
Thanks for pointing out fixed
| idfToolsPathText = new Text(toolsInstallationGroup, SWT.SINGLE | SWT.NONE); | ||
| GridData idfToolsPathTextGridData = new GridData(SWT.FILL, SWT.CENTER, true, false); | ||
| idfToolsPathTextGridData.widthHint = 200; | ||
| idfToolsPathText.setLayoutData(idfToolsPathTextGridData); |
There was a problem hiding this comment.
Can we also provide a Browse button to choose a directory?
|
Hi @AndriiFilippov Please verify this |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (2 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (7 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties (1 hunks)
Additional comments not posted (20)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties (4)
16-16: LGTM!The label for the ESP-IDF tools installation directory is clear and correctly formatted.
17-17: LGTM!The title for the directory selection dialog is clear and correctly formatted.
18-18: LGTM!The message for the directory selection dialog is clear and correctly formatted.
19-19: LGTM!The label for the browse button is clear and correctly formatted.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (4)
24-24: LGTM!The string constant for the ESP-IDF tools installation directory label is clear and correctly formatted.
25-25: LGTM!The string constant for the directory selection message is clear and correctly formatted.
26-26: LGTM!The string constant for the directory selection title is clear and correctly formatted.
27-27: LGTM!The string constant for the browse button label is clear and correctly formatted.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (2)
35-35: LGTM!The constant for the IDF tools path preference key is clear and correctly formatted.
36-38: Verify the default IDF tools path for macOS and Linux.Ensure that the default path works correctly on macOS and Linux as questioned in the previous review comment.
Verification successful
Verify the default IDF tools path for macOS and Linux.
Ensure that the default path
"$HOME/.espressif"works correctly on macOS and Linux by checking if there are any specific platform checks or overrides in the codebase for these operating systems.
The default IDF tools path works correctly on macOS and Linux.
The default path
"$HOME/.espressif"is used universally for non-Windows platforms, including macOS and Linux, without any specific overrides or additional checks in the codebase.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the default IDF tools path for macOS and Linux. # Test: Check if the default path is correctly set based on the operating system. # Expect: The default path should be "$HOME/.espressif" on macOS and Linux. grep -r 'IDF_TOOLS_PATH_DEFAULT' .Length of output: 592
Script:
#!/bin/bash # Description: Verify if there are any platform-specific overrides for the default IDF tools path on macOS and Linux. # Search for any platform-specific checks or overrides in the codebase related to IDF_TOOLS_PATH_DEFAULT. rg 'IDF_TOOLS_PATH_DEFAULT' --context 5Length of output: 4330
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (10)
3-14: LGTM!The import statements for necessary classes are correctly formatted and provide the necessary context.
45-45: LGTM!The Text field for the IDF tools path is correctly formatted and provides a clear field.
91-91: LGTM!The layout for the tools installation group is correctly formatted and provides a clear layout.
100-103: LGTM!The layout data for the git assets and python wheel text fields are correctly formatted and provide clear layout data.
108-113: LGTM!The label and text field for the IDF tools path are correctly formatted and provide clear UI elements.
114-128: LGTM!The browse button for the IDF tools path and its selection listener are correctly formatted and provide clear functionality.
132-144: LGTM!The default values for the preference store are correctly formatted and provide clear default values.
249-250: LGTM!The IDF tools path is correctly saved to the preference store.
274-274: LGTM!The default value for the IDF tools path text field is correctly set.
291-291: LGTM!The default value for the IDF tools path is correctly set in the preference store.
|
@AndriiFilippov please try once the builds are fine, the issue was that the process runner was not resolving the home directory vars automatically. Also the config now should show the resolved value for the user home directory |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (7 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (3 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java
Additional comments not posted (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (2)
107-143: LGTM! Ensure correct initialization and saving of the new field.The changes are well-integrated and enhance the preferences UI. Verify that the new field is correctly initialized and saved in the preferences.
248-249: LGTM!The changes correctly save the new preference for the IDF tools path.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
751-785: LGTM!The changes are well-implemented and correctly handle environment variable resolution for both Windows and Unix platforms.
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | ||
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | ||
|
|
||
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | ||
| idfToolsPath); |
There was a problem hiding this comment.
Handle potential null values and optimize environment variable setting.
Ensure that idfToolsPath is not null before setting it in the environment. Additionally, the environment variable setting can be simplified.
- String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
- IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
-
- environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
+ IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
+ if (idfToolsPath != null) {
+ environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ }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.
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | |
| idfToolsPath); | |
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| if (idfToolsPath != null) { | |
| environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$ | |
| } |
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | ||
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | ||
|
|
||
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | ||
| idfToolsPath); |
There was a problem hiding this comment.
Handle potential null values and optimize environment variable setting.
Ensure that idfToolsPath is not null before setting it in the environment. Additionally, the environment variable setting can be simplified.
- String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
- IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
-
- environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
+ IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
+ if (idfToolsPath != null) {
+ environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ }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.
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | |
| idfToolsPath); | |
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| if (idfToolsPath != null) { | |
| environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$ | |
| } |
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | ||
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | ||
|
|
||
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | ||
| idfToolsPath); |
There was a problem hiding this comment.
Handle potential null values and optimize environment variable setting.
Ensure that idfToolsPath is not null before setting it in the environment. Additionally, the environment variable setting can be simplified.
- String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
- IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
-
- environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
+ IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
+ if (idfToolsPath != null) {
+ environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ }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.
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | |
| idfToolsPath); | |
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| if (idfToolsPath != null) { | |
| environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$ | |
| } |
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | ||
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | ||
|
|
||
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | ||
| idfToolsPath); |
There was a problem hiding this comment.
Handle potential null values and optimize environment variable setting.
Ensure that idfToolsPath is not null before setting it in the environment. Additionally, the environment variable setting can be simplified.
- String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
- IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
-
- environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID,
+ IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null);
+ if (idfToolsPath != null) {
+ environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$
+ }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.
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| environment.put("IDF_TOOLS_PATH", //$NON-NLS-1$ | |
| idfToolsPath); | |
| String idfToolsPath = Platform.getPreferencesService().getString(IDFCorePlugin.PLUGIN_ID, | |
| IDFCorePreferenceConstants.IDF_TOOLS_PATH, IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT, null); | |
| if (idfToolsPath != null) { | |
| environment.put("IDF_TOOLS_PATH", idfToolsPath); //$NON-NLS-1$ | |
| } |
|
@alirana01 hi ! now I could see see logs. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (3 hunks)
- bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (3 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/help/ProductInformationHandler.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (4 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tracing/AppLvlTracingDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (5 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java
Files skipped from review as they are similar to previous changes (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
Additional comments not posted (10)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/help/ProductInformationHandler.java (1)
134-134: Standardize environment variable access.The change replaces
System.getenv()withIDFUtil.getSystemEnv(), which helps in standardizing environment variable access across the project.bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1)
162-162: Standardize environment variable access.The change replaces
System.getenv()withIDFUtil.getSystemEnv(), which helps in standardizing environment variable access across the project.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.java (1)
169-169: Standardize environment variable access.The change replaces
System.getenv()withIDFUtil.getSystemEnv(), which helps in standardizing environment variable access across the project.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java (1)
146-146: Standardize environment variable access.The change replaces
System.getenv()withIDFUtil.getSystemEnv(), which helps in standardizing environment variable access across the project.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
322-322: LGTM!The change to use
IDFUtil.getSystemEnv()enhances consistency and maintainability.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tracing/AppLvlTracingDialog.java (1)
301-301: LGTM!The change to use
IDFUtil.getSystemEnv()enhances consistency and maintainability.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (3)
286-286: LGTM!The change to use
IDFUtil.getSystemEnv()enhances consistency and maintainability.
438-438: LGTM!The change to use
IDFUtil.getSystemEnv()enhances consistency and maintainability.
453-453: LGTM!The change to use
IDFUtil.getSystemEnv()enhances consistency and maintainability.bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (1)
318-318: LGTM!The change to use
IDFUtil.getSystemEnv()enhances consistency and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (3 hunks)
- bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (3 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/help/ProductInformationHandler.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java (7 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (4 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tracing/AppLvlTracingDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (5 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java (1 hunks)
Files skipped from review due to trivial changes (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java
Files skipped from review as they are similar to previous changes (11)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/help/ProductInformationHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tracing/AppLvlTracingDialog.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
Additional comments not posted (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java (4)
10-10: Approved: Necessary import added.The addition of the
Platformimport is required for the new logic.
16-16: Approved: Necessary import added.The addition of the
IDFUtilimport is required for the new logic.
37-37: Approved: New constant added.The addition of the
IDF_TOOLS_PATHconstant is necessary for the new feature.
38-40: Approved: New constant with platform-specific logic added.The addition of the
IDF_TOOLS_PATH_DEFAULTconstant with platform-specific logic ensures the correct default path is set for different operating systems.bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1)
162-162: Approved: Standardized environment variable retrieval.The change to use
IDFUtil.getSystemEnv()instead ofSystem.getenv()improves maintainability and consistency.bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (1)
318-318: Approved: Standardized environment variable retrieval.The change to use
IDFUtil.getSystemEnv()instead ofSystem.getenv()improves maintainability and consistency.
|
@alirana01 hi ! it still takes some python dependencies from user env path during build process: |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
Additional comments not posted (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
540-542: LGTM! Verify the correctness ofstartBuildProcess()method.The addition of the
idfToolsPathEnvVarto the list of environment variables forstartBuildProcess()is appropriate for including the new IDF tools path in the build process.To ensure correctness, verify that
startBuildProcess()correctly handles the environment variables.
416-418: LGTM! Verify the correctness ofIDFUtil.getIDFToolsPathFromPreferences().The addition of the
IDF_TOOLS_PATHenvironment variable to theenvVarslist is appropriate for including the new IDF tools path in the build process.To ensure correctness, verify that
IDFUtil.getIDFToolsPathFromPreferences()returns the expected path.Verification successful
Verification Successful: Definitions of
IDF_TOOLS_PATHandIDF_TOOLS_PATH_DEFAULTare correct.The constants
IDF_TOOLS_PATHandIDF_TOOLS_PATH_DEFAULTare correctly defined inIDFCorePreferenceConstants.java, ensuring that the methodIDFUtil.getIDFToolsPathFromPreferences()returns the expected path.
IDF_TOOLS_PATHis defined as"IDF_TOOLS_PATH".IDF_TOOLS_PATH_DEFAULTresolves to the appropriate path based on the operating system.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `IDFUtil.getIDFToolsPathFromPreferences()` method. # Test: Search for the method definition. Expect: Correct path retrieval logic. ast-grep --lang java --pattern $'public static String getIDFToolsPathFromPreferences() { $$$ }'Length of output: 869
Script:
#!/bin/bash # Description: Verify the correctness of `IDFCorePreferenceConstants.IDF_TOOLS_PATH` and `IDFCorePreferenceConstants.IDF_TOOLS_PATH_DEFAULT`. # Test: Search for the definitions of IDF_TOOLS_PATH and IDF_TOOLS_PATH_DEFAULT in IDFCorePreferenceConstants. ast-grep --lang java --pattern $'public static final String IDF_TOOLS_PATH = $_;' ./bundles/com.espressif.idf.core/src/com/espressif/idf/core/preferences/IDFCorePreferenceConstants.java ast-grep --lang java --pattern $'public static final String IDF_TOOLS_PATH_DEFAULT = $_;' ./bundles/com.espressif.idf.core/src/com/espressif/idf/core/preferences/IDFCorePreferenceConstants.javaLength of output: 683
Script:
#!/bin/bash # Locate the file defining `IDFCorePreferenceConstants` fd 'IDFCorePreferenceConstants.java' # Verify the definitions of `IDF_TOOLS_PATH` and `IDF_TOOLS_PATH_DEFAULT` ast-grep --lang java --pattern $'public static final String IDF_TOOLS_PATH = $_;' $(fd 'IDFCorePreferenceConstants.java') ast-grep --lang java --pattern $'public static final String IDF_TOOLS_PATH_DEFAULT = $_;' $(fd 'IDFCorePreferenceConstants.java')Length of output: 556
Script:
#!/bin/bash # Display the content of `IDFCorePreferenceConstants.java` to manually verify the definitions of `IDF_TOOLS_PATH` and `IDF_TOOLS_PATH_DEFAULT`. cat bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.javaLength of output: 3191
@AndriiFilippov can you please try again with latest build |
|
@alirana01 hi ! Using Still referring to user tool path -> during "ESP-IDF: Project Full Clean" execution: |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java (3 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (1 hunks)
Additional comments not posted (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (3)
28-30: Correct usage ofIDFUtil.getSystemEnv().The method now correctly uses
IDFUtil.getSystemEnv()to set environment variables forProcessBuilder.
37-39: Correct usage ofIDFUtil.getSystemEnv().The method now correctly uses
IDFUtil.getSystemEnv()to set environment variables forProcessBuilder.
46-48: Correct usage ofIDFUtil.getSystemEnv().The method now correctly uses
IDFUtil.getSystemEnv()to set environment variables forProcessBuilder.bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java (1)
Line range hint
131-141: Correct usage ofIDFUtil.getSystemEnv().The method now correctly uses
IDFUtil.getSystemEnv()to set environment variables.
|
@alirana01 hi ! Windows 10 affected (still refer to usr tool path) :
|
|
Hi @alirana01 Could you address the affected functionality as well? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java (2 hunks)
Additional comments not posted (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java (3)
21-21: Import statement looks good.The import statement for
IDFUtilis correctly added and necessary for the changes in the file.
131-131: UsingIDFUtil.getSystemEnv()enhances maintainability.The change to use
IDFUtil.getSystemEnv()centralizes environment variable management, which improves maintainability and consistency.
137-140: Conditional check forIDFCorePreferenceConstants.IDF_TOOLS_PATHis appropriate.The conditional check to skip
IDFCorePreferenceConstants.IDF_TOOLS_PATHensures that this specific environment variable is not included in the final map, preventing potential conflicts.
|
@AndriiFilippov can you verify these things again please make sure to test on other platforms as well by setting the IDF_TOOLS_PATH in system env to some other value |
|
@alirana01 hi ! Tested under: "Espressif" -> Product Information ✅ LGTM 👍 |
Description
A new setting in Espressif preference page is added that can allow the user to configure the idf tools path for installation.
Fixes # (IEP-1043)
Type of change
Please delete options that are not relevant.
How has this been tested?
Configure the property in the Preferences page to something else and install tools they should be installed and builds and basic operations should be working fine.
Test Configuration:
Checklist
Summary by CodeRabbit
Refactor
System.getenv()withIDFUtil.getSystemEnv()across various components for consistent environment variable handling.User Interface
New Features
Bug Fixes