Skip to content

Fix fish shell integration execution order #226589

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.

Sign up for GitHub

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

Jump to bottom
Merged
meganrogge merged 1 commit into from
Oct 25, 2024
Merged

Fix fish shell integration execution order #226589

meganrogge merged 1 commit into from
Oct 25, 2024
+23 −12

Conversation

@aravind-n
Copy link
Contributor

@aravind-n aravind-n commented Aug 26, 2024 •
edited
Loading

Summary

Related Issue: #226554

  • Changed the fish shell integration injection mechanism
  • Renamed areZshBashLoginArgs to areZshBashFishLoginArgs
  • Added Fish and FishLogin to the ShellIntegrationExecutable enum
  • Added entries for Fish and FishLogin in shellIntegrationArgs

Previous Behavior

  • Injection for fish was previously done by adding a custom location with a fish/vendor.d directory to the XDG_DATA_DIRS environment variable
  • Fish would source the file in this directory and would then (theoretically) have shell integration injected
  • Since config.fish is sourced after fish/vendor.d/*.fish, any changes to fish_prompt in config.fish would break shell integration

Current Behavior

  • The XDG_DATA_DIRS variable is no longer used for injection
  • fish is now invoked with an --init-command argument. This argument asks fish to execute a command before taking input from stdin
  • This ensures that shell integration is always performed after any and all configuration files have been loaded

@aravind-n aravind-n force-pushed the shellintegrationfix branch from 5be43b2 to fb7e686 Compare August 26, 2024 08:23
@Tyriar Tyriar added this to the September 2024 milestone Aug 26, 2024
@aravind-n aravind-n force-pushed the shellintegrationfix branch 3 times, most recently from 8ca0813 to 68cbae1 Compare August 31, 2024 00:07
@aravind-n aravind-n force-pushed the shellintegrationfix branch 3 times, most recently from f4217cf to 2f7dbf3 Compare September 11, 2024 00:36
@aravind-n
Copy link
Contributor Author

aravind-n commented Sep 11, 2024

Need to fix some regression errors. Will update once I'm back at my computer

@aravind-n
Copy link
Contributor Author

aravind-n commented Sep 11, 2024

Fixed accidental regression added for bash due to changed shell integration script directory

@aravind-n aravind-n requested a review from 818Nawaf September 11, 2024 03:53
@Tyriar Tyriar assigned meganrogge and unassigned Tyriar Sep 16, 2024
Tyriar
Tyriar reviewed Sep 16, 2024
View reviewed changes
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.

@meganrogge can you look at this when you get some time? This change is a little scary as it changes the approach used in fish unfortunately away from environment variables which is a much nicer way to inject. It also appears to keep the fish_xdg_data/ dir but maybe not use it anymore?

@aravind-n
Copy link
Contributor Author

aravind-n commented Sep 16, 2024

@meganrogge I have a summary of the changes in the first comment on here but there's a more detailed analysis of what's going on in the linked issue if that helps

@aravind-n aravind-n force-pushed the shellintegrationfix branch 3 times, most recently from c54aee0 to 8109d63 Compare September 23, 2024 22:55
@Tyriar
Copy link
Member

Tyriar commented Sep 26, 2024

Apologies on the delay, we hope to get to this some time next week (current week is stabilizing the upcoming release).

@meganrogge
Copy link
Contributor

meganrogge commented Sep 30, 2024

Hi @aravind-n, thanks for all of your work on this PR. I was eager to test it out this week and unfortunately, my mac has broken, so will take a look ASAP when my new one comes.

@davidmartos96
Copy link
Contributor

davidmartos96 commented Oct 10, 2024 •
edited
Loading

I was having issues with how the VSCode terminal was resolving the PATH variable on my system and I discovered this PR which fixes the execution order of the shell integration and the fish config, which I thought was the cause. In particular I was having this same old issue but on Linux with fish #99878 , which was fixed for macOS back in the day.

But despite the fix in this PR, my issue specifically with the PATH wasn't fixed. In particular, I'm using the official fish variable fish_user_paths to handle the PATH in my system. It looks like fish always prepends these paths at the very end of the fish shell setup, even after the initial-command, so the context.environmentVariableCollection.prepend("PATH", "/foo/my/path:"); functionality used by some extensions doesn't work.

I opened a PR on top of this one with the simple fix aravind-n#1
My fix is dependent on the changes on this PR, so if the team thinks this is a valid solution, it could be included in the same PR.

The linked old issue is particularly annoying when working with extensions that let you change the version of a language SDK per project, because the PATH change won't work in the terminal.

@aravind-n
Copy link
Contributor Author

aravind-n commented Oct 16, 2024

@meganrogge hi just checking if you've got any updates on the mac situation. I was in Redmond the other day and I heard it's tough being an apple product in that neighborhood

@aravind-n aravind-n force-pushed the shellintegrationfix branch from 8109d63 to 608495c Compare October 16, 2024 23:33
@meganrogge
Copy link
Contributor

meganrogge commented Oct 16, 2024

@aravind-n very frustrating - no eta atm, with the guess that it'll take several weeks yet 😢

@aravind-n
Copy link
Contributor Author

aravind-n commented Oct 17, 2024

@meganrogge lol np
That said, this issue is reproducible on linux as well and isn't OS dependent since it's based on the behavior of the fish shell itself. Any chance we could get an exception and just test on linux?

@aravind-n aravind-n force-pushed the shellintegrationfix branch from 608495c to 4fd3c6c Compare October 17, 2024 02:00
meganrogge
meganrogge approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

This works great and makes sense. Thank you!

@meganrogge meganrogge enabled auto-merge (squash) October 25, 2024 16:22
auto-merge was automatically disabled October 25, 2024 16:35

Head branch was pushed to by a user without write access

@aravind-n aravind-n force-pushed the shellintegrationfix branch from 943fe8c to a0f289f Compare October 25, 2024 16:35
@davidmartos96
Copy link
Contributor

davidmartos96 commented Oct 25, 2024

@meganrogge What's your take on adding the following changes to fix #99878 on Linux with Fish?
The changes depend on this PR to fix it.
Or should I create a separate PR?

davidmartos96@7e5a143

@meganrogge
Copy link
Contributor

meganrogge commented Oct 25, 2024

@davidmartos96 I think a separate PR would be better to keep things simple. Thanks

@meganrogge meganrogge enabled auto-merge (squash) October 25, 2024 17:16
@aravind-n
Copy link
Contributor Author

aravind-n commented Oct 25, 2024

@meganrogge how do I get this to merge lol do I just rebase?

@meganrogge
Copy link
Contributor

meganrogge commented Oct 25, 2024

@aravind-n nothing more you need to do. I want @Tyriar to also review and approve the PR - he's back next week.

My understanding of this PR:
- In our shell integration script, we use a variable, fish_prompt, and rely upon that being set correctly.
- Before, we were setting that in a location that is run before other configuration files, like config.fish.
- As a result, if a user set their fish prompt, in config.fish, for example, it would break shell integration, as our fish_prompt would get overwritten.
- This PR uses —init-command, which ensures our shell integration script is executed after all configuration files are loaded.
- I tested this PR using login and non-login fish terminals with and without setting fish_prompt in my config.fish file and shell integration works in all cases.

@aravind-n
Copy link
Contributor Author

aravind-n commented Oct 25, 2024

Sounds good. Your understanding is exactly what's going on

@meganrogge meganrogge merged commit 40a0424 into microsoft:main Oct 25, 2024
@davidmartos96 davidmartos96 mentioned this pull request Oct 25, 2024
Merged
Tyriar
Tyriar reviewed Oct 28, 2024
View reviewed changes
src/vs/platform/terminal/node/terminalEnvironment.ts
shellIntegrationArgs.set(ShellIntegrationExecutable.Zsh, ['-i']);
shellIntegrationArgs.set(ShellIntegrationExecutable.ZshLogin, ['-il']);
shellIntegrationArgs.set(ShellIntegrationExecutable.Bash, ['--init-file', '{0}/out/vs/workbench/contrib/terminal/common/scripts/shellIntegration-bash.sh']);
shellIntegrationArgs.set(ShellIntegrationExecutable.Fish, ['--init-command', '. {0}/out/vs/workbench/contrib/terminal/common/scripts/fish_xdg_data/fish/vendor_conf.d/shellIntegration.fish']);
Copy link
Member

@Tyriar Tyriar Oct 28, 2024

Choose a reason for hiding this comment

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

If we're no longer using XDG_DATA_DIRS we should move the script out of this folder: #232408

@Tyriar
Copy link
Member

Tyriar commented Oct 28, 2024

Change looks good to me if it was tested well

@meganrogge meganrogge mentioned this pull request Oct 29, 2024
Closed
@aravind-n aravind-n deleted the shellintegrationfix branch October 31, 2024 23:31
@meganrogge meganrogge mentioned this pull request Nov 18, 2024
Merged
@aravind-n aravind-n mentioned this pull request Dec 6, 2024
Closed
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Tyriar Tyriar Tyriar left review comments

@connor4312 connor4312 connor4312 approved these changes

@meganrogge meganrogge meganrogge approved these changes

Assignees

@meganrogge meganrogge

Labels

None yet

Projects

None yet

Milestone

November 2024

Development

Successfully merging this pull request may close these issues.

5 participants

@aravind-n @Tyriar @meganrogge @davidmartos96 @connor4312