-
Notifications
You must be signed in to change notification settings - Fork 994
feat: Added xonsh shell support #4626
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
Conversation
djc
left a comment
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.
Please stop reopening the PR, which loses all the context.
|
I believe this PR should work. Thanks for review. |
|
|
||
| impl UnixShell for Xonsh { | ||
| fn does_exist(&self, process: &Process) -> bool { | ||
| process.var("XONSHRC").is_ok() || utils::find_cmd(&["xonsh"], process).is_some() |
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.
I believe mirroring fish's implementation would be better here. Is there a particular reason to check for XONSHRC instead?
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.
Because of xonsh is not POSIX compatible it keeps path to bash or zsh in the SHELL env var for compatibility (dependent on OS etc). So we need to detect here that we're in xonsh. Checking for XONSHRC env variable is good enough.
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.
@anki-code It should be noted however that $SHELL, which usually signifies the login shell, can be set to a non-POSIX shell. This is already the case for other shells such as fish in this file. What do you think about that?
PS: I agree that most of the time this won't matter since the second catch-all will usually trigger.
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.
@rami3l The big picture:
- $SHELL can contain any shell and xonsh can be installed as a login shell as well and can set the path to xonsh to $SHELL.
- But most of tools in the world expect that $SHELL is POSIX. They blindfold grab the value of $SHELL and run POSIX commands there and have errors as result and this can follows to unexpected consequences. This is why it's preferred way to keep POSIX shell in the $SHELL forever.
- Some shells like zsh and maybe fish has POSIX-compatible mode. They detect POSIX commands and run them mostly without errors. This is why they set itselfs to the $SHELL.
- So this is why using $SHELL value is not the path for shell checking.
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.
most of tools in the world expect that $SHELL is POSIX
@anki-code I doubt whether that is the case.
I use fish as my login shell and it doesn't have a POSIX-compatibility mode, so I think myself can count as a data point. From my experience, shell script writers would trust sh as authentically POSIX rather than $SHELL.
Also, a quick GitHub global search seems to support this idea.
However, I guess it's okay to merge it while leaving this thread open. We can always come back when either approach is proven suboptimal, and then we can replace the existing with yours if applicable, for 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.
I use fish as my login shell
You're lucky. My first result from search shows that there is no support for non-posix + support fish so I believe this code will fail or have consequences for non-posix.
Try next examples from search and you'll see that people have no thoughts about this could be another shell. So the probability to have fail with non POSIX in $SHELL exists. I read a report where someone catch Linux distro freezing after rebooting with new $SHELL. So if it's work to you you're lucky. Or may be fish as pretty old shell has more support.
rami3l
left a comment
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.
@anki-code Many thanks for making this PR!
Could you just squash these commits into one so we can merge it?
33ae83d to
33d8ff7
Compare
|
@rami3l done |
Resolve #4622
Test: