IEP-1628: Remove conflicting System env variables for export command #1305
Conversation
WalkthroughAdds a new utility to prune IDF-related environment variables and invokes it before launching IDF subprocesses. The cleanup is implemented in IDFUtil and called from ExportIDFTools and ToolsJob (after PATH/git adjustments) with added logging around the cleaned environment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as ToolsJob / ExportIDFTools
participant Env as env Map (copy)
participant IDF as IDFUtil.cleanUpSystemEnvironment
participant Proc as subprocess (idf_tools.py / other)
Caller->>Env: copy system env\n(possibly add Git to PATH)
note right of Env: Environment prepared
Caller->>IDF: cleanUpSystemEnvironment(env)
note right of IDF #D3E4CD: Removes IDF_* and OPENOCD_SCRIPTS keys
IDF-->>Env: cleaned env
Caller->>Proc: launch process with cleaned env
Proc-->>Caller: output / exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (2)
163-182: Simplify cleanup, fix typo, drop extra allocation.No need for a LinkedList or containsKey checks; remove keys directly. Also fix the local variable typo keysToRenmove → keysToRemove.
Apply:
- List<String> keysToRenmove = new LinkedList<>(); - keysToRenmove.add(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH); - keysToRenmove.add(IDFEnvironmentVariables.IDF_PATH); - - for (String key : keysToRenmove) - { - if (environment.containsKey(key)) - { - environment.remove(key); - } - } + for (String key : java.util.Arrays.asList( + IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH, + IDFEnvironmentVariables.IDF_PATH)) { + environment.remove(key); + }Optional: add a one-line debug to aid support without exposing values:
// Logger.log("Removed conflicting env vars: IDF_PYTHON_ENV_PATH, IDF_PATH");Optional nit: tweak Javadoc grammar:
- “Remove variables that can affect … These variables can come from the system environment.”
11-11: Remove unnecessary LinkedList import.After simplifying cleanUpSystemEnvironment, LinkedList is unused. Replace with Arrays (if you use the Arrays.asList variant).
-import java.util.LinkedList; +import java.util.Arrays;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java(3 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). (2)
- GitHub Check: build_macos
- GitHub Check: build
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
LGTM! Could you also check the question I added in the review?
| List<String> keysToRenmove = new LinkedList<>(); | ||
| keysToRenmove.add(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH); | ||
| keysToRenmove.add(IDFEnvironmentVariables.IDF_PATH); |
There was a problem hiding this comment.
Don't we want to exclude other environment variables that have IDF in their name to avoid conflicts? For example, in IDFEnvironmentVariables we also have IDF_CCACHE_ENABLE, IDF_COMPONENT_MANAGER, IDF_MAINTAINER, and ESP_IDF_VERSION, which can potentially affect the plugin's functionality.
There was a problem hiding this comment.
yeah that sounds about right I think I missed those
Thanks
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (1)
170-178: Good coverage of additional IDF_ vars noted earlier.*Including IDF_CCACHE_ENABLE, IDF_COMPONENT_MANAGER, IDF_MAINTAINER, and ESP_IDF_VERSION addresses the prior feedback.
🧹 Nitpick comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (3)
163-187: Make removal case-insensitive, fix typo, simplify list creation.
- keysToRenmove → keysToRemove (typo).
- Environment keys are case-insensitive on Windows; remove using equalsIgnoreCase to be robust.
- Use a fixed list instead of building a LinkedList.
- private void cleanUpSystemEnvironment(Map<String, String> environment) - { - List<String> keysToRenmove = new LinkedList<>(); - keysToRenmove.add(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH); - keysToRenmove.add(IDFEnvironmentVariables.IDF_PATH); - keysToRenmove.add(IDFEnvironmentVariables.ESP_IDF_VERSION); - keysToRenmove.add(IDFEnvironmentVariables.IDF_CCACHE_ENABLE); - keysToRenmove.add(IDFEnvironmentVariables.IDF_COMPONENT_MANAGER); - keysToRenmove.add(IDFEnvironmentVariables.IDF_MAINTAINER); - keysToRenmove.add(IDFEnvironmentVariables.OPENOCD_SCRIPTS); - - for (String key : keysToRenmove) - { - if (environment.containsKey(key)) - { - environment.remove(key); - } - } - } + private void cleanUpSystemEnvironment(final Map<String, String> environment) + { + if (environment == null || environment.isEmpty()) + { + return; + } + + final List<String> keysToRemove = java.util.Arrays.asList( + IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH, + IDFEnvironmentVariables.IDF_PATH, + IDFEnvironmentVariables.ESP_IDF_VERSION, + IDFEnvironmentVariables.IDF_CCACHE_ENABLE, + IDFEnvironmentVariables.IDF_COMPONENT_MANAGER, + IDFEnvironmentVariables.IDF_MAINTAINER, + IDFEnvironmentVariables.OPENOCD_SCRIPTS + ); + + // Remove keys in a case-insensitive way to match Windows env semantics + for (String existingKey : new ArrayList<>(environment.keySet())) + { + for (String target : keysToRemove) + { + if (existingKey.equalsIgnoreCase(target)) + { + environment.remove(existingKey); + break; + } + } + } + }
163-167: Tighten Javadoc phrasing.Minor wording tweak and clarify in-place mutation.
- /** - * Remove the variables which can affect the idf_tools.py export command - * These variables can come from system environment - * @param environment - */ + /** + * Remove variables that can affect the idf_tools.py export command. + * These variables may come from the system environment. + * Mutates the provided environment map in place. + * @param environment environment map passed to the export process + */
11-11: Drop LinkedList import if adopting the refactor.If you switch to a fixed list (Arrays.asList/List.of), remove this import.
- import java.util.LinkedList; + // (no change if you fully qualify Arrays; otherwise) + // import java.util.Arrays;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java(3 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). (1)
- GitHub Check: build_macos
| cleanUpSystemEnvironment(environment); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Apply env cleanup to all export paths, not just the “given IDF path” flow.
The conflicting vars will still leak into these two flows:
- runToolsExportAndProcessOutput(...)
- getToolsExportOutput(...)
Call cleanUpSystemEnvironment(environment) there as well to make the fix complete.
*** a/.../ExportIDFTools.java
--- b/.../ExportIDFTools.java
@@ public IStatus runToolsExportAndProcessOutput(final String pythonExePath, final String gitExePath, final MessageConsoleStream console, MessageConsoleStream errorConsoleStream)
- if (gitExePath != null)
- {
- addGitToEnvironment(environment, gitExePath);
- }
+ if (gitExePath != null)
+ {
+ addGitToEnvironment(environment, gitExePath);
+ }
+ cleanUpSystemEnvironment(environment);
@@ public IStatus getToolsExportOutput(final String pythonExePath, final String gitExePath, final MessageConsoleStream console, MessageConsoleStream errorConsoleStream)
- if (gitExePath != null)
- {
- addGitToEnvironment(environment, gitExePath);
- }
+ if (gitExePath != null)
+ {
+ addGitToEnvironment(environment, gitExePath);
+ }
+ cleanUpSystemEnvironment(environment);Please also verify Windows tool installation via all three code paths.
🏁 Script executed:
#!/bin/bash
set -eux
# locate ExportIDFTools.java
FILE=$(rg -l "class ExportIDFTools" -g "*.java")
echo "Found file: $FILE"
# show all cleanUpSystemEnvironment calls with context
rg -nP -C3 "cleanUpSystemEnvironment" "$FILE"
# show definitions of target methods with context
echo "\nMethod runToolsExportAndProcessOutput definition with context:"
rg -nP -C3 "public IStatus runToolsExportAndProcessOutput" "$FILE"
echo "\nMethod getToolsExportOutput definition with context:"
rg -nP -C3 "public IStatus getToolsExportOutput" "$FILE"Length of output: 2461
Add cleanup in all export flows and verify Windows tool installation
- Insert
cleanUpSystemEnvironment(environment);immediately after theaddGitToEnvironment(...)block in bothrunToolsExportAndProcessOutput(...)andgetToolsExportOutput(...). - Confirm Windows tool installation works correctly across all three export paths.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
around lines 136-137, add a call to cleanUpSystemEnvironment(environment)
immediately after each addGitToEnvironment(...) call in both
runToolsExportAndProcessOutput(...) and getToolsExportOutput(...); ensure the
cleanup runs on all export code paths (normal, fallback and any Windows-specific
branch) and then run or write tests to verify Windows tool installation succeeds
across all three export flows so the environment is restored and Windows tools
install correctly.
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Hi @AndriiFilippov, could you try to reproduce this issue (even by manually configuring conflicting environment variables)?
#1303
That way, we can better test this PR.
|
@kolipakakondal hi ! 
|
|
@alirana01 hi ! Tested under: preconditions: to reproduce the original issue I have added IDF_PYTHON_ENV_PATH into my I was not able to install tools, got the 
just let me know, if I did reproduce it correctly |
Moved the cleanup to utility class and also added the cleanup call in required command runs
|
@AndriiFilippov kindly verify after the build |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (1)
49-55: Apply env cleanup to all export flows, not just the “given IDF path” one.
Both runToolsExportAndProcessOutput(...) and getToolsExportOutput(...) still pass unpruned env, so conflicts can persist.@@ public IStatus runToolsExportAndProcessOutput(...) - if (gitExePath != null) - { - addGitToEnvironment(environment, gitExePath); - } + if (gitExePath != null) + { + addGitToEnvironment(environment, gitExePath); + } + IDFUtil.cleanUpSystemEnvironment(environment); @@ public IStatus getToolsExportOutput(...) - if (gitExePath != null) - { - addGitToEnvironment(environment, gitExePath); - } + if (gitExePath != null) + { + addGitToEnvironment(environment, gitExePath); + } + IDFUtil.cleanUpSystemEnvironment(environment);#!/bin/bash # Verify all export paths prune env before launching idf_tools.py rg -nP -C2 'class ExportIDFTools|runToolsExportAndProcessOutput|getToolsExportOutput|getToolsExportOutputFromGivenIdfPath|cleanUpSystemEnvironment' bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.javaAlso applies to: 91-97
🧹 Nitpick comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
13-13: Remove unused LinkedList import.
Use List.of/Arrays.asList in the method and drop this import.-import java.util.LinkedList;
997-1021: Harden cleanup: fix typo, null/empty guard, and case‑insensitive removal.
Prevents NPEs and ensures removal on Windows where env keys can vary in case.- public static void cleanUpSystemEnvironment(Map<String, String> environment) - { - List<String> keysToRenmove = new LinkedList<>(); - keysToRenmove.add(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH); - keysToRenmove.add(IDFEnvironmentVariables.IDF_PATH); - keysToRenmove.add(IDFEnvironmentVariables.ESP_IDF_VERSION); - keysToRenmove.add(IDFEnvironmentVariables.IDF_CCACHE_ENABLE); - keysToRenmove.add(IDFEnvironmentVariables.IDF_COMPONENT_MANAGER); - keysToRenmove.add(IDFEnvironmentVariables.IDF_MAINTAINER); - keysToRenmove.add(IDFEnvironmentVariables.OPENOCD_SCRIPTS); - - for (String key : keysToRenmove) - { - if (environment.containsKey(key)) - { - environment.remove(key); - } - } - } + public static void cleanUpSystemEnvironment(Map<String, String> environment) + { + if (environment == null || environment.isEmpty()) { + return; + } + final java.util.List<String> keysToRemove = java.util.Arrays.asList( + IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH, + IDFEnvironmentVariables.IDF_PATH, + IDFEnvironmentVariables.ESP_IDF_VERSION, + IDFEnvironmentVariables.IDF_CCACHE_ENABLE, + IDFEnvironmentVariables.IDF_COMPONENT_MANAGER, + IDFEnvironmentVariables.IDF_MAINTAINER, + IDFEnvironmentVariables.OPENOCD_SCRIPTS + ); + // Remove matching keys ignoring case (important for Windows) + environment.keySet().removeIf(k -> keysToRemove.stream().anyMatch(r -> r.equalsIgnoreCase(k))); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(60-1022)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(60-1022)
⏰ 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). (1)
- GitHub Check: build_macos
🔇 Additional comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (1)
135-136: Good call: cleanup before export with explicit IDF path.
Matches the PR goal and should prevent the reported CreateProcess error.
| Logger.log("Environment Variables from System:"); | ||
| Logger.log(environment.toString()); | ||
| environment.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Stop logging full environment (secrets/PII risk).
Dumping env maps can leak tokens, proxies, and credentials to logs.
- Logger.log("Environment Variables from System:");
- Logger.log(environment.toString());
+ // Avoid logging full environment; contains secrets.
@@
-
- Logger.log("Environment Variables after filtering from system:");
- IDFUtil.cleanUpSystemEnvironment(environment);
- Logger.log(environment.toString());
-
+ IDFUtil.cleanUpSystemEnvironment(environment);Also applies to: 492-495
🏁 Script executed:
#!/bin/bash
# Find other places that log entire environment maps/values
rg -nP -C1 '(environment\\.toString\\(\\)|Logger\\.log\\(.+Environment Variables)' -g '!**/target/**' .Length of output: 72
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for environment.toString occurrences..."
rg -n -C1 --hidden --no-ignore 'environment\.toString\(' -g '!**/target/**' . || true
echo "Searching for Logger.log(...) containing 'Environment Variables'..."
rg -n -C1 --hidden --no-ignore 'Logger\.log\([^)]*Environment Variables' -g '!**/target/**' . || true
echo "Searching for Logger.log(...) that logs environment.toString..."
rg -n -C1 --hidden --no-ignore 'Logger\.log\([^)]*environment\.toString\(' -g '!**/target/**' . || true
echo "Searching for System.getenv / System.getProperty usages..."
rg -n -C1 --hidden --no-ignore 'System\.getenv\(|System\.getProperty\(' -g '!**/target/**' . || true
echo "Done"Length of output: 245
Stop logging full environment (secrets risk).
File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java — lines 474–476 and 492–495
- Logger.log("Environment Variables from System:");
- Logger.log(environment.toString());
+ // Avoid logging full environment; contains secrets.
@@
-
- Logger.log("Environment Variables after filtering from system:");
- IDFUtil.cleanUpSystemEnvironment(environment);
- Logger.log(environment.toString());
-
+ IDFUtil.cleanUpSystemEnvironment(environment);📝 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 Variables from System:"); | |
| Logger.log(environment.toString()); | |
| environment.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$ | |
| // Avoid logging full environment; contains secrets. | |
| environment.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$ | |
| ... | |
| IDFUtil.cleanUpSystemEnvironment(environment); |
🤖 Prompt for AI Agents
In bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
around lines 474–476 and 492–495, the code currently logs the full environment
map which may expose secrets; remove the Logger.log(environment.toString()) call
and any other direct prints of the environment, and instead log non-sensitive
metadata (e.g., number of variables or a filtered list of safe, whitelisted
variable names) or redact values before logging; ensure PYTHONUNBUFFERED is
still set but never include full environment contents or unredacted values in
logs.
| public static void cleanUpSystemEnvironment(Map<String, String> environment) | ||
| { | ||
| List<String> keysToRenmove = new LinkedList<>(); | ||
| keysToRenmove.add(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH); | ||
| keysToRenmove.add(IDFEnvironmentVariables.IDF_PATH); | ||
| keysToRenmove.add(IDFEnvironmentVariables.ESP_IDF_VERSION); | ||
| keysToRenmove.add(IDFEnvironmentVariables.IDF_CCACHE_ENABLE); | ||
| keysToRenmove.add(IDFEnvironmentVariables.IDF_COMPONENT_MANAGER); | ||
| keysToRenmove.add(IDFEnvironmentVariables.IDF_MAINTAINER); | ||
| keysToRenmove.add(IDFEnvironmentVariables.OPENOCD_SCRIPTS); | ||
|
|
||
| for (String key : keysToRenmove) | ||
| { | ||
| if (environment.containsKey(key)) | ||
| { | ||
| environment.remove(key); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of cleaning up system environment variables, why can't we rely only on the environment variables configured in the IDE and that includes PATH as well?
IDFEnvironmentVariables().getEnvMap()
Otherwise, if we go with the cleanup approach, we'll have to maintain this code all the time whenever there are changes in the IDF environment variables.
Please check?
Description
Original bug report: #1303
Some conflicting variables from the system environment can cause issues with the export script so we need to remove those.
Fixes # (IEP-1628)
Type of change
Please delete options that are not relevant.
How has this been tested?
Try to add IDF_PYTHON_ENV_PATH in your system env variables and run the IDE and try installing tools they should install without any problems
Test Configuration:
Checklist
Summary by CodeRabbit