Fix cleanup! and prepare! Deprecations with Rails 5 #948

atomaka · 2016-10-21T20:14:59Z

Rails 5 deprecates cleanup! and prepare! causing a large amount of log spam from Delayed::Job:

DEPRECATION WARNING: cleanup! is deprecated and will be removed from Rails 5.1 (use Rails.application.reloader.reload! instead of cleanup + prepare) (called from require at bin/rails:9)
DEPRECATION WARNING: prepare! is deprecated and will be removed from Rails 5.1 (use Rails.application.reloader.prepare! instead) (called from require at bin/rails:9)

The first commit fixes existing issues with rubocop. Any easy corrections were made. Those that required refactorings were disabled.

coveralls · 2016-10-21T20:23:49Z

Coverage decreased (-0.3%) to 90.867% when pulling aeb7088 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

coveralls · 2016-11-09T04:02:15Z

Coverage decreased (-0.3%) to 90.867% when pulling a866207 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

thomasdarde · 2017-03-15T10:16:06Z

Hi , any reason for this PR not to be merged ?

simonknoll · 2017-03-15T10:29:24Z

@thomasdarde because the build failed.

thomasdarde · 2017-03-15T10:31:13Z

@atomaka Hi andrew, do you think you can update your PR for the build to pass ? Or tell me and I'll create another one

atomaka · 2017-03-15T13:11:20Z

I'll take a look this evening. The build is failing because of a dependency issue on JRuby builds and, unfortunately, I haven't used JRuby. I briefly look at this five months ago when I submitted the PR, but didn't see an immediate fix. If this will be considered for merge, though, I'll spend a bit more time to get it to pass the builds.

For those that are still struggling with this, you can monkey patch on load to save yourselves from the log headaches: #944 (comment)

thomasdarde · 2017-03-15T13:12:45Z

Thanks @atomaka , maybe just rebasing the PR will solve the issue, I hope it's not to much work.

albus522

Couple of nit picks, but then we can probably get this merged in.

albus522 · 2017-03-15T14:29:39Z

.gitignore

@@ -5,3 +5,4 @@
 .rvmrc
 /coverage
 Gemfile.lock
+.ruby-version


Nit pick, can we not include this please

This was just for my personal testing. This isn't prepared. Seeing what it takes to pass build.

albus522 · 2017-03-15T14:30:10Z

Gemfile

+  if rails_version == 'edge' || rails_version.match(/5\.\d+\.\d+/)
+    gem 'activerecord-jdbcsqlite3-adapter',
+        git: 'https://github.com/jruby/activerecord-jdbc-adapter.git',
+        branch: 'rails-5'


Maybe add a comment here about when this can be taken out.

Yup. See first comment. Still in process of testing. See comment I am about to type

albus522 · 2017-03-15T14:31:04Z

Sorry for the inactivity, things have been kind of crazy for me

coveralls · 2017-03-15T14:36:50Z

Coverage decreased (-0.3%) to 90.867% when pulling 44c7478 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

atomaka · 2017-03-15T14:37:30Z

The issue here is that the activerecord-jdbc-adapter no longer works with Rails 5. There's a long lived issue about it dating back to December 2015. It was closed for some reason, but the issue persists (as can bee seen in the comment history). There is a branch that has Rails 5 support (that I am testing builds against now), but I don't know the support level on that branch.

It might be wiser to drop Rails 5 support with Jruby until they resolve the issue upstream.

coveralls · 2017-03-15T14:59:22Z

Coverage decreased (-0.3%) to 90.867% when pulling 73a92a4 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

Using Delayed::Job with Rails 5 will currently spam logs with a deprecation message for cleanup! and prepare! in favor of ActiveSupport::Reloader. When ActiveSupport::Reloader is present, it will be used. Otherwise, it will fall back to cleanup! and prepare!.

See jruby/activerecord-jdbc-adapter#700 which is unresolved since December 2015, but closed. A branch exists and we can try using that to maintain support.

coveralls · 2017-03-15T16:16:51Z

Coverage decreased (-0.3%) to 90.867% when pulling d5af5c5 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

coveralls · 2017-03-15T16:36:06Z

Coverage decreased (-0.3%) to 90.875% when pulling f2a2850 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

coveralls · 2017-03-15T16:36:06Z

Coverage decreased (-0.3%) to 90.867% when pulling f2a2850 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

coveralls · 2017-03-15T16:36:06Z

Coverage decreased (-0.3%) to 90.867% when pulling f2a2850 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

coveralls · 2017-03-15T16:36:06Z

Coverage decreased (-0.3%) to 90.867% when pulling f2a2850 on atomaka:bugfix/rails5-reloader into e3772d4 on collectiveidea:master.

Some important notes for this: BlockLength cop was disabled completely because of a large amount of violations in the specs. DuplicatedGem cop was disabled completely because we are using conditionals to determine Gem versions and it was being triggered. I don't see any other options for this cop so it looks like the only option is to ignore. YAMLLoad wants to use safe_load over load. Not sure that's supported on older versions A few cops were disabled in specific code locations because I did not know enough about the code to update and be sure that there was no risk.

coveralls · 2017-03-15T16:50:08Z