incombatible with Ruby-3 / later gem versions (wrong number of arguments) #285
Comments
|
It's one of breaking changes in Ruby 3. |
|
to reproduce this issue: docker run --rm -i -v $(pwd):/src -w /src ruby:3.0-alpine sh -s <<EOS
apk add gcc make musl-dev sqlite-dev git &&
gem install bundler:1.17.3 &&
bundle _1.17.3_ add psych -v '~> 3.0' --skip-install &&
bundle _1.17.3_ install &&
bin/gemstash start --no-daemonize
EOSThe reason of output: |
|
After some investigation, I think it should be an upstream issue sinatra/sinatra#1701 |
|
I just ran into this. Looks like that upstream issue is fixed, is there anything we should do in gemstash? |
|
What version do you run? I just tried gemstash in docker ruby:3-alpine with ruby 3.0.2-p107, puma 4.3.10 and gemstash 2.1.0 and still do get /usr/local/bundle/gems/gemstash-2.1.0/lib/gemstash/web.rb:10:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError) |
Sorry, my bad. It's false positive because I mis-configured the gemstash. This is still an upstream issue: sinatra/sinatra#1531 (comment) |
|
One workaround is to give up keyword arguments. I made the changes by the following diff and it eventually works in Ruby 3. diff --git a/lib/gemstash/config.ru b/lib/gemstash/config.ru
index 78c2f12..495da43 100644
--- a/lib/gemstash/config.ru
+++ b/lib/gemstash/config.ru
@@ -11,4 +11,4 @@ use Puma::CommonLogger, Gemstash::Logging::StreamLogger.for_stdout if Gemstash::
use Gemstash::Env::RackMiddleware, Gemstash::Env.current
use Gemstash::GemSource::RackMiddleware
use Gemstash::Health::RackMiddleware
-run Gemstash::Web.new(gemstash_env: Gemstash::Env.current)
+run Gemstash::Web.new(Gemstash::Env.current)
diff --git a/lib/gemstash/web.rb b/lib/gemstash/web.rb
index 60ec7ca..ee89bfd 100644
--- a/lib/gemstash/web.rb
+++ b/lib/gemstash/web.rb
@@ -7,7 +7,7 @@ require "gemstash"
module Gemstash
#:nodoc:
class Web < Sinatra::Base
- def initialize(gemstash_env: nil, http_client_builder: nil)
+ def initialize(gemstash_env = nil, http_client_builder = nil)
@gemstash_env = gemstash_env || Gemstash::Env.new
@http_client_builder = http_client_builder || Gemstash::HTTPClient
Gemstash::Env.current = @gemstash_env |
Should we make that move, using that change, and add Ruby 3 to the build matrix? |
I don't know if we should. We don't even know when Sinatra would fully support Ruby 3. Besides, this might also be a breaking change depends on whether we consider I have another proposal, maybe we can make it compatible by supporting both sequence arguments and keyword arguments. Just like how ERB and Psych do:
If we don't think it's a breaking change, then I think applying the change I showed above would be OK. |
|
@tonytonyjan Even better! |
|
I can help make the PR if this is the conclusion :) |
|
@olleolleolle Yet another solution that I found is also good: diff --git a/lib/gemstash/web.rb b/lib/gemstash/web.rb
index 60ec7ca..32b3d2f 100644
--- a/lib/gemstash/web.rb
+++ b/lib/gemstash/web.rb
@@ -7,9 +7,11 @@ require "gemstash"
module Gemstash
#:nodoc:
class Web < Sinatra::Base
- def initialize(gemstash_env: nil, http_client_builder: nil)
- @gemstash_env = gemstash_env || Gemstash::Env.new
- @http_client_builder = http_client_builder || Gemstash::HTTPClient
+ ruby2_keywords def initialize(options = {})
+ raise ArgumentError unless options.is_a?(Hash)
+
+ @gemstash_env = options[:gemstash_env] || Gemstash::Env.new
+ @http_client_builder = options[:http_client_builder] || Gemstash::HTTPClient
Gemstash::Env.current = @gemstash_env
super()
endSo that we don't need to change the caller and they can still use keyword arguments: |
|
To be clearer, here are 2 options, both of them passes CI tests: Option 1 - support both sequence arguments and keyword arguments:It modifies 4 files:
diffdiff --git a/.github/workflows/gemstash-ci.yml b/.github/workflows/gemstash-ci.yml
index 6499b22..0881dee 100644
--- a/.github/workflows/gemstash-ci.yml
+++ b/.github/workflows/gemstash-ci.yml
@@ -8,7 +8,7 @@ jobs:
strategy:
fail-fast: false
matrix:
- ruby: [2.4, 2.5, 2.6, 2.7, jruby-9.2]
+ ruby: [2.4, 2.5, 2.6, 2.7, 3.0, jruby-9.2]
steps:
- uses: actions/checkout@v2
- name: Setup ruby
diff --git a/lib/gemstash/config.ru b/lib/gemstash/config.ru
index 78c2f12..3af2334 100644
--- a/lib/gemstash/config.ru
+++ b/lib/gemstash/config.ru
@@ -11,4 +11,5 @@ use Puma::CommonLogger, Gemstash::Logging::StreamLogger.for_stdout if Gemstash::
use Gemstash::Env::RackMiddleware, Gemstash::Env.current
use Gemstash::GemSource::RackMiddleware
use Gemstash::Health::RackMiddleware
-run Gemstash::Web.new(gemstash_env: Gemstash::Env.current)
+
+run Gemstash::Web.new(Gemstash::Env.current)
diff --git a/lib/gemstash/web.rb b/lib/gemstash/web.rb
index 60ec7ca..10a37d0 100644
--- a/lib/gemstash/web.rb
+++ b/lib/gemstash/web.rb
@@ -7,7 +7,23 @@ require "gemstash"
module Gemstash
#:nodoc:
class Web < Sinatra::Base
- def initialize(gemstash_env: nil, http_client_builder: nil)
+ NOT_GIVEN = Object.new
+ private_constant :NOT_GIVEN
+
+ def initialize(
+ legacy_gemstash_env = NOT_GIVEN,
+ legacy_http_client_builder = NOT_GIVEN,
+ gemstash_env: nil,
+ http_client_builder: nil
+ )
+ if legacy_gemstash_env != NOT_GIVEN
+ warn "Passing gemstash_env with the 1st argument of Gemstash::Web.new is deprecated. Use keyword argument like Gemstash::Web.new(gemstash_env: ...) instead.", uplevel: 1 if $VERBOSE
+ gemstash_env = legacy_gemstash_env
+ end
+ if legacy_http_client_builder != NOT_GIVEN
+ warn "Passing http_client_builder with the 2nd argument of Gemstash::Web.new is deprecated. Use keyword argument like Gemstash::Web.new(yaml, http_client_builder: ...) instead.", uplevel: 1 if $VERBOSE
+ http_client_builder = legacy_http_client_builder
+ end
@gemstash_env = gemstash_env || Gemstash::Env.new
@http_client_builder = http_client_builder || Gemstash::HTTPClient
Gemstash::Env.current = @gemstash_env
diff --git a/spec/gemstash/web_spec.rb b/spec/gemstash/web_spec.rb
index 18a102e..98a396a 100644
--- a/spec/gemstash/web_spec.rb
+++ b/spec/gemstash/web_spec.rb
@@ -31,8 +31,7 @@ RSpec.describe Gemstash::Web do
StubHttpBuilder.new
end
let(:app) do
- Gemstash::Web.new(http_client_builder: http_client_builder,
- gemstash_env: test_env)
+ Gemstash::Web.new(test_env, http_client_builder)
end
let(:upstream) { "https://rubygems.org" }
let(:gem_source) { Gemstash::GemSource::RubygemsSource }
diff --git a/spec/support/test_gemstash_server.ru b/spec/support/test_gemstash_server.ru
index 358a587..c1ee899 100644
--- a/spec/support/test_gemstash_server.ru
+++ b/spec/support/test_gemstash_server.ru
@@ -5,4 +5,4 @@ use Rack::Deflater
use Gemstash::Env::RackMiddleware, $test_gemstash_server_env
use Gemstash::GemSource::RackMiddleware
use Gemstash::Health::RackMiddleware
-run Gemstash::Web.new(gemstash_env: $test_gemstash_server_env)
+run Gemstash::Web.new($test_gemstash_server_env)
Option 2 - Use
|
|
@olleolleolle any thought about how to support Ruby 3? |
|
@tonytonyjan My take: This repo is an appplication, so it's fair to depend on and use ruby2_keywords. |
Hi,
just to make the report reproducible, I'm using a docker file. This dockerfile works (with ruby-2.7):
FROM ruby:2-alpine
RUN apk add --update --upgrade --no-cache --virtual .build-dependencies gcc musl-dev make sqlite-dev &&
gem install gemstash &&
apk del .build-dependencies &&
rm -rf /var/cache/apk/*
CMD ["gemstash","start","--no-daemonize"]
But when replacing the FROM image with ruby:alpine (i.e. using ruby 3) it still compiles,yet the process results in
/usr/local/bundle/gems/gemstash-2.1.0/lib/gemstash/web.rb:10:in
initialize': wrong number of arguments (given 1, expected 0) (ArgumentError) from /usr/local/bundle/gems/sinatra-2.1.0/lib/sinatra/base.rb:1527:innew'[...]
regards
The text was updated successfully, but these errors were encountered: