This repository has been archived by the owner. It is now read-only.

Add formula for Phusion Passenger #19981

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@FooBarWidget
Contributor

FooBarWidget commented May 21, 2013

This pull request adds a formula for Phusion Passenger, and modified the Nginx formula so that it automatically installs Phusion Passenger when --with-passenger is given.

@mistydemeo

This comment has been minimized.

Show comment
Hide comment
@mistydemeo

mistydemeo May 21, 2013

Contributor

nginx is designed to work with the Passenger gem installed via rubygems - is there a reason you need a Homebrew formula?

Contributor

mistydemeo commented May 21, 2013

nginx is designed to work with the Passenger gem installed via rubygems - is there a reason you need a Homebrew formula?

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget May 21, 2013

Contributor
  1. Convenience for users.
    • Homebrew is designed for OS X, therefore in the caveats message I can display OS X-specific instructions, e.g. how to edit the Apache configuration file. I cannot do this in the gem because gem post-install messages are static and cannot easily contain platform-specific instructions.
    • Homebrew's linking mechanism allows me to tell users to create an Apache config file /etc/apache2/other/passenger.conf, that has the same contents no matter which Phusion Passenger version is installed. I cannot do this with the gem because the filename inside the config file depends on the version that is installed. Not having to change the config file reduces the chance that users do something wrong when upgrading.
  2. To accomodate non-Ruby users. Phusion Passenger is no longer exclusive to Ruby, and also supports Python, with even more languages coming in the near future. Python users may not like to install gems, and may not even know how RubyGems works.
Contributor

FooBarWidget commented May 21, 2013

  1. Convenience for users.
    • Homebrew is designed for OS X, therefore in the caveats message I can display OS X-specific instructions, e.g. how to edit the Apache configuration file. I cannot do this in the gem because gem post-install messages are static and cannot easily contain platform-specific instructions.
    • Homebrew's linking mechanism allows me to tell users to create an Apache config file /etc/apache2/other/passenger.conf, that has the same contents no matter which Phusion Passenger version is installed. I cannot do this with the gem because the filename inside the config file depends on the version that is installed. Not having to change the config file reduces the chance that users do something wrong when upgrading.
  2. To accomodate non-Ruby users. Phusion Passenger is no longer exclusive to Ruby, and also supports Python, with even more languages coming in the near future. Python users may not like to install gems, and may not even know how RubyGems works.
@jacknagel

This comment has been minimized.

Show comment
Hide comment
@jacknagel

jacknagel May 22, 2013

Contributor

-1 for core, this is a great example of what taps should be used for though

Contributor

jacknagel commented May 22, 2013

-1 for core, this is a great example of what taps should be used for though

@adamv

This comment has been minimized.

Show comment
Hide comment
@adamv

adamv May 22, 2013

Contributor

Suggest this be in a new tap.

Contributor

adamv commented May 22, 2013

Suggest this be in a new tap.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget May 22, 2013

Contributor

Why do you think it is not a good fit for core? Do my arguments not make
sense?
Op 22 mei 2013 05:24 schreef "Jack Nagel" notifications@github.com het
volgende:

-1 for core, this is a great example of what taps should be used for though


Reply to this email directly or view it on GitHubhttps://github.com/mxcl/homebrew/pull/19981#issuecomment-18255398
.

Contributor

FooBarWidget commented May 22, 2013

Why do you think it is not a good fit for core? Do my arguments not make
sense?
Op 22 mei 2013 05:24 schreef "Jack Nagel" notifications@github.com het
volgende:

-1 for core, this is a great example of what taps should be used for though


Reply to this email directly or view it on GitHubhttps://github.com/mxcl/homebrew/pull/19981#issuecomment-18255398
.

@adamv

This comment has been minimized.

Show comment
Hide comment
@adamv

adamv May 22, 2013

Contributor

This formula doesn't make it explicit which version of ruby is used to install the gem, which will lead to more problems than having to learn how to install gems.

Contributor

adamv commented May 22, 2013

This formula doesn't make it explicit which version of ruby is used to install the gem, which will lead to more problems than having to learn how to install gems.

@jacknagel

This comment has been minimized.

Show comment
Hide comment
@jacknagel

jacknagel May 22, 2013

Contributor

It has nothing to do with the arguments you made, rather I think it is something we don't want to maintain in core, but should be maintained where someone who uses it can be responsible for updates.

Contributor

jacknagel commented May 22, 2013

