[WIP] Release/v4.0.0 This is just to have the release 4.0 builds for testing (Please dont merge it yet #1217
Conversation
WalkthroughThis update removes the legacy ESP-IDF tools installation, configuration, and management UI and backend logic, replacing it with a new JSON-based system centered around EIM (Espressif Integration Manager). The codebase eliminates preference-based and wizard-driven tool setup, introduces new classes for EIM JSON parsing and monitoring, and updates environment handling to use system defaults. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EclipseIDE
participant EspressifToolStartup
participant ToolInitializer
participant EimIdfConfiguratinParser
participant ESPIDFManagerEditor
User->>EclipseIDE: Start IDE
activate EclipseIDE
EclipseIDE->>EspressifToolStartup: earlyStartup()
EspressifToolStartup->>ToolInitializer: isEimInstalled()
alt EIM not installed
EspressifToolStartup->>User: Show warning dialog with EIM download link
else EIM installed
EspressifToolStartup->>EimIdfConfiguratinParser: getEimJson()
alt No ESP-IDF set
EspressifToolStartup->>User: Prompt to open ESP-IDF Manager
User-->>EspressifToolStartup: Accept/Decline
alt Accept
EspressifToolStartup->>EclipseIDE: Open ESPIDFManagerEditor with EimJson
end
end
EspressifToolStartup->>EimJsonStateChecker: wasModifiedSinceLastRun()
alt Modified
EspressifToolStartup->>User: Show info dialog about EIM JSON change
end
EspressifToolStartup->>EimJsonWatchService: addEimJsonChangeListener()
end
deactivate EclipseIDE
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: 28
🧹 Nitpick comments (24)
.github/workflows/ci.yml (1)
43-45: Remove trailing spaces
There are trailing spaces on thedistributionandcachelines. Cleaning these up improves file hygiene.- distribution: 'temurin' - cache: 'maven' + distribution: 'temurin' + cache: 'maven'🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 43-43: trailing spaces
(trailing-spaces)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties (1)
1-7: Message keys naming may need adjustment.The message keys have the prefix "InstallToolsHandler_" but according to the PR summary, the
InstallToolsHandlerclass has been replaced with a newSetupToolsInIdeclass. Consider renaming the message keys to reflect the new class name for better maintainability.-InstallToolsHandler_CopyingOpenOCDRules=Copying OpenOCD Rules -InstallToolsHandler_OpenOCDRulesCopyPaths=Copying File: %s to destination: %s -InstallToolsHandler_OpenOCDRulesCopyWarning=Warning -InstallToolsHandler_OpenOCDRulesCopyWarningMessage=The rules file is already present in the /etc/udev/rules.d/ directory, Do you want to replace the file with the file from OpenOCD directory -InstallToolsHandler_OpenOCDRulesNotCopied=Rules Not Copied to system -InstallToolsHandler_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory, try running the eclipse with sudo command -InstallToolsHandler_OpenOCDRulesCopied=Rules Copied to system +SetupToolsInIde_CopyingOpenOCDRules=Copying OpenOCD Rules +SetupToolsInIde_OpenOCDRulesCopyPaths=Copying File: %s to destination: %s +SetupToolsInIde_OpenOCDRulesCopyWarning=Warning +SetupToolsInIde_OpenOCDRulesCopyWarningMessage=The rules file is already present in the /etc/udev/rules.d/ directory, Do you want to replace the file with the file from OpenOCD directory +SetupToolsInIde_OpenOCDRulesNotCopied=Rules Not Copied to system +SetupToolsInIde_OpenOCDRulesCopyError=Unable to copy rules for OpenOCD to system directory, try running the eclipse with sudo command +SetupToolsInIde_OpenOCDRulesCopied=Rules Copied to systembundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
56-62: Integration with new EIM JSON-based configuration model.The changes correctly integrate the editor with the new JSON-driven configuration workflow by passing the EimJson object to the ESPIDFMainTablePage singleton and triggering initial setup when appropriate.
Consider adding null checks for the editor input to prevent potential NullPointerExceptions:
-ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage - .getInstance(((EimEditorInput) getEditorInput()).getEimJson()); +IEditorInput input = getEditorInput(); +if (input instanceof EimEditorInput) { + EimEditorInput eimInput = (EimEditorInput) input; + ESPIDFMainTablePage espidfMainTablePage = ESPIDFMainTablePage + .getInstance(eimInput.getEimJson()); + espidfMainTablePage.createPage(parent); + if (eimInput.isFirstStartup()) { + espidfMainTablePage.setupInitialEspIdf(); + } +}tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java (1)
31-41: Test refactored to use the new ESP-IDF Manager editor workflowThe test has been updated to use the new EIM-centric workflow, which aligns with the broader refactoring of the tool management system. Instead of manually interacting with installation dialogs, it now works with the ESP-IDF Manager editor directly.
However, the timeout value set for waiting is extremely large (99000000 ms ≈ 27.5 hours).
Consider using a more reasonable timeout value or making it configurable:
-TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 99000000); +TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 300000); // 5 minute timeoutAlternatively, you could define the timeout as a constant with a descriptive name:
+private static final int TOOLS_SETUP_TIMEOUT_MS = 300000; // 5 minute timeout -TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, 99000000); +TestWidgetWaitUtility.waitUntilViewContains(bot, "Tools Setup complete", consoleView, TOOLS_SETUP_TIMEOUT_MS);bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (2)
15-15: Consider using a mutable list for initializationThe field is initialized with an immutable list via
Collections.emptyList(). This could causeUnsupportedOperationExceptionif any code attempts to directly modify this list without first replacing it through the setter.- private List<IdfInstalled> idfInstalled = Collections.emptyList(); + private List<IdfInstalled> idfInstalled = new ArrayList<>();
37-40: Consider returning a defensive copy of the listReturning the list reference directly allows external code to modify the internal state of this object, which could lead to unexpected behavior in a multi-threaded environment or when the object needs to maintain invariants.
public List<IdfInstalled> getIdfInstalled() { - return idfInstalled; + return new ArrayList<>(idfInstalled); }bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java (2)
7-7: Use Path API for file path handlingString concatenation for path construction may not handle path separators correctly across all systems. Consider using the
java.nio.file.PathAPI which provides better cross-platform path handling.- String EIM_POSIX_DIR = System.getProperty("user.home").concat("/.espressif/tools/"); //$NON-NLS-1$//$NON-NLS-2$ + String EIM_POSIX_DIR = Paths.get(System.getProperty("user.home"), ".espressif", "tools").toString() + File.separator; //$NON-NLS-1$//$NON-NLS-2$//$NON-NLS-3$
3-21: Add documentation for constantsThe interface lacks documentation explaining the purpose and usage of each constant. This makes maintenance more difficult and can lead to misuse of these constants.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (1)
12-12: Fix spelling in class nameThere is a typo in the class name: "Configuratin" should be "Configuration".
-public class EimIdfConfiguratinParser +public class EimIdfConfigurationParserbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (1)
1-4: Update copyright yearThe copyright statement shows year 2025, which is in the future. Update to the current year (2024).
-/******************************************************************************* - * Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. - * Use is subject to license terms. - *******************************************************************************/ +/******************************************************************************* + * Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved. + * Use is subject to license terms. + *******************************************************************************/bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (3)
1-3: Update copyright year to match current year.The copyright year is set to 2025, which is in the future. It should be updated to the current year 2024.
-/******************************************************************************* - * Copyright 2025 Espressif Systems (Shanghai) PTE LTD. All rights reserved. - * Use is subject to license terms. +/******************************************************************************* + * Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved. + * Use is subject to license terms.
33-52: Add preference flushing when detecting first run.When no last seen timestamp is found (first run), the method should update the timestamp to avoid repeatedly detecting it as a first run if Eclipse crashes before properly updating the timestamp later.
if (lastSeen == 0L) { // First run ever, don't treat as changed Logger.log("eim_idf.json detected, but no last seen timestamp — assuming first run."); //$NON-NLS-1$ + // Set initial timestamp to avoid repeatedly detecting first run + preferences.putLong(LAST_MODIFIED_PREF_KEY, lastModified); + try { + preferences.flush(); + } catch (BackingStoreException e) { + Logger.log(e); + } return false; }
47-47: Consider using message constants for logging messages.The hardcoded log message should ideally use a message constant from a Messages class for proper internationalization.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java (3)
5-5: Remove unused import.The ArrayList import is not used anywhere in the code and should be removed.
-import java.text.MessageFormat; -import java.util.ArrayList; +import java.text.MessageFormat;
17-17: Typo in class name EimIdfConfiguratinParser.There's a spelling error in the class name (missing 'o' in 'Configuration').
-import com.espressif.idf.core.tools.EimIdfConfiguratinParser; +import com.espressif.idf.core.tools.EimIdfConfigurationParser;This should be fixed in the class name itself and all references to it.
46-57: Simplify EimJson initialization and improve error handling.The initialization of the empty EimJson object is redundant since you immediately try to populate it. Also, you should log more information about the error when JSON parsing fails.
-EimJson eimJson = new EimJson(); +EimJson eimJson; try { EimIdfConfiguratinParser eimIdfConfiguratinParser = new EimIdfConfiguratinParser(); eimJson = eimIdfConfiguratinParser.getEimJson(true); } catch (IOException e) { - Logger.log(e); + Logger.log("Failed to parse EIM JSON configuration: " + e.getMessage(), e); // Proceed with an empty EimJson object + eimJson = new EimJson(); }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (2)
72-85: Enhance equals method to consider EimJson content.The current implementation only compares the FILE_NAME constant, which means all instances of EimEditorInput will be equal regardless of their EimJson content. This could lead to unexpected behavior when multiple editors are open.
@Override public boolean equals(Object obj) { if (this == obj) { return true; } if (obj == null || getClass() != obj.getClass()) { return false; } EimEditorInput that = (EimEditorInput) obj; - return FILE_NAME.equals(that.FILE_NAME); + // Since FILE_NAME is constant, also compare EimJson content if available + if (this.eimJson == null && that.eimJson == null) { + return true; + } + return (this.eimJson != null && that.eimJson != null && + this.eimJson.getIdfSelectedId() != null && + this.eimJson.getIdfSelectedId().equals(that.eimJson.getIdfSelectedId()));
87-91: Update hashCode to be consistent with equals.If you update the equals method to consider EimJson content, the hashCode method should also be updated to maintain the contract that equal objects have equal hash codes.
@Override public int hashCode() { - return FILE_NAME.hashCode(); + final int prime = 31; + int result = FILE_NAME.hashCode(); + result = prime * result + ((eimJson == null || eimJson.getIdfSelectedId() == null) ? 0 : + eimJson.getIdfSelectedId().hashCode()); + return result; }bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java (1)
164-170: Consider using Map.Entry for enhanced type safety.Instead of using the raw Entry type, consider using Map.Entry for better type safety.
- for (Entry<String, String> entry : idfEnvironmentVariables.getEnvMap().entrySet()) + for (Map.Entry<String, String> entry : idfEnvironmentVariables.getEnvMap().entrySet())bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (1)
89-92: Consider providing a method to update the ESP-IDF set flag.The class provides a method to check if ESP-IDF is set, but no method to update this flag. This would be useful for classes that need to modify this state.
public boolean isEspIdfSet() { return preferences.getBoolean(EimConstants.INSTALL_TOOLS_FLAG, false); } + + public void setEspIdfSet(boolean value) + { + preferences.putBoolean(EimConstants.INSTALL_TOOLS_FLAG, value); + }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java (1)
83-92: Null-check listener & remove redundant@SuppressWarnings
launchBarListenercan benullif events are fired beforehookLaunchBarListeners()finishes registering – rare, but possible during early start-up.
Also, thestatic-accesssuppression is unnecessary; we are calling an instance method.-@SuppressWarnings("static-access") -private void disableLaunchBarEvents() -{ - launchBarListener.setIgnoreTargetChange(true); -} +private void disableLaunchBarEvents() +{ + if (launchBarListener != null) + launchBarListener.setIgnoreTargetChange(true); +}Apply the same pattern to
enableLaunchBarEvents()and drop the@SuppressWarnings.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
148-151: Typo in progress message
"Installing pyhton dependency web-socket"→"Installing Python dependency websocket-client".bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
106-122: Blocking browser launch on UI thread
java.awt.Desktop.getDesktop().browse()may block while the platform launches the external browser.
Triggering this directly inside the SWT selection listener freezes the UI if the operation takes noticeable time, especially on Linux/Wayland.
Wrap the call inDisplay.getDefault().asyncExec()or, better, run it in a background job and useProgram.launch()from SWT which is non-blocking.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
95-100: Possible NPE when comparing installation IDs
isIdfInstalledActivedoes
idfInstalled.getId().equals(espIdfIdEim).
WhengetId()returnsnull, this throws.
UseObjects.equals(idfInstalled.getId(), espIdfIdEim)to be safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bundles/com.espressif.idf.ui/icons/tools/delete.pngis excluded by!**/*.png
📒 Files selected for processing (76)
.github/workflows/ci.yml(2 hunks).github/workflows/ci_windows.yml(1 hunks)bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties(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/tools/vo/EimJson.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonChangeListener.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java(3 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(3 hunks)bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.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/OSGI-INF/l10n/bundle.properties(0 hunks)bundles/com.espressif.idf.ui/plugin.xml(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(0 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/handlers/NewProjectHandlerUtil.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java(0 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/install/GitRepositoryBuilder.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java(0 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/EspressifToolStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(13 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java(1 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(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java(0 hunks)tests/com.espressif.idf.ui.test/configs/default-test-linux.properties(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java(0 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java(2 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(1 hunks)tests/pom.xml(1 hunks)
💤 Files with no reviewable changes (30)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/GitRepositoryBuilder.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java
🧰 Additional context used
🧬 Code Graph Analysis (14)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
ESPIDFMainTablePage(56-557)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (1)
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
ProjectTestOperations(50-656)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (13)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
Messages(15-42)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (1)
ToolInitializer(26-93)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-47)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
EimJsonStateChecker(22-67)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (1)
EimJsonWatchService(23-152)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(58-889)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/MessageLinkDialog.java (1)
MessageLinkDialog(40-135)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/EclipseHandler.java (1)
EclipseHandler(21-79)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
ESPIDFManagerEditor(22-78)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
EimEditorInput(9-93)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (1)
EimJsonUiChangeHandler(23-44)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-47)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-47)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
SetupToolsInIde(53-564)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
ESPIDFMainTablePage(56-557)bundles/com.espressif.idf.core/src/com/espressif/idf/core/resources/OpenDialogListenerSupport.java (1)
OpenDialogListenerSupport(9-22)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (1)
Messages(9-63)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
Messages(15-42)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/ProcessBuilderFactory.java (1)
ProcessBuilderFactory(16-163)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(58-889)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-47)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (7)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java (1)
EimIdfConfiguratinParser(12-47)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
SetupToolsInIde(53-564)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
ToolsUtility(25-101)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-47)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java (1)
IdfInstalled(5-80)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/IDFConsole.java (1)
IDFConsole(27-100)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (1)
SetupToolsJobListener(28-88)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java (1)
IdfInstalled(5-80)
🪛 YAMLlint (1.35.1)
.github/workflows/ci_windows.yml
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
.github/workflows/ci.yml
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
[error] 43-43: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
19-19: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
25-25: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_macos
- GitHub Check: build_macos
- GitHub Check: build_linux
🔇 Additional comments (55)
.github/workflows/ci_windows.yml (1)
8-14: Approve expansion of CI triggers
Addingrelease/**alongsidemasterfor bothpushandpull_requestensures CI runs on release branches as intended.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
.github/workflows/ci.yml (3)
17-20: Job renamed tobuild_linuxand custom runners
The job name change frombuildtobuild_linuxis clear and descriptive. Confirm that the self-hosted runner labelseclipseandBrnoUBU0004are defined in your Actionlint configuration or consider replacing them with standard labels to avoid runner-label validation errors.🧰 Tools
🪛 actionlint (1.7.4)
19-19: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
29-33: Approve use ofespressif/install-esp-idf-action@v1
Switching from manual checkout of ESP-IDF to the officialinstall-esp-idf-action@v1streamlines environment setup and aligns with the new EIM-based tool management.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
8-14: Approve trigger expansion for release branches
Includingrelease/**in the trigger patterns matches the Windows workflow and ensures consistent CI coverage for release branches.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
tests/com.espressif.idf.ui.test/configs/default-test-linux.properties (1)
10-10:❓ Verification inconclusive
Verify updated ESP-IDF test path
The path fordefault.env.esp.idf.pathnow points todependencies/idf-tools/v5.3/esp-idf. Ensure that theinstall-esp-idf-action@v1step installs ESP-IDF to this exact location. You can add a CI check:
🏁 Script executed:
#!/bin/bash if [ ! -d "$GITHUB_WORKSPACE/dependencies/idf-tools/v5.3/esp-idf" ]; then echo "ERROR: ESP-IDF path not found at expected location" exit 1 fiLength of output: 173
Confirm ESP-IDF installation path in CI
We’ve updateddefault.env.esp.idf.pathto point atdependencies/idf-tools/v5.3/esp-idf. Make sure yourinstall-esp-idf-action@v1step installs ESP-IDF into exactly that directory under$GITHUB_WORKSPACE. For example, add a CI check:#!/bin/bash ESP_IDF_DIR="${GITHUB_WORKSPACE}/dependencies/idf-tools/v5.3/esp-idf" if [ ! -d "$ESP_IDF_DIR" ]; then echo "ERROR: ESP-IDF not found at $ESP_IDF_DIR" exit 1 fi• File: tests/com.espressif.idf.ui.test/configs/default-test-linux.properties
• Line: 10 (default.env.esp.idf.path)Please verify this path in your workflow and adjust if needed.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java (2)
54-54: Addition of ESP_IDF_EIM_ID constant looks good.This new environment variable constant is consistent with the naming pattern and will support the EIM-based configuration approach.
133-133: Simplified environment map initialization.The code now uses
System.getenv()directly instead of a utility method, which aligns with the broader environment variable handling refactoring in the codebase. This approach is more straightforward and maintains consistency with the changes made in related classes.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java (1)
1-27: Class structure follows NLS pattern correctly.The Messages class follows the standard Eclipse NLS pattern with proper static initialization. If you update the message keys in the properties file as suggested in the previous comment, remember to update the constant names here as well.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java (2)
19-19: Import updated to reflect architectural changes.Replacing the import of
InstallToolsHandlerwithEimConstantsaligns with the broader refactoring of the tool management system.
38-38: Preference key updated for tool installation flag.The change from using
InstallToolsHandler.INSTALL_TOOLS_FLAGtoEimConstants.INSTALL_TOOLS_FLAGaligns with the new EIM-based tool management approach. This ensures consistency with the new setup infrastructure.tests/pom.xml (1)
33-33: System property added for test automationAdding the
-DtestRun=trueflag to the JVM arguments enables conditional behavior in the IDE during automated tests. This is a well-structured approach for controlling UI behavior during test runs.bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1)
48-48: Properly exposing new watcher packageThe addition of
com.espressif.idf.core.tools.watcherto the exported packages list correctly makes the new EIM JSON file monitoring functionality accessible to other bundles in the plugin system.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.java (1)
208-208: Environment variable handling standardizationSwitching to
System.getenv()standardizes environment variable handling across the codebase, aligning with similar changes made throughout the PR. This simplifies the codebase by using Java's standard API directly instead of custom utility methods.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tracing/AppLvlTracingDialog.java (1)
301-301: Consistent environment variable managementUsing
System.getenv()here maintains consistency with similar changes throughout the PR. The approach of creating a HashMap copy still allows for potential environment customization before passing to the process builder if needed.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/help/ProductInformationHandler.java (1)
134-134: Consistent environment variable sourcing approach.The change from custom environment map to using
System.getenv()aligns with the broader standardization of environment variable handling across the codebase. This simplifies the code and reduces maintenance overhead.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (3)
286-287: Standardized environment variable handling.Good change to use the standard Java system environment variables instead of a custom environment map. Creating a mutable HashMap copy is the correct approach since
System.getenv()returns an unmodifiable map.
438-439: Consistent environment variable approach across methods.This change maintains consistency with the previous method by using the same standardized approach to environment variables.
453-454: Completed standardization of environment variable handling.This completes the standardization of environment variable handling in all three methods of this class. The consistent approach improves maintainability.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (3)
29-29: Standardized environment handling for process builders.Good approach using
putAllon the ProcessBuilder's environment map, which is already mutable. This directly incorporates the system environment variables without needing to create an intermediate HashMap.
38-38: Consistent process environment handling.Maintains the same standardized approach used in the previous method. The consistency across related methods is good for maintainability.
47-47: Completed environment standardization for all process methods.This completes the standardization across all three process creation methods in the class. Using
System.getenv()directly is more straightforward than the previous approach.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
582-582: Increased timeout for console output verification.The timeout was increased from 3 seconds to 30 seconds. This change is likely necessary due to the new EIM-based workflow which might take longer to complete operations.
Consider adding a comment explaining why the timeout needed to be increased to help future maintainers understand the rationale behind this change.
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectSDKconfigTest.java (2)
185-185: Same 15-minute timeout applied to context menu method.This method also has the timeout increased to 15 minutes for the same operation, suggesting a systematic change in how SDK configuration loading works in the new architecture.
Are there any plans to improve the performance in future releases so that these extreme timeouts won't be necessary?
178-178:❓ Verification inconclusive
The timeout value has been significantly increased to 15 minutes.
The timeout for waiting for the SDK Configuration tab to appear has been increased from what appears to be 5 seconds to 15 minutes (900,000ms). While this might be necessary for the new EIM-based workflow, such a long timeout could potentially mask underlying performance issues.
Could you:
- Explain why such a long timeout is necessary?
- Confirm whether there's any user feedback during this potentially lengthy wait?
- Consider if there might be a better approach, such as waiting for a specific event rather than using a fixed timeout?
🏁 Script executed:
#!/bin/bash # Check for any logging or feedback mechanism during the wait period grep -r "waitForSDKConfigurationTab" --include="*.java" . grep -r "TestWidgetWaitUtility" --include="*.java" .Length of output: 8896
🏁 Script executed:
#!/bin/bash # Show implementation and any logging inside waitForSDKConfigurationTab grep -R -n -C5 "waitForSDKConfigurationTab" tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.javaLength of output: 492
🏁 Script executed:
#!/bin/bash # Retrieve full implementation of waitForSDKConfigurationTab sed -n '273,330p' tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/common/utility/TestWidgetWaitUtility.javaLength of output: 931
Excessive maximum timeout in SDK Configuration test
The two calls to
TestWidgetWaitUtility.waitForSDKConfigurationTab(bot, 900_000);raise the max wait from a few seconds to 15 minutes. While
waitUntilis reactive (it ends as soon as the tab is active), a 15-minute ceiling can mask hangs or environment issues and there’s no intermediate feedback during that period.Could you:
- Explain why a 15-minute maximum is required under the new EIM workflow?
- Confirm how the test runner or user sees progress (any logs or UI cues) while waiting?
- Consider reducing the ceiling, adding interim logging, or surfacing a “loading…” indicator so failures don’t stall pipelines for so long?
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)
480-481:❓ Verification inconclusive
Removal of IDF_TOOLS_PATH from environment variables.
The code no longer explicitly passes the
IDF_TOOLS_PATHenvironment variable to the build process. This aligns with the broader changes in environment variable handling where standard system environment variables are used instead of custom environment maps.Can you confirm that the build process will still correctly access the
IDF_TOOLS_PATHthrough the system environment? This change seems to be part of a larger refactoring effort, but it's important to ensure the build functionality remains intact.
🏁 Script executed:
#!/bin/bash # Check for other uses of IDF_TOOLS_PATH in the codebase grep -r "IDF_TOOLS_PATH" --include="*.java" . # Check how environment variables are now handled grep -r "System.getenv" --include="*.java" . | grep -i "idf"Length of output: 5874
I want to confirm how
startBuildProcessconstructs the process environment—whether it merges the full system environment or only uses the provided vars. Let’s inspect its implementation:
🏁 Script executed:
#!/bin/bash grep -n -C3 "startBuildProcess" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.javaLength of output: 1031
Let’s inspect how
startBuildProcessis implemented—in particular, whether it merges the full system environment with the supplied vars or only uses the latter.
🏁 Script executed:
#!/bin/bash grep -n -C5 "startBuildProcess" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/CMakeBuildConfiguration.javaLength of output: 250
I need to inspect the
startBuildProcessimplementation inIDFBuildConfiguration.javato confirm how it handles environment variables:
🏁 Script executed:
#!/bin/bash grep -n -C10 "startBuildProcess" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.javaLength of output: 2925
I need to locate the
startBuildProcessmethod definition to verify how it merges (or replaces) environment variables:
🏁 Script executed:
#!/bin/bash # Find the method signature for startBuildProcess in IDFBuildConfiguration.java grep -R -n "Process startBuildProcess" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.javaLength of output: 131
Let’s search the entire repo for the
startBuildProcessdefinition to locate its implementation:
🏁 Script executed:
#!/bin/bash grep -R -n "startBuildProcess" .Length of output: 598
I need to identify the class hierarchy to locate
startBuildProcess. Let’s list the build package and inspect theIDFBuildConfigurationdeclaration:
🏁 Script executed:
#!/bin/bash echo "Build package files:" ls bundles/com.espressif.idf.core/src/com/espressif/idf/core/build echo echo "IDFBuildConfiguration declaration:" grep -n "class IDFBuildConfiguration" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.javaLength of output: 769
Confirmation needed: ensure
IDF_TOOLS_PATHis picked up from the system environmentWe’ve removed the explicit passing of
IDF_TOOLS_PATHto the build process—now onlyPYTHONUNBUFFERED(and any other vars) are passed tostartBuildProcess. SincestartBuildProcessis inherited fromCBuildConfiguration, please verify that:
CBuildConfiguration.startBuildProcess(...)mergesSystem.getenv()into the child process’s environment rather than replacing it.- Under normal usage,
IDF_TOOLS_PATHis present inSystem.getenv()when the build runs.- No code path calls
startBuildProcess(...)with an empty or minimal env array without first seeding it with system vars.Without this confirmation, builds may lose access to the tools in
IDF_TOOLS_PATH.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonChangeListener.java (1)
7-17: Well-designed interface for the new EIM architecture.This interface follows good design principles with a focused single responsibility, clear method signature, and descriptive naming. It will serve as an important component in the new EIM JSON-driven workflow, allowing components to react to configuration changes.
bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1)
158-158: Environment variable handling standardized to use System.getenv()This change aligns with the broader refactoring across the codebase to use
System.getenv()directly instead of custom environment maps viaIDFUtil.getSystemEnv(). This simplifies environment handling and creates more consistent behavior.bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (2)
180-181: Improved directory filtering to prevent potential issuesThe refactored method now properly filters out non-existent paths and non-directory entries, which is a good defensive programming practice. This will prevent potential issues when these paths are used to find toolchains later in the code.
319-319: Environment variable handling standardized to use System.getenv()This change aligns with the broader refactoring across the codebase to use
System.getenv()directly instead of custom environment maps fromIDFUtil. This creates more consistent behavior in environment variable handling.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java (1)
144-147: Environment variable handling standardizedThe code now uses
System.getenv()directly instead of a custom environment map fromIDFUtil.getSystemEnv(). This aligns with the broader refactoring of environment variable handling across the codebase.Also note that the removal of the
IDF_GITHUB_ASSETSandPIP_EXTRA_INDEX_URLenvironment variables (which were previously set from preferences) is part of the cleanup of deprecated or unused environment configuration related to tool installation URLs.bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (3)
37-39: Appropriate message constants added for EIM-related notifications.These new message constants properly support the EIM-based workflow by providing standardized messages for notifying users when EIM tooling is missing.
42-44: LGTM - Clear notification messages for ESP-IDF workspace state.These message constants appropriately inform users when there's no active ESP-IDF in the workspace, which is essential for the new EIM-centric workflow.
45-52: Well-structured message constants for configuration migration.These constants provide a complete set of messages for guiding users through the old configuration export process, covering all necessary dialog states (title, selection, success, and failure messages).
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java (4)
28-37: Class correctly refactored to work with SetupToolsInIde.The class has been properly renamed and updated to store a reference to the SetupToolsInIde job it's listening to. The constructor parameters and member initialization align with the new architecture.
53-60: Good addition of rollback mechanism for error handling.The listener now correctly checks the job result severity and invokes rollback on the SetupToolsInIde job if it failed, which provides a clean recovery mechanism for installation failures.
61-68: Successfully updated preference flag to use EimConstants.The code correctly uses the new EimConstants.INSTALL_TOOLS_FLAG instead of the removed InstallToolsHandler constant, properly aligning with the refactored architecture.
79-86: Improved UI thread handling with async execution.Moving the UI refresh operations to run asynchronously on the UI thread is a good practice that prevents blocking the job thread and improves responsiveness.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java (1)
1-80: Well-structured POJO for ESP-IDF installation representation.This is a clean, well-structured value object class that represents an installed ESP-IDF instance. The Gson
@Exposeannotations properly mark fields for JSON serialization/deserialization, which aligns with the JSON-driven configuration approach mentioned in the PR summary.bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1)
22-41: Good user guidance messages for ESP-IDF installation and configuration.These new message keys provide clear guidance to users about ESP-IDF installation status, configuration migration, and actions needed. The messages are well-structured with placeholders for dynamic content and include HTML formatting for hyperlinks where appropriate.
bundles/com.espressif.idf.ui/plugin.xml (1)
546-550: Updated startup classes to match new architecture.The change replaces the previous startup classes with new
EspressifGeneralStartupandEspressifToolStartupclasses. This aligns with the PR's objective to refactor the tool management system and move to a JSON-driven configuration approach.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (1)
5-12: Simplified UI messages focused on ESP-IDF Manager.The changes streamline the message keys to focus on the ESP-IDF Manager UI elements and notifications, replacing the previous detailed installation messages. This reflects the architectural shift from wizard-driven tool installation to a JSON-based configuration model.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
212-235: Good addition of overloaded method for Python environment path resolution.This new overloaded method for resolving Python environment paths is well-implemented and supports the new EIM-based workflow. It properly handles platform-specific subdirectories and has appropriate fallback logic.
802-803: Updated to use standard system environment.The change to use
System.getenv()directly instead of a custom environment map is consistent with the PR's goal of standardizing environment variable handling across the codebase.bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java (2)
95-127: Well-refactored process launching with improved error handling.The process launching code has been nicely refactored to use a more modular approach with better encapsulation:
- Environment setup is extracted to dedicated methods
- Arguments are built incrementally for better readability
- The ProcessBuilderFactory encapsulates process creation
- Exception handling is broadened to catch all exceptions
These changes improve maintainability and make the code more robust.
134-139: Good method extraction for environment preparation.Extracting environment setup into a dedicated method improves code organization and reusability.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (6)
1-44: Well-designed file watcher implementation with proper threading.The EimJsonWatchService is well implemented with:
- Proper daemon thread setup
- Platform-specific directory path detection
- Appropriate watch event registration
- Clean initialization with logging
46-67: Good singleton pattern implementation.The singleton pattern is implemented using the thread-safe initialization-on-demand holder idiom, which is a best practice for singleton creation in Java.
69-80: Thread-safe listener collection implemented correctly.Using CopyOnWriteArrayList for listeners and null-checking before adding listeners is a good practice for concurrent access patterns.
82-127: Well-implemented event processing with proper error handling.The run method contains proper exception handling for:
- InterruptedException with thread interruption
- ClosedWatchServiceException
- OVERFLOW event filtering
- Watch key validity checking
The pattern matching syntax for context objects is modern and clean.
128-138: Good resource cleanup in the watch service.The code properly closes the WatchService in a try-catch block, which is essential for resource management.
140-151: Well-designed thread termination methods.The interrupt and shutdown methods properly set the running flag to false before calling super.interrupt(), ensuring clean thread termination.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (3)
1-36: Well-structured initialization class with clear responsibilities.The ToolInitializer class is well-designed with appropriate field declarations, constructor, and clear purpose as indicated in the class javadoc.
60-69: Good file operation handling with exception propagation.The exportOldConfigIfNeeded method correctly:
- Checks for file existence before attempting to copy
- Uses Files.copy with appropriate StandardCopyOption
- Propagates IOException to caller for proper handling
- Updates preferences to track the export operation
82-87:❓ Verification inconclusive
Consider more robust path handling for cross-platform compatibility.
The isEimIdfJsonPresent method relies on platform-specific path constants. Consider adding a unit test to verify this works correctly across different operating systems.
🏁 Script executed:
#!/bin/bash # Check if the platform-specific path constants are defined correctly echo "Checking EimConstants path definitions..." ast-grep --pattern $'String EIM_WIN_PATH = $_;$\nString EIM_POSIX_PATH = $_;'Length of output: 170
🏁 Script executed:
#!/bin/bash # Locate the EimConstants class and inspect the path constants echo "Finding EimConstants class file..." EC_FILE=$(rg -l "class EimConstants") if [ -z "$EC_FILE" ]; then echo "EimConstants class not found." exit 1 fi echo "Found EimConstants at: $EC_FILE" echo echo "EIM_WIN_PATH definition:" grep -n "EIM_WIN_PATH" "$EC_FILE" echo echo "EIM_POSIX_PATH definition:" grep -n "EIM_POSIX_PATH" "$EC_FILE"Length of output: 213
Strengthen cross-platform path resolution and add tests
I wasn’t able to locate the
EimConstantsdefinitions in the repo, so please confirm thatEIM_WIN_PATHandEIM_POSIX_PATHare defined correctly (and cover all supported OS variants). In addition:
Consider using
java.nio.file.Path/PathsandFiles.exists(...)instead of hard-coded string constants to build the path (e.g.,Path eimJson = Paths.get(System.getProperty("user.home"), ".espressif", "idf.json"); return Files.exists(eimJson);).
Add a lightweight unit test (e.g. with JUnit and System rules or Mockito) to simulate different
Platform.getOS()values and verify that the detection logic resolves the expected file locations on both Windows and POSIX.This will make maintenance easier and ensure consistent behavior across environments.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
23-31: Remember to add the new keys to the properties bundle
EspIdfManagerNameCol,IDFGuideLinkLabel_Text, and the new EIM-JSON messages must exist incom.espressif.idf.ui.tools.messages.properties; otherwiseNLSwill log missing-key errors at runtime.
| branches: | ||
| - master | ||
| - release/** | ||
| pull_request: | ||
| branches: [ master ] | ||
| branches: | ||
| - master | ||
| - release/** |
There was a problem hiding this comment.
Fix YAML indentation under triggers
Indentation of list items under both push.branches and pull_request.branches is off by one space (5 vs. 6), which can lead to malformed YAML or unexpected behavior.
Proposed patch:
on:
push:
- branches:
- - master
- - release/**
+ branches:
+ - master
+ - release/**
pull_request:
- branches:
- - master
- - release/**
+ branches:
+ - master
+ - release/**📝 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.
| branches: | |
| - master | |
| - release/** | |
| pull_request: | |
| branches: [ master ] | |
| branches: | |
| - master | |
| - release/** | |
| on: | |
| push: | |
| branches: | |
| - master | |
| - release/** | |
| pull_request: | |
| branches: | |
| - master | |
| - release/** |
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
| - name: Set up Python | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: '3.10' | ||
| python-version: '3.10' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update Python setup action to latest version
The workflow uses actions/setup-python@v2, which is outdated. Align with other workflows and benefit from bug fixes by upgrading to:
- uses: actions/setup-python@v2
+ uses: actions/setup-python@v4📝 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.
| - name: Set up Python | |
| uses: actions/setup-python@v2 | |
| with: | |
| python-version: '3.10' | |
| python-version: '3.10' | |
| - name: Set up Python | |
| uses: actions/setup-python@v4 | |
| with: | |
| python-version: '3.10' |
🧰 Tools
🪛 actionlint (1.7.4)
25-25: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - name: Install ESP-IDF via eim | ||
| uses: espressif/install-esp-idf-action@v1 | ||
| with: | ||
| version: 'v5.3' | ||
|
|
There was a problem hiding this comment.
Remove tab characters to fix YAML syntax error
Line 29 contains tab characters, which YAML does not allow. Replace tabs with spaces:
- uses: espressif/install-esp-idf-action@v1 # contains tabs
+ uses: espressif/install-esp-idf-action@v1 # tabs → spaces📝 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.
| - name: Install ESP-IDF via eim | |
| uses: espressif/install-esp-idf-action@v1 | |
| with: | |
| version: 'v5.3' | |
| - name: Install ESP-IDF via eim | |
| uses: espressif/install-esp-idf-action@v1 | |
| with: | |
| version: 'v5.3' |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
| branches: | ||
| - master | ||
| - release/** | ||
| pull_request: | ||
| branches: [ master ] | ||
| branches: | ||
| - master | ||
| - release/** |
There was a problem hiding this comment.
Fix YAML indentation under triggers
The list items for push.branches and pull_request.branches are mis-indented (5 spaces instead of 6). Correcting this will prevent YAML parsing issues.
on:
push:
- branches:
- - master
- - release/**
+ branches:
+ - master
+ - release/**
pull_request:
- branches:
- - master
- - release/**
+ branches:
+ - master
+ - release/**🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
|
|
||
| String EIM_POSIX_DIR = System.getProperty("user.home").concat("/.espressif/tools/"); //$NON-NLS-1$//$NON-NLS-2$ | ||
|
|
||
| String EIM_WIN_DIR = "C:\\Espressif\\tools\\"; //$NON-NLS-1$ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding installation paths
The Windows installation directory is hardcoded, which may not respect user customizations or non-standard installation locations. Consider using environment variables or configuration settings to allow for customization.
| { | ||
| removeColumn.getColumn().dispose(); | ||
| } | ||
|
|
||
| if (nameColumn != null && nameColumn.getColumn() != null) | ||
| { | ||
| nameColumn.getColumn().dispose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential Colour leak when columns are recreated
setupColumns() disposes TableViewerColumns but the associated ColumnLabelProvider (which owns an SWT Color) is not disposed until the viewer is disposed.
Because setupColumns() can be invoked repeatedly (refreshEditorUI()), many Color objects can accumulate.
Add labelProvider.dispose() before the column is disposed, or cache/re-use a single label-provider instance.
| case 1: | ||
| cell.setText(idfToolSet.getIdfVersion()); | ||
| cell.setText(getIdfVersion(idfInstalled)); | ||
| break; | ||
| case 2: | ||
| cell.setText(idfToolSet.getIdfLocation()); | ||
| cell.setText(idfInstalled.getName()); | ||
| break; | ||
| case 3: | ||
| cell.setText(idfInstalled.getPath()); | ||
| break; | ||
| case 4: | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| private String getIdfVersion(IdfInstalled idfInstalled) | ||
| { | ||
| String version = ToolsUtility.getIdfVersion(idfInstalled, eimJson.getGitPath()); | ||
| return version; | ||
| } |
There was a problem hiding this comment.
Heavy external process executed on UI thread for every cell refresh
getIdfVersion() launches a shell, executes the activation script and waits for completion.
Because update() runs in the SWT UI thread, the table freezes while every row (and every repaint) spawns a new process – catastrophic for responsiveness on large lists.
Cache the version once per IdfInstalled (e.g., in a Map<IdfInstalled,String>), and/or compute it in a background Job then update the viewer asynchronously.
| public static ESPIDFMainTablePage getInstance(EimJson eimJson) | ||
| { | ||
| if (espidfMainTablePage == null) | ||
| { | ||
| espidfMainTablePage = new ESPIDFMainTablePage(); | ||
| } | ||
|
|
||
| ESPIDFMainTablePage.eimJson = eimJson; | ||
| return espidfMainTablePage; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Singleton keeps mutable global state – risk of stale data & race-conditions
eimJson is held in a static field that is overwritten every time getInstance() is called.
Because the method is not synchronized, concurrent calls from UI thread / background jobs can race, and previously opened editors might suddenly see a different JSON instance than they were created with.
Consider either
- removing the static field and passing the current
EimJsonthrough explicit method parameters, or - synchronising access and cloning the model to avoid unexpected mutation.
| nameColumn = new TableViewerColumn(tableViewer, SWT.NONE); | ||
| nameColumn.getColumn().setText(Messages.EspIdfManagerNameCol); | ||
| nameColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider()); | ||
| setComparatorForCols(nameColumn, colIndex++); | ||
| tableColumnLayout.setColumnData(nameColumn.getColumn(), new ColumnWeightData(3, 50, true)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Comparator index/order mismatch leads to wrong sorting
The comparator is told that versionColumn is index 0, nameColumn is index 1 and locationColumn is index 2, but the compare() implementation (lines 530-545) assumes 0 → name, 1 → path, 2 → active‐state.
Clicking “Version” currently sorts by name, etc.
Patch either the index assignment or the switch logic.
A minimal fix is shown below.
@@
- switch (propertyIndex)
+ switch (propertyIndex)
{
- case 0: // Version column
- rc = ToolsUtility.getIdfVersion(p1, eimJson.getGitPath())
- .compareTo(
- ToolsUtility.getIdfVersion(p2, eimJson.getGitPath()));
+ case 0: // Version column
+ rc = ToolsUtility
+ .getIdfVersion(p1, eimJson.getGitPath())
+ .compareTo(
+ ToolsUtility.getIdfVersion(p2, eimJson.getGitPath()));
break;
- case 1:
- rc = p1.getPath().compareTo(p2.getPath()); // Name column
+ case 1: // Name column
+ rc = p1.getName().compareTo(p2.getName());
break;
- case 2:
- Boolean p1State = ToolsUtility.isIdfInstalledActive(p1); // Location column
- Boolean p2State = ToolsUtility.isIdfInstalledActive(p2);
- rc = p1State.compareTo(p2State);
+ case 2: // Location column
+ rc = p1.getPath().compareTo(p2.getPath());
break;
}📝 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.
| nameColumn = new TableViewerColumn(tableViewer, SWT.NONE); | |
| nameColumn.getColumn().setText(Messages.EspIdfManagerNameCol); | |
| nameColumn.setLabelProvider(new IdfManagerTableColumnLabelProvider()); | |
| setComparatorForCols(nameColumn, colIndex++); | |
| tableColumnLayout.setColumnData(nameColumn.getColumn(), new ColumnWeightData(3, 50, true)); | |
| // … somewhere inside your compare(p1, p2, propertyIndex) method … | |
| switch (propertyIndex) | |
| { | |
| case 0: // Version column | |
| rc = ToolsUtility | |
| .getIdfVersion(p1, eimJson.getGitPath()) | |
| .compareTo( | |
| ToolsUtility.getIdfVersion(p2, eimJson.getGitPath())); | |
| break; | |
| case 1: // Name column | |
| rc = p1.getName().compareTo(p2.getName()); | |
| break; | |
| case 2: // Location column | |
| rc = p1.getPath().compareTo(p2.getPath()); | |
| break; | |
| } |
| Files.copy(tararchiveinputstream, pathEntryOutput, StandardCopyOption.REPLACE_EXISTING); | ||
| Runtime.getRuntime().exec("/bin/chmod 755 ".concat(pathEntryOutput.toString())); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| } | ||
|
|
||
| tararchiveinputstream.close(); | ||
| process.waitFor(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| public static void extractTarXz(String tarFile, String outputDir) | ||
| { | ||
| Path pathOutput = Paths.get(outputDir); | ||
| Map<Path, Path> symLinks = new HashMap<>(); | ||
| Map<Path, Path> hardLinks = new HashMap<>(); | ||
| try | ||
| { | ||
| FileInputStream fileInputStream = new FileInputStream(tarFile); | ||
| BufferedInputStream bufferedInputStream = new BufferedInputStream(fileInputStream); | ||
| XZInputStream xzInputStream = new XZInputStream(bufferedInputStream); | ||
| TarArchiveInputStream tararchiveinputstream = new TarArchiveInputStream(xzInputStream); | ||
| TarArchiveEntry archiveentry = null; | ||
| while ((archiveentry = tararchiveinputstream.getNextTarEntry()) != null) | ||
| { | ||
| Path pathEntryOutput = pathOutput.resolve(archiveentry.getName()); | ||
| if (archiveentry.isSymbolicLink()) | ||
| { | ||
| symLinks.put(pathEntryOutput, | ||
| pathOutput.resolve(archiveentry.getName()).getParent().resolve(archiveentry.getLinkName())); | ||
| continue; | ||
| } | ||
| else if (archiveentry.isLink()) | ||
| { | ||
| hardLinks.put(pathEntryOutput, | ||
| pathOutput.resolve(archiveentry.getLinkName())); | ||
| continue; | ||
| } | ||
| else if (archiveentry.isDirectory()) | ||
| { | ||
| if (!Files.exists(pathEntryOutput)) | ||
| Files.createDirectories(pathEntryOutput); | ||
| } | ||
| else | ||
| { | ||
| System.out.println(pathEntryOutput.toString() + " " + archiveentry.getSize()); //$NON-NLS-1$ | ||
| Files.copy(tararchiveinputstream, pathEntryOutput, StandardCopyOption.REPLACE_EXISTING); | ||
| Runtime.getRuntime().exec("/bin/chmod 755 ".concat(pathEntryOutput.toString())); //$NON-NLS-1$ | ||
| } | ||
| } | ||
| tararchiveinputstream.close(); | ||
| xzInputStream.close(); | ||
| fileInputStream.close(); | ||
| hardLinks.forEach(ToolsUtility::createHardLinks); | ||
| symLinks.forEach(ToolsUtility::createSymLinks); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| private static void createHardLinks(Path link, Path target) | ||
| { | ||
| try | ||
| { | ||
| Files.deleteIfExists(link); | ||
| Files.createLink(link, target); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| private static void createSymLinks(Path link, Path target) | ||
| { | ||
| try | ||
| { | ||
| Files.deleteIfExists(link); | ||
| Files.createSymbolicLink(link, target.toRealPath()); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| public static String getReadableSizeMB(double size) | ||
| { | ||
| size /= 1024; // KB | ||
| size /= 1024; // MB | ||
| DecimalFormat df = new DecimalFormat("0.00"); //$NON-NLS-1$ | ||
| return String.valueOf(df.format(size)).concat(" MB"); //$NON-NLS-1$ | ||
| return espIdfVersion; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-safety & early-exit checks missing
activationScript and idfInstalled fields are assumed non-null. If the JSON is malformed, activationScript can be null, causing NPEs in new File(...) or ProcessBuilder.
Guard with:
if (idfInstalled == null || idfInstalled.getActivationScript() == null) {
return null;
}
There was a problem hiding this comment.
Hi @alirana01,
Thanks for the PR! It looks good overall. I’ve left a few comments with suggestions we can check together.
| private void prepEnvMap(Map<String, String> env) | ||
| { | ||
| env.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| env.put("IDF_CCACHE_ENABLE", "false"); //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
Why do we need to set IDF_CCACHE_ENABLE to false here? I believe it will cause a redundant build if IDF_CCACHE_ENABLE is set to true in the environment variables.
| } | ||
| catch (CoreException e) | ||
| catch (Exception e) |
There was a problem hiding this comment.
I think throwing a generic Exception is not a good practice. It's better to be as specific as possible with exception handling to make the code more robust and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (13)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
120-134: Potential NPE when workbench window is not yet availablebundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (4)
100-108: Modified environment is ignored when running activation script
257-258: Environment dump to log may leak secrets
496-503: Duplicate keys inparseEnvKeyswill crash withIllegalStateException
543-562: Null-safe handling & Windows path separatorbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (4)
83-92:⚠️ Potential issueDuplicate Issue: Static mutable state still poses concurrency risks
The static
eimJsonfield continues to create the same concurrency and stale data issues identified in previous reviews. The field is overwritten on everygetInstance()call without synchronization, and previously opened editors may see different JSON instances than they were created with.
274-279:⚠️ Potential issueComparator logic still incorrect after column reordering
The comparator index/order mismatch persists. The
versionColumnis assigned index 0,nameColumnindex 1, andlocationColumnindex 2, but thecompare()method still assumes different mappings, causing incorrect sorting behavior.Apply this fix to align the comparator with the actual column order:
switch (propertyIndex) { case 0: - rc = p1.getName().compareTo(p2.getName()); + rc = ToolsUtility.getIdfVersion(p1, eimJson.getGitPath()) + .compareTo(ToolsUtility.getIdfVersion(p2, eimJson.getGitPath())); break; case 1: - rc = p1.getPath().compareTo(p2.getPath()); + rc = p1.getName().compareTo(p2.getName()); break; case 2: - Boolean p1State = ToolsUtility.isIdfInstalledActive(p1); - Boolean p2State = ToolsUtility.isIdfInstalledActive(p2); - rc = p1State.compareTo(p2State); + rc = p1.getPath().compareTo(p2.getPath()); break;
392-396:⚠️ Potential issueUI thread blocking issue unresolved
The
getIdfVersion()method still executes heavy external processes on the UI thread during table cell updates, causing UI freezing as previously identified. This affects table responsiveness especially with multiple IDF installations.Cache the version results or compute them in background jobs to avoid blocking the UI thread.
248-256:⚠️ Potential issueResource leak: Column label providers not disposed
The color resource leak issue remains unresolved. While
TableViewerColumns are disposed indisposeColumns(), their associatedColumnLabelProviders (which own SWTColorobjects) are not disposed until the viewer is disposed, potentially causing resource accumulation.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
27-66:⚠️ Potential issueNull safety checks still missing
The null safety issues identified in previous reviews remain unaddressed. The method assumes
idfInstalledandactivationScriptare non-null, which can cause NPEs if the JSON is malformed.Add the recommended null safety guards:
public static String getIdfVersion(IdfInstalled idfInstalled, String gitPath) { + if (idfInstalled == null || idfInstalled.getActivationScript() == null) { + return null; + } String activationScript = idfInstalled.getActivationScript();.github/workflows/ci_windows.yml (1)
8-10: Fix YAML indentation under triggersThe
branches:key and its list items under bothpushandpull_requestare mis-indented by one space, which will cause the workflow to fail parsing.Suggested patch:
on: push: - branches: - - master - - release/** + branches: + - master + - release/** pull_request: - branches: - - master - - release/** + branches: + - master + - release/**Also applies to: 12-14
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
.github/workflows/ci.yml (2)
8-10: Fix YAML indentation under triggersJust like in the Windows CI, the
branches:key and its items underpushandpull_requestneed an extra two spaces of indentation to be valid YAML.on: push: - branches: - - master - - release/** + branches: + - master + - release/** pull_request: - branches: - - master - - release/** + branches: + - master + - release/**Also applies to: 12-14
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
29-29:⚠️ Potential issueRemove trailing tab characters
Line ends with tab characters, which YAML does not allow. Trim to:- - name: Install ESP-IDF via eim + - name: Install ESP-IDF via eim🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
106-122: Consider error handling for desktop browse operationThe desktop browse operation has basic exception handling but doesn't provide user feedback when the operation fails.
Consider showing a user-friendly error message:
catch (Exception ex) { Logger.log(ex); + // Consider showing user notification about failed browser launch }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bundles/com.espressif.idf.ui/icons/tools/delete.pngis excluded by!**/*.png
📒 Files selected for processing (75)
.github/workflows/ci.yml(2 hunks).github/workflows/ci_windows.yml(1 hunks)bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties(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/tools/vo/EimJson.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonChangeListener.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java(3 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(3 hunks)bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.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/OSGI-INF/l10n/bundle.properties(0 hunks)bundles/com.espressif.idf.ui/plugin.xml(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(0 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/handlers/NewProjectHandlerUtil.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java(0 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/install/GitRepositoryBuilder.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java(0 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/EspressifToolStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(13 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java(1 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(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java(0 hunks)tests/com.espressif.idf.ui.test/configs/default-test-linux.properties(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java(0 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java(2 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(1 hunks)tests/pom.xml(1 hunks)
💤 Files with no reviewable changes (30)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/GitRepositoryBuilder.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java
✅ Files skipped from review due to trivial changes (4)
- bundles/com.espressif.idf.ui/plugin.xml
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.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
🚧 Files skipped from review as they are similar to previous changes (33)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/help/ProductInformationHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java
- bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java
- tests/pom.xml
- tests/com.espressif.idf.ui.test/configs/default-test-linux.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonChangeListener.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java
- bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (12)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
Messages(15-42)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (1)
ToolInitializer(26-93)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-47)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
EimJsonStateChecker(22-67)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (1)
EimJsonWatchService(23-152)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/MessageLinkDialog.java (1)
MessageLinkDialog(40-135)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/EclipseHandler.java (1)
EclipseHandler(21-79)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
ESPIDFManagerEditor(22-78)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
EimEditorInput(9-93)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (1)
EimJsonUiChangeHandler(23-44)
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
19-19: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
25-25: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
[error] 43-43: trailing spaces
(trailing-spaces)
.github/workflows/ci_windows.yml
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build_macos
🔇 Additional comments (9)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (1)
8-8:⚠️ Potential issueFix typo: "pyhton" → "python"
-IDFToolsHandler_ToolsManagerConsole=Espressif IDF Tools Console +IDFToolsHandler_ToolsManagerConsole=Espressif IDF Tools ConsoleWait, that's not the right line. Let me check line 148 in SetupToolsInIde.java where the typo appears to originate from.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
124-140:✅ Verification successful
Verify preference scope and key usage
The initial setup logic uses
InstanceScopeandEimConstants.INSTALL_TOOLS_FLAG- ensure this preference key is properly documented and the scope is appropriate for this use case.
🏁 Script executed:
#!/bin/bash # Description: Verify the usage of INSTALL_TOOLS_FLAG preference key across the codebase # Expected: Find where this flag is set/reset to understand the lifecycle rg -A 3 -B 3 "INSTALL_TOOLS_FLAG"Length of output: 4676
🏁 Script executed:
#!/bin/bash # Inspect SetupToolsJobListener to see how scopedPreferenceStore is initialized sed -n '1,200p' bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.javaLength of output: 2661
Preference scope and key usage confirmed
TheINSTALL_TOOLS_FLAGconstant is defined inEimConstantsand consistently used by:
ESPIDFMainTablePage(checks flag before setup)SetupToolsJobListener(sets/clears flag on job completion)NewProjectHandlerUtil(reads flag when creating new projects)ToolInitializer(reads/clears flag in core)All modules use
InstanceScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID), making the workspace-wide scope and key usage appropriate. No changes needed.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2)
68-93: Platform detection logic is completeThe command construction correctly handles Windows, Linux, and defaults to zsh for other platforms (likely macOS). The implementation looks solid.
95-100: Active IDF detection implementation is correctThe method properly uses the new
ESP_IDF_EIM_IDenvironment variable to determine if an IDF installation is active. The logic is straightforward and correct.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
15-42: Message class properly structuredThe
Messagesclass follows standard Eclipse NLS patterns with proper bundle initialization and private constructor. The reduced message set aligns well with the simplified UI design..github/workflows/ci.yml (4)
17-17: Rename job tobuild_linux
The new job name clearly distinguishes the Linux build from Windows and macOS jobs, improving readability.
19-19: Validate custom runner labels
You’re using[self-hosted, eclipse, BrnoUBU0004]. The labelseclipseandBrnoUBU0004aren’t standard GitHub-hosted runners. Ensure these are correctly registered in your self-hosted runner group.🧰 Tools
🪛 actionlint (1.7.7)
19-19: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
27-27: The Python version change (3.10) is intentional and valid; no action required here.
30-32: Use official ESP-IDF action
Switching toespressif/install-esp-idf-action@v1withversion: 'v5.3'correctly replaces the legacy checkout logic for ESP-IDF. This aligns with the new EIM-based setup.
| monitor.worked(1); | ||
|
|
||
| monitor.setTaskName("Installing pyhton dependency web-socket"); //$NON-NLS-1$ | ||
| log("Installing pyhton dependency web-socket", IStatus.INFO); //$NON-NLS-1$ |
There was a problem hiding this comment.
Fix typo: "pyhton" → "python"
-log("Installing pyhton dependency web-socket", IStatus.INFO); //$NON-NLS-1$
+log("Installing python dependency web-socket", IStatus.INFO); //$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.
| log("Installing pyhton dependency web-socket", IStatus.INFO); //$NON-NLS-1$ | |
| log("Installing python dependency web-socket", IStatus.INFO); //$NON-NLS-1$ |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java
at line 149, correct the typo in the log message by changing "pyhton" to
"python" to ensure the message is spelled correctly.
| setUpToolChainsAndTargets(false); | ||
| monitor.worked(1); | ||
|
|
||
| monitor.setTaskName("Installing pyhton dependency web-socket"); //$NON-NLS-1$ |
There was a problem hiding this comment.
Fix typo: "pyhton" → "python"
-monitor.setTaskName("Installing pyhton dependency web-socket"); //$NON-NLS-1$
+monitor.setTaskName("Installing python dependency web-socket"); //$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.
| monitor.setTaskName("Installing pyhton dependency web-socket"); //$NON-NLS-1$ | |
| monitor.setTaskName("Installing python dependency web-socket"); //$NON-NLS-1$ |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java
at line 148, correct the typo in the task name string from "pyhton" to "python"
to ensure the message reads "Installing python dependency web-socket".
| distribution: 'temurin' | ||
| cache: 'maven' |
There was a problem hiding this comment.
Remove trailing spaces
The lines for distribution: 'temurin' and cache: 'maven' end with extra spaces, which YAMLlint flags. Trim them:
- distribution: 'temurin'
+ distribution: 'temurin'
- cache: 'maven'
+ cache: 'maven'📝 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.
| distribution: 'temurin' | |
| cache: 'maven' | |
| distribution: 'temurin' | |
| cache: 'maven' |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 43-43: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/ci.yml at lines 43 to 44, remove the trailing spaces at the
end of the lines containing "distribution: 'temurin'" and "cache: 'maven'".
Ensure there are no extra spaces after the values to comply with YAMLlint rules.
| path: dependencies/idf-tools | ||
| submodules: 'true' | ||
| ref: release/v5.1 | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
Correct indentation of checkout step
The - uses: actions/checkout@v4 line is only indented 4 spaces under steps. It should be indented 6 spaces to align with YAML expectations:
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
🤖 Prompt for AI Agents
In .github/workflows/ci.yml at line 22, the indentation of the line '- uses:
actions/checkout@v4' is incorrect; it is currently indented 4 spaces under
'steps' but should be indented 6 spaces to conform to YAML syntax. Adjust the
indentation of this line to 6 spaces so it aligns properly with other step
entries under 'steps'.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (2)
44-47: Add null check for preferences parameter
49-54: Add error handling for UI operationsbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
179-193: Fix potential NPE when workbench window is not available
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
92-130: Consider extracting nested method to improve readabilityThe
handleOldConfigExportmethod has deeply nested logic that could be simplified for better maintainability.private void handleOldConfigExport() { - final int[] response = new int[] { -1 }; - Display display = Display.getDefault(); - display.syncExec(() -> { - MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), - Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, 0, 0, - new String[] { Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, - Messages.ToolsInitializationDifferentPathMessageBoxOptionNo }); - response[0] = messageDialog.open(); - }); - - if (response[0] == 0) + int response = promptUserForOldConfigExport(); + if (response == 0) { - try - { - IStatus status = toolInitializer.exportOldConfig(); - Logger.log("Tools Conversion Process Message: "); - Logger.log(status.getMessage()); - if (status.getSeverity() != IStatus.ERROR) - { - preferences.putBoolean(EimConstants.OLD_CONFIG_EXPORTED_FLAG, true); - displayInformationMessageBox(Messages.OldConfigExportCompleteSuccessMsgTitle, - Messages.OldConfigExportCompleteSuccessMsg); - } - else - { - displayInformationMessageBox(Messages.OldConfigExportCompleteFailMsgTitle, - Messages.OldConfigExportCompleteFailMsg); - } - } - catch (IOException e) - { - Logger.log("Error exporting old configuration", e); - displayInformationMessageBox(Messages.OldConfigExportCompleteFailMsgTitle, - Messages.OldConfigExportCompleteFailMsg); - } + performOldConfigExport(); } } + +private int promptUserForOldConfigExport() +{ + final int[] response = new int[] { -1 }; + Display display = Display.getDefault(); + display.syncExec(() -> { + MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), + Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, 0, 0, + new String[] { Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, + Messages.ToolsInitializationDifferentPathMessageBoxOptionNo }); + response[0] = messageDialog.open(); + }); + return response[0]; +} + +private void performOldConfigExport() +{ + try + { + IStatus status = toolInitializer.exportOldConfig(); + Logger.log("Tools Conversion Process Message: "); + Logger.log(status.getMessage()); + if (status.getSeverity() != IStatus.ERROR) + { + preferences.putBoolean(EimConstants.OLD_CONFIG_EXPORTED_FLAG, true); + displayInformationMessageBox(Messages.OldConfigExportCompleteSuccessMsgTitle, + Messages.OldConfigExportCompleteSuccessMsg); + } + else + { + displayInformationMessageBox(Messages.OldConfigExportCompleteFailMsgTitle, + Messages.OldConfigExportCompleteFailMsg); + } + } + catch (IOException e) + { + Logger.log("Error exporting old configuration", e); + displayInformationMessageBox(Messages.OldConfigExportCompleteFailMsgTitle, + Messages.OldConfigExportCompleteFailMsg); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java
🚧 Files skipped from review as they are similar to previous changes (5)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
🧰 Additional context used
🧬 Code Graph Analysis (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (14)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (1)
Messages(9-63)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (1)
ToolInitializer(30-107)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-59)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
EimJsonStateChecker(22-67)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java (1)
EimJsonWatchService(23-153)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(58-889)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/UIPlugin.java (1)
UIPlugin(22-104)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/MessageLinkDialog.java (1)
MessageLinkDialog(40-135)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/EclipseHandler.java (1)
EclipseHandler(21-79)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java (1)
ESPIDFManagerEditor(22-78)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java (1)
EimEditorInput(9-93)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (1)
EimJsonUiChangeHandler(39-150)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
🔇 Additional comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java (2)
145-149: LGTM!The console stream helper method is correctly implemented.
121-143:⚠️ Potential issueFix potential NPE when accessing workbench window
The method doesn't check if
EclipseHandler.getActiveWorkbenchWindow()returns null before calling methods on it, which could cause a NullPointerException.private void launchEspIdfManager() throws PartInitException { Display.getDefault().asyncExec(() -> { IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); if (activeww == null || activeww.getActivePage() == null) { Logger.log("Cannot open ESP-IDF Manager. No active workbench window or active page."); return; } try { IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), ESPIDFManagerEditor.EDITOR_ID, true); } catch (PartInitException e) { Logger.log("Failed to open ESP-IDF Manager Editor."); Logger.log(e); } }); }Likely an incorrect or invalid review comment.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
50-90: LGTM! Startup initialization logic is well-structuredThe
earlyStartupmethod properly handles the initialization flow with appropriate checks for EIM installation, old config migration, and tool setup.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
23-35: LGTM! Message field additions are consistentThe new message fields follow proper naming conventions and align with the new JSON-driven tool management approach.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (5)
108-109: Environment map created but not passed to the process
addGitToEnvironment(env, …)mutatesenv, yetrunInBackground(…, System.getenv())discards those edits. Use the modified map or the Git binary will stay offPATHon systems that need it.
148-149: Typo: “pyhton” → “python”
String appears twice; fix both to keep UI text professional.
255-258: Full environment dump leaks secrets
Logger.log(environment.toString())prints every variable—including credentials—to the IDE log. Redact or omit sensitive values.
496-503: Duplicate keys inparseEnvKeyswill blow up
Collectors.toMapwithout a merge function throwsIllegalStateExceptionwhen a key (e.g.,PATH) appears twice. Provide a merge strategy ((oldV,newV)->newV).
545-560: Null-safety & platform separator
executablePathmay be null; check before creatingnew Path(). Also useFile.pathSeparatorinstead of hard-coding;, so Unix hosts get “:”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(4 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
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build_linux
| private void loadIdfPathWithSystemPath(Map<String, String> systemEnv) | ||
| { | ||
| String idfExportPath = envVarsFromActivationScriptMap.get(IDFEnvironmentVariables.PATH); | ||
| String pathVar = "PATH"; // for Windows //$NON-NLS-1$ | ||
| String pathEntry = systemEnv.get(pathVar); // $NON-NLS-1$ | ||
| if (pathEntry == null) | ||
| { | ||
| pathVar = "Path"; //$NON-NLS-1$ | ||
| pathEntry = systemEnv.get(pathVar); | ||
| if (pathEntry == null) // no idea | ||
| { | ||
| Logger.log(new Exception("No PATH found in the system environment variables")); //$NON-NLS-1$ | ||
| } | ||
| } | ||
|
|
||
| if (!StringUtil.isEmpty(pathEntry)) | ||
| { | ||
| idfExportPath = idfExportPath.replace("$PATH", pathEntry); // macOS //$NON-NLS-1$ | ||
| idfExportPath = idfExportPath.replace("%PATH%", pathEntry); // Windows //$NON-NLS-1$ | ||
| } | ||
| systemEnv.put(pathVar, idfExportPath); | ||
| for (Entry<String, String> entry : envVarsFromActivationScriptMap.entrySet()) |
There was a problem hiding this comment.
Possible NPE when PATH missing from activation output
idfExportPath = envVarsFromActivationScriptMap.get(PATH) can be null; subsequent replace(...) calls will throw. Guard for null and fail gracefully.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java
around lines 404 to 425, the variable idfExportPath is assigned from
envVarsFromActivationScriptMap.get(IDFEnvironmentVariables.PATH) which can be
null, causing a NullPointerException when calling replace on it. To fix this,
add a null check for idfExportPath before calling replace, and handle the null
case gracefully, such as logging an error or returning early to avoid the NPE.
| while (process.isAlive() && waitCount > 0) | ||
| { | ||
| try | ||
| { | ||
| Thread.sleep(300); | ||
| } | ||
| catch (InterruptedException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| waitCount--; | ||
| } | ||
|
|
||
| if (waitCount == 0) | ||
| { | ||
| log("Process possibly stuck", IStatus.ERROR); //$NON-NLS-1$ | ||
| return Status.CANCEL_STATUS; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Timeout far too short – 3 s can kill legitimate builds
list-targets often takes >3 s on first run. The 10×300 ms spin loop aborts good processes and returns CANCEL_STATUS. Replace with process.waitFor(timeout, unit) using a sensible timeout (e.g. 2 min) or make it user-configurable.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java
around lines 360 to 377, the current timeout loop waits only 3 seconds total by
sleeping 300 ms increments 10 times, which is too short and may cancel
legitimate long-running processes. Replace this loop with a single call to
process.waitFor(timeout, unit) using a longer timeout such as 2 minutes to allow
sufficient time for the process to complete. Optionally, make this timeout
configurable by the user. Remove the manual sleep and waitCount logic and handle
the case where waitFor returns false by logging the error and returning
Status.CANCEL_STATUS.
* Initial integration Initial integration for the eim starting from the start to handle the empty workspace. Tools activation is still to be done along with code cleanup. Just the basic display is working which will also be enhanced. * Initial first flow with successful installation and loading of env in IDE with EIM * constants moved to single place and added refresh function * minor cleanup and logging for user * IEP-1334: code cleanup (Active Review) (#1125) * cleanup of unused classes and code * String internalization * update to ci file * ci error fixed * ci error fix * ci fix * fixing ci error * ci error fix * ci error fix * ci syntax fix * ci syntax indentation fix * ci syntax fix * ci syntax fix * ci fixed * ci fixed2 * ci trigger * workflow trigger fix * cleanup * ci deps isntalled * eim fix * using eim action to install * using Petr's fork * switched back to official action repo * Revert "switched back to official action repo" This reverts commit f8cd7a7. * trying to fix action for eim * trying with petrs fork again * ci fix * back to espressif * name fixed * updated url for eim * old config export handling * fixing tests env setup * logging to verify skip tests * fixing POSIX path resolution * activation script properly sourced * added test run variable in test pom directly * adding cache option for the maven * file name updated * increasing timeout for tests * test fixes and removing redundant and outdated tests * cleanup and update for user messages * updated to handle activation of single esp-idf from eim * removing unwanted test for fixing failures * increased timeout for tests * ci updated for the release branch trigger as well * ci error fix * fix for sdkconfig server * cleaning up the idf_tools_path configuration in the IDE * added the option to include homebrew paths on macos * changes for guide link * Guidance link added to NLS * fix: Launch ESP-IDF Manager view even there is no eim_idf.json file found * fix: Don't launch multiple ESP-IDF Manager editors * fix: Update the msg and convert it to MessageDialog * fix: notify IDF not found while opening Manager view * fix: java.lang.UnsupportedOperationException * fix: File Not found issue and others * updated code for eim watcher added Next commits will do some refactoring and then implement the logic to handle these changes * refactored startup classes * initial base level logic for file watcher So this is very basic first level of file watcher service that watches the file and then also takes care that the file is wasnt changes since last startup. Initial changes only take that the file last modified is verified and just any change in file simply will give you a message box telling that something changed. More advanced and robust checks in upcoming commits * missing copyright added --------- Co-authored-by: Kondal Kolipaka <[email protected]>
* automatic refresh added * switched to message dialog * improved null checks * file watcher added first
…df.json (#1218) * eim auto import for old config added
* IEP-1552: Fix for system python executable
alirana01
force-pushed
the
release/v4.0.0
branch
from
716555d to
667197c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (15)
.github/workflows/ci_windows.yml (1)
8-14: Fix YAML indentation under triggers
branches:items are indented 5 spaces instead of 6, breaking YAML structure. Align them with two extra spaces as already suggested earlier..github/workflows/ci.yml (1)
8-14: Address remaining YAML-lint / action-lint errorsThe earlier remarks still apply:
- Mis-indented
branches:lists (lines 8-14).- Step item
- uses: actions/checkout@v4needs two more spaces (line 22).- The
Install ESP-IDFstep contains tab characters – replace with spaces (line 29).- Upgrade
actions/setup-python@v2→@v4(line 25, inside this hunk).- Strip trailing spaces after
distribution:andcache:(lines 43-44).These must be fixed for the workflow to parse.
Also applies to: 22-23, 29-33, 43-44
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (7)
100-109: Environment map prepared but never used
addGitToEnvironment(env, …)mutatesenv, yetrunInBackgroundreceivesSystem.getenv(), discarding the change. Passenvinstead.- IStatus status = processBuilderFactory.runInBackground(arguemnts, Path.ROOT, System.getenv()); + IStatus status = processBuilderFactory.runInBackground(arguemnts, Path.ROOT, env);
148-150: Typo: “pyhton”Spelling mistake in user-visible strings.
-Installing pyhton dependency web-socket +Installing python dependency web-socket
253-259: Avoid dumping full environment to log
Logger.log(environment.toString())can leak secrets (tokens, keys) into the Eclipse log. Log only the size or redact sensitive entries.
330-377: Three-second spin-wait cancels valid buildsThe manual 10×300 ms loop is too aggressive; legitimate
list-targetsruns often exceed 3 s. Replace withprocess.waitFor(long, TimeUnit)with a sane timeout (e.g. 2 min) or make it configurable.
404-425: Possible NPE when PATH absent
idfExportPath = envVarsFromActivationScriptMap.get(PATH)may be null; subsequentreplacecalls would crash. Add null check and fail gracefully.
496-503: Handle duplicate env keys when parsing
Collectors.toMapwithout a merge function throws if the activation script exports the same variable twice (e.g.,PATH). Provide a merge strategy.
543-560: Hard-coded ‘;’ path separator & missing null-check
- Use
File.pathSeparatorinstead of";"for cross-platform compatibility.executablePathcan be null – guard beforenew Path(executablePath).bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
181-188: Null-check workbench window before use
EclipseHandler.getActiveWorkbenchWindow()can return null during early startup or headless tests. Guard before dereferencing.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (4)
70-72: StaticeimJsonfield is still a mutable singleton – existing race-condition remains
eimJsonis overwritten on everygetInstance()call. Concurrent calls from background jobs/UI threads can race and editors created earlier may suddenly operate on a different JSON instance.
Previous review already raised this (see history) but code is unchanged.
268-284: Comparator ↔ column index mapping is wrong again – sorting yields unexpected order
setupColumns()assigns
• index 0 → versionColumn
• index 1 → nameColumn
• index 2 → locationColumnbut
compare()interprets
• case 0 → name
• case 1 → path
• case 2 → active-stateClicking “Version” will sort by Name, etc. This is the exact issue pointed out in the former review and still un-fixed.
--- in compare() -case 0: - rc = p1.getName().compareTo(p2.getName()); +case 0: // Version column + rc = ToolsUtility.getIdfVersion(p1, null) + .compareTo(ToolsUtility.getIdfVersion(p2, null)); ... -case 1: - rc = p1.getPath().compareTo(p2.getPath()); +case 1: // Name column + rc = p1.getName().compareTo(p2.getName()); ... -case 2: - Boolean p1State = ToolsUtility.isIdfInstalledActive(p1); - Boolean p2State = ToolsUtility.isIdfInstalledActive(p2); - rc = p1State.compareTo(p2State); +case 2: // Location column + rc = p1.getPath().compareTo(p2.getPath());Also applies to: 530-545
230-256: Colour objects still leak when columns are recreated
setupColumns()disposesTableViewerColumns but their associatedIdfManagerTableColumnLabelProviderinstances (each holding an SWTColor) are not disposed, so every refresh leaks oneColor.
Same issue flagged previously. Dispose the label-provider before or indisposeColumns().+ if (activateColumn != null) { + ColumnLabelProvider lp = (ColumnLabelProvider) activateColumn.getLabelProvider(); + if (lp != null) { + lp.dispose(); + } + }(Repeat for the other columns.)
Also applies to: 317-326
379-396:getIdfVersionis invoked for every paint → spawns external process on UI thread
updateDataIntoCells()callsToolsUtility.getIdfVersion()which launches a shell and blocks (waitFor()).
Rendering the table for 20 rows means 20 processes and UI freeze – the same performance problem highlighted in the last round.Immediate mitigation: cache once per
IdfInstalledor compute in a backgroundJoband refresh asynchronously.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
27-41: Null-safety still missing and unused parameter detected
activationScriptmay benull;new File(null)will NPE.
Also, thegitPathparameter is never referenced – delete it or use it.-public static String getIdfVersion(IdfInstalled idfInstalled, String gitPath) +public static String getIdfVersion(IdfInstalled idfInstalled) { - String activationScript = idfInstalled.getActivationScript(); + if (idfInstalled == null || idfInstalled.getActivationScript() == null) { + return null; + } + String activationScript = idfInstalled.getActivationScript();
🧹 Nitpick comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (1)
5-13: Clean up typos & leading-space keys
refereshing→refreshinginEimJsonChangedMsgDetail.- Remove the leading space before
MsgYes/MsgNo; they currently save the key with the space included, so look-ups will fail.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (2)
68-93: Platform command construction – brittle fallback
/bin/zshis assumed for “other” OSes, which includes macOS and unknowns.
Many Linux/macOS systems lack zsh; prefer/bin/bashfallback or detect shell availability.
95-100: Repeated environment lookup could be cached
isIdfInstalledActiveinstantiatesIDFEnvironmentVariablesevery call; this runs for every table repaint.
Cache the env value once or expose a static helper.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
23-35: Minor: Keep message fields sorted alphabetically for maintainabilityThe list is small now; alphabetic order helps during future additions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bundles/com.espressif.idf.ui/icons/tools/delete.pngis excluded by!**/*.png
📒 Files selected for processing (76)
.github/workflows/ci.yml(2 hunks).github/workflows/ci_windows.yml(1 hunks)bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties(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/tools/vo/EimJson.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonChangeListener.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java(3 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(4 hunks)bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.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/OSGI-INF/l10n/bundle.properties(0 hunks)bundles/com.espressif.idf.ui/plugin.xml(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java(0 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/handlers/NewProjectHandlerUtil.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java(0 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/install/GitRepositoryBuilder.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java(0 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/EspressifToolStartup.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java(4 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(13 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java(1 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(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java(0 hunks)tests/com.espressif.idf.ui.test/configs/default-test-linux.properties(1 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java(0 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java(2 hunks)tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java(1 hunks)tests/pom.xml(1 hunks)
💤 Files with no reviewable changes (30)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/GitWizardRepProgressMonitor.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IDFToolSet.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages_zh.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/GitRepositoryBuilder.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/UpdateEspIdfHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationJob.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.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/InstallToolsHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/LogMessagesThread.java
🚧 Files skipped from review as they are similar to previous changes (38)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tracing/AppLvlTracingDialog.java
- tests/pom.xml
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/NewProjectHandlerUtil.java
- tests/com.espressif.idf.ui.test/configs/default-test-linux.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeDataManager.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.core/src/com/espressif/idf/core/util/EspToolCommands.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/messages.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonChangeListener.java
- bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/ESPIDFManagerEditor.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFEnvironmentVariables.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/EnvSetupOperations.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java
- bundles/com.espressif.idf.ui/plugin.xml
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageEspIdfVersionsHandler.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/SetupToolsJobListener.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/EimEditorInput.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
- bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EspressifGeneralStartup.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/watcher/EimJsonUiChangeHandler.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (10)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePlugin.java (1)
IDFCorePlugin(20-77)bundles/com.espressif.idf.core/src/com/espressif/idf/core/ProcessBuilderFactory.java (1)
ProcessBuilderFactory(16-163)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (1)
ESPToolChainManager(61-680)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (1)
ToolsUtility(25-101)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/EimJson.java (1)
EimJson(8-59)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/IdfInstalled.java (1)
IdfInstalled(5-80)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(58-838)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/Messages.java (1)
Messages(6-27)
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
19-19: label "eclipse" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: label "BrnoUBU0004" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
25-25: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 29-29: syntax error: found character '\t' that cannot start any token
(syntax)
[error] 43-43: trailing spaces
(trailing-spaces)
.github/workflows/ci_windows.yml
[warning] 9-9: wrong indentation: expected 6 but found 5
(indentation)
[warning] 13-13: wrong indentation: expected 6 but found 5
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build_macos
| public void rollback() | ||
| { | ||
| IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); | ||
|
|
||
| for (Entry<String, String> entry : existingEnvVarsInIdeForRollback.entrySet()) | ||
| { | ||
| idfEnvironmentVariables.addEnvVariable(entry.getKey(), entry.getValue()); | ||
| } | ||
| IDFUtil.updateEspressifPrefPageOpenocdPath(); |
There was a problem hiding this comment.
Null-safe rollback
existingEnvVarsInIdeForRollback may still be null when rollback() is called after an early failure, leading to NPE. Guard for null before iterating.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java
around lines 77 to 85, the rollback() method iterates over
existingEnvVarsInIdeForRollback without checking if it is null, which can cause
a NullPointerException. Add a null check before the for loop to ensure
existingEnvVarsInIdeForRollback is not null before iterating over its entries.
| display.syncExec(() -> { | ||
| MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), | ||
| Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, 0, 0, | ||
| new String[] { Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, | ||
| Messages.ToolsInitializationDifferentPathMessageBoxOptionNo }); | ||
| response[0] = messageDialog.open(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
MessageDialog constructor parameters out of order – compilation fails
MessageDialog expects (Shell, String, Image, String, int, String[], int). You pass two ints before the label array. Swap the arguments.
- MessageDialog messageDialog = new MessageDialog(display.getActiveShell(),
- Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, 0, 0,
- new String[] { ... });
+ MessageDialog messageDialog = new MessageDialog(display.getActiveShell(),
+ Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg,
+ MessageDialog.QUESTION, new String[] { ... }, 0);📝 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.
| display.syncExec(() -> { | |
| MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), | |
| Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, 0, 0, | |
| new String[] { Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, | |
| Messages.ToolsInitializationDifferentPathMessageBoxOptionNo }); | |
| response[0] = messageDialog.open(); | |
| }); | |
| display.syncExec(() -> { | |
| MessageDialog messageDialog = new MessageDialog(display.getActiveShell(), | |
| Messages.OldConfigFoundMsgBoxTitle, null, Messages.OldConfigFoundMsgBoxMsg, | |
| MessageDialog.QUESTION, new String[] { | |
| Messages.ToolsInitializationDifferentPathMessageBoxOptionYes, | |
| Messages.ToolsInitializationDifferentPathMessageBoxOptionNo | |
| }, 0); | |
| response[0] = messageDialog.open(); | |
| }); |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java
around lines 96 to 103, the MessageDialog constructor parameters are incorrectly
ordered, causing compilation errors. Change the constructor call to pass the int
parameter before the String array and then the int parameter last, matching the
expected signature (Shell, String, Image, String, int, String[], int). Swap the
two int arguments accordingly to fix the issue.
| @Override | ||
| public void earlyStartup() | ||
| { | ||
| Preferences preferences = org.eclipse.core.runtime.preferences.InstanceScope.INSTANCE | ||
| .getNode(UIPlugin.PLUGIN_ID); | ||
| ToolInitializer toolInitializer = new ToolInitializer(preferences); | ||
| EimJsonStateChecker stateChecker = new EimJsonStateChecker(preferences); | ||
| eimJsonUiChangeHandler = new EimJsonUiChangeHandler(preferences); | ||
| stateChecker.updateLastSeenTimestamp(); | ||
| EimJsonWatchService.getInstance().addEimJsonChangeListener(eimJsonUiChangeHandler); | ||
|
|
There was a problem hiding this comment.
Instance fields never initialised
earlyStartup() creates local variables preferences and toolInitializer, leaving the declared fields null. Subsequent methods dereference toolInitializer / preferences, causing NPEs.
- Preferences preferences = ...
- ToolInitializer toolInitializer = ...
+ this.preferences = org.eclipse.core.runtime.preferences.InstanceScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID);
+ this.toolInitializer = new ToolInitializer(this.preferences);📝 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.
| @Override | |
| public void earlyStartup() | |
| { | |
| Preferences preferences = org.eclipse.core.runtime.preferences.InstanceScope.INSTANCE | |
| .getNode(UIPlugin.PLUGIN_ID); | |
| ToolInitializer toolInitializer = new ToolInitializer(preferences); | |
| EimJsonStateChecker stateChecker = new EimJsonStateChecker(preferences); | |
| eimJsonUiChangeHandler = new EimJsonUiChangeHandler(preferences); | |
| stateChecker.updateLastSeenTimestamp(); | |
| EimJsonWatchService.getInstance().addEimJsonChangeListener(eimJsonUiChangeHandler); | |
| @Override | |
| public void earlyStartup() | |
| { | |
| // initialize the instance fields instead of shadowing them | |
| this.preferences = org.eclipse.core.runtime.preferences.InstanceScope.INSTANCE | |
| .getNode(UIPlugin.PLUGIN_ID); | |
| this.toolInitializer = new ToolInitializer(this.preferences); | |
| EimJsonStateChecker stateChecker = new EimJsonStateChecker(preferences); | |
| eimJsonUiChangeHandler = new EimJsonUiChangeHandler(preferences); | |
| stateChecker.updateLastSeenTimestamp(); | |
| EimJsonWatchService.getInstance().addEimJsonChangeListener(eimJsonUiChangeHandler); | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java
around lines 50 to 60, the instance fields for preferences and toolInitializer
are not being initialized and only local variables are created in
earlyStartup(), leading to null pointer exceptions later. To fix this, assign
the preferences and toolInitializer objects to the corresponding instance fields
instead of local variables so they are properly initialized and accessible
throughout the class.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (6)
81-84: Possible NPE whenexistingEnvVarsInIdeForRollbackis still null
rollback()iterates overexistingEnvVarsInIdeForRollback.entrySet()without guarding for the field being unset when the job aborts early.
This is the same issue flagged in an earlier review round.
100-109: Environment prepared inenvis ignored
addGitToEnvironment()mutates the localenvmap, yetrunInBackgroundis invoked withSystem.getenv()instead of the updated map—identical to the previously reported bug.
270-296: 3 s spin-loop still cancels legitimate runs
The manual 10 × 300 ms wait is unchanged; long-running first invocations ofidf.py list-targetscan legitimately exceed this.
325-346:idfExportPathmay be null → NPE onreplace(...)
If the activation output is missingPATH,idfExportPathis null and thereplacecalls crash. This was already raised in the previous review.
417-424: Duplicate keys in activation output still throwIllegalStateException
Collectors.toMapis used without a merge function, so repeated exports (e.g. forPATH) will break. Same as the earlier comment.
464-481: Hard-coded";"separator & missing null check forexecutablePath
Platform-specific path separator and potential NPE remain unaddressed, matching the prior feedback.
🧹 Nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java (1)
98-105: Progress monitor ticks don’t add up
beginTask("…", 7)is followed by only 5worked(1)calls, so the monitor never reaches 100 %. Either adjust the total or add the missing ticks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/SetupToolsInIde.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build_macos
Summary by CodeRabbit
New Features
Improvements
Removals
Bug Fixes
Chores