incombatible with Ruby-3 / later gem versions (wrong number of arguments) #285

Activity

1. tonytonyjan commented on Jun 3, 2021

   

   tonytonyjan

   on Jun 3, 2021

   Contributor

   More actions

   It's one of breaking changes in Ruby 3.

   https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
2. tonytonyjan commented on Jun 3, 2021

   

   tonytonyjan

   on Jun 3, 2021

   Last edited by tonytonyjan

   Contributor

   More actions

   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
   EOS

   The reason of bundle _1.17.3_ add psych -v '~> 3.0' --skip-install is taht gemstash is not compatible with psych >=4, see #297 for more detail. cc @hsbt

   output:

   Starting gemstash!                                                                                                                                                                  [212/1693]
   [2021-06-03 05:12:36 +0000] - INFO - [573] Puma starting in cluster mode...
   [2021-06-03 05:12:36 +0000] - INFO - [573] * Version 4.3.8 (ruby 3.0.1-p64), codename: Mysterious Traveller
   [2021-06-03 05:12:36 +0000] - INFO - [573] * Min threads: 0, max threads: 16
   [2021-06-03 05:12:36 +0000] - INFO - [573] * Environment: development
   [2021-06-03 05:12:36 +0000] - INFO - [573] * Process workers: 1
   [2021-06-03 05:12:36 +0000] - INFO - [573] * Phased restart available
   [2021-06-03 05:12:36 +0000] - INFO - [573] * Listening on tcp://0.0.0.0:9292
   [2021-06-03 05:12:36 +0000] - INFO - [573] Use Ctrl-C to stop
   /src/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:in `new'
           from /usr/local/bundle/gems/sinatra-2.1.0/lib/sinatra/base.rb:1527:in `new'
           from /src/lib/gemstash/config.ru:14:in `block in <main>'
           from /usr/local/bundle/gems/rack-2.2.3/lib/rack/builder.rb:116:in `eval'
           from /usr/local/bundle/gems/rack-2.2.3/lib/rack/builder.rb:116:in `new_from_string'
           from /usr/local/bundle/gems/rack-2.2.3/lib/rack/builder.rb:105:in `load_file'
           from /usr/local/bundle/gems/rack-2.2.3/lib/rack/builder.rb:66:in `parse_file'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/configuration.rb:321:in `load_rackup'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/configuration.rb:246:in `app'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/runner.rb:165:in `app'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/runner.rb:172:in `start_server'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cluster.rb:269:in `worker'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cluster.rb:138:in `block (2 levels) in spawn_workers'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cluster.rb:138:in `fork'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cluster.rb:138:in `block in spawn_workers'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cluster.rb:134:in `times'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cluster.rb:134:in `spawn_workers'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cluster.rb:469:in `run'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/launcher.rb:172:in `run'
           from /usr/local/bundle/gems/puma-4.3.8/lib/puma/cli.rb:80:in `run'
           from /src/lib/gemstash/cli/start.rb:16:in `run'
           from /src/lib/gemstash/cli.rb:79:in `start'
           from /usr/local/bundle/gems/thor-0.20.3/lib/thor/command.rb:27:in `run'
           from /usr/local/bundle/gems/thor-0.20.3/lib/thor/invocation.rb:126:in `invoke_command'
           from /usr/local/bundle/gems/thor-0.20.3/lib/thor.rb:387:in `dispatch'
           from /usr/local/bundle/gems/thor-0.20.3/lib/thor/base.rb:466:in `start'
           from /src/lib/gemstash/cli.rb:34:in `start'
           from bin/gemstash:4:in `<main>'
   
3. tonytonyjan commented on Jun 3, 2021

   

   tonytonyjan

   on Jun 3, 2021

   Contributor

   More actions

   After some investigation, I think it should be an upstream issue sinatra/sinatra#1701
4. kyrofa commented on Aug 18, 2021

   

   kyrofa

   on Aug 18, 2021

   Contributor

   More actions

   I just ran into this. Looks like that upstream issue is fixed, is there anything we should do in gemstash?
5. hadmut commented on Nov 6, 2021

   

   hadmut

   on Nov 6, 2021

   Author

   More actions

   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)
6. tonytonyjan commented on Nov 6, 2021

   

   tonytonyjan

   on Nov 6, 2021

   Contributor

   More actions

   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)
