Hi all, I''ve pushed the following patch out go git://git.bogomips.org/unicorn along with a few other Rails-related test updates. This is more of a shotgun fix (but less code is better :) since I haven''t been able to reproduce the brokeness people have been seeing with "unicorn_rails" and Rails 3 betas. Even though "unicorn" works perfectly well for Rails3, some folks will inevitably run "unicorn_rails" because of the "rails" in its name. If I were to do it all over again, I would''ve just made everybody write config.ru files for Rails. But yes, programming is like sex, make one mistake and support it for life :) You can grab a git gem here, too: http://unicorn.bogomips.org/files/unicorn-0.99.0.16.g59a625.gem>From 4b44e21957e4cb8ec6ace5604fbe096dfd8959d2 Mon Sep 17 00:00:00 2001From: Eric Wong <normalperson at yhbt.net> Date: Thu, 3 Jun 2010 22:52:11 +0000 Subject: [PATCH] unicorn_rails: avoid duplicating config.ru logic This should allow "unicorn_rails" to be used seamlessly with Rails 3 projects which package config.ru for you. --- bin/unicorn_rails | 50 ++++++++++++++------------------------------------ 1 files changed, 14 insertions(+), 36 deletions(-) diff --git a/bin/unicorn_rails b/bin/unicorn_rails index 72ab288..45a9b11 100755 --- a/bin/unicorn_rails +++ b/bin/unicorn_rails @@ -109,53 +109,30 @@ end ru = ARGV[0] || (File.exist?(''config.ru'') ? ''config.ru'' : nil) -if ru && ru =~ /\.ru\z/ - # parse embedded command-line options in config.ru comments - /^#\\(.*)/ =~ File.read(ru) and opts.parse!($1.split(/\s+/)) -end - -def rails_builder(ru, daemonize) +def rails_builder(daemonize) # this lambda won''t run until after forking if preload_app is false lambda do || # Load Rails and (possibly) the private version of Rack it bundles. begin require ''config/boot'' + require ''config/environment'' rescue LoadError => err abort "#$0 must be run inside RAILS_ROOT: #{err.inspect}" end - inner_app = case ru - when nil - require ''config/environment'' - - defined?(::Rails::VERSION::STRING) or - abort "Rails::VERSION::STRING not defined by config/{boot,environment}" - # it seems Rails >=2.2 support Rack, but only >=2.3 requires it - old_rails = case ::Rails::VERSION::MAJOR - when 0, 1 then true - when 2 then Rails::VERSION::MINOR < 3 ? true : false - else - false - end - - if old_rails - require ''unicorn/app/old_rails'' - Unicorn::App::OldRails.new - else - ActionController::Dispatcher.new - end - when /\.ru$/ - raw = File.read(ru) - raw.sub!(/^__END__\n.*/, '''') - eval("Rack::Builder.new {(#{raw}\n)}.to_app", TOPLEVEL_BINDING, ru) + defined?(::Rails::VERSION::STRING) or + abort "Rails::VERSION::STRING not defined by config/{boot,environment}" + # it seems Rails >=2.2 support Rack, but only >=2.3 requires it + old_rails = case ::Rails::VERSION::MAJOR + when 0, 1 then true + when 2 then Rails::VERSION::MINOR < 3 ? true : false else - require ru - Object.const_get(File.basename(ru, ''.rb'').capitalize) + false end Rack::Builder.new do map_path = ENV[''RAILS_RELATIVE_URL_ROOT''] || ''/'' - if inner_app.class.to_s == "Unicorn::App::OldRails" + if old_rails if map_path != ''/'' # patches + tests welcome, but I really cbf to deal with this # since all apps I''ve ever dealt with just use "/" ... @@ -163,23 +140,24 @@ def rails_builder(ru, daemonize) end $stderr.puts "LogTailer not available for Rails < 2.3" unless daemonize $stderr.puts "Debugger not available" if $DEBUG + require ''unicorn/app/old_rails'' map(map_path) do use Unicorn::App::OldRails::Static - run inner_app + run Unicorn::App::OldRails.new end else use Rails::Rack::LogTailer unless daemonize use Rails::Rack::Debugger if $DEBUG map(map_path) do use Rails::Rack::Static - run inner_app + run ActionController::Dispatcher.new end end end.to_app end end -app = rails_builder(ru, daemonize) +app = ru ? Unicorn.builder(ru, opts) : rails_builder(daemonize) options[:listeners] << "#{host}:#{port}" if set_listener if $DEBUG -- Eric Wong
Michael Guterl
2010-Jun-04 02:13 UTC
unicorn_rails cleanup (possible fix for Rails3) pushed
On Thu, Jun 3, 2010 at 9:58 PM, Eric Wong <normalperson at yhbt.net> wrote: <snip>> If I were to do it all over again, I would''ve just made everybody write > config.ru files for Rails. ?But yes, programming is like sex, make one > mistake and support it for life :)That is probably one of the funniest things I''ve read in awhile. Thanks for the unexpected source of humor! :) Best, Michael Guterl
Michael Guterl <mguterl at gmail.com> wrote:> On Thu, Jun 3, 2010 at 9:58 PM, Eric Wong <normalperson at yhbt.net> wrote: > <snip> > > If I were to do it all over again, I would''ve just made everybody write > > config.ru files for Rails. ?But yes, programming is like sex, make one > > mistake and support it for life :) > > That is probably one of the funniest things I''ve read in awhile. > Thanks for the unexpected source of humor! :)You''re welcome :> It''s not my line, but I heard it many years ago. -- Eric Wong
Eric Wong <normalperson at yhbt.net> wrote:> Hi all, > > I''ve pushed the following patch out go git://git.bogomips.org/unicorn > along with a few other Rails-related test updates. This is more of a > shotgun fix (but less code is better :) since I haven''t been able to > reproduce the brokeness people have been seeing with "unicorn_rails" > and Rails 3 betas.<snip>> You can grab a git gem here, too: > > http://unicorn.bogomips.org/files/unicorn-0.99.0.16.g59a625.gemAnybody get a chance to try this? -- Eric Wong
Hi Eric. It would appear that recent Rails 3 changes have broken unicorn_rails, just like they broke Phusion Passenger. Here''s a patch which fixes the problem. http://gist.github.com/429944 With kind regards, Hongli Lai -- Phusion | The Computer Science Company Web: http://www.phusion.nl/ E-mail: info at phusion.nl Chamber of commerce no: 08173483 (The Netherlands)
Hongli Lai <hongli at phusion.nl> wrote:> Hi Eric. > > It would appear that recent Rails 3 changes have broken unicorn_rails, > just like they broke Phusion Passenger. Here''s a patch which fixes the > problem. > http://gist.github.com/429944Thanks Hongli, so this only affects people who remove the config.ru that Rails 3 creates for them? Yikes... A few comments: + if ::Rails::VERSION::MAJOR >= 3 && ::File.exist?(''config/application.rb'') + ::File.read(''config/application.rb'') =~ /^module (.+)$/ Maybe a more flexible regexp like this: /^module\s+([\w:]+)\s*$/ (or maybe even starting with: "^\s*") would be more reliable for folks who leave extra spaces around. Unfortunately, Ruby is not Python :) @@ -148,9 +167,9 @@ def rails_builder(daemonize) else use Rails::Rack::LogTailer unless daemonize use Rails::Rack::Debugger if $DEBUG + use Rails::Rack::Static, map_path map(map_path) do - use Rails::Rack::Static - run ActionController::Dispatcher.new + run rails_dispatcher Changing the call to use Rails::Rack::Static there is wrong. map_path is the URI prefix (RAILS_RELATIVE_URL_ROOT) and not the static file path we serve from. It appears the deprecation in Rails 3 broke some things and ActionDispatch::Static is configured slightly differently. Let me know if the edited patch below looks alright to you. I''ll also push out a few Rails 3 tests that exercise the missing config.ru cases.>From 222ae0a353eda446a480e5c4b473a218304f9594 Mon Sep 17 00:00:00 2001From: Hongli Lai (Phusion) <hongli at phusion.nl> Date: Tue, 8 Jun 2010 13:22:25 +0200 Subject: [PATCH] Fix unicorn_rails compatibility with the latest Rails 3 code This allows us to properly detect Rails 3 installations in cases where config.ru is not present. [ew: expanded commit message fixed static file serving, more flexible regexp for matching module ] ref: mid.gmane.org/AANLkTiksBxIo_PFWoiPTWi1entXZRb7D2uE-Rl7H3lbw at mail.gmail.com Acked-by: Eric Wong <normalperson at yhbt.net> --- bin/unicorn_rails | 26 ++++++++++++++++++++++++-- 1 files changed, 24 insertions(+), 2 deletions(-) diff --git a/bin/unicorn_rails b/bin/unicorn_rails index 45a9b11..ed235ba 100755 --- a/bin/unicorn_rails +++ b/bin/unicorn_rails @@ -109,6 +109,24 @@ end ru = ARGV[0] || (File.exist?(''config.ru'') ? ''config.ru'' : nil) +def rails_dispatcher + if ::Rails::VERSION::MAJOR >= 3 && ::File.exist?(''config/application.rb'') + if ::File.read(''config/application.rb'') =~ /^module\s+([\w:]+)\s*$/ + app_module = Object.const_get($1) + begin + result = app_module::Application + rescue NameError + end + end + end + + if result.nil? && defined?(ActionController::Dispatcher) + result = ActionController::Dispatcher.new + end + + result || abort("Unable to locate the application dispatcher class") +end + def rails_builder(daemonize) # this lambda won''t run until after forking if preload_app is false lambda do || @@ -149,8 +167,12 @@ def rails_builder(daemonize) use Rails::Rack::LogTailer unless daemonize use Rails::Rack::Debugger if $DEBUG map(map_path) do - use Rails::Rack::Static - run ActionController::Dispatcher.new + if defined?(ActionDispatch::Static) + use ActionDispatch::Static, "#{Rails.root}/public" + else + use Rails::Rack::Static + end + run rails_dispatcher end end end.to_app -- Eric Wong
On Tue, Jun 8, 2010 at 9:20 PM, Eric Wong <normalperson at yhbt.net> wrote:> Thanks Hongli, so this only affects people who remove the > config.ru that Rails 3 creates for them? ?Yikes...No. The problem even occurs if you already have config.ru. But the thing is, Rails 3 has deprecated ActionController::Dispatcher a few weeks ago and replaced it with a stub. Rails::Rack::Static changed its interface and must be constructed differently. The only way to obtain a valid Rack endpoint for the app seems to be parsing config/application.rb> A few comments: > > + ?if ::Rails::VERSION::MAJOR >= 3 && ::File.exist?(''config/application.rb'') > + ? ?::File.read(''config/application.rb'') =~ /^module (.+)$/ > > Maybe a more flexible regexp like this: /^module\s+([\w:]+)\s*$/ (or > maybe even starting with: "^\s*") would be more reliable for folks who > leave extra spaces around.I think it''s easier to just call ''strip''. :)> Unfortunately, Ruby is not Python :) > > @@ -148,9 +167,9 @@ def rails_builder(daemonize) > ? ? ? else > ? ? ? ? use Rails::Rack::LogTailer unless daemonize > ? ? ? ? use Rails::Rack::Debugger if $DEBUG > + ? ? ? ?use Rails::Rack::Static, map_path > ? ? ? ? map(map_path) do > - ? ? ? ? ?use Rails::Rack::Static > - ? ? ? ? ?run ActionController::Dispatcher.new > + ? ? ? ? ?run rails_dispatcher > > Changing the call to use Rails::Rack::Static there is wrong. ?map_path > is the URI prefix (RAILS_RELATIVE_URL_ROOT) and not the static file > path we serve from. ?It appears the deprecation in Rails 3 broke some > things and ActionDispatch::Static is configured slightly differently. > > Let me know if the edited patch below looks alright to you.Yes it looks fine. A bit overcomplicated regexp compared to using ''strip'' but whatever works. :) With kind regards, Hongli Lai -- Phusion | The Computer Science Company Web: http://www.phusion.nl/ E-mail: info at phusion.nl Chamber of commerce no: 08173483 (The Netherlands)
Hongli Lai <hongli at phusion.nl> wrote:> On Tue, Jun 8, 2010 at 9:20 PM, Eric Wong <normalperson at yhbt.net> wrote: > > Thanks Hongli, so this only affects people who remove the > > config.ru that Rails 3 creates for them? ?Yikes... > > No. The problem even occurs if you already have config.ru. But the > thing is, Rails 3 has deprecated ActionController::Dispatcher a few > weeks ago and replaced it with a stub. Rails::Rack::Static changed its > interface and must be constructed differently. The only way to obtain > a valid Rack endpoint for the app seems to be parsing > config/application.rbActually, I made "unicorn_rails" completely bypass the "rails_builder" method if there''s a config.ru, so it should never hit the ActionController::Dispatcher stuff.> > Let me know if the edited patch below looks alright to you. > > Yes it looks fine. A bit overcomplicated regexp compared to using > ''strip'' but whatever works. :)I just kept the regexp as-is, works for me. I just managed to push this to git://git.bogomips.org/unicorn.git before my Internet connection died on me earlier today. I''ve beefed up the tests a bit but will probably do more later. Eric Wong (4): t0300: Rails 3 test actually uses unicorn_rails tests: libify common rails3 setup code unicorn_rails: fix requires for Ruby 1.9.2 tests: add Rails 3 test for the missing config.ru case Hongli Lai (Phusion) (1): Fix unicorn_rails compatibility with the latest Rails 3 code Thanks again! -- Eric Wong
Eric Wong <normalperson at yhbt.net> wrote:> Hongli Lai <hongli at phusion.nl> wrote: > > Hi Eric. > > > > It would appear that recent Rails 3 changes have broken unicorn_rails, > > just like they broke Phusion Passenger. Here''s a patch which fixes the > > problem. > > http://gist.github.com/429944 > > Thanks Hongli, so this only affects people who remove the > config.ru that Rails 3 creates for them? Yikes...And for the completely bizzare: Rails 3 is ultra aggressive when searching for config.ru, it walks up the directory tree, even going all the way up to "/" if it can''t find config.ru. I had a $HOME/config.ru leftover from a test on one of my boxes, and usually have my working directory in $HOME/unicorn. This was was causing the unicorn_rails test for this (t0301) to fail because that test relies on Rails /not/ being able to find config.ru. So yes, don''t leave random artifacts lying around and love strace :) -- Eric Wong