-
Notifications
You must be signed in to change notification settings - Fork 130
IEP-1554: Launch and Download EIM #1251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/v4.0.0
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self Review
716555d
to
667197c
Compare
* 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]>
Scenario: I launched my development environment with the existing workspace, and it prompted a dialog with the message ‘…do you want to convert?’. I clicked ‘Yes’ and saw the following error.
|
@AndriiFilippov fixed the issue that @kolipakakondal reported please test it |
@alirana01 hi ! Tested under: At first, I thought the problem was with the path But when I run eim.exe from within the IDE, the installation completes, yet something prevents the program from progressing past step 7. I believe that the way we launch the .exe file from IDE affects the final result. ![]() |
there is no validation if eim.exe already exists in the system |
Once tools are installed - the button "Download Launch EIM" changes to "Launch EIM", and it based on the validation of eim_idf.json file, not on validation of eim.exe itself, which could lead to scenario when, after installation user decides to delete eim.exe file, but the system won't be able to detect it since it validate only presents of eim_idf.json file, and pressing the button "Launch EIM" won't work, yes there is the error log about |
{ | ||
private static final String URL_JSON = "https://dl.espressif.com/dl/eim/eim_unified_release.json"; //$NON-NLS-1$ | ||
private static final Path DOWNLOAD_DIR = Paths.get(System.getProperty("java.io.tmpdir"), "eim_gui"); //$NON-NLS-1$ //$NON-NLS-2$ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OS - MacOS ARM64
-
no pop-up asking for EIM installation
⚠️ -
if proceed with manual installation, clicking on Download Launch EIM button, then there is error during eim download
Download Failed: Cannot read field "paused" because "watchService" is null
I believe this folder path cause the creation of eim_gui folder under this weird PATH:
/var/folders/fx/w33gzqp575gg615jlr_9mshh0000gn/T/eim_gui
instead of /temp/eim_gui
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit complicated to do that because as we discussed that this temporary path can easily be cleaned up on POSIX and for windows its also not very friendly if we decide on a constant place to put this EIM executable I can do that for now I have added some validation to the required paths as much as it can be done. |
This should also be resolved now with updated validation code but again we need to have a decisive point for eim executable placement on Linux and Windows for MacOS its already going to Applications as temporary path is only used to download the DMG |
Windows had weird issue with waiting for ui process causing it to get stuck so needed to find a workaround to handle this
OS - Windows 11 and error during tools installation(does not affect build or any other functional): |
OS - Windows 11 after latest changes, the IDE does not throws pop-up message and don't offer EIM Download / Launch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alirana01 Thanks for the PR. LGTM. Could you also contribute JUnit test cases for non-ui part - download mechansim, json reading from the eim, etc.
ToolsInitializationEimMissingMsgBoxTitle=ESP-IDF Not Found | ||
ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not found on your system. To use the IDE, install ESP-IDF using <a href="{0}">EIM - GUI Installer</a>. \n\nOnce installed, the IDE will automatically detect ESP-IDF. You can verify and activate it from the ESP-IDF Manager, accessible via the menu: Espressif > ESP-IDF Manager.\n\n | ||
ToolsInitializationEimMissingMsgBoxTitle=ESP-IDF Installation Required | ||
ToolsInitializationEimMissingMsgBoxMessage=ESP-IDF is not currently installed on your system. To proceed, you can download and launch the EIM - GUI Installer directly from this IDE.\n\nWould you like to download and start the EIM installer now?\n\nOnce installation is complete, the IDE will automatically detect ESP-IDF. You can also verify and activate it later from the ESP-IDF Manager (Espressif > ESP-IDF Manager). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a bit lengthy and unclear. Please rephrase it - especially the line: "To proceed, you can download and launch the EIM-GUI Installer directly from this IDE."
Since the IDE itself downloads and launches the EIM-GUI Installer, not the user. But the current sentence suggests otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
String EIM_WIN_DIR = "C:\\Espressif\\tools\\"; //$NON-NLS-1$ | ||
String EIM_WIN_ESPRESSIF_DIR = "C:\\Espressif"; //$NON-NLS-1$ | ||
|
||
String EIM_WIN_DIR = EIM_WIN_ESPRESSIF_DIR + "\\tools\\"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to use Paths or File.separator for portability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
public class EimLoader | ||
{ | ||
private static final String URL_JSON = "https://dl.espressif.com/dl/eim/eim_unified_release.json"; //$NON-NLS-1$ | ||
private static final Path DOWNLOAD_DIR = Paths.get(System.getProperty("java.io.tmpdir"), "eim_gui"); //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When multiple versions are downloaded, make sure to clean them up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
else if (os.equals(Platform.OS_LINUX)) | ||
{ | ||
command = List.of("bash", "-c", "\"" + eimPath.toString() + "\""); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eimPath is already a String, we can remove eimPath.toString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
else if (os.equals(Platform.OS_MACOSX)) | ||
{ | ||
command = List.of("open", "-W", "-a", eimPath.toString()); //$NON-NLS-1$//$NON-NLS-2$ //$NON-NLS-3$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eimPath is already a String, we can remove eimPath.toString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
String powershellCmd = String.format( | ||
"Start-Process -FilePath \"%s\" -PassThru | Select-Object -ExpandProperty Id", //$NON-NLS-1$ | ||
eimPath.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eimPath is already a String, we can remove eimPath.toString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
logMessage("Mounting DMG...\n"); //$NON-NLS-1$ | ||
ProcessBuilder mountBuilder = new ProcessBuilder("hdiutil", "attach", dmgPath.toString()); //$NON-NLS-1$ //$NON-NLS-2$ | ||
Process mountProcess = mountBuilder.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if it succeeded before reading..sames goes for unmounting and others
int mountExitCode = mountProcess.waitFor();
if (mountExitCode != 0) {
throw new IOException..
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
connection.setReadTimeout(10000); | ||
|
||
int contentLength = connection.getContentLength(); | ||
monitor.beginTask("Downloading " + targetPath.getFileName(), contentLength); //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple monitor.beginTask() calls for single moinitor, it was called already in the parent method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (called in this one and removed from parent to have proper progress reporting with contentLength
} | ||
else if (name.endsWith(".exe")) //$NON-NLS-1$ | ||
{ | ||
eimPath = Paths.get(eimPath.toString().concat("\\").concat(name)); //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use path resolve method to avoid converting to string and concatinating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
listener.onCompleted(downloadPath.toAbsolutePath().toString()); | ||
} | ||
} | ||
catch (Exception e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s common practice to catch more specific exceptions to make debugging easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
catch (InterruptedException e) | ||
{ | ||
Logger.log(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to propagate the interrupt here? Thread.currentThread().interrupt();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't get it, if there is an interrupted exception doesn't it mean the thread already interrupted why do we need to call interrupt again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching an InterruptedException clears the thread’s interrupt status, preventing higher-level code from detecting or responding to the interrupt unless the flag is reset.
@alirana01 hi ! OS - Windows 11 Using the external EIM.exe , after ESP-IDF installation, the IDE is able to detect modification to eim_idf.json file: but after clicking "Yes" the ESP-IDF Manager table is not updating with actual modifications. Have to do it manually by closing and reopening ESP-IDF Manager shell. Example: have 1 esp-idf in eim_idf.json file -> install one more using external eim_installer.exe -> after pop-up asking for "refreshing your installation" the ESP-IDF Manager window does not update values in table, you should close and re-open window to refresh table. |
Hi @alirana01, The tools initialization in the IDE works fine for me, but the Launch EIM button doesn't open eim-gui-windows-x64 (1).exe. OS: Windows 11 Steps to reproduce: !ENTRY com.espressif.idf.core 1 0 2025-06-24 12:37:35.581 !ENTRY com.espressif.idf.core 1 0 2025-06-24 12:37:35.582 Also, what do you think about making it clearer to users that they need to open the EIM Manager to add a new ESP-IDF? I can imagine a situation where someone installed ESP-IDF using EIM a while ago, and later decides to add another version but forgets that EIM was used initially. In that case, bringing back the "Add" button we previously had — which opens the EIM Manager — could be helpful. What do you think? |
Do we need a separate SYSTEM_PATH variable in the CDT build environment? If it’s alrady appended to the end of PATH, what’s the reason for keeping it separate? |
If a newer version of EIM is available, what should the user do? Can we document that somewhere? |
Description
Downloads if EIM is not present or will simply launch the EIM with added button
Fixes # (IEP-1554)
Type of change
Please delete options that are not relevant.
How has this been tested?
Try to remove the EIM and all the relevant files specially eim_idf.json and then try to launch the IDE.
The IDE will prompt to download the EIM and then launch it
Also test if you are able to download and launch via ESP-IDF Manager
Test Configuration:
Dependent components impacted by this PR:
Checklist