Use Rack.release over Rack::RELEASE (rack 1..6.X compatibility) (#3156)

* Use Rack.release over Rack::RELEASE The constant Rack::RELEASE was introduced only in rack version 2 (specifically: rack/rack@a2fe30a) Earlier versions provided the class method Rack.release which still exists. #3061 has introduced usage of Rack::RELEASE and therefore broke compatibility with rack 1.6.X. * test.yaml, Gemfile - allow Rack 1.6.x CI * lib/rack/handler/puma.rb - allow Rack 1.6.x * test_rack_server.rb - allow testing with Rack 1.6.x * test_puma_server_hijack.rb - allow testing with Rack 1.6.x --------- Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
puma · May 16, 2023 · 3d156ed · 3d156ed
1 parent 898dc44
commit 3d156ed
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 28 deletions.
diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml
@@ -23,7 +23,7 @@ jobs:
 
   test_mri:
     name: >-
-      MRI: ${{ matrix.os }} ${{ matrix.ruby }}${{ matrix.no-ssl }}${{ matrix.yjit }}${{ matrix.rack-2 }}
+      MRI: ${{ matrix.os }} ${{ matrix.ruby }}${{ matrix.no-ssl }}${{ matrix.yjit }}${{ matrix.rack-v }}
     needs: [rubocop, skip_duplicate_runs]
     env:
       CI: true
@@ -41,16 +41,17 @@ jobs:
         os: [ ubuntu-20.04, ubuntu-22.04, macos-11, macos-12, macos-13, windows-2022 ]
         ruby: [ 2.4, 2.5, 2.6, 2.7, '3.0', 3.1, 3.2, head ]
         no-ssl: ['']
-        rack-2: ['']
+        rack-v: ['']
         yjit: ['']
         include:
           - { os: windows-2022 , ruby: ucrt }
           - { os: windows-2022 , ruby: mswin }
           - { os: windows-2022 , ruby: 2.7  , no-ssl: ' no SSL' }
           - { os: ubuntu-20.04 , ruby: 2.7  , no-ssl: ' no SSL' }
           - { os: ubuntu-22.04 , ruby: head , yjit: ' yjit' }
-          - { os: ubuntu-22.04 , ruby: 2.4  , rack-2: ' rack2' }
-          - { os: ubuntu-22.04 , ruby: 3.2  , rack-2: ' rack2' }
+          - { os: ubuntu-22.04 , ruby: 2.4  , rack-v: ' rack2' }
+          - { os: ubuntu-22.04 , ruby: 3.2  , rack-v: ' rack2' }
+          - { os: ubuntu-22.04 , ruby: 2.4  , rack-v: ' rack1' }
 
         exclude:
           - { os: ubuntu-22.04 , ruby:  2.4  }
@@ -83,12 +84,9 @@ jobs:
         shell: bash
         run: echo 'PUMA_DISABLE_SSL=true' >> $GITHUB_ENV
 
-      - name: Test with Rack 2
-        if: |
-          (matrix.rack-2 == ' rack2') &&
-          (needs.skip_duplicate_runs.outputs.should_skip != 'true')
+      - name: Set Rack version, see Gemfile
         shell: bash
-        run: echo 'PUMA_CI_RACK_2=true' >> $GITHUB_ENV
+        run: echo 'PUMA_CI_RACK=${{ matrix.rack-v }}' >> $GITHUB_ENV
 
       - name: load ruby
         if: ${{ needs.skip_duplicate_runs.outputs.should_skip != 'true' }}

diff --git a/Gemfile b/Gemfile
@@ -11,8 +11,20 @@ gem "minitest-retry"
 gem "minitest-proveit"
 gem "minitest-stub-const"
 
-gem "rack", (ENV['PUMA_CI_RACK_2'] ? "~> 2.2" : ">= 2.2")
-gem "rackup" unless ENV['PUMA_CI_RACK_2']
+use_rackup = false
+rack_vers =
+  case ENV['PUMA_CI_RACK']&.strip
+  when 'rack2'
+    '~> 2.2'
+  when 'rack1'
+    '~> 1.6'
+  else
+    use_rackup = true
+    '>= 2.2'
+  end
+
+gem "rack", rack_vers
+gem "rackup" if use_rackup
 
 gem "jruby-openssl", :platform => "jruby"
 

diff --git a/lib/puma/rack_default.rb b/lib/puma/rack_default.rb
@@ -11,7 +11,7 @@ def self.default(options = {})
       end
     end
   end
-elsif Object.const_defined?(:Rack) && Rack::RELEASE < '3'
+elsif Object.const_defined?(:Rack) && Rack.release < '3'
   module Rack
     module Handler
       def self.default(options = {})

diff --git a/lib/rack/handler/puma.rb b/lib/rack/handler/puma.rb
@@ -31,7 +31,11 @@ def config(app, options = {})
 
       conf = ::Puma::Configuration.new(options, default_options.merge({events: @events})) do |user_config, file_config, default_config|
         if options.delete(:Verbose)
-          require 'rack/common_logger'
+          begin
+            require 'rack/commonlogger'  # Rack 1.x
+          rescue LoadError
+            require 'rack/common_logger' # Rack 2 and later
+          end
           app = ::Rack::CommonLogger.new(app, STDOUT)
         end
 
@@ -123,7 +127,7 @@ class << self
     end
   end
 else
-  do_register = Object.const_defined?(:Rack) && Rack::RELEASE < '3'
+  do_register = Object.const_defined?(:Rack) && Rack.release < '3'
   module Rack
     module Handler
       module Puma

diff --git a/test/helper.rb b/test/helper.rb
@@ -182,7 +182,7 @@ def skip_if(*engs, suffix: '', bt: caller)
         when :fork        then "Skipped if Kernel.fork exists"   if HAS_FORK
         when :unix        then "Skipped if UNIXSocket exists"    if Puma::HAS_UNIX_SOCKET
         when :aunix       then "Skipped if abstract UNIXSocket"  if Puma.abstract_unix_socket?
-        when :rack3       then "Skipped if Rack 3.x"             if Rack::RELEASE >= '3'
+        when :rack3       then "Skipped if Rack 3.x"             if Rack.release >= '3'
         else false
       end
       skip skip_msg, bt if skip_msg
@@ -201,7 +201,7 @@ def skip_unless(eng, bt: caller)
       when :fork    then MSG_FORK                       unless HAS_FORK
       when :unix    then MSG_UNIX                       unless Puma::HAS_UNIX_SOCKET
       when :aunix   then MSG_AUNIX                      unless Puma.abstract_unix_socket?
-      when :rack3   then "Skipped unless Rack >= 3.x"   unless ::Rack::RELEASE >= '3'
+      when :rack3   then "Skipped unless Rack >= 3.x"   unless ::Rack.release >= '3'
       else false
     end
     skip skip_msg, bt if skip_msg

diff --git a/test/test_config.rb b/test/test_config.rb
@@ -17,7 +17,7 @@ def test_default_max_threads
   end
 
   def test_app_from_rackup
-    if Rack::RELEASE >= '3'
+    if Rack.release >= '3'
       fn = "test/rackup/hello-bind_rack3.ru"
       bind = "tcp://0.0.0.0:9292"
     else

diff --git a/test/test_puma_server_hijack.rb b/test/test_puma_server_hijack.rb
@@ -29,6 +29,7 @@ def setup
   end
 
   def teardown
+    return if skipped?
     @server.stop(true)
     assert_empty @log_writer.stdout.string
     assert_empty @log_writer.stderr.string
@@ -172,13 +173,15 @@ def test_http_10_header_with_content_length
   end
 
   def test_partial_hijack_body_closes_body
+    skip 'Not supported with Rack 1.x' if Rack.release.start_with? '1.'
     @available = true
     hdrs = { 'Content-Type' => 'text/plain' }
     body = ::Rack::BodyProxy.new(HIJACK_LAMBDA) { @available = true }
     partial_hijack_closes_body(hdrs, body)
   end
 
   def test_partial_hijack_header_closes_body_correct_precedence
+    skip 'Not supported with Rack 1.x' if Rack.release.start_with? '1.'
     @available = true
     incorrect_lambda = ->(io) {
       io.syswrite 'incorrect body.call'

diff --git a/test/test_rack_server.rb b/test/test_rack_server.rb
@@ -3,14 +3,18 @@
 require "net/http"
 
 # don't load Rack, as it autoloads everything
-require "rack/body_proxy"
-require "rack/lint"
-require "rack/version"
-require "rack/common_logger"
+begin
+  require "rack/body_proxy"
+  require "rack/lint"
+  require "rack/version"
+  require "rack/common_logger"
+rescue LoadError # Rack 1.6
+  require "rack"
+end
 
 # Rack::Chunked is loaded by Rack v2, needs to be required by Rack 3.0,
 # and is removed in Rack 3.1
-require "rack/chunked" if Rack::RELEASE.start_with? '3.0'
+require "rack/chunked" if Rack.release.start_with? '3.0'
 
 require "nio"
 
@@ -41,7 +45,7 @@ def call(env)
 
   class ServerLint < Rack::Lint
     def call(env)
-      if Rack::RELEASE < '3'
+      if Rack.release < '3'
         check_env env
       else
         Wrapper.new(@app, env).check_environment env
@@ -206,13 +210,15 @@ def test_rack_body_proxy_content_length
 
     headers = header_hash socket
 
-    content_length = headers["Content-Length"].to_i
-
     socket.close
 
     stop
 
-    assert_equal str_ary_bytes, content_length
+    if Rack.release.start_with? '1.'
+      assert_equal "chunked", headers["Transfer-Encoding"]
+    else
+      assert_equal str_ary_bytes, headers["Content-Length"].to_i
+    end
   end
 
   def test_common_logger
@@ -241,7 +247,7 @@ def test_rack_chunked_array1
     resp = Net::HTTP.get_response URI(@tcp)
     assert_equal 'chunked', resp['transfer-encoding']
     assert_equal STR_1KB, resp.body.force_encoding(Encoding::UTF_8)
-  end if Rack::RELEASE < '3.1'
+  end if Rack.release < '3.1'
 
   def test_rack_chunked_array10
     body = Array.new 10, STR_1KB
@@ -253,7 +259,7 @@ def test_rack_chunked_array10
     resp = Net::HTTP.get_response URI(@tcp)
     assert_equal 'chunked', resp['transfer-encoding']
     assert_equal STR_1KB * 10, resp.body.force_encoding(Encoding::UTF_8)
-  end if Rack::RELEASE < '3.1'
+  end if Rack.release < '3.1'
 
   def test_puma_enum
     body = Array.new(10, STR_1KB).to_enum