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

Upgrade to Ruby 2.7.0 #5281

Merged
merged 5 commits into from
Apr 3, 2020
Merged

Upgrade to Ruby 2.7.0 #5281

merged 5 commits into from
Apr 3, 2020

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Dec 30, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

As tradition goes, the 25th of December Ruby had a new major release.

The release notes are here: Ruby 2.7.0 Released, it was available on Heroku the same day

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Dec 30, 2019
@rhymes rhymes changed the title Upgrade to Ruby 2.7.0 [WIP] Upgrade to Ruby 2.7.0 Dec 30, 2019
@pr-triage pr-triage bot removed the PR: unreviewed bot applied label for PR's with no review label Dec 30, 2019
@rhymes
Copy link
Contributor Author

rhymes commented Dec 30, 2019

Ruby 2.7 introduces a bunch of warnings on positional and keyword arguments (finally and yay copying this feature from Python!) in preparation for Ruby 3.

Unfortunately gems don't yet address all of these warnings and the Travis can't finish because it outputs too much data.

We could disable the warnings but I'd rather keep this PR open and updated until the various gems have been brought up to date and we can address our own code.

I'll keep tracking our master branch in the following weeks

@rhymes rhymes self-assigned this Dec 30, 2019
@maestromac
Copy link
Contributor

I'm going to merge all the dependabot update first before merging this. If that's alright.

@rhymes
Copy link
Contributor Author

rhymes commented Dec 30, 2019

@maestromac this is currently broken and a WIP, don't merge it :P

@rhymes
Copy link
Contributor Author

rhymes commented Jan 20, 2020

Even though the build passes, there's an infinite list of warning, due to how Ruby 2.7 is setting the ground for more explicit parameter passing, which I'd still prefer to be largely addressed before we move on with this.

Let's give the various gem maintainers time to update their respective libraries :)

