-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Detect shell type from $SHELL variable instead of .$SHELLrc files #765
Conversation
Thanks for the contribution! I am all for improving profile file detection as there's a number of issues open about it. I'm concerned about changing this though, so I'll want to test it on as many configurations as possible before merging it. |
(Note that |
@ljharb, I had also tested that possibility, but omitted it before. Anyway, I believe that even launching the script from dash (i.e. Also note that since my Here's the test executing the script with bash. Piping to bashzsh
bash
ksh
sh (dash)
|
What about on a Mac, where by default, |
@ljharb, I have changed my pull request and rebased. Here's the test. Piping to shzsh to sh
bash to sh
ksh to sh
sh to sh
Piping to bashzsh to bash
bash to bash
ksh to bash
sh to bash
|
Why are "bash to bash" and "ksh to bash" both doing "Source string already in /home/cristian/.zshrc"? |
@ljharb because the |
Right but shouldn't "bash to bash", to be a useful test, have |
Setting the Setting $SHELL to bashzsh to bash
bash to bash
ksh to bash
sh to bash
|
lol, now your |
Please do also add/modify the automated |
@ljharb it was already there exactly because when I tried the first install it added the line in Anyhow, the important thing is the shell interpreter that is on the right hand side of the pipe (and so far I have tested it with |
I'd love to see this change (or something similar) land. Is there anything I can do to help out in terms of verifying various shell combinations? |
@dfreeman #765 (comment) and #765 (comment) are my latest comments. If @CristianCantoro is unavailable, unable, or unwilling to complete the PR, I'd say it's a great idea to cherry-pick his commits into your own branch, and complete it as a new PR! :-) |
Oops! That's what I get for skimming up and down the comment thread too quickly :) |
Thanks - I had to disable the |
0131cc0
to
935f5df
Compare
Added the test and rebased the commit. Please note that I have also moved |
elif [ -f "$HOME/.profile" ]; then | ||
echo "$HOME/.profile" | ||
|
||
DETECTED_PROFILE='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding:
local DETECTED_PROFILE
local SHELLTYPE
and re-rebasing, to avoid global var pollution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in bc91bc5.
I notice that when I start in Detecting the current shell is difficult, but I wonder if we could somehow use |
@ljharb it seems the intended behaviour to me. If you want to change your shell you should use |
Both for testing, and expectations, I'd expect that if i run the install script in X, it installs in X, even if that's not my default. The current behavior is definitely attempting to detect the default, but I don't think that's quite as useful - since "install into the current shell" will match the default in most cases anyways. |
I am not sure of what would be the benefit of adding the configuration to a shell different from your default.
the instructions also suggest to All in all it seems more useful to me that the configuration line gets added only to the default |
elif [ -f "$HOME/.profile" ]; then | ||
echo "$HOME/.profile" | ||
|
||
local DETECTED_PROFILE='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the local declaration, and the variable assignment, unfortunately need to be on separate lines to ensure compatibility with ksh. see lines 87-90 for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5f6f55f.
Fair point. When I run |
@ljharb, yes, also on ubuntu I see what you mean for the installation. What about adding a different profile detection function if the configuration is already detected in the profile file? |
I'd be totally fine with two modes of profile detection, "default" and "current". We can do that in a separate PR tho. |
Ok, so can we merge this PR? :) |
Detect shell type from $SHELL variable instead of .$SHELLrc files
Done, thanks! |
As per commit message.
I am using zsh on Ubuntu, but I am keeping also the default
.bashrc
that gets shipped with Ubuntu in my home folder. As of now, the nvm "loading" configuration gets added to$HOME/.bashrc
because the check on.bashrc
is encountered before the check on.zshrc
.basename
is part of the GNU coreutils so it should be widely available and this should work also on other OSes.Testing the various shells
(
sh
isdash
on my system)Launch from zsh
Launch from bash
Launch from ksh
Launch from sh