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): use eval instead of a procsub for the POSIX mode #5020

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

akinomyoga
Copy link
Contributor

@akinomyoga akinomyoga commented Mar 23, 2023

Description

Use eval -- "$(/path/to/starship init bash --print-full-init)", which works in any version of Bash and also in the POSIX mode, instead of the current version of source <(...) which does not work when the POSIX mode is turned on in Bash <= 5.0. I have added a detailed explanation in the code comments.

Motivation and Context

In Bash 5.0 and below (edit: After I submitted this PR, #5735 introduced the approach using PS0 for Bash 4.4 and above, so the problem arises only in Bash 4.3 and below as of 2024-04-01), Starship initialization failed when the POSIX mode is turned on. This PR fixes the issue.

Closes #1674
Closes #3660
Closes #5382

Screenshots (if appropriate):

With the following bashrc:

# bashrc
set -o posix
eval "$(starship init bash)"

Before the fix:
image

After the fix:
image

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.

There doesn't seem to be any documentation about the internal implementation of the original approach <(...), so I judged we do not have to add documentation for the new approach either.

  • I have updated the tests accordingly.

I'm not familiar with Rust, but cargo check just seems to pass.

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Apr 1, 2024

@davidkna Could you take a look at this PR? I just wanted to submit another PR to solve issues with the DEBUG trap (used for Bash <= 4.3), but to explain the problem the coming PR tries to solve, I need this PR to be merged first.

Maybe the code coverage error in the CI blocks the reviewing but I'd like some advice about how to fix it (or whether I should fix it in the first place because I think the part I modified wasn't covered by the test even before this change. In the part to which codecov issues an error, I haven't added any code but only changed the content of the string literal).

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Apr 1, 2024

I think this PR also solves the problem that PR #3660 tried to solve. The problem PR #3660 attempts to solve is that trap -p DEBUG is called inside a function _main and source, so the DEBUG trap at the top level is not captured by trap -p DEBUG (see also #1524). To solve the problem, PR #3660 tried to temporarily set set -o functrace to make all the DEBUG trap handlers inherited by the child function calls, which is a global behavioral change of the entire shell. However, the problem disappears with this PR because this PR makes trap -p DEBUG executed at the top level.

Copy link
Member

@davidkna davidkna left a comment

Choose a reason for hiding this comment

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

LGTM

@akinomyoga
Copy link
Contributor Author

Thank you for your quick response! If there is anything additional needed, please let me know.

@andytom andytom merged commit 0f859e8 into starship:master Apr 2, 2024
16 of 17 checks passed
@andytom
Copy link
Member

andytom commented Apr 2, 2024

Thank you for your contribution @akinomyoga and thanks for reviewing @davidkna.

@akinomyoga akinomyoga deleted the bash-posix-mode branch April 2, 2024 09:18
@akinomyoga
Copy link
Contributor Author

Thank you!

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.

Error on launching with Kitty Terminal Lightdm session won't start because of failed eval of starship
3 participants