Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

Travis: Update Ruby versions in CI matrix #243

Merged
merged 5 commits into from
Jun 8, 2017

Conversation

olleolleolle
Copy link
Contributor

@olleolleolle olleolleolle commented May 17, 2017

This PR updates Ruby versions to latest releases in their series.

  • jruby-1.7.27
  • jruby-9.1.10.0
  • 2.1.10
  • 2.2.7
  • 2.3.4
  • 2.4.1 update! - added in this PR
  • wrap rake ci in bundle exec - update and then making it "the default Travis task" (removing its specific configuration from the Travis YAML config)
  • evict a Rake task file for rspec tests to the automatically available rakelib folder
  • Rid the Gemfile of some duplication, making it shorter and easier to follow

@olleolleolle
Copy link
Contributor Author

olleolleolle commented May 17, 2017

Proposal: Perhaps we could let go of 2.1?

@olleolleolle
Copy link
Contributor Author

olleolleolle commented May 17, 2017

The current JRuby 1.7 build failure is about http_form's missing required_ruby_version in its .gemspec which does not preclude 1.9 Rubies from automatically using it.

It'd be nice if that gem had such metadata in its .gemspec.

@olleolleolle
Copy link
Contributor Author

JRuby 1.7 is EOL'd, would it be more beneficial in the long run to hop over to JRuby 9.1.9.0?

@ixti
Copy link
Contributor

ixti commented May 17, 2017

@olleolleolle I'm sorry about that. Will fix form_data asap

ixti added a commit to httprb/form_data that referenced this pull request May 18, 2017
This commit contains no real changes except it reverts keyword args
usage to use optional options hash instead for ruby < 2 compatibility.

See: #16
     celluloid/reel#243
ixti added a commit to httprb/form_data that referenced this pull request May 18, 2017
This commit contains no real changes except it reverts keyword args
usage to use optional options hash instead for ruby < 2 compatibility.

Resoves #16

------------------------------------------------------------------------

- #16
- celluloid/reel#243
@ixti
Copy link
Contributor

ixti commented May 18, 2017

@olleolleolle Can you please re-run tests. v1.0.3 of form data now fixed to play nice with jruby 1.7 and ruby 1.9+

@olleolleolle
Copy link
Contributor Author

@ixti That fixed things here! Yes!

♥️ 💛 💚 💙 💜

@olleolleolle
Copy link
Contributor Author

@digitalextremist Is there anything more you'd like from this PR? Or, any change you'd like to see done to it?

.travis.yml Outdated
@@ -1,23 +1,19 @@
script: rake ci
script: bundle exec rake
Copy link
Member

Choose a reason for hiding this comment

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

i think this is default, we could remove script: ... altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default has more options. All of them good for us:

bundle install --jobs=3 --retry=3

https://docs.travis-ci.com/user/languages/ruby/#Travis-CI-uses-Bundler

I'll make this change!

Gemfile Outdated
@@ -1,5 +1,8 @@
source 'https://rubygems.org'

ruby RUBY_VERSION

gem 'bundler'
Copy link
Member

Choose a reason for hiding this comment

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

in the context of a gem intended to run on multiple ruby versions, does it make sense to specify in Gemfile? is bundler needed to be specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can experiment with leaving it out - perhaps it is needed by the Rakefile, which gets read in our build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just so we talk about the same things, some basics: The Gemfile for a Rubygem is only a development and testing configuration tool. It won't affect users of the Rubygem.

The ruby RUBY_VERSION trick tells Bundler to "narrow your installation requirements to gems that support our current Ruby". Without this annotation, Bundler only has the version requirements like gem 'example', '~> 2.2', '>= 2.2.2' to narrow its gem version resolution with.

This allows a jruby-1.7.27 (Ruby 1.9) to navigate dependency chains which have Ruby 1.9/2.x differences (like json or mime-types libraries) in their "gem upgrade path", picking 1.9-compatible gems along the way.


Vacillating: Since a build takes a long time, I'd be happy to make the experimentation with dropping gem 'bundler' outside the context of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kenichi Update: Have now tested without duplication of what was in the gemspec in the Gemfile, and it worked fine!

Copy link
Member

@kenichi kenichi Jun 9, 2017

Choose a reason for hiding this comment

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

sorry @olleolleolle, GH hid these comments from me behind an "outdated diff", and I just thought to check. I do understand about the Gemfile vs. .gemspec for a gem. I'm reading & trying to learn more about this issue. I work on a project where this could be very helpful in our tests...

... but I'm really confused about this! https://gist.github.com/kenichi/bf9d83fc0a179850a45d2774281478ec

.travis.yml Outdated
- rvm: 2.1.8
- rvm: 2.3.0
- rvm: 2.3.4
Copy link
Member

Choose a reason for hiding this comment

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

should we add 2.4 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm for such an addition, I'll add it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@kenichi
Copy link
Member

kenichi commented Jun 8, 2017

thank you - i think this is great as a specific PR, and i'm curious as to your thoughts about my other comments and what i've done to CI and deps in my own PR. i would be happy to revert/rebase from this!

.travis.yml
Gemfile
reel.gemspec

@olleolleolle
Copy link
Contributor Author

@tarcieri I'm ready for code review on this one, now.

@tarcieri tarcieri merged commit a180b37 into celluloid:master Jun 8, 2017
@olleolleolle olleolleolle deleted the patch-1 branch June 8, 2017 15:13
@olleolleolle
Copy link
Contributor Author

@kenichi Oh, there we are, merged and good to go. Thank you for all of the input, it made this a lot better. Better is better!

gem 'celluloid'
gem 'celluloid-io'
gem 'http'
ruby RUBY_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

@olleolleolle @tarcieri sorry to harp on this, but this Gemfile directive is intended to specify a version of ruby the application requires. I don't think it's valid to use it here like this. thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I've been explicitly told by the Bundler developers to use ruby RUBY_VERSION to allow bundler to calculate variadic dependencies based on the current Ruby version, and that this behavior will become the default in future versions of Bundler.

Copy link
Member

@kenichi kenichi Jun 9, 2017

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants