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

Don't clutter PATH with bash initialisation #37

Closed
wants to merge 1 commit into from

Conversation

muellerj
Copy link

@muellerj muellerj commented Aug 6, 2019

Passing -l to bash enables all path manipulations from the bash
initialisation chain (/etc/profile, /etc/bashrc, ~/.bash_profile,
~/.bash_login, ~/.profile, ~/.bashrc, etc.) to bleed through to
the fish shell. In order to avoid that, we should simply execute
chruby.sh without their influence.

@muellerj
Copy link
Author

The CI error is not related to my change, I think.

@JeanMertz
Copy link
Owner

Thanks for the contribution!

This was changed at some point, but I can't remember why I did that (I was terrible at Git and documenting things back then 🙈).

This would be a breaking change, but I agree that this would be a better solution.

We'll still want to get the CI working again though, to make sure nothing else breaks. I don't have a lot of time right now to fix this, so if you can find some time, I'd appreciate that. If not, I'll keep this open until someone can spend some time on fixing the CI, or I find some time myself.

@muellerj
Copy link
Author

My apologies for not responding sooner.

There seem to be two separate problems:

  1. brew install fish --HEAD seems to fail on the CI without ever getting to the tests
  2. When installing brew install fish, the tests get run and there is one failing test, which actually tests for .bash_profile support:
# test/chruby_test.fish:53
test "$TESTNAME: chruby supports .bash_profile"
  "$HOME" = (touch $HOME/tmp; chruby "2.2"; cat $HOME/tmp)
end

I don't quite understand how the test works. Also, evaluating (and thus allowing it to modify $PATH) .bash_profile seems to be a feature, since there are tests for it?

I would argue that with my proposed change, we should test that ~/.bash_profile is ignored. Since I don't understand how the test works, I don't feel confident modifying the tests. Can you give me a hint how (touch $HOME/tmp; chruby "2.2"; cat $HOME/tmp) prints the value of $HOME if chruby sources ~/.bash_profile?

Passing `-l` to bash enables all path manipulations from the bash
initialisation chain (`/etc/profile`, `/etc/bashrc`, `~/.bash_profile`,
`~/.bash_login`, `~/.profile`, `~/.bashrc`, etc.) to bleed through to
the fish shell. In order to avoid that, we should simply execute
`chruby.sh` without their influence.
@muellerj muellerj force-pushed the master branch 2 times, most recently from e62a302 to f19f508 Compare August 21, 2019 12:09
@bouk bouk mentioned this pull request Feb 17, 2020
@ioquatix ioquatix closed this in #39 May 17, 2022
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.

2 participants