Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A followup to #171066 fixing zsh and fish shells implementations #223421

Merged

Conversation

anton-matosov
Copy link
Contributor

@anton-matosov anton-matosov commented Jul 23, 2024

#171066 introduced a fix for applying PATH prefix on macOS login shells that resolved #99878. However it had little issues for each shell implementation.
This PR contains the following fixes:

bash fix:

  • Add missing quotes to avoid path interpretation

zsh fix:

  • Add missing quotes to avoid path interpretation
  • Move patching outside of .zprofile check as clean macOS install doesn't include this file, nevertheless PATH patching should still happen

fish fix:

  • use set -gx PATH instead of fish_add_path as the latter has no effect on updating the path if entries already exist in it. Which is the case for some extensions, like vscode-micromamba which modify process environment and after login shell rc processing path entries end up in the end.

…ions

microsoft#171066 introduced a fix for applying PATH prefix on macOS login shells that resolved microsoft#99878. However it had little issues for each shell implementation.
This PR contains the following fixes:

`bash` fix:
 - Add missing `:` separator in the path setter to avoid path corruption
 - Add missing quotes to avoid path interpretation

`zsh` fix:
 - Add missing `:` separator in the path setter to avoid path corruption
 - Add missing quotes to avoid path interpretation
 - Move patching outside of `.zprofile` check as clean macOS install doesn't include this file, nevertheless PATH patching should still happen

 `fish` fix:
 - use `set -gx PATH` instead of `fish_add_path` as the latter has no effect on updating the path if entries already exist in it. Which is the case for some extensions, like [`vscode-micromamba`](https://github.com/mamba-org/vscode-micromamba) which modify process environment and after login shell rc processing path entries end up in the end.
@anton-matosov
Copy link
Contributor Author

anton-matosov commented Jul 24, 2024

I just found that there a second implementation of variables application #179476 that uses {applyAtShellIntegration: true} option and it works for all types of modification and works better in fish as it runs after fish.config and conf.d configs.
(Before I found it I extended original PATH only fix to work with all mutator types to patch up macOS behavior https://github.com/microsoft/vscode/compare/main...anton-matosov:vscode:implement-all-environment-mutations-for-shell-integrations?expand=1)

Considering that #179476 provides a complete implementation that works for all mutation types and for all shells, including login shell on mac. Shall the workaround for PATH (via VSCODE_PATH_PREFIX) be removed?
Moreover, if both VSCODE_PATH_PREFIX and {applyAtShellIntegration: true} are used, PATH ends up prepended twice on mac.

--
Note
I've tested on mac with append/prepend/replace for PATH for all shells, and it works, but still has a bug of missing : that glues paths on append/prepend. PR with fixes #223450

as generic env append/prepend can't handle path
separators, neither should it be handled here
@Tyriar Tyriar added this to the August 2024 milestone Jul 26, 2024
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and investigation! Looks reasonable and fairly safe to me.

Also yes for #223450 the path separator must be provided by the extension

@Tyriar Tyriar enabled auto-merge July 26, 2024 12:37
@Tyriar
Copy link
Member

Tyriar commented Jul 26, 2024

FYI @meganrogge in case you notice anything going wrong with zsh which has the biggest change.

@Tyriar Tyriar merged commit 9d97e5e into microsoft:main Jul 26, 2024
6 checks passed
@anton-matosov anton-matosov deleted the fix-path-prefix-in-shell-integrations branch July 26, 2024 20:10
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepending PATH env var with environmentVariableCollection doesn't work on macOS
3 participants