IEP-1668 idf.py command not recognized in the Linux terminal 4.0.0 #1375
Conversation
WalkthroughModifies IDFConsoleLauncherDelegate.java to append the ESP-IDF tools directory to the PATH environment variable if not already present. Computes idfToolsPath from IDFPath and conditionally appends it, with an added import for java.nio.file.Paths. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 1
🧹 Nitpick comments (2)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (2)
342-347: Consider validating directory existence before adding to PATH.The code computes
idfToolsPathbut doesn't verify whether this directory actually exists before adding it to the PATH. Adding non-existent directories to PATH is generally harmless but can clutter the environment and may cause confusion during debugging.Apply this diff to validate the directory exists:
String idfPath = IDFUtil.getIDFPath(); if (idfPath != null) { String idfToolsPath = Paths.get(idfPath, "tools").toAbsolutePath().toString(); //$NON-NLS-1$ + File idfToolsDir = new File(idfToolsPath); String currentPath = envMap.get("PATH"); //$NON-NLS-1$ - if (currentPath != null && !currentPath.contains(idfToolsPath)) { + if (idfToolsDir.exists() && idfToolsDir.isDirectory() && currentPath != null && !currentPath.contains(idfToolsPath)) { envMap.put("PATH", currentPath + File.pathSeparator + idfToolsPath); //$NON-NLS-1$ } }
345-345: Path comparison using contains() may miss normalized or symlinked paths.The
contains()check is a simple string match that won't detect if the same directory is already in PATH under a different representation (e.g., through symlinks, with/without trailing separators, or using relative path components like..). While this is acceptable for a temporary workaround, be aware that it could result in duplicate path entries in edge cases.For more robust path comparison, consider normalizing and comparing canonical paths, though this adds complexity:
// More robust but complex approach (optional) File idfToolsDir = new File(idfToolsPath); if (idfToolsDir.exists() && idfToolsDir.isDirectory() && currentPath != null) { String canonicalToolsPath = idfToolsDir.getCanonicalPath(); boolean alreadyInPath = Arrays.stream(currentPath.split(File.pathSeparator)) .map(p -> { try { return new File(p).getCanonicalPath(); } catch (IOException e) { return p; } }) .anyMatch(canonicalToolsPath::equals); if (!alreadyInPath) { envMap.put("PATH", currentPath + File.pathSeparator + idfToolsPath); } }Note: This requires additional exception handling and may impact performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(58-1044)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_windows
- GitHub Check: build_linux
🔇 Additional comments (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
19-19: LGTM!The import is necessary for the new path computation logic and is correctly placed.
| // Adding esp-idf/tools in the path if it's missing (IEP-1668) | ||
| String idfToolsPath = Paths.get(IDFUtil.getIDFPath(), "tools").toAbsolutePath().toString(); //$NON-NLS-1$ | ||
| String currentPath = envMap.get("PATH"); //$NON-NLS-1$ | ||
|
|
||
| if (currentPath != null && !currentPath.contains(idfToolsPath)) { | ||
| envMap.put("PATH", currentPath + File.pathSeparator + idfToolsPath); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
Add null check for IDFUtil.getIDFPath() to prevent NullPointerException.
According to the relevant code snippets, IDFUtil.getIDFPath() can return null when the IDF path is not found. Passing null to Paths.get() will throw a NullPointerException.
Apply this diff to add defensive null checks:
// Adding esp-idf/tools in the path if it's missing (IEP-1668)
-String idfToolsPath = Paths.get(IDFUtil.getIDFPath(), "tools").toAbsolutePath().toString(); //$NON-NLS-1$
-String currentPath = envMap.get("PATH"); //$NON-NLS-1$
-
-if (currentPath != null && !currentPath.contains(idfToolsPath)) {
- envMap.put("PATH", currentPath + File.pathSeparator + idfToolsPath); //$NON-NLS-1$
-}
+String idfPath = IDFUtil.getIDFPath();
+if (idfPath != null) {
+ String idfToolsPath = Paths.get(idfPath, "tools").toAbsolutePath().toString(); //$NON-NLS-1$
+ String currentPath = envMap.get("PATH"); //$NON-NLS-1$
+
+ if (currentPath != null && !currentPath.contains(idfToolsPath)) {
+ envMap.put("PATH", currentPath + File.pathSeparator + idfToolsPath); //$NON-NLS-1$
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Adding esp-idf/tools in the path if it's missing (IEP-1668) | |
| String idfToolsPath = Paths.get(IDFUtil.getIDFPath(), "tools").toAbsolutePath().toString(); //$NON-NLS-1$ | |
| String currentPath = envMap.get("PATH"); //$NON-NLS-1$ | |
| if (currentPath != null && !currentPath.contains(idfToolsPath)) { | |
| envMap.put("PATH", currentPath + File.pathSeparator + idfToolsPath); //$NON-NLS-1$ | |
| } | |
| // Adding esp-idf/tools in the path if it's missing (IEP-1668) | |
| String idfPath = IDFUtil.getIDFPath(); | |
| if (idfPath != null) { | |
| String idfToolsPath = Paths.get(idfPath, "tools").toAbsolutePath().toString(); //$NON-NLS-1$ | |
| String currentPath = envMap.get("PATH"); //$NON-NLS-1$ | |
| if (currentPath != null && !currentPath.contains(idfToolsPath)) { | |
| envMap.put("PATH", currentPath + File.pathSeparator + idfToolsPath); //$NON-NLS-1$ | |
| } | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java
around lines 341 to 347, guard against IDFUtil.getIDFPath() returning null
before calling Paths.get(): retrieve the idfPath into a local variable, if it's
null do not call Paths.get() and skip modifying PATH (optionally log or set a
fallback), otherwise compute idfToolsPath = Paths.get(idfPath,
"tools").toAbsolutePath().toString() and then proceed to check and append it to
envMap.get("PATH") as before.
There was a problem hiding this comment.
I think if we can improve the whole terminal experience.
We can do something like this and this is working on powershell
C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe -NoProfile -NoExit -Command C:\Espressif\tools\Microsoft.v5.5.1.PowerShell_profile.ps1
we have to modify the defaultShell function in this console launcher delegate to something like this
if (Platform.OS_WIN32.equals(Platform.getOS())) {
if (Files.exists(Paths.get("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"))) { //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
shell = "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"; //$NON-NLS-1$
} else {
shell = "cmd.exe"; //$NON-NLS-1$
}
}
A good failsafe can be implemented if powershell is not found there
For POSIX you can look for similar logic to source the script with launched shell as a profile.
sh -ic ". /path/to/activation_v5.5.1_idf.sh"
Also these changes may help cleanup this class to avoid using too much of the code from the actual abstraction
| envMap.remove("Path"); //$NON-NLS-1$ | ||
| } | ||
| // Adding esp-idf/tools in the path if it's missing (IEP-1668) | ||
| String idfToolsPath = Paths.get(IDFUtil.getIDFPath(), "tools").toAbsolutePath().toString(); //$NON-NLS-1$ |
There was a problem hiding this comment.
we have a constant in IDFConstants TOOLS_FOLDER.
It is not ideal because ideal will be to have a utility function in IDFUtil to return this path
|
Hi @AndriiFilippov Could you verify this? |
|
@sigmaaa hi ! Tested under: idf.py commands recognized. able to build flash monitor via Terminal ✔️ |
Description
Check if esp-idf/tools is in PATH and adding it if it's missing. Temporary workaround
Fixes # (IEP-1668)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.