projectblacklight / blacklight

Blacklight provides a discovery interface for any Solr (http://lucene.apache.org/solr) index.
http://projectblacklight.org/
Other
757 stars 256 forks source link

Build and support on Rails 7.1 #3099

Closed jrochkind closed 8 months ago

jrochkind commented 8 months ago

gemspec already (mistakenly?) allows Rails 7.1, even though build does not pass on Rails 7.1, including for at least one non-test-harness reason.

This is passing for me locally on Rails 7.1.1, let's see how it does in CI.

There were actually only a couple fairly simple issues (although not always simple to fix!).

  1. The "ActiveModelShim" needed a couple more boilerplate methods in order to pass tests. I don't really understand what's going on here, especially the has_query_constraints? method. (and things like primary keys suggest to me that what we're testing isn't really an ActiveModel shim but an ActiveRecord one?). Not sure what this is all used for, but some simple changes make tests green.

  2. RSpec-rails does not currently have a stub_template method that works in Rails 7.1. There is an unreleased one in rspec-rails main that tries, but suffers from problems with stubs persisting to subsequent tests. I found an implementation that appears to work though, and does let our tests pass.

    • I patched that implementation in to rspec-rails, in our local spec_helper.rb. The patch expires with a subsequent rspec-rails release. Conditional implementation on Rails version, like rspec-rails was trying.
      • I think it could be a while until rspec-rails has a release, and is acceptable to use a local patch here, that only effects test harness and not production code, in order to get a blacklight release that supports rails 7.1.
    • The codebase has three other places where we copy-paste-modify rspec-rails stub_template implementation to work in non-view-specs. I modified these too, each still inline one-off, to use new implementation that works.
  3. Was getting a deprecation warning on serialize :query_params, Blacklight::SearchParamsYamlCoder telling me it should be serialize :query_params, coder: Blacklight::SearchParamsYamlCoder, so went ahead and fixed that with a conditional for Rails version too.

jrochkind commented 8 months ago

OK getting close -- having one failure in Rails 7.1 CI that does not reproduce locally for me.

Done generating test app
/opt/hostedtoolcache/Ruby/3.2.2/x64/bin/ruby -I/opt/hostedtoolcache/Ruby/3.2.2/x64/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib:/opt/hostedtoolcache/Ruby/3.2.2/x64/lib/ruby/gems/3.2.0/gems/rspec-support-3.12.1/lib /opt/hostedtoolcache/Ruby/3.2.2/x64/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/exe/rspec --pattern spec/\*\*/\*_spec.rb

An error occurred while loading spec_helper.
Failure/Error: EngineCart.load_application!

Zeitwerk::NameError:
  expected file /home/runner/work/blacklight/blacklight/.internal_test_app/lib/generators/test_app_generator.rb to define constant Generators::TestAppGenerator, but didn't
# ./.internal_test_app/config/environment.rb:5:in `<top (required)>'
# ./spec/spec_helper.rb:15:in `<top (required)>'
No examples found.

Actually I'll see if I can reproduce locally running rake all-in-one instead of creating engine cart separately and then running tests as I've done. Then I'll see if I can fix it.

Can repro locally if we export CI=true to turn on eager loading in tests. It is of course a zeitwerk issue. Working on it.

jrochkind commented 8 months ago

OK, so engine_cart (I guess!) puts a generator into the local test app (.internal_test_app), ./lib/generators. test_app_generator.rb, TestAppGenerator

I'm not sure why this is happening, why is it putting the generator for a test app into the generated test app?

But it confuses zeitwerk, because it's not called Generators::TestAppGenerator. The zeitwerk docs do talk about this lib/generators use case in fact, although when the lib is in a gem, it's not expecting you to put it into an app like we do.

For example, if the gem has Rails generators under lib/generators, by convention that directory defines a Generators Ruby module. If generators is just a container for non-autoloadable code and templates, not acting as a project namespace, you need to setup things accordingly.

If the warning is legit, just tell the loader to ignore the offending file or directory:

loader.ignore("#{__dir__}/generators")

What's weird here is that the generators dir is in our generated app, not in the gem.

I am trying to avoid having to get engine_cart to do something different, because that way lies tragedy.

So one fix is having the blacklight gem configure the whole local app to tell zeitwerk to ignore the local app's whole ./lib/generators folder.

While this could conceivably effect someone if they actually wanted to use ./lib/generators for something namespaced Generators? This seems unlikely? And maybe would conflict with Blacklight already?

If someone has a better solution, it's welcome!

update: repoted to engine_cart at https://github.com/cbeer/engine_cart/issues/117