@rhymes rhymes requested a review from a team as a code owner February 13, 2020 15:45
@rhymes rhymes requested review from a team and citizen428 and removed request for a team February 13, 2020 15:45
@@ -139,6 +139,7 @@ DEV requires Redis version 4.0 or higher.
We recommend to follow
[this guide](https://redislabs.com/blog/redis-on-windows-10/) to run Redis under
WSL.
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple merge conflict issues left 🙃

@rhymes
Copy link
Contributor Author

rhymes commented Feb 13, 2020

I have to stop force pushing to this as it's triggering the code owners files :D
Thanks for the catch, I'm going to let this branch sit here for a while more as it has lots of warnings

@rhymes rhymes added the PR: draft bot applied label for PR's that are a work in progress label Feb 20, 2020
@pr-triage pr-triage bot removed the PR: draft bot applied label for PR's that are a work in progress label Feb 25, 2020
@citizen428
Copy link
Contributor

@rhymes Do you think it's worth slowly picking this one up again?

@rhymes
Copy link
Contributor Author

rhymes commented Mar 10, 2020

@citizen428 I've rebased the branch, let's see how this latest build fares in terms of warnings, the previous one had a lot, though the tests pass...

@rhymes rhymes requested a review from a team as a code owner March 21, 2020 11:46
@mstruve
Copy link
Contributor

mstruve commented Mar 23, 2020

What is the goal here? Do we want to upgrade first then take care of the warnings or take care of them all in this PR?

@citizen428
Copy link
Contributor

citizen428 commented Mar 24, 2020

I think @mstruve raises a good question here @rhymes. Maybe we can leave warninga enabled locally so we remember to eventually fix them, but run with RUBYOPT="-W0" on Travis and Heroku to not clutter up the logs?

@rhymes
Copy link
Contributor Author

rhymes commented Mar 24, 2020

I actually was thinking about going ahead with this yesterday while I was reviewing again what's new with Ruby 2.7 for unrelated reasons.

+1 on adding the warning masking in production and Travis. What's the best strategy? Should I add it to the global section of Travis and ask @mstruve to add the same in production?

@rhymes
Copy link
Contributor Author

rhymes commented Mar 24, 2020

@citizen428 what about setting it to 1? I have often found warnings that lead to fixes when randomly checking builds outputs but maybe 1 is still too much :D

Anyway, let's circle back on this when our deployment is back in order

@rhymes
Copy link
Contributor Author

rhymes commented Mar 24, 2020

I did some tests.

Using:

RUBYOPT="-W:no-deprecated -W:no-experimental"

doesn't work (I picked that up from https://prathamesh.tech/2019/12/26/managing-warnings-emitted-by-ruby-2-7/) as Rails probably bypasses that.

-W1 still emits too many warnings. -W0 works.

I think we should enable it also in development, I just tried running a single spec file and got hundreds if not thousands of warning. For example each time URI.escape is called in the validates_url gem, which is a lot. They apparently fixed it in master but haven't release an update yet.

My proposal is to silence the warnings everywhere, even though I'm a bit uncomfortable with the concept as it might hide something actually useful, but there doesn't seem to be a way to separate Ruby 2.7 warnings from other warnings.

@mstruve
Copy link
Contributor

mstruve commented Mar 24, 2020

My proposal is to silence the warnings everywhere, even though I'm a bit uncomfortable with the concept as it might hide something actually useful,

I am game for this, on the plus side, they are just warnings so they shouldn't actually cause problems as is. But it is definitely something we will want to make sure someone is staying on top of.

Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

🚂

@rhymes
Copy link
Contributor Author

rhymes commented Mar 24, 2020

I went on investigating, RUBYOPT=-W0 works if passed in the command line:

 [17:20:32]   devto git:(rhymes/ruby-270)  RUBYOPT="-W0" rails c
Warning: the running version of Bundler (2.1.2) is older than the version that created the lockfile (2.1.4). We suggest you to upgrade to the version that created the lockfile by running `gem install bundler:2.1.4`.
DEPRECATION WARNING: Default values will be removed in the next minor-release of ENVied (i.e. > v0.9). For more info see https://gitlab.com/envied/envied/tree/0-9-releases#defaults. (called from block in load at /Users/rhymes/Development/devto/Envfile:4)
** [Honeybadger] Initializing Honeybadger Error Tracker for Ruby. Ship it! version=4.6.0 framework=rails level=1 pid=10647
** [Honeybadger] Development mode is enabled. Data will not be reported until you deploy your app. level=2 pid=10647
   (1.7ms)  SELECT "data_update_scripts"."file_name", "data_update_scripts"."status" FROM "data_update_scripts" ORDER BY "data_update_scripts"."file_name" ASC
Loading development environment (Rails 5.2.4.2)
[1] pry(main)> ENV["RUBYOPT"]
=> "-r/Users/rhymes/.rvm/rubies/ruby-2.7.0/lib/ruby/2.7.0/bundler/setup -W0"

but it doesn't if put in the Envfile:

 [17:24:11]   devto git:(rhymes/ruby-270)  cat Envfile | rg RUBYOPT
13:variable :RUBYOPT, :String, default: "-W0"
 [17:24:20]   devto git:(rhymes/ruby-270)  rails c
Warning: the running version of Bundler (2.1.2) is older than the version that created the lockfile (2.1.4). We suggest you to upgrade to the version that created the lockfile by running `gem install bundler:2.1.4`.
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activerecord-5.2.4.2/lib/active_record/type.rb:27: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activerecord-5.2.4.2/lib/active_record/type/adapter_specific_registry.rb:9: warning: The called method `add_modifier' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:17: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:24: warning: The called method `env!' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied/configuration.rb:13: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied/configuration.rb:5: warning: The called method `initialize' is defined here
DEPRECATION WARNING: Default values will be removed in the next minor-release of ENVied (i.e. > v0.9). For more info see https://gitlab.com/envied/envied/tree/0-9-releases#defaults. (called from block in load at /Users/rhymes/Development/devto/Envfile:4)
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied/configuration.rb:34: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied/variable.rb:4: warning: The called method `initialize' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:18: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:29: warning: The called method `error_on_missing_variables!' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:19: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:38: warning: The called method `error_on_uncoercible_variables!' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:21: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/envied-0.9.3/lib/envied.rb:55: warning: The called method `ensure_spring_after_fork_require' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/actionpack-5.2.4.2/lib/action_dispatch/middleware/stack.rb:37: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/honeycomb-beeline-2.0.0/lib/honeycomb/integrations/rack.rb:24: warning: The called method `initialize' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/actionpack-5.2.4.2/lib/action_dispatch/middleware/stack.rb:37: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/actionpack-5.2.4.2/lib/action_dispatch/middleware/static.rb:111: warning: The called method `initialize' is defined here
** [Honeybadger] Initializing Honeybadger Error Tracker for Ruby. Ship it! version=4.6.0 framework=rails level=1 pid=10768
** [Honeybadger] Development mode is enabled. Data will not be reported until you deploy your app. level=2 pid=10768
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/rolify-5.2.0/lib/rolify.rb:30: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activerecord-5.2.4.2/lib/active_record/associations.rb:1821: warning: The called method `has_and_belongs_to_many' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activerecord-5.2.4.2/lib/active_record/associations.rb:1855: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activerecord-5.2.4.2/lib/active_record/associations.rb:1368: warning: The called method `has_many' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/algoliasearch-rails-1.24.0/lib/algoliasearch-rails.rb:392: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/algoliasearch-rails-1.24.0/lib/algoliasearch-rails.rb:275: warning: Capturing the given block using Proc.new is deprecated; use `&block` instead
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activemodel-5.2.4.2/lib/active_model/type/integer.rb:13: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activemodel-5.2.4.2/lib/active_model/type/value.rb:8: warning: The called method `initialize' is defined here
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activerecord-5.2.4.2/lib/active_record/connection_adapters/postgresql/oid/specialized_string.rb:12: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/rhymes/.rvm/gems/ruby-2.7.0/gems/activemodel-5.2.4.2/lib/active_model/type/value.rb:8: warning: The called method `initialize' is defined here
   (1.2ms)  SELECT "data_update_scripts"."file_name", "data_update_scripts"."status" FROM "data_update_scripts" ORDER BY "data_update_scripts"."file_name" ASC
Loading development environment (Rails 5.2.4.2)
[1] pry(main)>

If I only add it to the config/application.yml instead I get this:

 [17:25:19]   devto git:(rhymes/ruby-270)  rails c
Warning: the running version of Bundler (2.1.2) is older than the version that created the lockfile (2.1.4). We suggest you to upgrade to the version that created the lockfile by running `gem install bundler:2.1.4`.
WARNING: Skipping key "RUBYOPT". Already set in ENV.

and all the deprecations of the previous example.

So I was back to square one.

The only solution I found that covers both the tests and other commands is:

$VERBOSE = nil on the top of config/boot.rb inspired from this post Turning Off Ruby Deprecation Warnings When Running Tests.

I didn't know about that magic variable, but it seems to control the verbose mode in Ruby, see https://ruby.fandom.com/wiki/Special_variable

Don't know what the implications are though 🤷‍♂

@citizen428
Copy link
Contributor

but it doesn't if put in the Envfile

I guess this doesn't work because Ruby has already been loaded at this point, so you're modifying the environment of the current Ruby process, which won't pick up the -W0 after the fact.

Like previously suggested, for Travis and Heroku we have obvious ways to set the env variable, for local development we could update bin/startup to set up the environment accordingly. People who run other setups probably know enough about what they are doing to adapt that to their own flow as needed.

@rhymes
Copy link
Contributor Author

rhymes commented Mar 25, 2020

I guess this doesn't work because Ruby has already been loaded at this point, so you're modifying the environment of the current Ruby process, which won't pick up the -W0 after the fact.

Yeah :)

