-
Notifications
You must be signed in to change notification settings - Fork 93
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
Use POSIX syntax for Bash detection #127
Conversation
Is there a particular shell / version where the existing code doesn't work? |
Most major linux distributions ship |
Ok, if the goal is to support Dash it'd be good to add test coverage of that. Can't hurt to add a comment here as well, noting the syntax is intentional to support Dash. I'm still not certain there's a need to support Dash, but it doesn't hurt. |
It's not really about dash specifically, but rather about the assumption that |
I don't mean to come across as thick, but where exactly is that assumption being made? The installation instructions (not to mention the project's name) are clearly specific to bash, and I don't see any reference in this repo to |
I don't disagree, but then why check at all? |
IMO it would be reasonable to not have this check and put the burden on the user to ensure their shell(s) are configured correctly, but it's probably not practical to remove at this point. Notably, #113 reported Zsh users accidentally sourcing bash-preexec (I imagine because they chose to source So just to be clear, you're not actually running into a situation where you expect to source bash-preexec in a Dash shell, right? |
Correct 👍 |
In that case I'm -1 on this change simply because it's not needed. But if you feel strongly this line should be dash-compatible it would be appropriate to include test coverage to enforce this support going forward. But in that vein it's also worth considering what would happen if bash-preexec ever utilized some bash syntax that dash couldn't even parse (causing it to fail before even reaching this line). IMO it would be wrong to avoid that syntax just because it breaks dash, since the library isn't intended to work with dash in the first place. Which suggests to me that we shouldn't bother with dash compatibility at all. |
That's fair. Since it's a shellscript, it would exit before reaching anything past those top lines, which I understand to be the point of moving those lines to the top of the file. It won't even parse the whole script at all; it's not compiled 😉 |
My point is simply that if such a situation arose I don't think it'd be worth reverting the (presumably desirable) bashism in order to satisfy dash. |
I don't think the current check in the master, which requires Bash to check if it is Bash, makes any sense. I think we should merge this PR or otherwise completely remove the check for Bash. In my opinion, even if it is not consistent with the later codes which use full Bash features, it is better to check the current shell using the POSIX feature. There are always beginner users doing something strange that we cannot predict. |
I for one don't mind removing the check, but if people feel strongly that the check needs to be dash-compatible there should be test coverage of that behavior. |
Thanks for your prompt reply! Then, I feel we can add tests and merge it. [ This is a side note but the check is not only affected by ash family (dash/ash/busybox sh) but also by other (non-ksh-like) POSIX shells including |
Late arrival to this thread! FWIW, this check was originally POSIX compatible and was changed without much consideration to use |
Appreciate the PR and everyone's input. I agree with @dimo414's take. However, in this situation I'm supportive if other contributors are interested in merging. I don't think there's any harm and would rather be pragmatic than turn into a larger debate. |
Don't depend on running modern Bash/ksh/zsh when we're trying to detect if we're running a modern bash.