IEP-1762: Eim gui cli launch fixes#1457
Conversation
This comment was marked as spam.
This comment was marked as spam.
dc1e322 to
87c8ecf
Compare
|
@alirana01 hi ! Tested under: I think we should fix this issue if we really want to apply vscode feature here. the way it works in vscode -> if EIM-CLI was installed via homebrew or winget then extension executes EIM-CLI.mp4 |
@AndriiFilippov can you please test it again |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalConnector.java`:
- Around line 96-99: In EimCliTerminalConnector's catch(IOException e) block
(currently only calling Logger.log(e)), invoke the same completion/cleanup path
that is executed on normal terminal exit so the command doesn't stall; after
logging the exception call the existing terminal completion method used
elsewhere in this class (for example the method that handles terminal
exit/cleanup such as onTerminalExit, notifyCommandFinished, completeFuture or
whatever concrete completion handler this class uses) and ensure any associated
resources/state are updated the same way as in the normal exit path.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLaunchSupport.java`:
- Around line 27-29: The static fields completionFired and pendingCompletion
create a race across concurrent launches; make completion state per-launch (not
process-global) by replacing completionFired and pendingCompletion with
launch-scoped state (e.g., a ConcurrentHashMap<String, Runnable>
pendingCompletions and ConcurrentHashMap<String, AtomicBoolean> completionFired
keyed by a unique launch/terminal id) and update the methods in
EimCliTerminalLaunchSupport that set/clear/invoke pendingCompletion and
completionFired to use the launch id (or Process/ILaunch instance) to store and
retrieve the corresponding Runnable/AtomicBoolean so each launch manages its own
completion callback without overwriting others.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimGuiOrCliLauncher.java`:
- Around line 44-49: The call to
EimJsonWatchService.getInstance().pauseListeners() can leave listeners paused if
CLI launch fails early; update the EimGuiOrCliLauncher flow so listeners are
always resumed on any early failure or non-arming launch path: wrap the
display.syncExec block / the call to EimCliTerminalLaunchSupport.launch(eimPath,
...) in a try/catch/finally or provide an explicit failure callback to
EimCliTerminalLaunchSupport.launch that invokes
EimJsonWatchService.getInstance().resumeListeners(); apply the same guard to the
other launch site around lines 68-77 so pauseListeners() is always paired with
resumeListeners() regardless of exceptions or launch-result conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 352d3463-1530-485b-97a2-3c23ae23043a
📒 Files selected for processing (11)
bundles/com.espressif.idf.ui/META-INF/MANIFEST.MFbundles/com.espressif.idf.ui/plugin.xmlbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimGuiOrCliLauncher.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalConnector.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLaunchSupport.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLauncherDelegate.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages_zh.properties
✅ Files skipped from review due to trivial changes (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java
| catch (IOException e) | ||
| { | ||
| Logger.log(e); | ||
| } |
There was a problem hiding this comment.
Command write failure should trigger completion path.
If writing the CLI command fails, flow currently stalls waiting for terminal exit. Trigger completion immediately in this error branch.
Suggested fix
catch (IOException e)
{
Logger.log(e);
+ EimCliTerminalLaunchSupport.invokeCompletionIfArmed();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalConnector.java`
around lines 96 - 99, In EimCliTerminalConnector's catch(IOException e) block
(currently only calling Logger.log(e)), invoke the same completion/cleanup path
that is executed on normal terminal exit so the command doesn't stall; after
logging the exception call the existing terminal completion method used
elsewhere in this class (for example the method that handles terminal
exit/cleanup such as onTerminalExit, notifyCommandFinished, completeFuture or
whatever concrete completion handler this class uses) and ensure any associated
resources/state are updated the same way as in the normal exit path.
| private static final AtomicBoolean completionFired = new AtomicBoolean(); | ||
| private static volatile Runnable pendingCompletion; | ||
|
|
There was a problem hiding this comment.
Global completion state is racy across concurrent launches.
pendingCompletion/completionFired are process-global, so a second launch can overwrite the first callback and break completion routing.
Also applies to: 49-51, 82-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLaunchSupport.java`
around lines 27 - 29, The static fields completionFired and pendingCompletion
create a race across concurrent launches; make completion state per-launch (not
process-global) by replacing completionFired and pendingCompletion with
launch-scoped state (e.g., a ConcurrentHashMap<String, Runnable>
pendingCompletions and ConcurrentHashMap<String, AtomicBoolean> completionFired
keyed by a unique launch/terminal id) and update the methods in
EimCliTerminalLaunchSupport that set/clear/invoke pendingCompletion and
completionFired to use the launch id (or Process/ILaunch instance) to store and
retrieve the corresponding Runnable/AtomicBoolean so each launch manages its own
completion callback without overwriting others.
77945c8 to
96c923f
Compare
|
@alirana01 hi ! I think we misunderstood each other when we were discussing the VS Code approach. Peter added the If |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/LinuxEimLauncherStrategy.java`:
- Around line 47-48: The code in LinuxEimLauncherStrategy builds argsStr by
joining raw args which allows shell injection and breaks on spaces; change
construction of argsStr to individually shell-quote each element of args before
joining (escape any single quotes inside an arg by replacing ' with '"'"' and
wrap each arg in single quotes), then build bashCmd using quotedPath plus this
safely quoted argsStr; update references around the args, argsStr and bashCmd
variables in the class to use the per-arg quoting logic.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/WindowsEimLauncherStrategy.java`:
- Around line 47-50: The code builds powershellCmd by injecting raw joined args
into a single-quoted -ArgumentList string which can break parsing or allow
injection when args contain single quotes or other special characters; update
WindowsEimLauncherStrategy to build argsStr by escaping each argument separately
(replace any single quote ' with two single quotes ''), wrap each escaped
argument in single quotes, and pass them as a comma-separated list to
-ArgumentList (e.g. " -ArgumentList " + String.join(", ", escapedArgs)); use
escapedPathForPowershell for -FilePath as before and ensure the args null/empty
case still yields an empty argsStr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d74a3c91-d551-437e-b9ff-55ece976042c
📒 Files selected for processing (19)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/EimLaunchService.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/EimLauncherStrategy.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/LinuxEimLauncherStrategy.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/WindowsEimLauncherStrategy.javabundles/com.espressif.idf.ui/META-INF/MANIFEST.MFbundles/com.espressif.idf.ui/plugin.xmlbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimGuiOrCliLauncher.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalConnector.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLaunchSupport.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLauncherDelegate.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages_zh.properties
✅ Files skipped from review due to trivial changes (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages_zh.properties
🚧 Files skipped from review as they are similar to previous changes (11)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
- bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.ui/plugin.xml
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalConnector.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimGuiOrCliLauncher.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLauncherDelegate.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/eim/terminal/EimCliTerminalLaunchSupport.java
| String argsStr = (args != null && args.length > 0) ? " " + String.join(" ", args) : ""; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | ||
| String bashCmd = "nohup " + quotedPath + argsStr + " > /dev/null 2>&1 & echo $!"; //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
Quote each CLI arg before composing the bash command.
Line 47 and Line 48 concatenate raw args into bash -lc, which allows shell metacharacter injection and breaks args containing spaces/quotes.
Proposed fix
- String argsStr = (args != null && args.length > 0) ? " " + String.join(" ", args) : ""; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+ String argsStr = (args != null && args.length > 0)
+ ? " " + java.util.Arrays.stream(args).map(ProcessUtils::bashSingleQuote).collect(java.util.stream.Collectors.joining(" ")) //$NON-NLS-1$
+ : StringUtil.EMPTY;
String bashCmd = "nohup " + quotedPath + argsStr + " > /dev/null 2>&1 & echo $!"; //$NON-NLS-1$ //$NON-NLS-2$🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/LinuxEimLauncherStrategy.java`
around lines 47 - 48, The code in LinuxEimLauncherStrategy builds argsStr by
joining raw args which allows shell injection and breaks on spaces; change
construction of argsStr to individually shell-quote each element of args before
joining (escape any single quotes inside an arg by replacing ' with '"'"' and
wrap each arg in single quotes), then build bashCmd using quotedPath plus this
safely quoted argsStr; update references around the args, argsStr and bashCmd
variables in the class to use the per-arg quoting logic.
| String argsStr = (args != null && args.length > 0) ? " -ArgumentList '" + String.join(" ", args) + "'" : ""; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ | ||
| String powershellCmd = String.format( | ||
| "Start-Process -FilePath '%s' -PassThru | Select-Object -ExpandProperty Id", //$NON-NLS-1$ | ||
| escapedPathForPowershell); | ||
| "Start-Process -FilePath '%s'%s -PassThru | Select-Object -ExpandProperty Id", //$NON-NLS-1$ | ||
| escapedPathForPowershell, argsStr); |
There was a problem hiding this comment.
Escape and structure PowerShell -ArgumentList safely.
Line 47–Line 50 inject raw joined args into a quoted PowerShell expression. Args containing ' (or complex quoting) can break command parsing and may enable command injection.
Proposed fix
- String argsStr = (args != null && args.length > 0) ? " -ArgumentList '" + String.join(" ", args) + "'" : ""; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
+ String argsStr = StringUtil.EMPTY;
+ if (args != null && args.length > 0)
+ {
+ String quotedArgs = java.util.Arrays.stream(args)
+ .map(a -> "'" + a.replace("'", "''") + "'") //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+ .collect(java.util.stream.Collectors.joining(", ")); //$NON-NLS-1$
+ argsStr = " -ArgumentList " + quotedArgs; //$NON-NLS-1$
+ }
String powershellCmd = String.format(
"Start-Process -FilePath '%s'%s -PassThru | Select-Object -ExpandProperty Id", //$NON-NLS-1$
escapedPathForPowershell, argsStr);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/WindowsEimLauncherStrategy.java`
around lines 47 - 50, The code builds powershellCmd by injecting raw joined args
into a single-quoted -ArgumentList string which can break parsing or allow
injection when args contain single quotes or other special characters; update
WindowsEimLauncherStrategy to build argsStr by escaping each argument separately
(replace any single quote ' with two single quotes ''), wrap each escaped
argument in single quotes, and pass them as a comma-separated list to
-ArgumentList (e.g. " -ArgumentList " + String.join(", ", escapedArgs)); use
escapedPathForPowershell for -FilePath as before and ensure the args null/empty
case still yields an empty argsStr.
9ff13f9 to
dbd74f9
Compare
@AndriiFilippov my bad i forgot to test that flow should be working now, please test it on all platforms in a clean env and try installing eim via package managers on POSIX and then launching from IDE. |
|
@alirana01 hi |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/WindowsEimLauncherStrategy.java (1)
22-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix class Javadoc: says "Linux" but this is the Windows strategy.
/** - * Linux EIM launcher strategy + * Windows EIM launcher strategy * `@author` Ali Azam Rana <ali.azamrana@espressif.com> */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/WindowsEimLauncherStrategy.java` around lines 22 - 24, The class Javadoc for WindowsEimLauncherStrategy incorrectly says "Linux EIM launcher strategy"; update the Javadoc comment above the WindowsEimLauncherStrategy class to accurately describe it (e.g., "Windows EIM launcher strategy") and keep or correct the `@author` tag if needed so the comment matches the class purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.java`:
- Around line 81-87: The code builds argsForOpen by joining raw args and injects
it into the AppleScript invoked by MacOsEimLauncherStrategy, allowing misparsing
or command injection; change the construction so each element of args is
shell-escaped/quoted before joining (escape quotes and backslashes and wrap each
arg in single quotes or use a safe quoting function) and set OPEN_ARGS to that
escaped string (update the logic that creates argsForOpen and any other places
referenced—e.g., the environment key "OPEN_ARGS" and usage in the AppleScript
invocation) so the final do shell script call receives properly quoted arguments
instead of raw user input.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/WindowsEimLauncherStrategy.java`:
- Around line 22-24: The class Javadoc for WindowsEimLauncherStrategy
incorrectly says "Linux EIM launcher strategy"; update the Javadoc comment above
the WindowsEimLauncherStrategy class to accurately describe it (e.g., "Windows
EIM launcher strategy") and keep or correct the `@author` tag if needed so the
comment matches the class purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fbd0344-803c-443d-b12e-769e68b7cc4d
📒 Files selected for processing (8)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/EimLaunchService.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/EimLauncherStrategy.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/LinuxEimLauncherStrategy.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/WindowsEimLauncherStrategy.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimGuiOrCliLauncher.java
🚧 Files skipped from review as they are similar to previous changes (4)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/EimLauncherStrategy.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/EimLaunchService.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/LinuxEimLauncherStrategy.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimGuiOrCliLauncher.java
Add GUI/CLI launch support so that when EIM is detected as CLI-only (no GUI capability), the wizard runs inside the Eclipse integrated terminal instead of being launched as a detached background process. - Add EimCliTerminalConnector, EimCliTerminalLauncherDelegate, and EimCliTerminalLaunchSupport for terminal-based EIM CLI launch - Add EimGuiOrCliLauncher to branch between GUI and CLI launch paths - Enrich shell PATH with package-manager directories (Homebrew, WinGet, Chocolatey, Scoop, Snap, etc.) per platform - Wire completion callback to reload eim_idf.json and refresh UI after EIM CLI wizard exits
d021e5a to
fd68f26
Compare
EIM GUI/CLI launch support
Description
Adds support for both GUI and CLI EIM installations, aligning the Eclipse plugin with the approach taken in vscode-esp-idf-extension PR #1822.
Previously, the plugin only checked the GUI install location (
~/.espressif/eim_gui/) when resolving the EIM executable path. Users who install EIM via CLI (e.g., Homebrew on macOS, or the CLI-only binary) would not have their installation detected. Additionally, on macOS, attempting to launch a CLI-only binary through AppleScript'sopen -awould fail with anIllegalArgumentExceptionsince there is no.appbundle to derive.This PR:
~/.espressif/eim/) as the final fallback inresolveEimExecutablePathisEimGuiCapable(String eimPath)method that probes the binary witheim gui --helpto detect GUI supportopenChanges
EimConstants.javaUSER_EIM_CLI_DIRconstantToolInitializer.javagetDefaultCliEimPath(),isEimGuiCapable()MacOsEimLauncherStrategy.javaisAppBundle()check +launchCliDirect()for non-app binariesResolution Priority (updated)
PATH(e.g., Homebrew-installedeim)eimPathfromeim_idf.json(existence-checked)EIM_PATHenvironment variable (existence-checked)Testing
eimin PATH) — verify detected and launched directly.appin/Applications— verify launched via AppleScript as before~/.espressif/eim/eim— verify detected and launched~/.espressif/eim/eim— verify detectedeim.exeat~/.espressif/eim/eim.exe— verify detectedisEimGuiCapablereturnstruefor GUI builds andfalsefor CLI-only buildsDependencies
This PR builds on
eim_path_discoverybranch (PR #1446) which contains the base path resolution fixes.Related
Summary by CodeRabbit
Improvements
New Features
Localization