IEP-1619 DFU flash output truncated in IDE console #1294
Conversation
WalkthroughDfuCommandsUtil.flashDfuBins now merges IDF/CDT environment with the system environment (via IDFUtil.getSystemEnv), adds PYTHONUNBUFFERED=1, normalizes/merges PATH/Path (deduplicating entries), logs the merged map, and uses that merged environment for building command strings and launching the process. IDFUtil.getSystemEnv now prepends common Homebrew bin paths to PATH when present. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as flashDfuBins()
participant IDF as IDFEnvironmentVariables
participant SYS as IDFUtil.getSystemEnv()
participant Merge as Env Merger
participant Normalize as PATH Normalizer
participant Proc as Process.exec
Caller->>IDF: get IDF env map
Caller->>SYS: get system env map
Merge->>Merge: combine IDF + system maps
Merge->>Merge: put PYTHONUNBUFFERED=1
Merge->>Normalize: merge PATH & Path, dedupe entries
Normalize-->>Merge: single normalized PATH
Merge->>Proc: build envArray & command strings
Proc-->>Caller: process result / exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/DfuCommandsUtil.java (1)
138-144: Don’t override PYTHONUNBUFFERED if the user already set it.Setting PYTHONUNBUFFERED directly on idfEnvMap will override any user-specified value. Prefer setting it on the merged environment with putIfAbsent to avoid clobbering user intent.
Proposed change (place after environment.putAll(idfEnvMap)):
// Disable buffering of Python output (do not override if user set it) environment.putIfAbsent("PYTHONUNBUFFERED", "1"); // $NON-NLS-1$ //$NON-NLS-2$
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/DfuCommandsUtil.java(2 hunks)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: build_macos
- GitHub Check: build_windows
🔇 Additional comments (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/DfuCommandsUtil.java (4)
10-16: Imports look appropriate for the new env merge logic.The added imports (HashMap, Objects, Collectors, Stream) align with the new environment merging code.
158-161: LGTM: Environment array construction is correct.Building env entries as KEY=VALUE strings for DebugPlugin.exec is appropriate.
167-171: LGTM: Passing the merged env to the spawned process.Using envArray with DebugPlugin.exec ensures the child sees the normalized PATH and the unbuffered Python setting.
147-156: Use File.pathSeparator and correct PATH key per OS, with deduplicationHard-coding “;” only works on Windows, and merging into “PATH” will fail on Unix/macOS (which use “:” and the “PATH” key) and may not be the canonical key on Windows (usually “Path”). Instead:
- Use
File.pathSeparatorfor portability.- Split and dedupe each segment before rejoining.
- On Windows, write back to “Path” and remove “PATH”; on other OSes, write to “PATH” and remove “Path”.
Suggested diff:
- // Merge PATH and Path - String mergedPath = Stream.of(environment.get("PATH"), environment.get("Path")).filter(Objects::nonNull) //$NON-NLS-1$ //$NON-NLS-2$ - .collect(Collectors.joining(";")); //$NON-NLS-1$ - - // Remove duplicate Path - if (!mergedPath.isEmpty()) - { - environment.put("PATH", mergedPath); //$NON-NLS-1$ - environment.remove("Path"); //$NON-NLS-1$ - } + // Merge PATH and Path with correct separator and dedupe + String sep = File.pathSeparator; + String pWin = environment.get("Path"); //$NON-NLS-1$ + String pUnix = environment.get("PATH"); //$NON-NLS-1$ + String merged = Stream.of(pWin, pUnix) + .filter(Objects::nonNull) + .flatMap(s -> Arrays.stream(s.split(Pattern.quote(sep)))) + .filter(s -> !s.isEmpty()) + .distinct() + .collect(Collectors.joining(sep)); + if (!merged.isEmpty()) { + boolean isWin = System.getProperty("os.name").toLowerCase().contains("win"); //$NON-NLS-1$ //$NON-NLS-2$ + if (isWin) { + environment.put("Path", merged); //$NON-NLS-1$ + environment.remove("PATH"); //$NON-NLS-1$ + } else { + environment.put("PATH", merged); //$NON-NLS-1$ + environment.remove("Path"); //$NON-NLS-1$ + } + }Please verify manually:
- Windows: launch via IDE, confirm
dfu-utilis found on the mergedPathand that flashing works as in the terminal.- Linux/macOS: check
$ echo $PATHbefore/after and ensuredfu-utilresolution remains intact.
| Logger.log(environment.toString()); | ||
|
|
There was a problem hiding this comment.
Avoid logging the full environment — risk of leaking secrets and noisy logs.
environment.toString() can expose sensitive variables (tokens, proxies) and an excessively long PATH into the IDE log. Log only keys or remove the log.
Apply this diff:
- Logger.log(environment.toString());
+ Logger.log("DFU merged environment keys: " + environment.keySet()); //$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.
| Logger.log(environment.toString()); | |
| Logger.log("DFU merged environment keys: " + environment.keySet()); //$NON-NLS-1$ |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/DfuCommandsUtil.java
around lines 145-146, the call Logger.log(environment.toString()) exposes full
environment contents (potential secrets, long PATH) and must be removed or
replaced; instead, log only safe information such as the environment variable
names (keys) or a filtered map that excludes sensitive keys (e.g., TOKEN,
SECRET, PASSWORD, AWS*, HTTP_PROXY, HTTPS_PROXY) and truncate long values, or
remove the log entirely; implement filtering to build a short, non-sensitive
summary (keys or key=value with masked/trimmed values) and use that for
Logger.log.
| Map<String, String> environment = new HashMap<>(IDFUtil.getSystemEnv()); | ||
| environment.putAll(idfEnvMap); | ||
|
|
||
| Logger.log(environment.toString()); | ||
|
|
There was a problem hiding this comment.
Why can't we rely only on the PATH environment variable configured in the CDT build environment path?
|
@sigmaaa hi ! Tested under: Windows 11: Mac: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
853-876: Make PATH merging robust (use platform separator, avoid trailing empty entry, and dedup by component).Current logic:
- Hard-codes ":" as the separator (non-portable).
- Uses contains() which can yield false positives (e.g., "/opt/homebrew/binutils" contains "/opt/homebrew/bin").
- Leaves a trailing ":" if PATH is empty, adding CWD implicitly to PATH on POSIX.
Refactor to:
- Use File.pathSeparator.
- Dedup by components with a LinkedHashSet, preserving order and prepending Homebrew paths only if present.
- Avoid trailing separators.
Apply this diff within this range:
- // Merge Homebrew bin paths into PATH - String existingPath = env.getOrDefault("PATH", ""); //$NON-NLS-1$ //$NON-NLS-2$ - StringBuilder newPath = new StringBuilder(); - - String[] brewPaths = { "/usr/local/bin", "/opt/homebrew/bin" }; //$NON-NLS-1$ //$NON-NLS-2$ - - for (String brewPath : brewPaths) - { - if (Files.exists(Paths.get(brewPath)) && !existingPath.contains(brewPath)) - { - newPath.append(brewPath).append(":"); //$NON-NLS-1$ - } - } - - // Append the original PATH at the end - newPath.append(existingPath); - env.put("PATH", newPath.toString()); //$NON-NLS-1$ + // Merge Homebrew bin paths into PATH + String existingPath = env.getOrDefault("PATH", ""); //$NON-NLS-1$ //$NON-NLS-2$ + String[] brewPaths = { "/usr/local/bin", "/opt/homebrew/bin" }; //$NON-NLS-1$ //$NON-NLS-2$ + + java.util.LinkedHashSet<String> parts = new java.util.LinkedHashSet<>(); + for (String brewPath : brewPaths) { + if (Files.isDirectory(Paths.get(brewPath))) { + parts.add(brewPath); + } + } + if (!existingPath.isBlank()) { + for (String p : existingPath.split(java.util.regex.Pattern.quote(File.pathSeparator))) { + if (!p.isBlank()) { + parts.add(p); + } + } + } + env.put("PATH", String.join(File.pathSeparator, parts)); //$NON-NLS-1$Add this import outside this range:
import java.util.LinkedHashSet;
929-995: Optional: Tighten file checks & unify environment setupNon-blocking improvements in bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (runEspDetectConfigScript, lines 929–995):
- Replace
exists()withisFile()when checking esp-config.json, esp_detect_config.py, and the OpenOCD binary.- Reuse the computed
scriptFile(from toolsDir) instead of calling espDetectConfigScriptExists().- In your command list, use
scriptFile.getAbsolutePath()rather than the oldscriptPathvariable.- For consistency with other call sites (e.g. AbstractToolsHandler, ToolsJob), consider swapping
new IDFEnvironmentVariables().getSystemEnvMap()
for
IDFUtil.getSystemEnv()
to carry through any PATH tweaks (Homebrew, etc.).- Optionally, to unbuffer Python output, add
env.putIfAbsent("PYTHONUNBUFFERED", "1");Suggested diff:
--- a/bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java +++ b/.../IDFUtil.java @@ runEspDetectConfigScript() { - if (!configFile.exists()) { + if (!configFile.isFile()) { @@ - if (!espDetectConfigScriptExists()) { - Logger.log("esp_detect_config.py not found at expected location: " - + new File(toolsDir, "esp_detect_config.py").getAbsolutePath()); - return null; - } + File scriptFile = new File(toolsDir, "esp_detect_config.py"); + if (!scriptFile.isFile()) { + Logger.log("esp_detect_config.py not found at expected location: " + + scriptFile.getAbsolutePath()); + return null; + } @@ - File openocdBin = new File(openocdBinDir, openocdExecutable); - if (!openocdBin.exists()) { + File openocdBin = new File(openocdBinDir, openocdExecutable); + if (!openocdBin.isFile()) { Logger.log("OpenOCD binary not found at expected location."); return null; } @@ - command.add(scriptPath); + command.add(scriptFile.getAbsolutePath()); @@ - Map<String, String> env = new IDFEnvironmentVariables().getSystemEnvMap(); + Map<String, String> env = IDFUtil.getSystemEnv(); + env.putIfAbsent("PYTHONUNBUFFERED", "1");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(4 hunks)
⏰ 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). (3)
- GitHub Check: build_macos
- GitHub Check: build
- GitHub Check: build_windows
🔇 Additional comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
908-923: LGTM on API/logic changes for espDetectConfigScriptExists.Bracing, doc improvements, and early return on missing OpenOCD path are clear and do not change behavior.
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
LGTM.
@AndriiFilippov Implementation has changed after your review - PTAL.
|
Windows 11 ✅ LGTM 👍 |
Description
The issue was that the child process couldn’t detect dfu-util because the environment contained duplicate Path variables. This was fixed by merging the paths and removing the duplicate.
Fixes # (IEP-1619)
Type of change
Please delete options that are not relevant.
How has this been tested?
Run dfu flash with UI option and compare it to the idf.py dfu flash from the terminal. Output and functionality must be the same
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit