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

Remove darwin linker hack #2963

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maths22
Copy link

@maths22 maths22 commented Aug 21, 2023

What problem is this PR intended to solve?

This PR allows us to continue to support both newer rubies on macOS and
support binary extensions linked against nokogiri.

See comment in body for details on why and how it can be replaced.

The python community has been running into the same issues because
cpython extensions operate very similarly to ruby. They got an
explanation from Apple on some of how the changes work which can be
found here and gives some additional useful context for understanding
this change:
python/cpython#97524 (comment)

(I do wonder if there should be some upstream conversation with ruby
and rake-compiler-dock to move in this direction?)

Have you included adequate test coverage?

There should be no functional changes here, so I don't think we can do much
better than the existing test suite continuing to pass.B

Does this change affect the behavior of either the C or the Java implementations?

This change should have no behavioral effect.

See comment in body for details on why and how it can be replaced.

The python community has been running into the same issues because
cpython extensions operate very similarly to ruby.  They got an
explanation from Apple on some of how the changes work which can be
found here and gives some additional useful context for understanding
this change:
python/cpython#97524 (comment)
@maths22
Copy link
Author

maths22 commented Aug 21, 2023

@flavorjones I'd be interested in your thoughts on if this approach is actually the direction that you want to go. It obviously is a reversal of the proposal in #2746 , but it does allow nokogiri-xmlsec (and anything else that someone might choose to build in the future) to continue to exist on its own and prevents needing to deal with a lot of the grossness implied in #2896

@stevecheckoway
Copy link
Contributor

So if I understand correctly, nokogiri would be compiled with a two-level namespace and using chained fixups except for symbols from ruby/libruby which would be using the standard dynamic symbol resolution. That seems like a win all around.

I'm not thrilled about using nm to get a list of symbols. But I don't have a better suggestion absent something like apple-opensource/ld64#1. I don't think we use a lot of functions from ruby/libruby so we could just maintain a list by hand but that's not a great experience for anyone trying to modify Nokogiri not on macOS.

Does Nokogiri-xmlsec just need symbols from Nokogiri or does it also need symbols from libxml/libxslt? If it's the latter, I would think that'd run into the same issues Nokogiri had with the system libxml being used. And two-level namespaces wouldn't work since Nokogiri can use the system libraries so other extensions wouldn't know if they needed to link against Nokogiri or libxml.

@maths22
Copy link
Author

maths22 commented Aug 22, 2023

That understanding is correct.

I don't love the nm method either, but without something like that linker method there's not another option I can really come up with. Nokogiri currently depends on 104 upstream symbols, so we theoretically could maintain a list, but I definitely like that less. Upstream ruby supports generating a list of those functions (with nm) in its makefile at ruby build time, which would really be ideal, but that's only automatically built and available on AIX, not darwin best I can tell.

nokogiri-xmlsec does use libxml symbols, but it doesn't support precompiled binary extensions, so in practice it doesn't need to worry all the two-level precompiled namepace complications. I agree though that there isn't an elegant solution there short of having 2 precompiled gems, one linked against system libxml and one linked against nokogiri-integrated libxml.

@maths22
Copy link
Author

maths22 commented Aug 22, 2023

One alternative I just thought of would be to create a separate repository with symbol list files that are autogenerated with GitHub actions and then nokogiri could download them as part of its build process (and ship them with the gen for the shipped gem). That would mean though that the gem would be uninstallable on a new major version of ruby until it had a release explicitly supporting it, or we'd want to keep the nm logic as a fallback.

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.

None yet

2 participants