Skip to content

IEP-1762: Eim gui cli launch fixes #1457

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.

Sign up for GitHub

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

Open
alirana01 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

IEP-1762: Eim gui cli launch fixes #1457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,7 @@ public interface EimConstants

String USER_EIM_DIR = Paths.get(System.getProperty("user.home"), ".espressif", "eim_gui").toString(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$

String USER_EIM_CLI_DIR = Paths.get(System.getProperty("user.home"), ".espressif", "eim").toString(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$

String EIM_JSON_VALID_VERSION = "2.0"; //$NON-NLS-1$
}
70 changes: 66 additions & 4 deletions bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;

import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.IPath;
Expand Down Expand Up @@ -65,9 +66,14 @@ private String findEimOnSystemPath()
}

/**
* Resolves the EIM executable path: <strong>system {@code PATH} first</strong>, then {@code eimPath} from
* {@code eim_idf.json} when the path exists on disk, then {@code EIM_PATH} env variable, then
* {@link #getDefaultEimPath()} (existence-checked).
* Resolves the EIM executable path using priority-based resolution:
* <ol>
* <li>System {@code PATH}</li>
* <li>{@code eimPath} from {@code eim_idf.json} (when the path exists on disk)</li>
* <li>{@code EIM_PATH} env variable (existence-checked)</li>
* <li>Default GUI install location (existence-checked)</li>
* <li>Default CLI install location (existence-checked)</li>
* </ol>
*
* @param eimJson parsed JSON or {@code null}
* @return resolved absolute path string, or empty if nothing could be resolved
Expand Down Expand Up @@ -101,6 +107,12 @@ public String resolveEimExecutablePath(EimJson eimJson)
return defaultEimPath.toString();
}

Path cliEimPath = getDefaultCliEimPath();
if (cliEimPath != null && Files.exists(cliEimPath))
{
return cliEimPath.toString();
}

return StringUtil.EMPTY;
}

Expand Down Expand Up @@ -227,5 +239,55 @@ else if (os.equals(Platform.OS_MACOSX))

return defaultEimPath;
}


/**
* Returns the default CLI EIM binary path per platform. Unlike the GUI path, this points to the CLI-only install
* directory ({@code ~/.espressif/eim/}).
*/
public Path getDefaultCliEimPath()
{
String userHome = System.getProperty("user.home"); //$NON-NLS-1$
String os = Platform.getOS();
if (os.equals(Platform.OS_WIN32))
{
return Paths.get(userHome, ".espressif", "eim", "eim.exe"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
}
return Paths.get(userHome, ".espressif", "eim", "eim"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
}

/**
* Checks whether the EIM binary at the given path supports GUI mode by running {@code eim gui --help} and checking
* for a successful exit code. This is used to determine whether to launch EIM as a GUI application or in CLI/wizard
* mode.
*
* @param eimPath absolute path to the EIM executable
* @return {@code true} if the binary supports the {@code gui} subcommand, {@code false} otherwise
*/
public boolean isEimGuiCapable(String eimPath)
{
if (StringUtil.isEmpty(eimPath))
{
return false;
}

try
{
ProcessBuilder pb = new ProcessBuilder(eimPath, "gui", "--help"); //$NON-NLS-1$ //$NON-NLS-2$
pb.redirectErrorStream(true);
Process process = pb.start();
boolean finished = process.waitFor(5, TimeUnit.SECONDS);
if (!finished)
{
process.destroyForcibly();
return false;
}
return process.exitValue() == 0;
}
catch (IOException | InterruptedException e)
{
Logger.log("EIM does not support the gui subcommand, falling back to CLI mode."); //$NON-NLS-1$
return false;
}
}
Comment on lines +266 to 291
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

isEimGuiCapable can deadlock and report false negatives — drain the process output.

redirectErrorStream(true) is set, but the combined stdout/stderr stream is never read. If eim gui --help ever writes more than the OS pipe buffer (typically 16–64 KB on Linux/macOS), the child process blocks on write(), waitFor(5, SECONDS) returns false, the process is force-killed, and the method returns false — incorrectly reporting a GUI‑capable binary as CLI‑only. Even if today's help text is small, this is a fragile contract for an external CLI whose output you don't control.

Drain the stream while waiting (or before exitValue()), and close it explicitly:

🛡️ Proposed fix 
 		try
 		{
 			ProcessBuilder pb = new ProcessBuilder(eimPath, "gui", "--help"); //$NON-NLS-1$ //$NON-NLS-2$
 			pb.redirectErrorStream(true);
 			Process process = pb.start();
+			// Drain output on a background thread so the child doesn't block on a full pipe buffer.
+			Thread drainer = new Thread(() -> {
+				try (var in = process.getInputStream())
+				{
+					in.transferTo(OutputStream.nullOutputStream());
+				}
+				catch (IOException ignored)
+				{
+					// process exit will be observed via waitFor
+				}
+			}, "eim-gui-help-drain"); //$NON-NLS-1$
+			drainer.setDaemon(true);
+			drainer.start();
 			boolean finished = process.waitFor(5, TimeUnit.SECONDS);
 			if (!finished)
 			{
 				process.destroyForcibly();
 				return false;
 			}
 			return process.exitValue() == 0;
 		}
🤖 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/ToolInitializer.java`
around lines 242 - 267, The method isEimGuiCapable currently redirects stderr to
stdout but never consumes the process output, which can deadlock; update
isEimGuiCapable to drain the combined stream from process.getInputStream() (e.g.
spawn a short-lived background reader thread or read fully into a buffer) while
waiting for the process, ensure you close the InputStream when done, and only
then check waitFor/exitValue; keep the existing timeout and error handling
around ProcessBuilder pb / process so the process output is always consumed and
streams are closed to avoid false negatives.


}
40 changes: 40 additions & 0 deletions ...idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ public MacOsEimLauncherStrategy(Display display, MessageConsoleStream standardCo
@Override
public LaunchResult launch(String eimPath) throws IOException
{
if (!isAppBundle(eimPath))
{
return launchCliDirect(eimPath);
}

String appBundlePath = deriveAppBundlePath(eimPath);
String execPath = deriveExecPath(eimPath, appBundlePath);
String bundleId = readBundleId(appBundlePath);
Expand Down Expand Up @@ -108,6 +113,41 @@ public LaunchResult launch(String eimPath) throws IOException
"osascript exit=" + exit + "\n" + out); //$NON-NLS-1$ //$NON-NLS-2$
}

private LaunchResult launchCliDirect(String eimPath) throws IOException
{
String quotedPath = ProcessUtils.bashSingleQuote(eimPath);
String bashCmd = "nohup " + quotedPath + " > /dev/null 2>&1 & echo $!"; //$NON-NLS-1$ //$NON-NLS-2$

Process launcher = new ProcessBuilder("bash", "-lc", bashCmd) //$NON-NLS-1$ //$NON-NLS-2$
.redirectErrorStream(true).start();

String out = ProcessUtils.readAll(launcher.getInputStream());
Long pid = ProcessUtils.parseFirstLongLine(out);

if (pid == null)
{
Logger.log("macOS CLI launcher output was:\n" + out); //$NON-NLS-1$
throw new IOException("No PID found in launcher output. Output was:\n" + out); //$NON-NLS-1$
}

return LaunchResult.ofPid(pid.longValue(), eimPath, out);
}
Comment on lines +116 to +134
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reap the launcher process and harden parseFirstLongLine against shell stderr.

Two related concerns in launchCliDirect:

  1. The launcher Process is never waitFor()-ed and never explicitly destroyed on the error path. Reading to EOF usually implies bash has finished, but there's no guarantee the kernel has reaped it before this method returns; calling waitFor() (with a small timeout) makes lifecycle deterministic.
  2. With redirectErrorStream(true), any shell-init message printed to stderr (warnings from .bash_profile, locale errors, etc.) is interleaved with echo $!. parseFirstLongLine will then pick whichever line appears first — e.g. a bash: warning: ... line — and fail to find the PID. Combined with the bash -lc issue, this is a realistic failure mode on user machines.

Consider separating stderr (don't redirect), or print the PID with a marker (e.g. echo "PID:$!") and parse that.

♻️ Suggested hardening 
-		String bashCmd = "nohup " + quotedPath + " > /dev/null 2>&1 & echo $!"; //$NON-NLS-1$ //$NON-NLS-2$
-
-		Process launcher = new ProcessBuilder("bash", "-lc", bashCmd) //$NON-NLS-1$ //$NON-NLS-2$
-				.redirectErrorStream(true).start();
-
-		String out = ProcessUtils.readAll(launcher.getInputStream());
+		String bashCmd = "nohup " + quotedPath + " > /dev/null 2>&1 & echo EIM_PID:$!"; //$NON-NLS-1$ //$NON-NLS-2$
+
+		ProcessBuilder pb = new ProcessBuilder("bash", "-c", bashCmd); //$NON-NLS-1$ //$NON-NLS-2$
+		Process launcher = pb.start();
+
+		String out = ProcessUtils.readAll(launcher.getInputStream());
+		try
+		{
+			launcher.waitFor();
+		}
+		catch (InterruptedException e)
+		{
+			Thread.currentThread().interrupt();
+		}
🤖 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/MacOsEimLauncherStrategy.java`
around lines 116 - 134, launchCliDirect currently starts the bash launcher
without waiting for or reliably reaping the Process and mixes stderr into stdout
so parseFirstLongLine can pick up shell warnings instead of the PID; change the
invocation so the shell prints a distinct marker (e.g. echo "PID:$!") into
stdout by modifying bashCmd, stop using redirectErrorStream(true) so stderr
stays separate, read stdout and parse the marked PID (update parsing
expectations in ProcessUtils.parseFirstLongLine or add a new parser), and after
reading output call launcher.waitFor with a short timeout and on timeout call
launcher.destroyForcibly() to ensure the process is reaped (also ensure the
error path destroys/waits the Process).


private boolean isAppBundle(String eimPath)
{
Path p = Paths.get(eimPath).toAbsolutePath().normalize();
while (p != null)
{
String name = p.getFileName() != null ? p.getFileName().toString() : ""; //$NON-NLS-1$
if (name.endsWith(".app")) //$NON-NLS-1$
{
return true;
}
p = p.getParent();
}
return false;
}

@Override
public IStatus waitForExit(LaunchResult launchResult, IProgressMonitor monitor)
{
Expand Down
Loading