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

Resolve Ruby 3.2 "undefining the allocator of T_DATA class" warnings #299

Merged
merged 3 commits into from
Dec 19, 2022

Conversation

BoboFraggins
Copy link
Contributor

warning: undefining the allocator of T_DATA class Ox::Cache
warning: undefining the allocator of T_DATA class Ox::Sax::Value

See: https://bugs.ruby-lang.org/issues/18007

Order of dumped XML elements changed in a test, but I believe the change is not significant.

warning: undefining the allocator of T_DATA class Ox::Cache
warning: undefining the allocator of T_DATA class Ox::Sax::Value

See: https://bugs.ruby-lang.org/issues/18007

Order of dumped XML elements changed in a test, but I believe the
change is not significant.
@ohler55
Copy link
Owner

ohler55 commented Dec 16, 2022

Looks like some tests are failing. Would you mind taking a look?

@mtasaka
Copy link

mtasaka commented Dec 16, 2022

I guess this patch may make test suite pass on ruby3.2preview3 and onwards, but this makes test failure on ruby3.1 and downwards.

@ohler55
Copy link
Owner

ohler55 commented Dec 17, 2022

Maybe a change in the extconf.rb along with a #if guard in the code would take care of the Ruby API change.

exported, but not requiring them be in a particular order.
@BoboFraggins
Copy link
Contributor Author

I updated the test to require both elements be exported, but not to depend on the order of the elements.

@BoboFraggins
Copy link
Contributor Author

I'm happy to put an #if guard in, if you prefer that as a solution. Just let me know.

@ohler55
Copy link
Owner

ohler55 commented Dec 19, 2022

Ox does need to work with older Rubies as well so unless you can get it to work in some other way the #if is needed. Well there are other ways but that is the simplest I think.

@BoboFraggins
Copy link
Contributor Author

Yeah, sorry. Not enough coffee this morning. How does this look? I tested with 3.2-rc1, 3.1.3 and 2.7.6.

@ohler55
Copy link
Owner

ohler55 commented Dec 19, 2022

Kicked off the CI to see how it fares.

@ohler55
Copy link
Owner

ohler55 commented Dec 19, 2022

LGTM

@ohler55 ohler55 merged commit 1572779 into ohler55:develop Dec 19, 2022
@ohler55
Copy link
Owner

ohler55 commented Dec 19, 2022

Thanks. It's nice to have others helping out.

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.

3 participants