search

samvera / questioning_authority

Question your authorities
Other
54 stars 30 forks source link

Two changes to qa.gemspec #364

Closed eddierubeiz closed 2 years ago

eddierubeiz commented 2 years ago

These two changes to qa.gemspec should ease the transition to Rails 3.1:

jrochkind commented 2 years ago

Changes to gemspec have pretty significant and potentially horrible effects on any app using this gem, we have to be careful with them, and I'm not sure about these.

net-smtp

I'm not sure if adding net-smtp to gemspec is the right solution. Does this gem actually use net-smtp at all?

The problem trying to be solved is that Rails 6.1 depends on the mail gem, which uses net-smtp but does not yet (in a final released version) declare teh dependency, which starting in ruby 3.1 needs to be declared. Current Rails 7.0.x solves that by explicitly declaring net-smtp (and other) bugs.

If we don't use net-smtp at all within this gem (do we?), it's not appropriate to add to gemspec, just as a generic fix tow hat's really Rails 6.1's problem with Rails 3.1 Yet we do want to test under Rails 6.1 and ruby 3.1 -- I think there could be another workaround.

If we add something to the Gemfile, it will be present for testing, but not in the gemspec such that it effects all downstream apps. We could add net-smtp to Gemfile, or the unreleased mail 2.8.0.rc1, which declares it's dependencies, which is what I did in browse-everything.

(If only mail would release 2.8.0 final this would stop being an issue, arghhh).

psych

If the gemspec says psych < 4, then any app using this gem can't use psych 4. If the app has another dependency that reuqires psych 4, then the app is out of luck and can't use both that other dependency and questioning_authority at all.

(It would be even worse if you did what your message actually said, and literallly pinned to 3.3.2, that would mean any app using this gem would have to use psych exactly and only 3.3.2, couldn't use anything older or newer, even when new things come out. That's not actually what you did, you said < 4, but still concerned as above).

I'm not sure this is the best/only solution to the problem. I don't entirely understand the bug or what versions of psych it exhibits in.

Has the bug not been fixed in a subsequent version of psych? Can we get around it by requiring a more recent psych, and/or the barely used "!" specifier in a gem requirement, if we identify what version of psych is subject to bug? (eg "! 4.0.1" if we know that's the only version with the bug).

Or, can we change our own app logic to work around bug?

eddierubeiz commented 2 years ago

Noted and converting to draft! Thank you VERY much.

jrochkind commented 2 years ago

Re psych, looking at the SO you helpfully link to... it's not a bug, it's a difference in behavior between psych 3 and 4, right??

https://stackoverflow.com/a/71192990/307106

That answer has a workaround if the call to YAML.safe_load is in our code... if the code is in some dependent gem, then that gem is not (yet?) compatible with ruby 3.1 -- if it's a samvera gem, maybe we can make it so? Would be interested to see the stack trace to see what is doing the YAML.safe_load call that is backwards incompat in ruby 3.1.

This could also maybe wait until a PR that does ruby 3.1 support total.

eddierubeiz commented 2 years ago

I'm closing this pull request and will reopen a new one with changes to Gemfile and Gemfile.extra shortly. Fairly new to gem architecture in general, so this was a great opportunity to see how not to solve this problem :) .