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

fix(bash): add case for vscode integrated terminal #5817

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NeodymiumFerBore
Copy link

@NeodymiumFerBore NeodymiumFerBore commented Mar 3, 2024

Description

In starship.bash init script, handle vscode integrated terminal PROMPT_COMMAND override.

Motivation and Context

Fix an issue where manually sourcing a file evaluating starship init bash (typically a bashrc) would crash vscode integrated terminal if starship init had already been evaluated at terminal startup.

Closes #5816

Screenshots (if appropriate):

How Has This Been Tested?

  • Tested on Windows 10 with vscode 1.87.0, with lot of extensions and WSL as the terminal backend
  • Tested on Ubuntu 22.04 with vscode 1.86.2, fresh install, no extensions
tests details

After reproducing the bug described in issue #5816 :

In vscode
  • open a new bash in vscode terminal
  • assess that starship init was evaluated with the fancy prompt appearing
  • cd into starship git repository
  • git checkout fix/vscode-infinite-loop
  • ::STARSHIP::() { starship "$@"; }
  • eval "$(cat ./src/init/starship.bash)"
  • the terminal does not crash
  • starship environment variables are correctly setup, the prompt works as expected
  • __vsc_original_prompt_command=()
  • eval "$(cat ./src/init/starship.bash)"
  • the terminal does not crash, ${__vsc_original_prompt_command[@]} contains starship_precmd as expected
Outside vscode
  • tested in WSL and Ubuntu 22.04
  • open a new terminal
  • assess that starship init was evaluated with the fancy prompt appearing
  • cd into starship git repository
  • git checkout fix/vscode-infinite-loop
  • ::STARSHIP::() { starship "$@"; }
  • eval "$(cat ./src/init/starship.bash)"
  • the terminal does not crash
Both inside and outside vscode: check we didn't break something
  • unset PROMPT_COMMAND
  • the PS1 does not change anymore, we "broke" starship prompt
  • eval "$(cat ./src/init/starship.bash)"
  • PROMPT_COMMAND is restored to starship_precmd, the PS1 works as expected
  • starship_precmd() { :; }
  • the PS1 does not change anymore, we "broke" starship function
  • eval "$(cat ./src/init/starship.bash)"
  • the PS1 works as expected

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows (WSL)

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

I have 2 questions:

  • I haven't change any Rust code, and my change does not require any configuration. Do I need to do something more?
  • I don't have MacOS, could someone help in testing this please?

@NeodymiumFerBore
Copy link
Author

Problem: if starship was init as part of bashrc (or any user profile script), vscode will define function __vsc_preexec_all, and use it as DEBUG trap, re-using custom traps. If not, and if no custom DEBUG trap is defined in user profile scripts, vscode defines instead __vsc_preexec_only.

Current behavior: if we are in vscode and starship was NOT init in a user profile script, DEBUG trap is replaced by starship trap, so it breaks some vscode features when running init during the session.

New behavior: if we are in vscode and starship was NOT init in a user profile script, then starship_preexec is never executed as part of the DEBUG trap. So, instead of breaking vscode, we break starship. This is unwanted.

Converting this PR as draft, it needs more digging.

@NeodymiumFerBore NeodymiumFerBore marked this pull request as draft March 3, 2024 19:03
@davidkna
Copy link
Member

davidkna commented Mar 3, 2024

@NeodymiumFerBore Now that #5735 is merged, debug traps are avoided on recent bash versions.

@NeodymiumFerBore
Copy link
Author

@davidkna thank you for your comment, I wish I saw it before haha, integration with vscode is very tricky because of this debug trap.

I will rebase my branch on master and only treat the PROMPT_COMMAND issue then. Also, will move my code to avoid skipping the new PS0 part.

@NeodymiumFerBore
Copy link
Author

NeodymiumFerBore commented Mar 3, 2024

Update:

Rebased onto master (f66bfd9 as of now). Squashed commits.

Edited my tests: sourcing a file is not the same as evaluating it, and traps are not handled the same way. Tests done with eval instead of source (documentation says to use eval).

With this patch, evaluating multiple times starship init bash in a vscode terminal with shell integration does not trigger an infinite loop. However:

  • with bash < 4.4, when starship init bash is re-evaluated, vscode features relying on DEBUG trap no longer work (command status icon, status history next to the scrollbar, etc.).
  • with bash >= 4.4, when evaluating for the first time starship init bash manually in a vscode terminal (outside of user profile scripts), the same vscode features no longer work. It was already the case, so that's not a regression.

For bash >= 4.4 evaluating starship init bash in a user profile script, then manually, nothing breaks/crashes. The fix is not perfect, but at least it prevents the crash described in #5816 in all cases

@NeodymiumFerBore NeodymiumFerBore marked this pull request as ready for review March 3, 2024 22:03
@davidkna
Copy link
Member

I can't reproduce the issue, would you mind sharing more detailed instructions for reproducing the breakage in vscode?

@davidkna davidkna added the needs info Needs more information from the issue creator. label Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs info Needs more information from the issue creator.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

starship init bash in vscode: integrated terminal crashes due to infinite loop
2 participants