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

[WIP] hold backpsych to version 3 in Gemfile.extra? #366

Closed
wants to merge 3 commits into from

Conversation

eddierubeiz
Copy link
Contributor

@eddierubeiz eddierubeiz commented Jun 23, 2022

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.

@eddierubeiz eddierubeiz self-assigned this Jun 23, 2022
@eddierubeiz eddierubeiz changed the title Changes to gemfile Changes to Gemfile and Gemfile.extra to deal with 3.1 incompatibilities Jun 23, 2022
@jrochkind
Copy link
Contributor

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
Copy link
Contributor Author

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

@jrochkind
Copy link
Contributor

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
Copy link
Contributor

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. :(

rails/rails@179d0a1

(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
Copy link
Contributor Author

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

@eddierubeiz eddierubeiz changed the title Changes to Gemfile and Gemfile.extra to deal with 3.1 incompatibilities Changes to Gemfile.extra to deal with 3.1 incompatibilities Jun 23, 2022
@eddierubeiz eddierubeiz changed the title Changes to Gemfile.extra to deal with 3.1 incompatibilities Change to Gemfile.extra to deal with 3.1 incompatibilities Jun 23, 2022
@eddierubeiz eddierubeiz changed the title Change to Gemfile.extra to deal with 3.1 incompatibilities WIP -- hold backpsych to version 3 in Gemfile.extra? Jun 24, 2022
@eddierubeiz eddierubeiz changed the title WIP -- hold backpsych to version 3 in Gemfile.extra? [WIP] hold backpsych to version 3 in Gemfile.extra? Jun 24, 2022
@eddierubeiz
Copy link
Contributor Author

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

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.

2 participants