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

Doesn't work when login shell is csh #186

Closed
wkoszek opened this issue Aug 28, 2015 · 25 comments
Closed

Doesn't work when login shell is csh #186

wkoszek opened this issue Aug 28, 2015 · 25 comments

Comments

@wkoszek
Copy link

wkoszek commented Aug 28, 2015

Darwin wkoszek-macbook.local 14.5.0 Darwin Kernel Version 14.5.0: Wed Jul 29 02:26:53 PDT 2015; root:xnu-2782.40.9~1/RELEASE_X86_64 x86_64

My login shell is CSH. To rule out CSH, I've run Bash in iTerm2 window and tested too. Same problem. I tested in Vagrant with ubuntu/trusty64. Stuff works OK there, so what I describe below seems to be MacOSX specific.

I did:

brew update
brew install fpp.

cd /tmp
mkdir sample
cd sample
cal 2010 > 2010
cal 2011 > 2011
cal 2012 > 2012

/bin/ls -1 ./* | fpp

I select 2010, press f, select 2011, press f, press c, type 'cat', . Nothing happens. I'm dropped to fpp subshell. I'd expect to see output from cat'ed files.

---------------- cat ~/.fpp/.fpp.sh ----------------------
if type shopt > /dev/null; then
shopt -s expand_aliases
fi

echo "executing command:"
echo "cat './2010' './2011'"

cat './2010' './2011'

So selection part works OK. Execution is doing something wrong.

I started to add some debugging code: https://github.com/wkoszek/PathPicker/

@pcottle
Copy link
Contributor

pcottle commented Aug 29, 2015

Hrmmmm this is quite odd. I assuming running cat './2010' './2011' normally works right?

What about opening up in an editor? can you open those files in vim?

@wkoszek
Copy link
Author

wkoszek commented Aug 29, 2015

Yes. Cat and Vim normally work. In fpp neither works.

Do you test on Yosemite? What's your login shell?

W.

On Saturday, August 29, 2015, Peter Cottle notifications@github.com wrote:

Hrmmmm this is quite odd. I assuming running cat './2010' './2011'
normally works right?

What about opening up in an editor? can you open those files in vim?


Reply to this email directly or view it on GitHub
#186 (comment)
.

Wojciech A. Koszek
wojciech@koszek.com
http://www.koszek.com/

@pcottle
Copy link
Contributor

pcottle commented Aug 30, 2015

Do you have any weird customization in your .bashrc? can you attach it

I'm on Yosemit as well (as well as all my coworkers, couple thousand FB engineers) and it works for everyone here. Login shell is standard bash:

[pcottle:~:]$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin14)
Copyright (C) 2007 Free Software Foundation, Inc.

but i know a few people who use zsh with it. Never heard of csh though :O

@walterdolce
Copy link

Looks not to be working on iTerm2 with fish either

Yosemite // iTerm2 2.1.1 // fish 2.1.0

@wkoszek
Copy link
Author

wkoszek commented Sep 16, 2015

I use iTerm2 too. Just tried with Terminal. Didn't work at all (even ncurses UI didn't start)

You reminded me:

Tried from my test account--plain MacOSX user with bash as a shell. I sudo'ed to it. And then it worked.

@wkoszek
Copy link
Author

wkoszek commented Sep 16, 2015

Ok, I've looked at it closer: fpp is dependent on bash. If your login shell is csh just like mine, it doesn't work. Proof:

/w/repos/PathPicker] wk% cat ~/.fpp/.fpp.sh


if type shopt > /dev/null; then
  shopt -s expand_aliases
fi

echo "executing command:"
echo "vim './README.md'"
vim './README.md'

So nothing special. But it's a Bash syntax. In line 36 I've put an echo:

/w/repos/PathPicker] wk% sed -n 36p fpp
  echo $SHELL -i ~/.fpp/.fpp.sh < /dev/tty

Then I run it in my terminal and of course it showed bin/csh. So I reproduced lack of result by doing:

[wkoszek-macbook:/w/repos/PathPicker] wk% /bin/csh -i ~/.fpp/.fpp.sh < /dev/tty
[wkoszek-macbook:/w/repos/PathPicker] wk%

But:

[wkoszek-macbook:/w/repos/PathPicker] wk% /bin/bash -i ~/.fpp/.fpp.sh < /dev/tty
executing command:
vim './README.md'

In CSH when I force $SHELL to point to /bin/bash, it works. We need to either put this, or make fpp detect the shell type accordingly.

@pcottle
Copy link
Contributor

pcottle commented Sep 25, 2015

Yeah @wkoszek we definitely have a dependency on bash. the reason why we use $SHELL is that we also support zsh which is fairly popular, so that way its dynamic.

however since we don't support csh at all, what do you think about fpp instead just dropping down to bash? Basically I'm assuming fpp working in bash is better than no fpp at all.

do you think most csh users have either bash or zsh installed?

I'd be down to throw up a PR for this in a bit, but I'd probably ask for you to test it (since I don't have csh or zsh installed :O I'm such a bad engineer clearly)

@pcottle pcottle changed the title Doesn't work for me under MacOSX Yosemite Doesn't work when login shell is csh Sep 25, 2015
@walterdolce
Copy link

^ It's not just for csh. It doesn't work on fish either.

@wkoszek
Copy link
Author

wkoszek commented Sep 29, 2015

@pcottle I think just referring to /bin/sh instead of $SHELL should work as the "best effort" solution. Bash shell I believe is always there as /bin/sh.

@pcottle
Copy link
Contributor

pcottle commented Sep 29, 2015

Yeah I'm basically proposing the logic that if you're not on zsh or sh, we just use bash shell by default rather than optimistically hoping that your shell can interpret the script we generated.

however we do have support for zsh, so we can't just hardcode using bash shell @wkoszek.

What do you guys think about the above? Would fish or csh users be annoyed that bash executes their commands?

@benmccormick
Copy link

@pcottle Fish users at least run commands that get executed by bash all the time. The reasons for running fish (nicer defaults, cleaner scripting language for user-written scripts) are orthagonal to whatever 3rd party scripts use. I use bash scripts, python scripts, ruby scripts/etc all the time. It doesn't matter.

FWIW I was coming to report this issue as a regression. Previously fpp worked fine on fish.

@wkoszek
Copy link
Author

wkoszek commented Oct 1, 2015

@pcottle When I dissected fpp, my understanding was that we use $SHELL for a script that fpp generates. Unless we stick some ZSHisms there, and this script is simple and clean, I think calling /bin/sh on ~/.fpp/.fpp.sh should work. I think this:

if type shopt > /dev/null; then
  shopt -s expand_aliases
fi

is an obstacle, but not hard to get rid of.

@pcottle
Copy link
Contributor

pcottle commented Oct 5, 2015

Unless we stick some ZSHisms there, and this script is simple and clean, I think calling /bin/sh on ~/.fpp/.fpp.sh should work.

We delegate to $SHELL in order to preserve the aliases of whatever shell we're using, whether that be zsh or bash. We had some pretty high demand for supporting zsh aliases, which is why we did the switch. Sorry @benmccormick about the unintentional regression though :-/

I think we should do:

@pcottle
Copy link
Contributor

pcottle commented Oct 5, 2015

Lemme whip up the PR for this real quick

@pcottle
Copy link
Contributor

pcottle commented Oct 5, 2015

Alright PR is up! Sorry this took so long to fix when it ended up being reasonably simple :P

Csh / shell users -- do you all mind giving this a whirl and seeing if it fixes things?

@pcottle pcottle closed this as completed in f416e26 Oct 7, 2015
pcottle added a commit that referenced this issue Oct 7, 2015
[SHELL] Delegate to bash for all other shells Resolves #186
@pcottle
Copy link
Contributor

pcottle commented Oct 7, 2015

alright merging in the PR since im pretty confident it works. woohoo! would love for one of you all to test locally though, if thats possible :P @benmccormick ?

@benmccormick
Copy link

@pcottle haha sure, give me a second.

@benmccormick
Copy link

Hmm, so not working for me. I'm still seeing

❯ ack test | ./fpp
$? is not the exit status. In fish, please use $status.
~/.fpp/.fpp.sh (line 3): exit $?;

interesting I see that even if I'm currently in bash

bash-3.2$ ack test | ./fpp
$? is not the exit status. In fish, please use $status.
~/.fpp/.fpp.sh (line 3): exit $?;

@benmccormick
Copy link

Ok so after a little debugging it looks like $BASH is set for me when run from within a bash script

so while in fish, running this script

#!/bin/bash

echo $BASH

echoes /bin/bash because the shebang causes it to be run from within a bash context.

@benmccormick
Copy link

Maybe just check directly if $SHELL ends with zsh and use $SHELL, or otherwise use sh?

@pcottle
Copy link
Contributor

pcottle commented Oct 7, 2015

interesting I see that even if I'm currently in bash

hrm thats super odd -- can you debug inside of ./fpp and make sure that indeed bash is being run? im almost thinking that somehow you're going back into fish from bash, since fish is clearly complaining about the exit status.

Maybe just check directly if $SHELL ends with zsh and use $SHELL, or otherwise use sh?

yep I can do that too. what happens if you hardcode ./fpp to run bash?

@benmccormick
Copy link

@pcottle so after looking some more it was really this PR: #190 that broke things for fish.

exit $? is not a valid command for fish.

I think changing this to just exit should implicitly do the same thing right? My understanding is that for both bash and fish, returning the last exit code is the default behavior for exit and the $? is just making explicit

@pcottle
Copy link
Contributor

pcottle commented Oct 24, 2015

i thought that exit wont properly punt up the exit code of the last command for bash at least -- is that not the case? if so we can just switch to exit then, but i thought it had to be explicitly piped up

@benmccormick
Copy link

@pcottle internet says that this should be ok:

http://tldp.org/LDP/abs/html/exit-status.html

And a quick test makes it seem ok.

Tossed a quick stackoverflow question up to see if we could learn from wiser scripters than me, and it seems like the answer is that there is no difference

@pcottle
Copy link
Contributor

pcottle commented Oct 25, 2015

Resolved with #205

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

No branches or pull requests

4 participants