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

Support Ruby >=2.4 #1876

Merged
merged 18 commits into from
May 29, 2017
Merged

Support Ruby >=2.4 #1876

merged 18 commits into from
May 29, 2017

Conversation

knu
Copy link
Member

@knu knu commented Jan 17, 2017

I know that the json gem version 1.8.6 was released the other day which supports ruby >=2.4, but there are other gems that need to be updated for ruby >=2.4, and I think this is a good opportunity for us to check on existing dependencies and move forward.

@knu knu force-pushed the support_ruby2.4 branch 2 times, most recently from 97fea4d to bcc57d9 Compare January 19, 2017 05:48
@knu
Copy link
Member Author

knu commented Jan 19, 2017

Travis does not seem to properly check out the latest commit...

@knu
Copy link
Member Author

knu commented Jan 19, 2017

The push build is getting back to normal but the pr build is not, removing the cache and seeing if it works.

@knu knu force-pushed the support_ruby2.4 branch 3 times, most recently from 0a42836 to 739bdd8 Compare January 19, 2017 08:11
@knu
Copy link
Member Author

knu commented Jan 19, 2017

All green, finally! And the conflicts are easy to resolve.

@dsander
Copy link
Collaborator

dsander commented Jan 19, 2017

Awesome work! Can you bump the ruby version in the docker images? I would like to test the Twitter Agents for a few days.

@knu
Copy link
Member Author

knu commented Jan 20, 2017

@dsander Done!

@dsander
Copy link
Collaborator

dsander commented Jan 20, 2017

Thanks! Looks good so far.

@cantino
Copy link
Member

cantino commented Jan 21, 2017

Nice! I'll get a test deploy out to my system as well.

Copy link
Collaborator

@dsander dsander left a comment

Choose a reason for hiding this comment

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

My instance is working well. 👍

@knu Do you think we should update the manual installation guide to ruby 2.4 as well?

@knu
Copy link
Member Author

knu commented Feb 7, 2017

@cantino How's it going for you?

@dsander Yeah, we could probably add UPGRADING.md containing a general upgrade guide plus version-specific notes.

@dsander
Copy link
Collaborator

dsander commented Feb 7, 2017

Right, a note similar that should probably be enough https://github.com/gitlabhq/gitlabhq/blob/master/doc/update/8.15-to-8.16.md#3-update-ruby. The first version of the manual installation guide used ruby 2.2.2 which we will probably support until Rails 6, so nobody has to upgrade soon.

@cantino
Copy link
Member

cantino commented Feb 12, 2017

Sorry, I use the backup gem and it doesn't seem to work on 2.4.0 yet. I'll see if I can find a work around.

@knu
Copy link
Member Author

knu commented Feb 20, 2017

@cantino Ideally the gem should completely remove its version constraints on the json gem, but in the meantime you could locally bundle update the json gem to 1.8.6 to fix the problem.

cf. backup/backup#830

@cantino
Copy link
Member

cantino commented Feb 27, 2017

Do you think we should wait for Rails 5.1 so that some of these deprecations go away?

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/activesupport-5.0.1/lib/active_support/xml_mini.rb:51:
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/activesupport-5.0.1/lib/active_support/xml_mini.rb:52:
warning: constant ::Bignum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/builder-3.2.2/lib/builder/xchar.rb:111:
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/activesupport-5.0.1/lib/active_support/core_ext/numeric/conversions.rb:138:
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/activejob-5.0.1/lib/active_job/arguments.rb:38: 
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/activejob-5.0.1/lib/active_job/arguments.rb:38:
warning: constant ::Bignum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/erector-0.10.0/lib/erector/needs.rb:86: 
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/erector-0.10.0/lib/erector/needs.rb:86: 
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/erector-0.10.0/lib/erector/needs.rb:86: 
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/nokogiri-1.6.8.1/lib/nokogiri/html/document.rb:164:
warning: constant ::Fixnum is deprecated

/Users/andrew/.rvm/gems/ruby-2.4.0@huginn/gems/nokogiri-1.6.8.1/lib/nokogiri/html/document.rb:164:
warning: constant ::Fixnum is deprecated

@0xdevalias
Copy link
Member

0xdevalias commented Mar 1, 2017

@cantino My personal feeling is that deprecation warnings aren't a terrible thing, and particularly not one that should hold back an upgrade. I think do this now, and then move to 5.1 as soon as it becomes available.

Having 2.4 available will likely be a prerequisite for docker-ng landing (#1874)

Gemfile Outdated
@@ -48,12 +48,12 @@ gem 'hypdf', '~> 1.0.10' # PDFInfoAgent
gem 'weibo_2', github: 'dsander/weibo_2', branch: 'master'

# GoogleCalendarPublishAgent
gem "google-api-client", require: 'google/api_client'
gem "google-api-client", '~> 0.7.1', require: 'google/api_client'
Copy link
Member

Choose a reason for hiding this comment

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

I was looking into this a little for (#1874) While latest brings in 'breaking changes', it seems to be a minimal fix.

It's not updated in the migrating notes in master yet, but see:

I changed to the following, and inital tests (aka, installation) seemed like it might work ok:

gem "google-api-client", require: 'google/apis'

Gemfile Outdated
@@ -107,7 +107,7 @@ gem 'httparty', '~> 0.13'
gem 'httmultiparty', '~> 0.3.16'
gem 'jquery-rails', '~> 4.2.1'
gem 'huginn_agent', '~> 0.4.0'
gem 'json', '~> 1.8.1'
gem 'json'
Copy link
Member

Choose a reason for hiding this comment

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

I had this changed to gem 'json', '~> 1.8.5' in #1874, but if this works, even better!

0xdevalias
0xdevalias previously approved these changes Mar 1, 2017
Copy link
Member

@0xdevalias 0xdevalias left a comment

Choose a reason for hiding this comment

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

Looks good to me! A couple of minor inline comments.

@0xdevalias 0xdevalias dismissed stale reviews from themself March 1, 2017 22:47
Copy link
Member

@0xdevalias 0xdevalias left a comment

Choose a reason for hiding this comment

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

See inline comments. Not 100% if they are required/best to implement now.

  • google-api-client

@dsander
Copy link
Collaborator

dsander commented Mar 1, 2017

I only tested the docker image before, the deprecation warnings are a bit annoying when developing locally. Also running rake seems to be broken:

$ rake
/Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/xml_mini.rb:51: warning: constant ::Fixnum is deprecated
/Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/xml_mini.rb:52: warning: constant ::Bignum is deprecated
/Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/builder-3.2.2/lib/builder/xchar.rb:111: warning: constant ::Fixnum is deprecated
/Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/dependencies.rb:293:in `require': cannot load such file -- /Users/dominik/code/ruby/huginn/test (LoadError)
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/dependencies.rb:293:in `block in require'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/dependencies.rb:259:in `load_dependency'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/dependencies.rb:293:in `require'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/railties-5.0.1/lib/rails/test_unit/test_requirer.rb:11:in `block in require_files'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/railties-5.0.1/lib/rails/test_unit/test_requirer.rb:10:in `each'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/railties-5.0.1/lib/rails/test_unit/test_requirer.rb:10:in `require_files'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/railties-5.0.1/lib/rails/test_unit/minitest_plugin.rb:86:in `plugin_rails_init'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/minitest-5.10.1/lib/minitest.rb:80:in `block in init_plugins'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/minitest-5.10.1/lib/minitest.rb:78:in `each'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/minitest-5.10.1/lib/minitest.rb:78:in `init_plugins'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/minitest-5.10.1/lib/minitest.rb:129:in `run'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/railties-5.0.1/lib/rails/test_unit/minitest_plugin.rb:73:in `run'
	from /Users/dominik/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/minitest-5.10.1/lib/minitest.rb:62:in `block in autorun'

If rails 5.0.1 does not officially support ruby 2.4 I think we should wait for 5.1.

@0xdevalias
Copy link
Member

0xdevalias commented Mar 2, 2017

Unsure of offical 2.4 support in 5.0.1, though found the following:

This implies rails master is 2.4 compatible.

4.2.8 was the first 4.2.x version to add support for 2.4:

This is the first version of the 4.2 series that officially support Ruby 2.4.

In reading the 5.1-beta notes, came across this snippet:

Basecamp 3 is already running this beta in production. This is an incremental upgrade to Rails 5.0.

So perhaps we could run the beta version if it truly only landed in 5.1? (See also: #1912)

Also, there appears to be a 5.0.2 now, it's tested against 2.4.0, and has a couple of merges specifically mentioning 2.4.0:

Worth a try?

Edit: Rails 5.1.0.rc1 has been released

@dsander
Copy link
Collaborator

dsander commented May 19, 2017

At first I thought it was not related to changes in this PR but this test fails for me both on 2.4 and 2.3:

  1) Agents::WebsiteAgent checking with http basic auth #check should check for changes
     Failure/Error: expect { @checker.check }.to change { Event.count }.by(1)
       expected result to have changed by 1, but was changed by 0
     # ./spec/models/agents/website_agent_spec.rb:1328:in `block (4 levels) in <top (required)>'

After bisecting I found this commit to be the cause:

commit 91c0ba5048cf6cf5ce2166ce483a90289ad7cf10
Author: Akinori MUSHA <knu@idaemons.org>
Date:   Thu Jan 12 15:35:52 2017 +0900

    Update webmock to 2.3.2 for ruby >=2.4

My FARADAY_HTTP_BACKEND is set to net_http and this mock does not match.

      stub_request(:any, "www.example.com").
        with(headers: { 'Authorization' => "Basic #{['user:pass'].pack('m0')}" }).
        to_return(body: File.read(Rails.root.join("spec/data_fixtures/xkcd.html")), status: 200)

Works both for net_http and typhoeus.

Maybe we should additionally set FARADAY_HTTP_BACKEND in env.test or run the specs with multiple backends on Travis?

@knu
Copy link
Member Author

knu commented May 24, 2017

@dsander I updated the mock. Hopefully it's fixed now...

@dsander
Copy link
Collaborator

dsander commented May 25, 2017

@knu Yes, thanks looks good now. Can you re enable the cantino/* Docker image builds in the Travis configuration?
Everything else looks good to me.

@knu
Copy link
Member Author

knu commented May 27, 2017

@dsander Done, but one of the image builds failed...

@dsander
Copy link
Collaborator

dsander commented May 29, 2017

@knu Thanks, I fixed the build. The status did not update, but the image was build and pushed successfully: https://huginnbuilder.dsander.de/repositories/2/builds/571

Everyhing looks good to me

@knu
Copy link
Member Author

knu commented May 29, 2017

Great! I'll merge this soon.

@knu knu merged commit d7339a7 into master May 29, 2017
@knu knu deleted the support_ruby2.4 branch May 29, 2017 17:03
@dsander
Copy link
Collaborator

dsander commented May 30, 2017

Nice! 👏

@cantino
Copy link
Member

cantino commented May 31, 2017

Awesome!

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.

4 participants