Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support Ruby 3.3.0+ #53

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support Ruby 3.3.0+ #53

wants to merge 4 commits into from

Conversation

akito19
Copy link
Member

@akito19 akito19 commented Feb 2, 2024

Treasure Data released td-client-ruby v2.0, which version includes Ruby 3.3+ support.
However, this gem doesn't allow install the new version of client library since this gem requires < 2.0. Then, this pull request relaxes the gem requirements for td client.

After merging this pull request, TD Toolbelt will be able to install the client library v2.0 as well.

@@ -1,5 +1,6 @@
require 'spec_helper'
require "rack/mock"
require 'rack/builder'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this line because my local test failed due to uninitialized constant.

Failures:

  1) TreasureData::Logger::Agent::Rack::Hook.before
     Failure/Error:
       @app ||= Rack::Builder.new {
         use TreasureData::Logger::Agent::Rack::Hook
         run lambda {|env| [200, {}, ["body"]]}
       }.to_app

     NameError:
       uninitialized constant Rack::Builder
     # ./spec/td/logger/agent/rack_spec.rb:7:in `app'
     # ./spec/td/logger/agent/rack_spec.rb:15:in `block (2 levels) in <top (required)>'
     # ./spec/td/logger/agent/rack_spec.rb:22:in `block (3 levels) in <top (required)>'

  2) TreasureData::Logger::Agent::Rack::Hook.after
     Failure/Error:
       @app ||= Rack::Builder.new {
         use TreasureData::Logger::Agent::Rack::Hook
         run lambda {|env| [200, {}, ["body"]]}
       }.to_app

     NameError:
       uninitialized constant Rack::Builder
     # ./spec/td/logger/agent/rack_spec.rb:7:in `app'
     # ./spec/td/logger/agent/rack_spec.rb:15:in `block (2 levels) in <top (required)>'
     # ./spec/td/logger/agent/rack_spec.rb:33:in `block (3 levels) in <top (required)>'

Finished in 0.30078 seconds (files took 1.03 seconds to load)
147 examples, 2 failures, 5 pending

@@ -32,7 +32,7 @@ EOF
gem.require_paths = ['lib']

gem.add_dependency "msgpack", ">= 0.5.6", "< 2.0"
gem.add_dependency "td-client", ">= 0.8.66", "< 2.0"
gem.add_dependency "td-client", ">= 0.8.66", "< 3.0"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since 0.8.66 is too old, it may be able to restrict version to 1.0.8...?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good (y)

Copy link
Member Author

@akito19 akito19 May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 50ba787:

- gem.add_dependency "td-client", ">= 0.8.66", "< 3.0"
+ gem.add_dependency "td-client", ">= 1.0.8", "< 3.0"

@akito19 akito19 force-pushed the relax-td-client-version branch from 98de1f6 to 5ef3bdb Compare February 9, 2024 09:27
@akito19 akito19 force-pushed the relax-td-client-version branch from 5ef3bdb to 8e8a971 Compare February 9, 2024 09:30
@@ -6,10 +6,10 @@
unless ENV['APPVEYOR']
require 'coveralls'

SimpleCov.formatter = SimpleCov::Formatter::MultiFormatter[
SimpleCov.formatter = SimpleCov::Formatter::MultiFormatter.new([
Copy link
Member Author

@akito19 akito19 Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up warnings,

/home/runner/work/td-logger-ruby/td-logger-ruby/spec/spec_helper.rb:9:in `<top (required)>': [DEPRECATION] ::[] is deprecated. Use ::new instead.

gem.add_dependency "fluent-logger", ">= 0.5.0", "< 2.0"
gem.add_dependency "mutex_m", ">= 0.2.0", "< 1.0"
Copy link
Member Author

@akito19 akito19 Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutex_m was independent as a Ruby gem from Ruby.
https://www.ruby-lang.org/en/news/2023/12/11/ruby-3-3-0-rc1-released/

If users have used this gem with Ruby 2.4 or lower, they failed td-logger process because this gem supports Ruby 2.5+. But I believe this is no problem since Ruby 2.6 or lower reached EOL.
https://github.com/ruby/mutex_m/blob/2a0c5af457a85b91ca39122511658661479f149e/.github/workflows/test.yml#L10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, but I don't understand why this added line is needed? If we're targeting ruby 2.7..5 and up here, IIUC, user should be able to use their own mutex_m without issue, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tung-vu-td Sorry for my late response. mutex_m will be apart from Ruby as a standard library.
If I removed the line, test case (Ruby 3.4+) got an error.
https://github.com/treasure-data/td-logger-ruby/actions/runs/9186667898/job/25262771815

Since this library uses mutex process, I believe this gem should be needed.

@akito19 akito19 changed the title Allow use td-client v2.0+ Add support Ruby 3.3.0+ Feb 9, 2024
@akito19 akito19 force-pushed the relax-td-client-version branch from 2e96676 to 50ba787 Compare May 22, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants