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

Use an interactive shell to run .fpp.sh #138

Merged
merged 2 commits into from
May 26, 2015

Conversation

hlian
Copy link
Contributor

@hlian hlian commented May 25, 2015

    use interactive shell to execute .fpp.sh

    This conveniently executes all the appropriate rc files for both zsh and
    bash, thus expanding out aliases. There are cons to this approach
    though:

    * Though the Bash documentation strongly suggests that expand_aliases is
      turned on when interactive, I was not able to reproduce this behavior
      on Yosemite bash. (It's possible Yosemite just comes with a weird
      setup though.)

    * $SHELL only ever expands out to the user's default shell and not the
      current shell. (So in the rare case where you might open up zsh, then
      run bash, and then use fpp, fpp will choose zsh and not bash.)

    * Not all shells support the -i flag. However, the two shells currently
      supported by fpp (zsh and bash) do indeed have this handy dandy flag.

    A controversial change, perhaps! My feelings would not be hurt if you
    guys chose to not accept this PR, but my heart would be ecstatic if you
    did.

This fixes issue #125.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

This conveniently executes all the appropriate rc files for both zsh and
bash, thus expanding out aliases. There are cons to this approach
though:

* Though the Bash documentation strongly suggests that expand_aliases is
  turned on when interactive, I was not able to reproduce this behavior
  on Yosemite bash. (It's possible Yosemite just comes with a weird
  setup though.)

* $SHELL only ever expands out to the user's default shell and not the
  current shell. (So in the rare case where you might open up zsh, then
  run bash, and then use fpp, fpp will choose zsh and not bash.)

* Not all shells support the -i flag. However, the two shells currently
  supported by fpp (zsh and bash) do indeed have this handy dandy flag.

A controversial change, perhaps! My feelings would not be hurt if you
guys chose to not accept this PR, but my heart would be ecstatic if you
did.

This fixes issue facebook#125.
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@pcottle
Copy link
Contributor

pcottle commented May 26, 2015

Hey @hlian! Thanks for the PR -- I remember interactive mode coming up when I was figuring out alias expansion but I think I got stuck on a similar issue you had -- yosemite needing shopt to turn on alias expansion yet that not being an option in zshrc. also stack overflow seemed to widely discourage use of interactive mode, but I think our use case is a valid exception.

I guess checking for the existence of shopt and then flipping the flag isn't the worst solution, but it does seem a little blunt (compared to sourcing).

Let me pull this down and test locally -- I might actually prefer this though since sometimes sourcing bashrc's can have pretty aggressive side effects (I personally have a script to login automatically to my facebook dev server if a given flag is present, so that always trips me up when using fpp with bash sourcing)

@pcottle
Copy link
Contributor

pcottle commented May 26, 2015

@frantic do you remember any reasons why we avoided interactive mode in the past? I think the shopt was the main reason but given how others might be running into #125 my preference is to merge this in.

Let's do it!

pcottle added a commit that referenced this pull request May 26, 2015
Use an interactive shell to run .fpp.sh
@pcottle pcottle merged commit c518a4a into facebook:master May 26, 2015
@frantic
Copy link
Contributor

frantic commented May 26, 2015

Definitely cleaner than my attempt #59. Yeah, shopt I think, also bash on osx was weird.

(P.S. My hack to get $SHELL by invoking ps is so lame)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants