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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Be more restrictive with bash init fallback #278

Merged
merged 2 commits into from
Sep 4, 2019
Merged

fix: Be more restrictive with bash init fallback #278

merged 2 commits into from
Sep 4, 2019

Conversation

nickwb
Copy link
Contributor

@nickwb nickwb commented Sep 3, 2019

Changes the bash init fallback to be Major Version > 3 rather than Major Version > 4.

Description

The issue being worked around affects bash 3.2, so Major Version > 3 is sufficient.
The fallback requires /dev/stdin, which may not exist on all systems - so it's preferable to use the normal bootstrap when possible.

This should fix starship for some Windows users who are using bash without full POSIX emulation.

For example: Git Bash, which is part of Git for Windows, does not recognise /dev/stdin, but does ship with bash 4.x.

Motivation and Context

Improve support for starship on Windows 馃槂

Bash on Windows is a bit of a minefield, because there are various projects maintaining various POSIX emulation layers and various patch lists. This change will not fix starship for ALL bash users on windows, but it should fix it for a portion of them.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

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

(I did specifically test this on bash 3.2 on MacOS).

Checklist:

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

@chipbuster
Copy link
Contributor

@nickwb Do you know what minor version git bash ships with specifically? I ask because we discovered while testing #241 that for some reason, the standard init pattern doesn't work with bash-4.0. The earliest version I can confirm it working with is 4.1.

@nickwb
Copy link
Contributor Author

nickwb commented Sep 3, 2019

Git for Windows 2.22.0 and 2.23.0 (the latest) both ship with bash: GNU bash, version 4.4.23(1)-release (x86_64-pc-msys).

Sadly there is some regression in 2.23.0, because eval "$(starship init bash)" is broken in 23, but works fine in 22.

Bash 4.4.23 seems to have been around since 2.22.0 (December 2018), and Bash 4.4.x for numerous years before that.

@nickwb
Copy link
Contributor Author

nickwb commented Sep 4, 2019

@chipbuster - Do you think we should take the bash >= 4.1 logic that was proposed at one point for #241?

Based on the versions of Git Bash I mentioned in the previous comment, v4.1 as a minimum should be suitable for everyone who has installed/updated Git for Windows in the last few years.

Edit: As an aside, if you're OK with this as a plan, I'll update this PR to add a few more comments around this check

@chipbuster
Copy link
Contributor

@nickwb I think that's probably the best way to go here. It won't be perfect, but we'll be supporting a lot more configurations than we are now.

This should improve compatibility with "Git Bash" with Git for Windows.
@nickwb
Copy link
Contributor Author

nickwb commented Sep 4, 2019

@chipbuster - Ok, I've pushed an update as discussed.

Copy link
Contributor

@chipbuster chipbuster 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 detailed comment!

Gotta be honest, it still weirds me out a bit that neither method works for bash-4.0, but we'll cross that bridge if we come to it.

@chipbuster chipbuster merged commit 5a0f269 into starship:master Sep 4, 2019
@chipbuster
Copy link
Contributor

@nickwb Please leave a comment reminding me to add you to all-contributors once #276 is merged.

@nickwb nickwb deleted the bash-init branch September 4, 2019 23:16
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.

None yet

2 participants