Like previously suggested, for Travis and Heroku we have obvious ways to set the env variable, for local development we could update bin/startup to set up the environment accordingly. People who run other setups probably know enough about what they are doing to adapt that to their own flow as needed.

The drawback with this though is that some of us and probably external contributors alike don't use bin/startup as it's hard to navigate breakpoints with it, so I know they separate the various commands into different tabs, thus setting -W0 or $VERBOSE = nil there won't affect them.

I sent a change for config/application.rb which will activate only in development mode. So Travis and production can use RUBYOPT=-W0 and both the regular developer experience and Docker's wil pick up the other silencing.

I tried with config/boot.rb but it's too early to know which environment you're in.

@citizen428
Copy link
Contributor

some of us and probably external contributors alike don't use bin/startup

That's true, I don't for example. In a lot of projects it's implicit that if you run them in a non-standard way, it's up to you to figure out how to make it work. That said, I don't think that's the sort of project we aspire to be, so 👍 on your proposed change 😃

@@ -13,6 +13,9 @@
require "sprockets/railtie"
# require "rails/test_unit/railtie"

# Silence all Ruby 2.7 deprecation warnings in development mode
$VERBOSE = nil if Rails.env.development?
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this line work when you put it in development.rb before the Rails.application.configure block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

I added it to development.rb and test.rb (disabling it for CI by default) too

Copy link
Contributor

Choose a reason for hiding this comment

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

Neat, I like having it in the specific environemnts as compared to application.rb. Thanks!

@rhymes rhymes changed the title [WIP] Upgrade to Ruby 2.7.0 Upgrade to Ruby 2.7.0 Apr 3, 2020
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Apr 3, 2020
@rhymes rhymes added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Apr 3, 2020
@rhymes rhymes merged commit 6bede2a into forem:master Apr 3, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Apr 3, 2020
@rhymes rhymes deleted the rhymes/ruby-270 branch April 3, 2020 15:36
@rhymes rhymes mentioned this pull request Apr 9, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants