Allow sprockets4 in gemspec, but installer injects sprockets3 requirement to local app

[jrochkind]

While restricting to sprockets 3 in Gemfile, since our CI does not currently work with sprockets 4.

One attempt at addressing #310

OK, so this is passing. Interesting. I'm going to try to write up the situation again, as currently. It's pretty confusing, in part because the sprockets 4 rollout in Rails ended up so confusing, being spread over a couple years. This is all pretty confusing.

• browse_everything up to 1.0.1 did not express a sprockets dependency
• browse_everything in 1.0.2 started requiring sprockets 3.x, not allowing 4.x
• old versions of Rails install with sprockets 3 if you just run run rails something, you have to change some things in your Gemfile to use sprockets 4. You can do so however.
• newer versions of Rails (I think starting 6.0.1 or 6.0.2 -- not actually 6.0.0!) will install using sprockets if you just run rails new something.
• sprockets 4 in rails by default will not compile Javascript only CSS. But you can modify your config to have sprockets compiling JS even with sprockets 4
• I have an app using rails 6 and sprockets 4 and browse_everything that is working fine. browse_everything doesn't actually require sprockets 3, sprockets 4 is not actually incompatible with it in any way -- once you've adjusted your config to have sprockets compiling JS, to get the browse-everything JS in there.
• I can not upgrade that app to browse_everything 1.0.2, because 1.0.2 does not allow sprockets 4, it can only be used in an app that is sprockets 3. This is a problem
• browse_everything's installer won't work with sprockets 4.
  • this is because the files it expects to be modifying aren't there, and sprockets 4 doesn't by default compile JS at all, which is the only way we have to supply sprockets JS to the app. (Rails seems to expect you to switch to delivering all your JS from engines/gems as npm packages, but this is a lot more work to get right).
  • you can get b-e to work with sprockets 4 by configuring things properly and configuring sprockets 4 to compile JS, and adding a javascript_include_tag to the sprockets JS package in your layout... but the b-e installer doesn't do any of these things.
  • which also means as it is, CI (circle-ci presently) would fail if b-e allowed sprockets 4
  • which is why b-e 1.0.2 stopped doing so.
• by sprockets 1.0.2 requiring sprockets 3, it means if you create a rails app with rails new someapp, it will start out using sprockets 4... then when you run the b-e installer, it will downgrade to sprockets 3.
  • you might never notice if you do this in a fresh app right away, say as part of a hyrax setup
  • but if you had an existing app already using sprockets 4, downgrading to sprockets 4 might break things. this is also not okay.
  • or depending on what other dependencies you have in the future, trying to install b-e might just fail (if you have something in your dependency tree that insists on sprockets 4).

So what a mess!

Ideal solution?

Ideally, browse_everything should work with sprockets 4, including it's automatic installer working with sprockets 4. Making the installer do that is kind of complicated. Especially if it's going to be trying to reconfigure sprockets 4 to compile Javascript, create necessary config files, add a sprockets-powered javascript_include_tag, etc. Complicated, and opening up the attack surface for bugs a lot.

It perhaps also needs to work with sprockets 3, for apps that still have sprockets 3. Which is even more complicated.

Making things more complicated at least for me is that I find dealing with engine_cart very challenging, have never really wrapped my head around that. And I don't understand circle_ci either, haven't used it much and find it difficult. And I personally can't get b-e tests to run locally either.

@jrgriffiniii made an attempt in #311 , but it's been languishing for nearly 6 months, becuase, well, it's hard. I don't feel capable of solving the problem either at the moment, it's super messy.

Can we do a less ideal for-now solution that at least doesn't forbid an app using b-e from using sprockets 4?

Really stupid solution: don't require a sprockets version in gemspec, do require it in Gemfile

The first version of this PR removes the sprocketes requirement from the gemspec, returning the stuation to what it was in browse_everything 1.0.1.

Now the installer won't work on any app using sprockets 4. And since latest rails new apps use sprockets 4, CI also won't work.

Then we add the sprockets 3.x dependency to our local Gemfile. The Gemfile is used for CI -- so CI passes again. But the Gemfile has no effect on an actual app using the b-e gem at all.

So the b-e installer still doesn't work on any app that is using sprockets 4, the b-e installation instructions won't work for a lot of people.

This is probably not good enough.

Better, generate a sprockets 3.x requirement into app?

OK what if we enhance this. Take the sprockets requirement out of the gemspec.

But make the sprockets installer add a sprockets, "~> 3.7" requirement to the local app's Gemfile.

This will still have some of the downsides of the current b-e 1.0.2 situation -- if your app started out using sprockets 4, installing b-e will insist on downgrading to sprockets 3 (and fail if it can't because of a conflict). This is not ideal. But it's what is already happening.

The difference is because the restriction is generated into the Gemfile, the local developers have the option to edit their own apps files (Gemfiles, sprockets configuration, etc), to upgrade to sprockets 4.

So may app could keep using sprockets 4 and upgrade to b-e 1.0.2 and subsequent.

This is really still not ideal, it's a big mess... but seems to be preferable to any other option, unless we can finish #311 maybe, which I personally do not feel confident I can, and it's been sitting there for six months.

I am going to look into trying to do this.

[jrochkind]

Huh, when I tried to do the final idea, injecting a sprockets 3.x dependency into Gemfile... I don't understand why it passes on rails 5.1 and 5.2 in ruby 2.4, but fails on rails 5.1 and 5.2 in ruby 2.5 and 2.6.

I can't figure out why the ruby version would make a difference here, and it's also failing in a really weird way involving complaining about duplicate entries of a whole bunch of gems in Gemfile, and then in inability to satisfy dependencies. But why would ruby version make a difference there?

My own inability to figure out how to run test suite locally is really a challenge in debugging this, I feel like i'm flying blind. Is anyone able to run this test suite locally?

[jrochkind]

OK!

This is now an attempt at the final proposal above -- having the browse_everything install generator inject a sprockets 3.x requirement in the local app Gemfile, instead of having in the browse_everything gemspec as a requirement that can't be changed.

Does this make sense? I am not sure. Better would be to make things work with sprockets 4, but that seems quite challenging. This is a way to

a) get CI to pass
b) get b-e installation process to keep working as documented with no manual intervention (at the cost of forcing use/downgrade to sprockets 3 -- but that's what b-e before this PR was doing too, just without 'c' below)
c) Let users who are able to do the manual reconfiguration work to use sprockets 4 do so, browse_everything gemspec no longer prevents it, as it started doing only in 1.0.2.

I finally got tests to pass locally on my workstation, and I can get tests to pass LOCALLY at 25461ea -- it is baffling to me why they do not pass in circle-ci at that commit. One guess is that it has something to do with the caching the samvera circle-ci "pod" is doing, but that may have been a false lead, I was never able to get anywhere with it.

e6c841 is only necessary for CI, to make CI pass. It has no effect on an actual application using browse-everything as a gem.

Not totally trusting the testing, I did manually set up a new Rails 5.2 app, and start following the b-e installation instructions. It did seem to work fine (including successfully downgrading to sprockets 3). Although I didn't get all the way finished with a working b-e demo, I was having trouble figuring out how to do that even on master; I did get a running app with CSS and JS delivered, by following instructions.

So I dunno... does this make sense, as a stopgap until #311, which seems like it could be a while?

[jrgriffiniii]

This is really excellent work @jrochkind , thank you very much for this, as well as the other pull requests. I apologize for unavailable to address and review these problems. Much later this afternoon I should have the opportunity to review this more carefully and provide my opinion in comments.

[jrochkind]

No problem, we're all having weird schedules these days, @jrgriffiniii . Thanks for updating me as to your expected schedule to have a chance to look at this.

I'm not totally sure it's a good idea.... I just can't figure out any other solution to allow apps to use sprockets 4 (which works with b-e, if you set it up; I am doing it), in the absence of being able to get #311 done, and getting #311 done seems very challenging and is beyond me.

[jrochkind]

actually i'm not sure if we should remove this dependency entirely. It was not there at all until 5685a4a. Thoughts?

[jrochkind]

Not sure if these should be in Gemfile instead of Gemfile.extra. If Gemfile.extra is present, it seems an appropriate place for these... but I also don't understand why it's present, and am worried it will mess up the circleci samvera orb which has rules for cache keys based on checksums of the Gemfile but not necessarily Gemfile.extra.

I would be inclined to get rid of this Gemfile.extra entirely and just put all of these in the Gemfile. But I may not have the context of why Gemfile.extra was being used (and apparently recommended by engine_cart?) in the first place... which also means I may not be making the right decision about when to put things there.

[jrochkind]

Oops, i see "much later this afternoon" was Friday last week! What are your current thoughts @jrgriffiniii ? I guess I should just not wait for it, ok!

It would be awesome if we could get maybe someone else to advise on it; this is really weird and complicated stuff; the parts involving sprockets I actually understand pretty well, but the parts involving engine_cart and circleci I do not.

[jrochkind]

@jrgriffiniii hi, checking in some weeks (and a pandemic) later.

I am considering forking browse-everything and using a forked version. Right now my app is locked using thor 0.20.3 when 1.0.1 is out; and faraday 0.17.3 when 1.0.1 is out; and I have no way to upgrade to a browse-everything that doesn't have those restrictions without downgrading my sprockets, which I don't want to do. I need a solution of some kind.

I'm also not sure this is the right solution. It was just my attempt to find some solution when #310 is stalled out, and too much for me.

[jrgriffiniii]

I'm going to try and get #317 merged and the build passing for Rails 6.y.z releases before I address rebasing and merging this.

[jrochkind]

Yeah, I just noticed that the gemspec prevents rails 6.0 too.

I am using browse_everything 1.0.1 with Rails 6 with no problem. It works fine. And the gemspec in 1.0.1 allows Rails 6.

It is likely that 1.0.2 and master work fine with Rails 6 too; but the restriction was added because the test setup did not work with Rails 6, I guess?

I may end up just having to make a personal fork of browse-everything that allows Rails 6 and sprockets 4. The code already works fine with them, my personal fork would do nothing but relax the gemspec. I could return to using an actual official release if browse-everything is ever updated to allow Rails 6 and Sprockets 4 again -- although if it also requires me to switch Javascript to use webpacker instead of sprockets, that would be an added challenge requiring more work for me to update. Again, I am using browse-everything 1.0.1 with Rails 6, Sprockets 4, and sprockets not webpacker to include b-e js, and it is in fact working just fine.

The only reason I want to update browse-everything is because 1.0.1 is forcing me to stay on very old verisons of thor, faraday, etc. But I can't update beyond 1.0.1 because then it won't let me use Rails 6 and Sprockets 4. Even though the code works fine for me, the gemspec won't allow it.

I've been fighting with this for a while, unless you think you will have a solution soon that allows me to use Rails 6 and Sprockets 4 without other backwards breaking changes, I'll probably just go ahead and make a personal fork. If you plan to have other backwards breaking chagnes (like requiring me to switch from sprockets to webpacker), I'll probably go ahead and make the personal fork for sure.

[hackartisan]

superceded by #358