7. tonytonyjan commented on Nov 6, 2021

   

   tonytonyjan

   on Nov 6, 2021

   Last edited by tonytonyjan

   Contributor

   More actions

   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
8. olleolleolle commented on Nov 9, 2021

   

   olleolleolle

   on Nov 9, 2021

   Member

   More actions

   One workaround is to give up keyword arguments.

   I made the changes by the following diff and it eventually works in Ruby 3.

   Should we make that move, using that change, and add Ruby 3 to the build matrix?
9. tonytonyjan commented on Nov 9, 2021

   

   tonytonyjan

   on Nov 9, 2021

   Last edited by tonytonyjan

   Contributor

   More actions

   One workaround is to give up keyword arguments.
   I made the changes by the following diff and it eventually works in Ruby 3.

   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 Gemstash::Web as an internal class or not.

   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:

   • https://ruby-doc.org/stdlib-3.0.2/libdoc/psych/rdoc/Psych.html#load-method
   • https://ruby-doc.org/stdlib-3.0.2/libdoc/erb/rdoc/ERB.html#method-c-new

   If we don't think it's a breaking change, then I think applying the change I showed above would be OK.

   👍React with 👍1Reacted by Olle Jonsson
10. olleolleolle commented on Nov 9, 2021

    

    olleolleolle

    on Nov 9, 2021

    Member

    More actions

    @tonytonyjan Even better!
11. tonytonyjan commented on Nov 9, 2021

    

    tonytonyjan

    on Nov 9, 2021

    Contributor

    More actions

    I can help make the PR if this is the conclusion :)

    👍React with 👍1Reacted by Olle Jonsson
12. tonytonyjan

    added 3 commits that reference this issue on Nov 10, 2021

    feat: support Ruby 3

    ...

    This issue will close once commit de4a2b9 is merged into the 'master' branch.

    de4a2b9

    feat: support Ruby 3

    ...

    This issue will close once commit e6c6c2c is merged into the 'master' branch.

    e6c6c2c

    feat: support Ruby 3

    ...

    This issue will close once commit 3c657aa is merged into the 'master' branch.

    3c657aa
13. tonytonyjan commented on Nov 10, 2021

    

    tonytonyjan

    on Nov 10, 2021

    Contributor

    More actions

    @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()
         end

    So that we don't need to change the caller and they can still use keyword arguments:

    $ grep -F Web.new -R .
    ./spec/gemstash/web_spec.rb:    Gemstash::Web.new(http_client_builder: http_client_builder,
    ./spec/support/test_gemstash_server.ru:run Gemstash::Web.new(gemstash_env: $test_gemstash_server_env)
    ./lib/gemstash/config.ru:run Gemstash::Web.new(gemstash_env: Gemstash::Env.current)
    
14. tonytonyjan commented on Nov 10, 2021

    

    tonytonyjan

    on Nov 10, 2021

    Last edited by tonytonyjan

    Contributor

    More actions

    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:

    • lib/gemstash/config.ru
    • lib/gemstash/web.rb
    • spec/gemstash/web_spec.rb
    • spec/support/test_gemstash_server.ru

    diff

    diff --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 ruby2_keywords

    It modifies 1 files:

    • lib/gemstash/web.rb

    diff

    diff --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/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()
         end
15. tonytonyjan commented on Nov 11, 2021

    

    tonytonyjan

    on Nov 11, 2021

    Contributor

    More actions

    @olleolleolle any thought about how to support Ruby 3?
16. olleolleolle commented on Nov 11, 2021

    

    olleolleolle

    on Nov 11, 2021

    Member

    More actions

    @tonytonyjan My take: This repo is an appplication, so it's fair to depend on and use ruby2_keywords.
17. tonytonyjan

    added a commit that references this issue on Nov 11, 2021

    feat: support Ruby 3

    ...

    This issue will close once commit 968a8bf is merged into the 'master' branch.

    968a8bf
18. olleolleolle

    closed this as completedin #312on Nov 11, 2021
19. olleolleolle

    added a commit that references this issue on Nov 11, 2021

    feat: support Ruby 3 (#312)

    ...

    This issue will close once commit 8899d01 is merged into the 'main' branch.

    8899d01