bash: Fix bashisms being applied to non-bash shells #4334
Conversation
|
There is one problem I still need to solve. The block to source |
|
As requested: Gentle suggestion to perhaps also consider adding zsh Another (alternative?) option is to check the shell being run in the vendor scripts under /usr/share/defaults, and then, depending on shell, auto-loading |
I can add that to the .zshrc file, similar to how .bashrc loads
That is not standard, and fragile. If you load a shell, it loads its startup files (like .bashrc / .zshrc) which then (per this PR) load the scripts directory. What you suggest would add unnecessary complication IMO. Thanks for the feedback :) |
TraceyC77
force-pushed
the
zsh_source_app_configs
branch
from
231d736 to
b69573a
Compare
|
After testing changes after restoring zsh loading the profile.d folder I discovered bugs in 50-prompt.sh with zsh. I've updated it to fix a bug with using $SHELL to determine which shell to provide for. I also added a prompt that works with zsh in the same style as the one for bash. Testing: |
TraceyC77
force-pushed
the
zsh_source_app_configs
branch
from
15e6397 to
0803406
Compare
Move sourcing of bashrc.d directory to default skel/.bashrc Make all scripts in profile.d POSIX compliant Fixes issues with errors being emitted by non-bash shells Fixes bash specific user scripts improperly being loaded by other shells Add README for profile.d files needing to be POSIX compliant
Source profile.d files again to regain default settings now that they are POSIX compliant
Also fix method of getting shell to get current, not default **Summary**
TraceyC77
force-pushed
the
zsh_source_app_configs
branch
from
0803406 to
6405097
Compare
|
@ermo , if you could give this another look after sync, I'd like to get this in early enough for most of a week of testing. Thanks! |
There was a problem hiding this comment.
I am not keen on being on the hook for approving this. It would be better if an avid zsh user could try it out and see if it is fit for purpose.
Nonetheless, I have left some comments that came to me when reading through the files. I hope you find them useful. =)
| @@ -1,7 +1,10 @@ | |||
| # Begin /usr/share/defaults/etc/profile.d/50-history.sh | |||
|
|
|||
| # Append to history file on exit instead of overwrite (parallel terminals) | |||
| shopt -s histappend | |||
| # shopt is bash only | |||
| if [ $SHELL = "/usr/bin/bash" ]; then | |||
There was a problem hiding this comment.
It turns out that
| if [ $SHELL = "/usr/bin/bash" ]; then | |
| if [ -n "$BASH_VERSION" ]; then |
... is a very effective way to check if the running shell is bash.
| endchar="\$" | ||
| if [ "$UID" = "0" ]; then | ||
| endchar="#" | ||
| elif [[ "$current_shell" =~ zsh ]]; then |
There was a problem hiding this comment.
I am not sure this logic is sound.
What I would do is to add a check like this:
# Default that doesn't look like anything a shell would normally ship
endchar="?" # pick a suitable default here
if [ -n "$BASH_VERSION" ]; then
endchar="\$"
elif [ -n "$ZSH_VERSION" ]; then
endchar="%#"
fi
... as those change in a shell-defined way, depending on whether a shell is running as euid 0 or not.
| @@ -1,23 +1,53 @@ | |||
| # Begin /usr/share/defaults/etc/profile.d/50-prompt.sh | |||
|
|
|||
| # We cant use $SHELL since we need the *current* shell. | |||
There was a problem hiding this comment.
Since the prompt is only really relevant for interactive shells, maybe return early if the terminal is not interactive?
| # We cant use $SHELL since we need the *current* shell. | |
| # No point parsing this file for non-interactive shells that don't need a prompt | |
| [ -t 0 ] || return 0 | |
| # We cant use $SHELL since we need the *current* shell. |
| if [[ "$current_shell" =~ bash ]]; then | ||
| PS1="\[${FG}\]\u\[${AT}\]@\[${HCOLOR}\]\H \[${DIR}\]\w \[${FG}\]$endchar \[${reset_color}\]" | ||
| elif [[ "$current_shell" =~ zsh ]]; then | ||
| PS1="%{${FG}%}%n%{${AT}%}@%{${HCOLOR}%}%m %{${DIR}%}%1~ %{${FG}%}$endchar# ${reset_color}" |
There was a problem hiding this comment.
Taken together with my suggestion above, the # after $endchar is no longer necessary.
Note also that this uses double brackets, which is not very POSIX-y.
|
|
||
| # bash and zsh handle prompt setup and character escaping differently | ||
|
|
||
| if [[ "$current_shell" =~ bash ]]; then |
There was a problem hiding this comment.
This is another instance where the [ -n "$BASH_VERSION"] or [ -n "$ZSH_VERSION" ] idiom might be nice?
| unset endchar | ||
|
|
||
| # shopt is bash only | ||
| if [[ "$current_shell" =~ bash ]]; then |
There was a problem hiding this comment.
One more instance where [ -n "$BASH_VERSION" ] might be fruitful (and more POSIX-y).
| if [ -f /usr/share/defaults/etc/profile.d/vte.sh ]; then | ||
| source /usr/share/defaults/etc/profile.d/vte.sh | ||
| fi | ||
| # This should always be sourced regardless of the above files |
There was a problem hiding this comment.
This block should probably be at the top of the script, so the system zprofile in /etc can override vendor config settings...?
Summary
Move sourcing of bashrc.d directory to default skel/.bashrc
Make all scripts in profile.d POSIX compliant
Fixes issues with errors being emitted by non-bash shells
Fixes bash specific user scripts improperly being loaded by other shells
Add README for profile.d files needing to be POSIX compliant
Test Plan
Installed the new eopkgs & rebooted. Verified:
bash loads properly with no errors
Scripts in
~/.bashrc.d/loadedzsh loads properly with no errors
Scripts in
Scripts in
/usr/share/defaults/etc/profilewere loaded for both shellsChecklist
Fixes #4214