It has nothing to do with the arguments you made, rather I think it is something we don't want to maintain in core, but should be maintained where someone who uses it can be responsible for updates.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget May 22, 2013

Contributor

@adamv It does not matter which version of Ruby is used to install the Phusion Passenger gem. Phusion Passenger works with all Ruby versions. It does not even matter if you install Phusion Passenger with Ruby interpreter A, and then switch to Ruby interpreter B; Phusion Passenger will continue to work. The gem is merely used as an easy form of "tar xzvf".

@jacknagel Would it be possible for this formula to be in core and for us to maintain it?

Contributor

FooBarWidget commented May 22, 2013

@adamv It does not matter which version of Ruby is used to install the Phusion Passenger gem. Phusion Passenger works with all Ruby versions. It does not even matter if you install Phusion Passenger with Ruby interpreter A, and then switch to Ruby interpreter B; Phusion Passenger will continue to work. The gem is merely used as an easy form of "tar xzvf".

@jacknagel Would it be possible for this formula to be in core and for us to maintain it?

@liquid1982

This comment has been minimized.

Show comment
Hide comment
@liquid1982

liquid1982 May 25, 2013

+1 for FooBarWidget pull request.

It is exactly what I was looking for right now. I think this is a good idea because the --with-passenger let me think that the module will be installed for me too, and that's not true. And to customize a brew nginx install is unpractical and difficult for most users.

+1 for FooBarWidget pull request.

It is exactly what I was looking for right now. I think this is a good idea because the --with-passenger let me think that the module will be installed for me too, and that's not true. And to customize a brew nginx install is unpractical and difficult for most users.

@adamv

This comment has been minimized.

Show comment
Hide comment
@adamv

adamv Jun 18, 2013

Contributor

Maintainers: if we're not going to pull this in, close it. I'm not opposed to allow it in, given some cleanup to use more brew idioms.

Contributor

adamv commented Jun 18, 2013

Maintainers: if we're not going to pull this in, close it. I'm not opposed to allow it in, given some cleanup to use more brew idioms.

@samueljohn

View changes

Library/Formula/passenger.rb
+ depends_on 'curl'
+
+ def install
+ system "rake apache2 nginx"

This comment has been minimized.

@samueljohn

samueljohn Jun 18, 2013

Contributor

Do I have rake on OS X by default?

@samueljohn

samueljohn Jun 18, 2013

Contributor

Do I have rake on OS X by default?

This comment has been minimized.

@adamv

adamv Jun 18, 2013

Contributor

On 10.6 and 10.7:

$ which rake
/usr/bin/rake
@adamv

adamv Jun 18, 2013

Contributor

On 10.6 and 10.7:

$ which rake
/usr/bin/rake

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 18, 2013

Member

Use the rake method instead which uses the right ruby.

@MikeMcQuaid

MikeMcQuaid Jun 18, 2013

Member

Use the rake method instead which uses the right ruby.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

I've fixed the 'rake' usage. Anything else that should be fixed?

Regarding maintenance: what is the best way for us to submit updates? We want to automatically update the Homebrew formula in our release scripts. Should we update our release scripts to modify the formula and send a pull request? Or is there a better way? Can we have direct commit/push access to the passenger formula on the official Homebrew repository?

Contributor

FooBarWidget commented Jun 20, 2013

I've fixed the 'rake' usage. Anything else that should be fixed?

Regarding maintenance: what is the best way for us to submit updates? We want to automatically update the Homebrew formula in our release scripts. Should we update our release scripts to modify the formula and send a pull request? Or is there a better way? Can we have direct commit/push access to the passenger formula on the official Homebrew repository?

@MikeMcQuaid

View changes

Library/Formula/nginx.rb
+ " \n\n" +
+ " To activate Phusion Passenger, add this to #{etc}/nginx/nginx.conf:\n" +
+ " passenger_root #{HOMEBREW_PREFIX}/opt/passenger\n" +
+ " passenger_ruby /usr/bin/ruby\n"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Use EOS here.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Use EOS here.

@MikeMcQuaid

View changes

Library/Formula/nginx.rb
@@ -21,6 +21,7 @@ class Nginx < Formula
option 'with-gunzip', 'Compile with support for gunzip module'
depends_on 'pcre'
+ depends_on 'passenger' if build.include?('with-passenger')

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Nit-pick but use include? without parens.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Nit-pick but use include? without parens.

@MikeMcQuaid

View changes

