Open
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR extends EIM executable resolution to support CLI-only installations, adds platform-specific CLI path helpers, implements GUI capability detection, and updates the macOS launcher strategy to route non-bundle executables directly via CLI without AppleScript. ChangesEIM CLI Executable Resolution and Direct Launch
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.java (2)
119-122: ⚡ Quick winAvoid
bash -l(login shell) for launching subprocesses.
bash -lcruns as a login shell and sources~/.bash_profile/~/.profile. This pulls in arbitrary user dotfile state, slows the launcher, and can produce stderr noise that ends up captured into the launcher's combined output (and then parsed byparseFirstLongLine). For a programmatic spawn,bash -c(or evensh -c) is sufficient and more predictable.♻️ Proposed change
- Process launcher = new ProcessBuilder("bash", "-lc", bashCmd) //$NON-NLS-1$ //$NON-NLS-2$ + Process launcher = new ProcessBuilder("bash", "-c", bashCmd) //$NON-NLS-1$ //$NON-NLS-2$ .redirectErrorStream(true).start();🤖 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 119 - 122, The ProcessBuilder in MacOsEimLauncherStrategy currently uses "bash", "-lc" which invokes a login shell; update the launcher invocation to avoid a login shell by replacing the "-lc" flag with "-c" (or use "sh", "-c") when building the Process (the code around bashCmd and the ProcessBuilder("bash", "-lc", bashCmd) call) so the spawned subprocess does not source user login dotfiles or introduce extra stderr noise.
136-149: 💤 Low valueDeduplicate
.appancestor walk withderiveAppBundlePath.
isAppBundleandderiveAppBundlePath(lines 163–178) walk the same parent chain looking for a*.appsegment. Extract a single helper (e.g.findAppBundleAncestorreturningOptional<Path>orPath/null) and implement both in terms of it.♻️ Suggested refactor
+ private Path findAppBundleAncestor(String eimPath) + { + Path p = Paths.get(eimPath).toAbsolutePath().normalize(); + while (p != null) + { + Path name = p.getFileName(); + if (name != null && name.toString().endsWith(".app")) //$NON-NLS-1$ + { + return p; + } + p = p.getParent(); + } + return null; + } + 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; + return findAppBundleAncestor(eimPath) != null; }(
deriveAppBundlePathbecomes the same call with a non-null assertion /IllegalArgumentException.)🤖 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 136 - 149, isAppBundle and deriveAppBundlePath duplicate the same parent-chain walk for finding a "*.app" ancestor; extract that logic into a single helper (e.g. findAppBundleAncestor returning Optional<Path> or null) and reimplement both to call it — isAppBundle should return true when the helper finds a path, and deriveAppBundlePath should return the found Path or throw an IllegalArgumentException if the helper yields empty/null; update references to use the helper and remove the duplicated loop from both isAppBundle and deriveAppBundlePath.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (1)
51-54: 💤 Low value
isEimInstalled()spawns an expensive SystemExecutableFinder lookup on every call—but current callers are not in hot paths.The method always invokes
findEimOnSystemPath()first, which instantiatesSystemExecutableFinderand scans the system PATH. However, the only two callers are:
EimButtonLaunchListener.widgetSelected()— runs once per user button clickEspressifToolStartup.earlyStartup()— runs once at startupNeither is in a tight loop, UI refresh mechanism, or frequent listener. If these usage patterns change in the future, consider memoizing with invalidation on
eim_idf.jsonmodification to avoid repeated scans.🤖 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 51 - 54, isEimInstalled() currently calls resolveEimExecutablePath(null) which always triggers findEimOnSystemPath() (creating a SystemExecutableFinder and scanning PATH); add a simple memoization in ToolInitializer by caching the last-resolved path and the last-modified timestamp of the eim_idf.json that influences resolution, return the cached value if the file timestamp hasn't changed, and invalidate/recompute the cache when eim_idf.json modification time differs (update the cache inside resolveEimExecutablePath / findEimOnSystemPath code paths so callers like isEimInstalled(), EimButtonLaunchListener.widgetSelected(), and EspressifToolStartup.earlyStartup() use the cached result).
🤖 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 `@BUILD_FAILURE_ANALYSIS_24703204267.md`:
- Around line 1-168: The file BUILD_FAILURE_ANALYSIS_24703204267.md is
out-of-scope for this PR and should not be merged here; remove it from the
branch/PR and instead move the analysis into one of: a new issue, a PR comment,
a discussion thread, or a dedicated docs PR (e.g. under docs/ci/ with a stable
name), referencing the failing test class NewEspressifIDFProjectClangFilesTest
and CI run details as needed; update the commit to delete
BUILD_FAILURE_ANALYSIS_24703204267.md from this PR and add the analysis to the
chosen external location so the feature PR only contains the intended EIM
GUI/CLI changes.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.java`:
- Around line 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).
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 81-117: The resolveEimExecutablePath method currently calls
Files.exists(Paths.get(...)) on values from eimJson.getEimPath() and
idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH) which can
throw InvalidPathException for malformed strings; fix by adding a small helper
(e.g., safePathExists(String)) that wraps Paths.get(...)/Files.exists(...) in a
try/catch and returns false on InvalidPathException, then replace the direct
Files.exists(Paths.get(jsonPath)) and Files.exists(Paths.get(eimExePathEnv))
checks with calls to that helper so resolveEimExecutablePath (and callers like
earlyStartup/startExportOldConfig) won’t crash on bad external input.
- Around line 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.
In `@docs/EIM_PATH_DISCOVERY.md`:
- Around line 102-217: Update the doc sections to match the refactor: replace
the old startup branching with EspressifToolStartup.earlyStartup calling
ToolInitializer.resolveEimExecutablePath(eimJson) as the single entry point for
EIM resolution, document the new five-step priority order (system PATH →
eim_idf.json eimPath → EIM_PATH env var → default GUI install → default CLI
install), remove references to the deleted findAndSetEimPath() and old
getDefaultEimPath-only behavior and instead describe getDefaultCliEimPath() and
USER_EIM_CLI_DIR, explain isEimGuiCapable(String) and the
MacOsEimLauncherStrategy.launchCliDirect path for non-.app binaries, and note
that isEimInstalled() now delegates to the resolver (so it scans PATH).
---
Nitpick comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.java`:
- Around line 119-122: The ProcessBuilder in MacOsEimLauncherStrategy currently
uses "bash", "-lc" which invokes a login shell; update the launcher invocation
to avoid a login shell by replacing the "-lc" flag with "-c" (or use "sh", "-c")
when building the Process (the code around bashCmd and the
ProcessBuilder("bash", "-lc", bashCmd) call) so the spawned subprocess does not
source user login dotfiles or introduce extra stderr noise.
- Around line 136-149: isAppBundle and deriveAppBundlePath duplicate the same
parent-chain walk for finding a "*.app" ancestor; extract that logic into a
single helper (e.g. findAppBundleAncestor returning Optional<Path> or null) and
reimplement both to call it — isAppBundle should return true when the helper
finds a path, and deriveAppBundlePath should return the found Path or throw an
IllegalArgumentException if the helper yields empty/null; update references to
use the helper and remove the duplicated loop from both isAppBundle and
deriveAppBundlePath.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 51-54: isEimInstalled() currently calls
resolveEimExecutablePath(null) which always triggers findEimOnSystemPath()
(creating a SystemExecutableFinder and scanning PATH); add a simple memoization
in ToolInitializer by caching the last-resolved path and the last-modified
timestamp of the eim_idf.json that influences resolution, return the cached
value if the file timestamp hasn't changed, and invalidate/recompute the cache
when eim_idf.json modification time differs (update the cache inside
resolveEimExecutablePath / findEimOnSystemPath code paths so callers like
isEimInstalled(), EimButtonLaunchListener.widgetSelected(), and
EspressifToolStartup.earlyStartup() use the cached result).
🪄 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: 3d1ac3f1-85d7-4059-8765-2ace704ecbd5
📒 Files selected for processing (6)
BUILD_FAILURE_ANALYSIS_24703204267.mdbundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimConstants.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/launch/strategies/MacOsEimLauncherStrategy.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javadocs/EIM_PATH_DISCOVERY.md
Comment on lines
+1
to
+168
| # Build failure analysis — CI run 24703204267 | ||
|
|
||
| Investigation of the failing Linux CI run triggered by the merge of PR [#1438 "fix: add pr comment after the build with download links"](https://github.com/espressif/idf-eclipse-plugin/pull/1438) on `master` (`0acc4deaf720cc1b46af6be5608d921842756194`, 2026-04-21). | ||
|
|
||
| Failing job: [`build_linux` (ID 72250690470)](https://github.com/espressif/idf-eclipse-plugin/actions/runs/24703204267/job/72250690470). `build_macos` on the same run passed. | ||
|
|
||
| ## TL;DR | ||
|
|
||
| The PR itself did not break the build. PR #1438 only touched `.github/workflows/pr-comment.yml` (a post-build commenter workflow). Nothing in the main `ci.yml` workflow, the plugin source, or the tests was modified. | ||
|
|
||
| The Linux job failed because **two UI tests in `NewEspressifIDFProjectClangFilesTest` failed on the self-hosted Linux runner**, and the `phoenix-actions/test-reporting@v12` step is configured with `fail-on-error: true`, which converts any failed JUnit result into a job failure (`Process completed with exit code 1.`). | ||
|
|
||
| The same test class passes on the macOS runner, so this is a Linux-runner-specific flakiness in a pair of cascading SWTBot test methods that depend on timing and UI state carried over between methods. | ||
|
|
||
| ## Failing tests | ||
|
|
||
| Both failures come from `tests/com.espressif.idf.ui.test/target/surefire-reports/TEST-com.espressif.idf.ui.test.executable.cases.project.NewEspressifIDFProjectClangFilesTest.xml`, extracted from the check-run annotations on the `Linux Test Reports` job (ID 72279770001). | ||
|
|
||
| ### 1. `shouldMatchExpectedClangdArgumentsAfterBuildingProjects` — `org.junit.ComparisonFailure` | ||
|
|
||
| ``` | ||
| org.junit.ComparisonFailure: expected:<...rk/workspace/Project[1]/build> but was:<...rk/workspace/Project[2]/build> | ||
| at org.junit.Assert.assertEquals(Assert.java:117) | ||
| at org.junit.Assert.assertEquals(Assert.java:146) | ||
| at ...NewEspressifIDFProjectClangFilesTest$Fixture.thenCompareActualClangdArgumentWithExpected(NewEspressifIDFProjectClangFilesTest.java:356) | ||
| at ...NewEspressifIDFProjectClangFilesTest$Fixture.thenCheckClangdArgumentAfterProjectBuilt(NewEspressifIDFProjectClangFilesTest.java:142) | ||
| at ...NewEspressifIDFProjectClangFilesTest.shouldMatchExpectedClangdArgumentsAfterBuildingProjects(NewEspressifIDFProjectClangFilesTest.java:111) | ||
| ``` | ||
|
|
||
| The expected vs actual paths correspond to `/opt/actions-runner/_work/workspace/Project1/build` and `.../Project2/build` respectively (the `...rk` prefix is JUnit's diff-trim of `/opt/actions-runner/_work`). | ||
|
|
||
| ### 2. `shouldRecreateClangdFileAfterDeletionAndVerifyContent` — `WidgetNotFoundException` | ||
|
|
||
| ``` | ||
| org.eclipse.swtbot.swt.finder.exceptions.WidgetNotFoundException: Timed out waiting for tree item Project1 | ||
| at org.eclipse.swtbot.swt.finder.widgets.SWTBotTree.getTreeItem(SWTBotTree.java:577) | ||
| at ...NewEspressifIDFProjectClangFilesTest$Fixture.whenClangdFileDeleted(NewEspressifIDFProjectClangFilesTest.java:168) | ||
| at ...NewEspressifIDFProjectClangFilesTest.shouldRecreateClangdFileAfterDeletionAndVerifyContent(NewEspressifIDFProjectClangFilesTest.java:81) | ||
| Caused by: TimeoutException: Timeout after: 5000 ms.: Could not find node with text Project1 | ||
| ``` | ||
|
|
||
| ## Root cause | ||
|
|
||
| ### Failure #1 — stale clangd `--compile-commands-dir=` preference | ||
|
|
||
| Method execution order is `@FixMethodOrder(MethodSorters.NAME_ASCENDING)`, so the sequence is: | ||
|
|
||
| 1. `shouldApplyClangFormatSettingsWhenAutoSaveIsEnabled` (uses `Project2`) | ||
| 2. `shouldHaveClangFilesPresentAndContentCorrectForNewProject` (uses `Project1`) | ||
| 3. `shouldMatchExpectedClangdArgumentsAfterBuildingProjects` ← **fails here** | ||
| 4. `shouldRecreateClangdFileAfterDeletionAndVerifyContent` ← cascading failure | ||
|
|
||
| Inside test #3, the relevant lines are: | ||
|
|
||
| ```105:113:tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java | ||
| public void shouldMatchExpectedClangdArgumentsAfterBuildingProjects() throws Exception | ||
| { | ||
| Fixture.whenProjectIsBuiltUsingContextMenu(CLEAN_PROJECT2); | ||
| Fixture.thenCheckClangdArgumentAfterProjectBuilt(CLEAN_PROJECT2); | ||
| Fixture.whenSelectProjectInLaunchConfig(); | ||
| Fixture.whenProjectIsBuiltUsingContextMenu(CLEAN_PROJECT1); | ||
| Fixture.thenCheckClangdArgumentAfterProjectBuilt(CLEAN_PROJECT1); | ||
| Fixture.thenClangdDriversUpdateOnSelectedTarget(); | ||
| } | ||
| ``` | ||
|
|
||
| The assertion verifies the value displayed in the _clangd Additional arguments_ preference textbox under Window → Preferences → C/C++ → Editor (LSP) → clangd. That preference is a **workspace-scope** (`InstanceScope`) singleton, not per-project: | ||
|
|
||
| ```79:93:bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java | ||
| public void updateCompileCommandsDir(String buildDir) | ||
| { | ||
| String qualifier = configuration.qualifier(); | ||
| String identifier = ClangdMetadata.Predefined.additionalOptions.identifer(); | ||
| IEclipsePreferences preferences = InstanceScope.INSTANCE.getNode(qualifier); | ||
|
|
||
| String existingOptions = preferences.get(identifier, StringUtil.EMPTY); | ||
| String compileCommandsDirString = "--compile-commands-dir="; //$NON-NLS-1$ | ||
| String newCompuileCommandsDirString = compileCommandsDirString + buildDir; | ||
| String updatedOptions = existingOptions.contains(compileCommandsDirString) | ||
| ? existingOptions.replaceAll(compileCommandsDirString + ".+", //$NON-NLS-1$ | ||
| Matcher.quoteReplacement(newCompuileCommandsDirString)) | ||
| : newCompuileCommandsDirString; | ||
| preferences.put(identifier, updatedOptions); | ||
| } | ||
| ``` | ||
|
|
||
| The only place that calls `updateCompileCommandsDir(...)` during a normal build is guarded by an error-count check: | ||
|
|
||
| ```410:419:bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java | ||
| if (!monitor.isCanceled() && epm.getErrorCount() == 0) | ||
| { | ||
| project.refreshLocal(IResource.DEPTH_INFINITE, monitor); | ||
| ParitionSizeHandler paritionSizeHandler = new ParitionSizeHandler(project, infoStream, console); | ||
| paritionSizeHandler.startCheckingSize(); | ||
|
|
||
| LspService lspService = new LspService(); | ||
| lspService.updateCompileCommandsDir(buildDir.toString()); | ||
| lspService.restartLspServers(); | ||
| } | ||
| ``` | ||
|
|
||
| So the preference is rewritten **only when**: | ||
| - the build is not cancelled, **and** | ||
| - `ErrorParserManager.getErrorCount() == 0`, **and** | ||
| - the build has actually progressed to the `Build complete` console line that `waitForProjectBuild` keys on. | ||
|
|
||
| On Linux this race is visible: | ||
| - After Project2 is built, the preference correctly reads `.../Project2/build` (assertion #1 passes). | ||
| - `whenSelectProjectInLaunchConfig()` switches the launch bar to `Project1` — this does **not** rewrite the clangd preference. | ||
| - Project1 is built. The test's `waitForProjectBuild` returns as soon as the CDT Build Console prints `Build complete`, but the preference write inside `IDFBuildConfiguration.build(...)` happens on the builder thread after the post-build refresh and `ParitionSizeHandler` start — any of these can lag, and if Project1's build reports a non-zero error count (e.g. from a transient parser error on the self-hosted runner) the `updateCompileCommandsDir` call is skipped entirely. | ||
| - `whenOpenClangdPreferences()` opens the dialog and reads the stale `.../Project2/build` value, so `assertEquals` fails at line 356. | ||
|
|
||
| The fix committed on 2026-01-13 (`4d9b9fe7` "fix: 'shouldMatchExpectedClangdArgumentsAfterBuildingProjects' test") already added `whenSelectProjectInLaunchConfig()` to address this same stale-value class of bug. On the Brno self-hosted Ubuntu runner (`BrnoUBU0004`) the fix is still not sufficient because Linux runs the full suite in ~31 m vs macOS's ~12 m — timing is ~2.5x slower. | ||
|
|
||
| ### Failure #2 — cascading UI state | ||
|
|
||
| `shouldRecreateClangdFileAfterDeletionAndVerifyContent` immediately calls: | ||
|
|
||
| ```80:82:tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java | ||
| { | ||
| Fixture.whenClangdFileDeleted(CLEAN_PROJECT1); | ||
| Fixture.thenClangdFileIsAbsent(CLEAN_PROJECT1); | ||
| ``` | ||
|
|
||
| …which calls `bot.tree().getTreeItem("Project1")`. When the previous test aborted with a `ComparisonFailure`, the Preferences dialog was still modal (or had just been closed), the Project Explorer no longer had focus, and the background LSP-server restart triggered after Project2's build was still running. SWTBot's default 5 s wait expires before `Project1` is re-rendered in the tree, producing the `WidgetNotFoundException`. | ||
|
|
||
| This is a **knock-on failure**, not an independent bug. Fixing Failure #1 is expected to fix Failure #2 too. | ||
|
|
||
| ## Why macOS passed but Linux failed | ||
|
|
||
| - `build_macos` completed in **11 m 47 s**; `build_linux` took **31 m 3 s**. The Linux runner is ~2.5x slower and far more likely to lose the race described above. | ||
| - Both jobs run on self-hosted runners (`runs-on: ide eclipse` group) but the Linux runner additionally ran the eim-based ESP-IDF installer, which emitted `Failed to copy OpenOCD rules: ... Permission denied (os error 13)` (benign) and a burst of Pydantic serializer warnings while resolving IDF component manager dependencies. Neither of these causes test failures directly, but they lengthen setup and push timing margins tighter. | ||
| - PR #1438 touched no product or test code, so there is no new regression — the failure is an existing flaky-test problem exposed by the slow Linux runner. | ||
|
|
||
| ## Other noise in the log (informational, not failure-causing) | ||
|
|
||
| - `ERROR - Failed to copy OpenOCD rules: Failed to copy /tmp/esp/openocd-esp32/.../60-openocd.rules to /etc/udev/rules.d/60-openocd.rules . Make sure you have the necessary permissions.` — the runner user isn't `root`; udev rules copy fails. Safe to ignore for CI. | ||
| - Many `PydanticSerializationUnexpectedValue` warnings from `/tmp/esp/python/v5.4/venv/lib/python3.10/site-packages/pydantic/main.py` during ESP-IDF component dependency resolution. | ||
| - `WARN - Mirror ping failed for https://mirrors.aliyun.com: 403` / `https://pypi.tuna.tsinghua.edu.cn: TimedOut` — expected from the Brno network reaching Chinese mirrors. | ||
| - `tm4e` grammar registry warnings (`No grammar source for scope [source.scala]` etc.) and `joni.exception.SyntaxException: invalid pattern in look-behind` from the TextMate tokenizer — known upstream noise, not test failures. | ||
| - `Node.js 20 actions are deprecated` — warning only; won't break until September 2026. | ||
|
|
||
| ## Suggested remediation | ||
|
|
||
| Short term (stabilise CI, same day): | ||
|
|
||
| 1. **Re-run the failing job.** The two tests are flaky on the Linux self-hosted runner and frequently pass on retry. The run has an explicit "fix: add pr comment after the build with download links" merge commit that didn't change product code, so a re-run is safe. | ||
| 2. If re-run still fails, temporarily set `fail-on-error: false` on `phoenix-actions/test-reporting@v12` in `ci.yml` for the Linux job, or mark the two tests `@Ignore` until the real fix lands. | ||
|
|
||
| Medium term (fix the flake): | ||
|
|
||
| 3. In `IDFBuildConfiguration.build(...)`, update the clangd `--compile-commands-dir=` preference **unconditionally** when `compile_commands.json` exists in `buildDir`, rather than gating it on `epm.getErrorCount() == 0`. A compile commands database is produced by CMake before compilation and is still meaningful for LSP even if later compile steps report errors. | ||
| 4. After writing the preference, call `preferences.flush()` to force the `InstanceScope` write to the backing store before `restartLspServers()` returns. | ||
| 5. In the test, replace the direct `whenOpenClangdPreferences() + assertEquals(...)` sequence with a polling wait on the `InstanceScope` preference value (`TestWidgetWaitUtility.waitUntil(...)`), then open the dialog once the value is correct. | ||
| 6. Split `shouldMatchExpectedClangdArgumentsAfterBuildingProjects` into two `@Test` methods — one per project — with a `@Before` hook that re-focuses the Project Explorer, so a Project2-phase failure can't cascade into a Project1-phase failure. | ||
| 7. Add a recovery step at the top of `shouldRecreateClangdFileAfterDeletionAndVerifyContent` that re-opens/focuses the Project Explorer and re-selects `Project1` (same pattern as `whenNewProjectIsSelected`) before assuming the tree item is visible. | ||
|
|
||
| Long term: | ||
|
|
||
| 8. Move the self-hosted Linux runner to hardware with comparable speed to the macOS runners, or lower the SWTBot default timeout only for this test class rather than globally. | ||
|
|
||
| ## References | ||
|
|
||
| - Failing run: <https://github.com/espressif/idf-eclipse-plugin/actions/runs/24703204267> | ||
| - Failing job: <https://github.com/espressif/idf-eclipse-plugin/actions/runs/24703204267/job/72250690470> | ||
| - Test source: [`tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java`](tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectClangFilesTest.java) | ||
| - Related previous fix: commit `4d9b9fe7` — "fix: 'shouldMatchExpectedClangdArgumentsAfterBuildingProjects' test" (2026-01-13) | ||
| - PR that triggered this run: [#1438](https://github.com/espressif/idf-eclipse-plugin/pull/1438) — documentation/CI-only change to `pr-comment.yml`. |
There was a problem hiding this comment.
Out-of-scope file: this CI-failure analysis doesn't belong in this PR.
This document analyzes a Linux CI failure attributed to PR #1438 and the flaky NewEspressifIDFProjectClangFilesTest. None of the content relates to the EIM GUI/CLI launch fixes that this PR delivers (path resolver, isEimGuiCapable, macOS CLI launcher). Mixing unrelated investigation notes into a feature PR makes the history harder to read and may end up shipped as a top-level repo artifact (BUILD_FAILURE_ANALYSIS_24703204267.md) that future readers won't be able to associate with anything.
Suggest moving this analysis to an issue/PR comment, a discussion, or — if it must live in the repo — to a dedicated docs PR (e.g. under docs/ci/ with a stable filename, not a run-id‑suffixed one). Also worth deciding whether such generated forensic notes belong in version control at all.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The operating system from Apple is written “macOS”.
Context: ...ions/runs/24703204267/job/72250690470). build_macos on the same run passed. ## TL;DR The...
(MAC_OS)
[uncategorized] ~9-~9: The official name of this software platform is spelled with a capital “H”.
Context: ... break the build. PR #1438 only touched .github/workflows/pr-comment.yml (a post-build...
(GITHUB)
[grammar] ~110-~110: Ensure spelling is correct
Context: ...thread after the post-build refresh and ParitionSizeHandler start — any of these can lag, and if Project1's bui...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@BUILD_FAILURE_ANALYSIS_24703204267.md` around lines 1 - 168, The file
BUILD_FAILURE_ANALYSIS_24703204267.md is out-of-scope for this PR and should not
be merged here; remove it from the branch/PR and instead move the analysis into
one of: a new issue, a PR comment, a discussion thread, or a dedicated docs PR
(e.g. under docs/ci/ with a stable name), referencing the failing test class
NewEspressifIDFProjectClangFilesTest and CI run details as needed; update the
commit to delete BUILD_FAILURE_ANALYSIS_24703204267.md from this PR and add the
analysis to the chosen external location so the feature PR only contains the
intended EIM GUI/CLI changes.
Comment on lines
+116
to
+134
| 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); | ||
| } |
There was a problem hiding this comment.
Reap the launcher process and harden parseFirstLongLine against shell stderr.
Two related concerns in launchCliDirect:
- The
launcherProcessis neverwaitFor()-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; callingwaitFor()(with a small timeout) makes lifecycle deterministic. - With
redirectErrorStream(true), any shell-init message printed to stderr (warnings from.bash_profile, locale errors, etc.) is interleaved withecho $!.parseFirstLongLinewill then pick whichever line appears first — e.g. abash: warning: ...line — and fail to find the PID. Combined with thebash -lcissue, 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).
Comment on lines
+81
to
117
| public String resolveEimExecutablePath(EimJson eimJson) | ||
| { | ||
| String fromPath = findEimOnSystemPath(); | ||
| if (!StringUtil.isEmpty(fromPath)) | ||
| { | ||
| return fromPath; | ||
| } | ||
|
|
||
| if (eimJson != null && !StringUtil.isEmpty(eimJson.getEimPath())) | ||
| { | ||
| String jsonPath = eimJson.getEimPath(); | ||
| if (Files.exists(Paths.get(jsonPath))) | ||
| { | ||
| return jsonPath; | ||
| } | ||
| } | ||
|
|
||
| String eimExePathEnv = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH); | ||
| boolean exists = !StringUtil.isEmpty(eimExePathEnv) && Files.exists(Paths.get(eimExePathEnv)); | ||
| if (!exists) | ||
| if (!StringUtil.isEmpty(eimExePathEnv) && Files.exists(Paths.get(eimExePathEnv))) | ||
| { | ||
| return eimExePathEnv; | ||
| } | ||
|
|
||
| Path defaultEimPath = getDefaultEimPath(); | ||
| if (defaultEimPath != null && Files.exists(defaultEimPath)) | ||
| { | ||
| return defaultEimPath.toString(); | ||
| } | ||
|
|
||
| Path cliEimPath = getDefaultCliEimPath(); | ||
| if (cliEimPath != null && Files.exists(cliEimPath)) | ||
| { | ||
| // Fallback: check in user home .espressif/eim_gui folder | ||
| Path defaultEimPath = getDefaultEimPath(); | ||
| if (defaultEimPath != null) | ||
| exists = Files.exists(defaultEimPath); | ||
| return cliEimPath.toString(); | ||
| } | ||
| return exists; | ||
|
|
||
| return StringUtil.EMPTY; | ||
| } |
There was a problem hiding this comment.
Guard Files.exists against malformed path strings.
Paths.get(jsonPath) and Paths.get(eimExePathEnv) can throw InvalidPathException (a RuntimeException) if the JSON config or the user's EIM_PATH env var contains an illegal path (e.g. NUL byte, malformed Windows path). Today that would bubble out of resolveEimExecutablePath and crash startup paths in EspressifToolStartup.earlyStartup/startExportOldConfig.
A simple try/catch around each existence check (or a small helper that returns false on InvalidPathException) keeps resolution robust against bad external input, and the call site can then continue to the next priority level.
🛡️ Proposed helper
+ private static boolean fileExistsSafe(String path)
+ {
+ try
+ {
+ return !StringUtil.isEmpty(path) && Files.exists(Paths.get(path));
+ }
+ catch (java.nio.file.InvalidPathException e)
+ {
+ Logger.log("Invalid path encountered during EIM resolution: " + path); //$NON-NLS-1$
+ return false;
+ }
+ }…and use it in the JSON/env-var checks.
🤖 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 81 - 117, The resolveEimExecutablePath method currently calls
Files.exists(Paths.get(...)) on values from eimJson.getEimPath() and
idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH) which can
throw InvalidPathException for malformed strings; fix by adding a small helper
(e.g., safePathExists(String)) that wraps Paths.get(...)/Files.exists(...) in a
try/catch and returns false on InvalidPathException, then replace the direct
Files.exists(Paths.get(jsonPath)) and Files.exists(Paths.get(eimExePathEnv))
checks with calls to that helper so resolveEimExecutablePath (and callers like
earlyStartup/startExportOldConfig) won’t crash on bad external input.
Comment on lines
+242
to
267
| public boolean isEimGuiCapable(String eimPath) | ||
| { | ||
| idfEnvironmentVariables.addEnvVariable(IDFEnvironmentVariables.EIM_PATH, 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
Comment on lines
+102
to
+217
| ## 3. Startup: how `EIM_PATH` gets set (`EspressifToolStartup`) | ||
|
|
||
| On IDE startup (`IStartup.earlyStartup`), logic is: | ||
|
|
||
| 1. If **EIM is not considered installed** and **`eim_idf.json` is missing**, the flow stops early (user notified). | ||
| 2. Otherwise **`eimJson`** is loaded via `ToolInitializer.loadEimJson()` (parser above). | ||
| 3. **`EIM_PATH`** is then set: | ||
| - If **`eimJson != null`** → use **`eimJson.getEimPath()`** (authoritative path from EIM’s JSON). | ||
| - Else → **fallback** `toolInitializer.findAndSetEimPath()` (see section 4). | ||
|
|
||
| ```84:127:bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java | ||
| if (!toolInitializer.isEimInstalled() && !toolInitializer.isEimIdfJsonPresent()) | ||
| { | ||
| Logger.log("EIM not installed"); | ||
| notifyMissingTools(); | ||
| return; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| eimJson = toolInitializer.loadEimJson(); | ||
| } | ||
| catch (EimVersionMismatchException e) | ||
| { | ||
| Logger.log(e); | ||
| return; | ||
| } | ||
| // ... | ||
| // Set EimPath on every startup to ensure proper path in configurations | ||
| if (eimJson != null) | ||
| { | ||
| idfEnvironmentVariables.addEnvVariable(IDFEnvironmentVariables.EIM_PATH, eimJson.getEimPath()); | ||
| } | ||
| else | ||
| { | ||
| // Fail-safe call to ensure if the eim is in Applications or user.home it is setup in env vars | ||
| toolInitializer.findAndSetEimPath(); | ||
| } | ||
| ``` | ||
|
|
||
| **Important:** If `loadEimJson()` fails or returns null (e.g. JSON version mismatch exception causes early `return` before lines 118–126), **`EIM_PATH` is never set** in that startup path—the code exits at line 96–98. | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Fallback “default locations” (`ToolInitializer`) | ||
|
|
||
| When there is **no usable `eimJson`**, `findAndSetEimPath()` assigns **`EIM_PATH`** to a **fixed default path per OS** (whether or not the file exists is **not** checked inside `findAndSetEimPath` itself): | ||
|
|
||
| ```163:169:bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java | ||
| public void findAndSetEimPath() | ||
| { | ||
| Path defaultEimPath = getDefaultEimPath(); | ||
|
|
||
| if (defaultEimPath != null) | ||
| setEimPathInEnvVar(defaultEimPath.toString()); | ||
| } | ||
| ``` | ||
|
|
||
| `getDefaultEimPath()`: | ||
|
|
||
| ```138:161:bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java | ||
| public Path getDefaultEimPath() | ||
| { | ||
| String userHome = System.getProperty("user.home"); //$NON-NLS-1$ | ||
| Path defaultEimPath; | ||
| String os = Platform.getOS(); | ||
| if (os.equals(Platform.OS_WIN32)) | ||
| { | ||
| defaultEimPath = Paths.get(userHome, ".espressif", "eim_gui", //$NON-NLS-1$//$NON-NLS-2$ | ||
| "eim.exe"); //$NON-NLS-1$ | ||
| } | ||
| else if (os.equals(Platform.OS_MACOSX)) | ||
| { | ||
| defaultEimPath = Paths.get("/Applications", //$NON-NLS-1$ | ||
| "eim.app", "Contents", //$NON-NLS-1$//$NON-NLS-2$ | ||
| "MacOS", "eim"); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } | ||
| else | ||
| { | ||
| defaultEimPath = Paths.get(userHome, ".espressif", //$NON-NLS-1$ | ||
| "eim_gui", "eim"); //$NON-NLS-1$//$NON-NLS-2$ | ||
| } | ||
|
|
||
| return defaultEimPath; | ||
| } | ||
| ``` | ||
|
|
||
| **Default executable paths encoded here:** | ||
|
|
||
| | OS | Default `EIM_PATH` string | | ||
| |----|---------------------------| | ||
| | **Windows** | `%USERPROFILE%\.espressif\eim_gui\eim.exe` | | ||
| | **macOS** | `/Applications/eim.app/Contents/MacOS/eim` | | ||
| | **Linux** | `~/.espressif/eim_gui/eim` | | ||
|
|
||
| ### 4.1 “Is EIM installed?” check | ||
|
|
||
| `isEimInstalled()` first checks whether **`EIM_PATH`** points to an **existing file**. If not, it falls back to checking **`getDefaultEimPath()`** on disk: | ||
|
|
||
| ```49:61:bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java | ||
| public boolean isEimInstalled() | ||
| { | ||
| String eimExePathEnv = idfEnvironmentVariables.getEnvValue(IDFEnvironmentVariables.EIM_PATH); | ||
| boolean exists = !StringUtil.isEmpty(eimExePathEnv) && Files.exists(Paths.get(eimExePathEnv)); | ||
| if (!exists) | ||
| { | ||
| // Fallback: check in user home .espressif/eim_gui folder | ||
| Path defaultEimPath = getDefaultEimPath(); | ||
| if (defaultEimPath != null) | ||
| exists = Files.exists(defaultEimPath); | ||
| } | ||
| return exists; | ||
| } | ||
| ``` | ||
|
|
||
| So detection **does not** scan `PATH`; it uses **saved `EIM_PATH`** and **default well-known paths**. |
There was a problem hiding this comment.
Documentation is out of sync with the code in this PR — it describes the removed behavior.
This file still documents the pre-refactor flow:
- Section 3 (Startup, lines 108–139) shows the old
if (eimJson != null) addEnvVariable(EIM_PATH, eimJson.getEimPath()) else findAndSetEimPath()branching. In this PR,EspressifToolStartup.earlyStartupwas changed to calltoolInitializer.resolveEimExecutablePath(eimJson)and thefindAndSetEimPathfallback was removed. - Section 4 (lines 146–158) documents
findAndSetEimPath(), which has been deleted fromToolInitializerin this PR (replaced byresolveEimExecutablePath). - Section 4.1 (lines 197–217) shows the old
isEimInstalled()body that readsEIM_PATHand falls back togetDefaultEimPath(). The new implementation simply delegates to!StringUtil.isEmpty(resolveEimExecutablePath(null)), which also scans systemPATH,eim_idf.json, env var, GUI default, and CLI default — a meaningfully different contract (e.g. the doc explicitly says "detection does not scanPATH", which is now false). - Section 7 summary table does not mention the new resolver,
getDefaultCliEimPath,isEimGuiCapable, orUSER_EIM_CLI_DIR.
Net effect: a reader following this doc will have completely wrong expectations about how EIM_PATH is resolved — and the doc even contradicts itself versus the very PR that introduces it.
Please refresh sections 3, 4, 4.1, and 7 to reflect:
- The five-step priority order: system
PATH→eimPathfromeim_idf.json→EIM_PATHenv var → default GUI install (~/.espressif/eim_gui/...) → default CLI install (~/.espressif/eim/...). - The single entry point
ToolInitializer.resolveEimExecutablePath(EimJson). - The new
getDefaultCliEimPath()andUSER_EIM_CLI_DIR. isEimGuiCapable(String)and how it gates GUI vs CLI launch on macOS (and the newMacOsEimLauncherStrategy.launchCliDirectpath for non-.appbinaries).- That
isEimInstalled()is now resolver-backed and does scanPATH.
Happy to draft the rewrite if useful.
🧰 Tools
🪛 LanguageTool
[style] ~148-~148: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...`** to a fixed default path per OS (whether or not the file exists is not checked insi...
(WHETHER)
🤖 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 `@docs/EIM_PATH_DISCOVERY.md` around lines 102 - 217, Update the doc sections
to match the refactor: replace the old startup branching with
EspressifToolStartup.earlyStartup calling
ToolInitializer.resolveEimExecutablePath(eimJson) as the single entry point for
EIM resolution, document the new five-step priority order (system PATH →
eim_idf.json eimPath → EIM_PATH env var → default GUI install → default CLI
install), remove references to the deleted findAndSetEimPath() and old
getDefaultEimPath-only behavior and instead describe getDefaultCliEimPath() and
USER_EIM_CLI_DIR, explain isEimGuiCapable(String) and the
MacOsEimLauncherStrategy.launchCliDirect path for non-.app binaries, and note
that isEimInstalled() now delegates to the resolver (so it scans PATH).
- Add CLI install path (~/.espressif/eim/) as final fallback in resolver - Add getDefaultCliEimPath() for platform-specific CLI binary locations - Add isEimGuiCapable() to detect GUI support via `eim gui --help` - Handle CLI-only EIM binaries on macOS by launching directly instead of attempting AppleScript open on non-.app paths - Add USER_EIM_CLI_DIR constant in EimConstants Aligns with vscode-esp-idf-extension PR #1822 approach of checking both GUI and CLI managed install locations and detecting GUI capability.
alirana01
force-pushed
the
eim_gui_cli_launch_fixes
branch
from
dc1e322 to
87c8ecf
Compare
Collaborator
|
@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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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