samvera / questioning_authority

Question your authorities
Other
54 stars 30 forks source link

[WIP] hold back`psych` to version 3 in `Gemfile.extra`? #366

Closed eddierubeiz closed 2 years ago

eddierubeiz commented 2 years ago

Second attempt to deal with breaking changes in psych by holding it back to '< 4' in Gemfile.extra.

Note: the initial PR also had a mail related change that has been moved into separate ref #365 so it's not in this PR anymore.

Still figuring this out; thanks for bearing with me.

jrochkind commented 2 years ago

Re mail -- I think we need the changes only in Gemfile.extra and not in Gemfile. And I think you can probably just put that in PE #365 if you want.

Re psych -- I am still understanding this differently than you, after reading the great answer at that StackOverflow you linked to. I don't think psych is going to "catch up" -- there are breaking changes in psych 4, they are going to remain, there is no catching up that will happen. And I don't think it's appropriate for this gem to force all apps using it to stay on psych 3 forever. I think we need to make this gem work with both psych 3 and psych 4. The SO answer has some suggestions for how to do that.

It can depend on where the problematic call to YAML is though, is it in this gem (in which case we can fix it), or a dependency (harder). The back trace on the exception should tell you that. I haven't seen it yet, but I think you have?

eddierubeiz commented 2 years ago

Yeah -- it's in ActiveSupport/ [...] / configuration_file.rb . So nothing we can change.

jrochkind commented 2 years ago

Yeah -- it's in ActiveSupport/ [...] / configuration_file.rb . So nothing we can change

Can I see the stack trace? I am very curious, because I believed Rails 6.1 was compatible with ruby 3.1 (but for the mail Gemfile issues), and passed it's own CI build for Rails 6.1, so I'm surprised to hear there's code inside ActiveSupport 6.1 that does not in fact work with the version of psych that comes with ruby 3.1

If this is really true though -- there is some part of Rails 6.1 that simply will not work with psych 4, and some apps don't use that part so get away with ruby 3.1 and Rails 6.1, but questioning_authority does use that part -- then I'd lean to saying we just can't actually support Rails 6.1/Ruby 3.1 combo, or test with it, and we just have to go on to Rails 7 and Ruby 3.1.

jrochkind commented 2 years ago

It does look like there's a Rails commit to "Fix compatibility with Psych 4" that is alas only included in Rails 7 versions, not any Rails 6 releases. :(

https://github.com/rails/rails/commit/179d0a1f474ada02e0030ac3bd062fc653765dbe

(Found taht URL from the SO you found).

So how the heck was I sure that Rails 6.1 passed it's own build for Ruby 3.1? GAHH. There is so lack of info on the web about this, it's very confusing.

So I'm not sure. Hypothetically a consuming app could use questioning_authority with Rails 6.1 and Ruby 3.1 if they themselves lock psych to 3, but that's ugly. And we definitely don't want to force everyone (on every ruby/rails version) to be stuck on psych 3 in the gemspec.

I guess we could do what you're doing here, and test with psych 3..... I don't know. There is no "once psych catches up" though, as I understand it -- that restriction will never be able to be removed. (But won't be needed in Rails 7.0, because rails has changed to support ruby 3.1).

eddierubeiz commented 2 years ago

I did put the mail conditional in ref #365 so we can just leave that there.

eddierubeiz commented 2 years ago

Moving these changes into #365 ( along with some helpful copy in README.md per some advice from @jrochkind ).