Library/Formula/passenger.rb
+ f.puts "exec #{orig_script} \"$@\""
+ end
+ File.chmod(0755, "#{bin}/#{name}")
+ end

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Homebrew has it's own functions for all of these functions. Also, don't use ohai here.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Homebrew has it's own functions for all of these functions. Also, don't use ohai here.

This comment has been minimized.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

What should I use instead of ohai?

Sent from my Android phone.
Op 20 jun. 2013 14:42 schreef "Mike McQuaid" notifications@github.com het
volgende:

In Library/Formula/passenger.rb:

  • system "cp -pR * #{prefix}/"
  • The various scripts in bin cannot correctly locate their root directory

  • when invoked as symlinks in /usr/local/bin. We create wrapper scripts

  • to solve this problem.

  • system "mv #{bin} #{prefix}/orig-bin"
  • system "mkdir #{bin}"
  • Dir["#{prefix}/orig-bin/*"].each do |orig_script|
  •  name = File.basename(orig_script)
    
  •  ohai "Generating wrapper script #{bin}/#{name}"
    
  •  File.open("#{bin}/#{name}", "w") do |f|
    
  •    f.puts "#!/bin/sh"
    
  •    f.puts "exec #{orig_script} \"$@\""
    
  •  end
    
  •  File.chmod(0755, "#{bin}/#{name}")
    
  • end

Homebrew has it's own functions for all of these functions. Also, don't
use ohai here.


Reply to this email directly or view it on GitHubhttps://github.com/mxcl/homebrew/pull/19981/files#r4795517
.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

What should I use instead of ohai?

Sent from my Android phone.
Op 20 jun. 2013 14:42 schreef "Mike McQuaid" notifications@github.com het
volgende:

In Library/Formula/passenger.rb:

  • system "cp -pR * #{prefix}/"
  • The various scripts in bin cannot correctly locate their root directory

  • when invoked as symlinks in /usr/local/bin. We create wrapper scripts

  • to solve this problem.

  • system "mv #{bin} #{prefix}/orig-bin"
  • system "mkdir #{bin}"
  • Dir["#{prefix}/orig-bin/*"].each do |orig_script|
  •  name = File.basename(orig_script)
    
  •  ohai "Generating wrapper script #{bin}/#{name}"
    
  •  File.open("#{bin}/#{name}", "w") do |f|
    
  •    f.puts "#!/bin/sh"
    
  •    f.puts "exec #{orig_script} \"$@\""
    
  •  end
    
  •  File.chmod(0755, "#{bin}/#{name}")
    
  • end

Homebrew has it's own functions for all of these functions. Also, don't
use ohai here.


Reply to this email directly or view it on GitHubhttps://github.com/mxcl/homebrew/pull/19981/files#r4795517
.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

There's not really any need for you to print stuff during installation, stick anything in the caveats.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

There's not really any need for you to print stuff during installation, stick anything in the caveats.

This comment has been minimized.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

Just like when brew install -v prints the commands that are executing, I think it would be a good idea to print such progress logs if -v is given. Would it be acceptable if this message is only printed when -v is enabled? If so, how do I check for -v?

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

Just like when brew install -v prints the commands that are executing, I think it would be a good idea to print such progress logs if -v is given. Would it be acceptable if this message is only printed when -v is enabled? If so, how do I check for -v?

@MikeMcQuaid

View changes

Library/Formula/nginx.rb
@@ -126,7 +136,7 @@ def caveats; <<-EOS.undent
If you want to host pages on your local machine to the wider network you
can change the port to 80 in: #{HOMEBREW_PREFIX}/etc/nginx/nginx.conf
- You will then need to run nginx as root: `sudo nginx`.
+ You will then need to run nginx as root: `sudo nginx`.#{passenger_caveats}

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Might want to look at how other formulae do optional caveats instead.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Might want to look at how other formulae do optional caveats instead.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Jun 20, 2013

Member

Squash your commits. For automated pull requests I recommend the hub gem.

Member

MikeMcQuaid commented Jun 20, 2013

Squash your commits. For automated pull requests I recommend the hub gem.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

All suggestions have been implemented. Commits have been squashed and rebased.

Contributor

FooBarWidget commented Jun 20, 2013

All suggestions have been implemented. Commits have been squashed and rebased.

@adamv

View changes

Library/Formula/passenger.rb
+ # The various scripts in bin cannot correctly locate their root directory
+ # when invoked as symlinks in /usr/local/bin. We create wrapper scripts
+ # to solve this problem.
+ mv bin, prefix/"orig-bin"

This comment has been minimized.

@adamv

adamv Jun 20, 2013

Contributor

We typically use "libexec" for this purpose.

@adamv

adamv Jun 20, 2013

Contributor

We typically use "libexec" for this purpose.

This comment has been minimized.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

I cannot use libexec. The original scripts must be placed in the source root, otherwise they cannot find the path to all the other Phusion Passenger files.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

I cannot use libexec. The original scripts must be placed in the source root, otherwise they cannot find the path to all the other Phusion Passenger files.

This comment has been minimized.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

Looks like I was wrong, libexec is in the source root.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

Looks like I was wrong, libexec is in the source root.

@adamv

View changes

Library/Formula/passenger.rb
+ mkdir bin
+ Dir[prefix/"orig-bin/*"].each do |orig_script|
+ name = File.basename(orig_script)
+ new_script = bin/name

This comment has been minimized.

@adamv

adamv Jun 20, 2013

Contributor

Call write on the pathname object:

(bin/name).write <<-EOS
  line 1
  line 2
EOS

Write will create the path if it doesn't exist, so the mkdir bin above can be removed after this.

@adamv

adamv Jun 20, 2013

Contributor

Call write on the pathname object:

(bin/name).write <<-EOS
  line 1
  line 2
EOS

Write will create the path if it doesn't exist, so the mkdir bin above can be removed after this.

@adamv

View changes

Library/Formula/passenger.rb
+ f.puts "#!/bin/sh"
+ f.puts "exec #{orig_script} \"$@\""
+ end
+ chmod 0755, new_script

This comment has been minimized.

@adamv

adamv Jun 20, 2013

Contributor

Is this line needed? The "cleaning" step should detect that this is a shell script and set the permissions we want.

@adamv

adamv Jun 20, 2013

Contributor

Is this line needed? The "cleaning" step should detect that this is a shell script and set the permissions we want.

This comment has been minimized.

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

I didn't know about the cleaning step. Where is it documented?

@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

I didn't know about the cleaning step. Where is it documented?

@adamv

View changes

Library/Formula/passenger.rb
+ To activate Phusion Passenger for Nginx, run:
+ brew install nginx --with-passenger
+
+ Please refer to https://www.phusionpassenger.com/support for support.

This comment has been minimized.

@adamv

adamv Jun 20, 2013

Contributor

I don't think we need this line; brew home foo should be enough.

@adamv

adamv Jun 20, 2013

Contributor

I don't think we need this line; brew home foo should be enough.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Jun 20, 2013

Contributor

All latest suggestions implemented.

Contributor

FooBarWidget commented Jun 20, 2013

All latest suggestions implemented.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Jun 27, 2013

Contributor

Is it okay now? Is there anything else I should change?

Contributor

FooBarWidget commented Jun 27, 2013

Is it okay now? Is there anything else I should change?

@zchrykng

This comment has been minimized.

Show comment
Hide comment
@zchrykng

zchrykng Jul 18, 2013

Any update on this? Would love to install using brew, if for no other reason than to remove the following message on brew doctor

Warning: "config" scripts exist outside your system or Homebrew directories.
`./configure` scripts often look for *-config scripts to determine if
software packages are installed, and what additional flags to use when
compiling and linking.

Having additional scripts in your path can confuse software installed via
Homebrew if the config script overrides a system or Homebrew provided
script of the same name. We found the following "config" scripts:

    /usr/local/opt/ruby/bin/passenger-config

Any update on this? Would love to install using brew, if for no other reason than to remove the following message on brew doctor

Warning: "config" scripts exist outside your system or Homebrew directories.
`./configure` scripts often look for *-config scripts to determine if
software packages are installed, and what additional flags to use when
compiling and linking.

Having additional scripts in your path can confuse software installed via
Homebrew if the config script overrides a system or Homebrew provided
script of the same name. We found the following "config" scripts:

    /usr/local/opt/ruby/bin/passenger-config
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Jul 18, 2013

Member

I'm happy with this, @jacknagel @mistydemeo @adamv any objections?

Member

MikeMcQuaid commented Jul 18, 2013

I'm happy with this, @jacknagel @mistydemeo @adamv any objections?

@adamv

This comment has been minimized.

Show comment
Hide comment
@adamv

adamv Jul 18, 2013

Contributor

OK with me.

Contributor

adamv commented Jul 18, 2013

OK with me.

handyman5 pushed a commit to handyman5/homebrew that referenced this pull request Oct 7, 2013

passenger 4.0.5 (new formula)
Closes #19981.

Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

@Homebrew Homebrew locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.