Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Prioritise CLI options over comment options
There are four sources of options for `Rack::Server`:

1. In the constructor, as a hash: `Rack::Server.new(server: 'thin')`
2. On the command line: `bundle exec rackup --server thin`
3. In a magic comment on the first line of config.ru (or whichever
   config file was used): `#\ --server thin`
4. `Rack::Server#default_options`

If 1 is provided, 4 and 2 won't be used. Previously, the order of
precedence - earlier 'wins' - was either:

* 1, 3
* 3, 2, 4

This changes the latter to 2, 3, 4. This allows an app to specify a
default port in the magic comment, but for that to be changed when using
the `rackup` script.
  • Loading branch information
smcgivern committed Feb 15, 2015
1 parent f4fa3af commit d924f80
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 20 deletions.
4 changes: 0 additions & 4 deletions lib/rack/handler.rb
Expand Up @@ -46,10 +46,6 @@ def self.pick(server_names)
def self.default(options = {})
# Guess.
if ENV.include?("PHP_FCGI_CHILDREN")
# We already speak FastCGI
options.delete :File
options.delete :Port

Rack::Handler::FastCGI
elsif ENV.include?(REQUEST_METHOD)
Rack::Handler::CGI
Expand Down
39 changes: 28 additions & 11 deletions lib/rack/server.rb
Expand Up @@ -99,7 +99,7 @@ def parse!(args)
abort opt_parser.to_s
end

options[:config] = args.last if args.last
options[:config] = args.last if args.last && !args.last.empty?
options
end

Expand Down Expand Up @@ -182,12 +182,22 @@ def self.start(options = nil)
# * :require
# require the given libraries
def initialize(options = nil)
@options = options
@app = options[:app] if options && options[:app]
@ignore_options = []

if options
@use_default_options = false
@options = options
@app = options[:app] if options[:app]
else
argv = defined?(SPEC_ARGV) ? SPEC_ARGV : ARGV
@use_default_options = true
@options = parse_options(argv)
end
end

def options
@options ||= parse_options(ARGV)
merged_options = @use_default_options ? default_options.merge(@options) : @options
merged_options.reject { |k, v| @ignore_options.include?(k) }
end

def default_options
Expand Down Expand Up @@ -287,7 +297,16 @@ def start &blk
end

def server
@_server ||= Rack::Handler.get(options[:server]) || Rack::Handler.default(options)
@_server ||= Rack::Handler.get(options[:server])

unless @_server
@_server = Rack::Handler.default(options)

# We already speak FastCGI
@ignore_options = [:File, :Port] if @_server.to_s == 'Rack::Handler::FastCGI'
end

@_server
end

private
Expand All @@ -297,7 +316,7 @@ def build_app_and_options_from_config
end

app, options = Rack::Builder.parse_file(self.options[:config], opt_parser)
self.options.merge! options
@options.merge!(options) { |key, old, new| old }
app
end

Expand All @@ -306,16 +325,14 @@ def build_app_from_string
end

def parse_options(args)
options = default_options

# Don't evaluate CGI ISINDEX parameters.
# http://www.meb.uni-bonn.de/docs/cgi/cl.html
args.clear if ENV.include?(REQUEST_METHOD)

options.merge! opt_parser.parse!(args)
options[:config] = ::File.expand_path(options[:config])
@options = opt_parser.parse!(args)
@options[:config] = ::File.expand_path(options[:config])
ENV["RACK_ENV"] = options[:environment]
options
@options
end

def opt_parser
Expand Down
2 changes: 1 addition & 1 deletion test/builder/options.ru
@@ -1,2 +1,2 @@
#\ -d
#\ -d -p 2929 --env test
run lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['OK']] }
2 changes: 2 additions & 0 deletions test/spec_builder.rb
Expand Up @@ -187,6 +187,8 @@ def config_file(name)
it "parses commented options" do
app, options = Rack::Builder.parse_file config_file('options.ru')
options[:debug].should.be.true
options[:environment].should.equal 'test'
options[:Port].should.equal '2929'
Rack::MockRequest.new(app).get("/").body.to_s.should.equal 'OK'
end

Expand Down
31 changes: 27 additions & 4 deletions test/spec_server.rb
Expand Up @@ -5,6 +5,9 @@
require 'open-uri'

describe Rack::Server do
SPEC_ARGV = []

before { SPEC_ARGV[0..-1] = [] }

def app
lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['success']] }
Expand Down Expand Up @@ -86,13 +89,33 @@ def with_stderr
opts[:pid].should.eql(::File.expand_path('testing.pid'))
end

should "get options from ARGV" do
SPEC_ARGV[0..-1] = ['--debug', '-sthin', '--env', 'production']
server = Rack::Server.new
server.options[:debug].should.be.true
server.options[:server].should.equal 'thin'
server.options[:environment].should.equal 'production'
end

should "only override non-passed options from parsed .ru file" do
builder_file = File.join(File.dirname(__FILE__), 'builder', 'options.ru')
SPEC_ARGV[0..-1] = ['--debug', '-sthin', '--env', 'production', builder_file]
server = Rack::Server.new
server.app # force .ru file to be parsed

server.options[:debug].should.be.true
server.options[:server].should.equal 'thin'
server.options[:environment].should.equal 'production'
server.options[:Port].should.equal '2929'
end

should "run a server" do
pidfile = Tempfile.open('pidfile') { |f| break f }.path
FileUtils.rm pidfile
pidfile = Tempfile.open('pidfile') { |f| break f }
FileUtils.rm pidfile.path
server = Rack::Server.new(
:app => app,
:environment => 'none',
:pid => pidfile,
:pid => pidfile.path,
:Port => TCPServer.open('127.0.0.1', 0){|s| s.addr[1] },
:Host => '127.0.0.1',
:daemonize => false,
Expand All @@ -105,7 +128,7 @@ def with_stderr

Process.kill(:INT, $$)
t.join
open(pidfile) { |f| f.read.should.eql $$.to_s }
open(pidfile.path) { |f| f.read.should.eql $$.to_s }
end

should "check pid file presence and running process" do
Expand Down

0 comments on commit d924f80

Please sign in to comment.