Skip to content

Conversation

@sigmaaa
Copy link
Collaborator

@sigmaaa sigmaaa commented Oct 13, 2025

Description

The issue was caused by retrieving the PATH key, which doesn’t exist on Windows, leading to a new conflicting variable. Fixed by fetching the existing key (PATH or Path) based on the platform.

Fixes # (IEP-1644)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Install tools -> open terminal -> run idf.py commands

Test Configuration:

  • ESP-IDF Version:
  • OS (Windows,Linux and macOS):

Dependent components impacted by this PR:

  • Component 1
  • Component 2

Checklist

  • PR Self Reviewed
  • Applied Code formatting
  • Added Documentation
  • Added Unit Test
  • Verified on all platforms - Windows,Linux and macOS

Summary by CodeRabbit

  • Bug Fixes
    • PATH handling now respects system-specific casing (e.g., "PATH" vs "Path"), avoiding missed tool lookups.
    • Homebrew-related directories are more reliably merged into the environment PATH on macOS, improving command discovery.
    • Provides more consistent environment setup across platforms, reducing failures when launching or locating external tools.

@coderabbitai
Copy link

coderabbitai bot commented Oct 13, 2025

Walkthrough

Updates IDFUtil.getSystemEnv to detect and use the correct PATH key ("PATH" or "Path"), merge Homebrew paths, and write back the final PATH using the selected key. Introduces keyPath tracking, fallback logic, and consistent environment mutation across differing OS conventions.

Changes

Cohort / File(s) Change summary
Environment PATH handling
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
Adjusts getSystemEnv to detect which PATH key exists ("PATH" or "Path"), store it as keyPath, merge Homebrew paths into the chosen PATH value, and write the merged value back using keyPath instead of a hard-coded "PATH".

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Caller
  participant U as IDFUtil.getSystemEnv
  participant E as System Environment

  C->>U: Request environment map
  U->>E: Read entries for "PATH" and "Path"
  alt "PATH" present
    U->>U: keyPath = "PATH"
  else "Path" present
    U->>U: keyPath = "Path"
  else
    U->>U: keyPath = "PATH" (default)
  end

  U->>U: Merge Homebrew paths into env[keyPath]
  U->>E: Write merged PATH back using keyPath
  U-->>C: Return updated environment map

  note right of U: Ensures consistent PATH mutation across OS conventions
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • kolipakakondal
  • AndriiFilippov
  • alirana01

Poem

In burrows of bytes I nudge the trail,
"PATH" or "Path" — I sniff and pick the rail.
Homebrew crumbs I tuck inside,
One key to guide the runner's stride.
Hooray — the paths are neat and hale. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title mentions regression testing and that the terminal does not work, but it does not describe the actual code change which fixes PATH handling by merging Homebrew paths and handling case‐insensitive keys. As a result it fails to clearly convey the primary improvement introduced by the patch. Consider renaming the title to clearly describe the fix, for example “Fix Homebrew PATH merging for terminal on macOS,” so that it highlights the code change rather than just the symptom.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch IEP-1644

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between baeb8a5 and 6dfe0aa.

📒 Files selected for processing (1)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.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_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)

924-924: Good: Using the detected PATH key.

Correctly writes the merged PATH back using the detected key rather than a hardcoded "PATH". This ensures consistency across different OS conventions once the key detection logic (lines 903-909) is corrected.

@sigmaaa sigmaaa changed the title WIP: IEP-1644 Release 3.7.0 regression testing: Terminal does not work IEP-1644 Release 3.7.0 regression testing: Terminal does not work Oct 14, 2025
Copy link
Collaborator Author

@sigmaaa sigmaaa left a comment

Choose a reason for hiding this comment

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

Self reviewed

@sigmaaa sigmaaa added this to the v3.7.0 milestone Oct 14, 2025
Copy link
Collaborator

@kolipakakondal kolipakakondal left a comment

Choose a reason for hiding this comment

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

LGTM

@AndriiFilippov
Copy link
Collaborator

@sigmaaa hi !

Tested under:
OS - Windows 11 / Mac arm64
ESP-IDF: v5.5.1

able to use idf.py tools via Terminal ✅
LGTM 👍

@sigmaaa sigmaaa merged commit 8d8ef63 into master Oct 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants