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

Require ActiveSupport.on_load in flipper-active_record #432

Merged
merged 2 commits into from
Sep 12, 2019

Conversation

bendb-instacart
Copy link
Contributor

Fixes #431

@bendb-instacart
Copy link
Contributor Author

Looks like one test run failed with what seems to be an entirely-unrelated ruby segfault on VM shutdown (the segfault happens in the function ruby_vm_destruct. Not sure what to do about that.

@jnunemaker jnunemaker merged commit 51d7c84 into flippercloud:master Sep 12, 2019
@jnunemaker
Copy link
Collaborator

@bendb-instacart thanks so much for this PR. I really appreciate it. If you don't mind, I would love to know what you are using flipper for as well and how it is working out for you (what went well, what was hard other than what you mentioned here). If you aren't comfortable leaving that information here in a public comment, feel free to email me directly john@flippercloud.io. I'm just always curious. :)

@bendb-instacart bendb-instacart deleted the require_activesupport branch September 12, 2019 16:50
@mike1o1
Copy link

mike1o1 commented Sep 13, 2019

I'm not sure if it's just me, but this change seems to have broken Rails for me using Rails 6 and the latest version of this gem with this PR included. I get an error now where bundler is trying to load the gem, and I get an error that it can't find activesupport/lazy_load_hooks.

Is there something I'm missing or need to do to get this to work?

DISABLE_SPRING=true rails tmp:clear --trace
rails aborted!
LoadError: cannot load such file -- activesupport/lazy_load_hooks
.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/activesupport-6.0.0/lib/active_support/dependencies.rb:325:in `require'
.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/activesupport-6.0.0/lib/active_support/dependencies.rb:325:in `block in require'
.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/activesupport-6.0.0/lib/active_support/dependencies.rb:291:in `load_dependency'
.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/activesupport-6.0.0/lib/active_support/dependencies.rb:325:in `require'
.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/flipper-active_record-0.17.0/lib/flipper-active_record.rb:1:in `<top (required)>'
.rbenv/versions/2.6.3/lib/ruby/2.6.0/bundler/runtime.rb:81:in `require'
...
etc

Thanks!

@bendb-instacart
Copy link
Contributor Author

Thanks for raising this! I think I made a dumb typo - the require line should be active_support/lazy_load_hooks, note the underscore in active_support. That's not in the PR.

I don't know how I missed that, nor how the tests missed it for the matter. I'll have a fix up momentarily - or, @jnunemaker can revert this for the moment if he feels that's better.

@jnunemaker
Copy link
Collaborator

jnunemaker commented Sep 13, 2019

Hmm. Not sure how the tests didn’t fail either. I can’t revert and I just did a release so I’ll have to do a new release and yank this one.

@bendb-instacart
Copy link
Contributor Author

PR opened. Very sorry for the regression!

@jnunemaker
Copy link
Collaborator

I know why. The gem requires 'flipper/adapters/active_record' and I often do personally as well. The file that does the incorrect thing is flipper-active_record, which doesn't get required anywhere. I'll get a new release out and get this all cleaned up.

@jnunemaker
Copy link
Collaborator

0.17.1 is out with this fix.

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.

flipper-activerecord breaks Sorbet
3 participants