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 Start With Fish Shell #25

Closed
ksylvest opened this issue Jul 30, 2016 · 6 comments
Closed

Doesn't Start With Fish Shell #25

ksylvest opened this issue Jul 30, 2016 · 6 comments

Comments

@ksylvest
Copy link
Contributor

ksylvest commented Jul 30, 2016

When tailing the ~/Library/Log/puma-dev.log I see:

* Directory for apps: /Users/kevin/.puma-dev
* Domains: dev
* DNS Server port: 9253
* HTTP Server port: inherited from launchd
* HTTPS Server port: inherited from launchd
! Puma dev listening on http and https
! Booted app 'ksylvest' on socket /Users/kevin/.puma-dev/ksylvest/tmp/puma-dev-811.sock
Missing end to balance this if statement
fish: if test -e Gemfile; then
      ^
^C⏎      

I'm running fish as the login shell (i.e. chsh -s /usr/local/bin/fish). Changing the login shell to bash fixes the problem. The offending lines appear to be the conditionals (if; fi vs. https://fishshell.com/docs/current/tutorial.html#tut_conditionals):

https://github.com/puma/puma-dev/blob/master/src/puma/dev/app.go#L183-L202

I've got no experience with go - but can a shebang be added to the exec.Command?

Alternatively - can the shell just be explicitly set to "sh" here https://github.com/puma/puma-dev/blob/master/src/puma/dev/app.go#L219?

@evanphx
Copy link
Member

evanphx commented Jul 30, 2016

Hm. Well, I explicitly ran those under the users shell to pickup things like rvm and rbenv configurations. And now I can see how that's problematic for non-bourne shells.

I'd be happy to add explicit support for fish though. Could you help me write a fish version of https://github.com/puma/puma-dev/blob/master/src/puma/dev/app.go#L158-L177?

Also, do you use rvm or rbenv? If so, how do you integrate them with your shell?

@ksylvest
Copy link
Contributor Author

ksylvest commented Jul 30, 2016

Didn't think of about that. I'm running on rbenv. To integrate I'm using:

# ~/.config/fish/config.fish
set PATH $HOME/.rbenv/shims $PATH
rbenv rehash

For modifying the conditionals to work with fish - I think this snippet does it:

if test -e ~/.powconfig
    source ~/.powconfig
end
if test -e .env
    source .env
end
if test -e .powrc
    source .powrc
end
if test -e .powenv
    source .powenv
end
if test -e Gemfile
    exec bundle exec puma -C $CONFIG --tag puma-dev:%s -w $WORKERS -t 0:$THREADS -b unix:%s
end
exec puma -C $CONFIG --tag puma-dev:%s -w $WORKERS -t 0:$THREADS -b unix:%s

Should the last conditional need to be wrapped in an else? That is should the bash version be modified to be the following or was that intentional:

if test -e Gemfile; then
    exec bundle exec puma -C $CONFIG --tag puma-dev:%s -w $WORKERS -t 0:$THREADS -b unix:%s
else
    exec puma -C $CONFIG --tag puma-dev:%s -w $WORKERS -t 0:$THREADS -b unix:%s
fi

What do you think the best approach is for integrating the two (and maybe more) scripts? Does it make sense to move them into separate files and then modify app.go to look something like this (excuse my syntax - never written go):

shell := os.Getenv("SHELL")
switch shell {
    case 'fish': path := "./execution/fish.sh"
    default: path := "./execution/default.sh"
}
dat, err := ioutil.ReadFile(path)
check(err)
executionShell := string()

@evanphx
Copy link
Member

evanphx commented Jul 30, 2016

I'd rather not externalize these scripts because that just becomes an installation headache. I'll go ahead and integrate your version in though. That all looks fine and the lack of else is fine because control never passes beyond the exec.

The thing I was most unsure about was if fish has source.

@sstephenson
Copy link

I explicitly ran those under the users shell to pickup things like rvm and rbenv configurations. And now I can see how that's problematic for non-bourne shells.

Really, you just want to start the server under the user’s login shell so it inherits their configured environment (PATH, etc). Once you have that parent environment, you can then explicitly spawn a new /bin/sh in which you source the rc scripts and start Puma.

launch_socket_server comes with a login_exec wrapper that runs a command under the user’s login shell. You could bundle or inline this in puma-dev’s launch plist.

(Related: #13)

@evanphx
Copy link
Member

evanphx commented Jul 31, 2016

I checked out your login_wrapper and puma-dev basically does that now,
which is how this bug was created.

Loading the users shell and the execing into sh is totally doable, seems
like getting PATH setup properly is the biggy for making sure the right
ruby is used.

I'll make that change shortly.
On Sun, Jul 31, 2016 at 7:11 AM Sam Stephenson notifications@github.com
wrote:

I explicitly ran those under the users shell to pickup things like rvm and
rbenv configurations. And now I can see how that's problematic for
non-bourne shells.

Really, you just want to start the server under the user’s login shell so
it inherits their configured environment (PATH, etc). Once you have that
parent environment, you can then explicitly spawn a new /bin/sh in which
you source the rc scripts and start Puma.

launch_socket_server comes with a login_exec wrapper that runs a command
under the user’s login shell
https://github.com/sstephenson/launch_socket_server#running-your-server-program-with-the-users-login-environment.
You could bundle or inline this in puma-dev’s launch plist.

(Related: #13 #13)


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#25 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAABzIVOLJWk_CkUFxKNwaOREwrbpfXks5qbK0agaJpZM4JY3DZ
.

@evanphx evanphx closed this as completed in eec2267 Aug 1, 2016
@ksylvest
Copy link
Contributor Author

ksylvest commented Aug 1, 2016

@evanphx Thanks for the fix!

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

3 participants