IEP-1686: Fix for eim download getting stuck #1387
Conversation
📝 WalkthroughWalkthroughReplaces direct Process-based EIM launching with a LaunchResult-driven service and OS-specific launcher strategies. EimLoader delegates launches to EimLaunchService; wait/closure handling now operates on LaunchResult. New utilities (LaunchResult, ProcessWaiter, ProcessUtils) and platform strategies (Windows, macOS, Linux) added; MANIFEST exports updated. Changes
sequenceDiagram
participant UI as UI (button/startup)
participant Service as EimLaunchService
participant Strategy as EimLauncherStrategy (OS-specific)
participant OS as Operating System / Process
participant Waiter as ProcessWaiter
UI->>Service: launch(eimPath)
Service->>Strategy: strategy.launch(eimPath)
Strategy->>OS: spawn process (osascript/powershell/nohup)
OS-->>Strategy: stdout (pid or details)
Strategy-->>Service: LaunchResult(pid?, execPath, details)
Service-->>UI: LaunchResult
UI->>Service: waitForExit(LaunchResult)
Service->>Strategy: strategy.waitForExit(LaunchResult, monitor)
Strategy->>Waiter: waitByPid or waitByExecPath
Waiter-->>Strategy: IStatus (OK/CANCEL/ERROR)
Strategy-->>Service: IStatus
Service-->>UI: completion callback run
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
379-399: Guard against invalid PID after launch failureIf
launchEimthrows,eimPidremains-1andwaitForEimClosurestill runs, which can fire the callback even though EIM never launched. Gate the wait on a valid PID (and log/notify on failure) to avoid opening the manager prematurely.🐛 Proposed fix
long eimPid = -1; String appToLaunch = filePath; try { if (filePath.endsWith(".dmg")) { appToLaunch = eimLoader.installAndLaunchDmg(Paths.get(filePath)); } idfEnvironmentVariables.addEnvVariable(IDFEnvironmentVariables.EIM_PATH, appToLaunch); eimPid = eimLoader.launchEim(appToLaunch); } catch ( IOException | InterruptedException e) { Logger.log(e); } -eimLoader.waitForEimClosure(eimPid, () -> { +if (eimPid > 0) +{ + eimLoader.waitForEimClosure(eimPid, () -> { if (toolInitializer.isOldEspIdfConfigPresent() && !toolInitializer.isOldConfigExported()) { Logger.log("Old configuration found and not converted"); handleOldConfigExport(); } try { eimJson = toolInitializer.loadEimJson(); } catch (EimVersionMismatchException e) { Logger.log(e); MessageDialog.openError(Display.getDefault().getActiveShell(), e.msgTitle(), e.getMessage()); return; } openEspIdfManager(eimJson); -}); + }); +} +else +{ + Logger.log("EIM launch failed; skipping wait/refresh."); +}bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java (1)
414-446: Use the method parameter inisWindowsProcessAlive
isWindowsProcessAlive(long pid)compares againsteimPidinstead ofpid, which will be wrong if the method is reused with a different PID.✅ Proposed fix
- if (line.contains(String.valueOf(eimPid))) + if (line.contains(String.valueOf(pid))) { return true; }
|
@alirana01 hi ! Able to download and launch EIM via IDE. However, the following errors still occur during testing:
|
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
Hi @alirana01, thanks for the PR. I’m still in the process of reviewing it, but so far everything looks good 👍
I only noticed a bit of dead code — for example, the waitForProcessWindows methods don’t seem to be used.
I’ll get back to you if I find anything else.
|
@AndriiFilippov kindly see if it works now there was a missing dependency that could have been causing the problem for welcome page not sure who removed it but please test again on all platforms again |
@sigmaaa code is updated now please review throughly to see any issues |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
@alirana01 please check the CodeRabbit comments — they seem reasonable. Otherwise, LGTM!
|
@alirana01 hi ! Tested under: Welcome page issue fixed ✅ |
Description
EIM download was getting stuck on POSIX when launched with IDE.
The issue seemed to consistent on EIM v0.6 but with v0.7 i am unable to reproduce. I have added the changes anyway since it seems to be a better approach to launch and monitor the process without actually launching it as a standard process with java and monitoring that we were already doing that on windows now implemented with AppleScript on mac and sh on linux for bash
Fixes # (IEP-1686)
Type of change
Please delete options that are not relevant.
How has this been tested?
Try launching and downloading on POSIX and windows and see if you are able to successfully install the tools
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.