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

upstream remaining truffleruby patches #2491

Closed
flavorjones opened this issue Apr 2, 2022 · 6 comments
Closed

upstream remaining truffleruby patches #2491

flavorjones opened this issue Apr 2, 2022 · 6 comments

Comments

@flavorjones
Copy link
Member

flavorjones commented Apr 2, 2022

See #1882 and #2095 for previous threads.

Conversation also happening in slack at https://graalvm.slack.com/archives/CMY63522F/p1648917137877149


Nokogiri patches live in https://github.com/oracle/truffleruby/blob/master/lib/truffle/truffle/patches/nokogiri_patches.rb

This approach is brittle in the face of trivial changes in Nokogiri's C extension. For example, this patch has been broken since this commit renamed thing to retval in Nokogiri v1.13.0. This is coming up again in #2489 where the code using variadic args is being modified and breaking more patches

One option is to upstream all of those changes, which I think would be preferable because it's easier to see and reason about.

@flavorjones
Copy link
Member Author

Note to self re: truffle dev env: comment out rbenv init in .bashrc and launch a new shell

@aardvark179
Copy link

I think I've now got an approach that works for completely disabling the patches when not building with system libraries, and allowing them to still be used by older version (oracle/truffleruby#2639 (comment)).

@flavorjones
Copy link
Member Author

@aardvark179 That's great! Can you tell me which of these patches is still needed so I can upstream them? (I imagine the varargs workarounds are still needed?)

@aardvark179
Copy link

The patch changes have now been merged to master. Current versions of Nokogiri no longer require any patches unless being build against system libraries (where we can support va_list and similar). I think the only change that really should be made to nokogiri is to disallow building against system libraries when using TruffleRuby.

@flavorjones
Copy link
Member Author

OK, closing this for now. I'll take a look at the truffle CI job which is failing in interesting ways.

@eregon
Copy link
Contributor

eregon commented Oct 12, 2022

PR to upstream all remaining patches: #2663

It's still a bit unclear to me if we should support using system libraries for nokogiri on truffleruby.
I guess we have these options:

  1. try to support it as much possible to respect the user's intent (what we do so far)
  2. raise an error if trying to compile nokogiri on truffleruby with system libraries, but that's possibly inconvenient for users using those e.g. on CRuby (although it no longer saves CI time to do so (except ruby-head I guess), so maybe few reasons to use system libs nowadays)
  3. ignore the --use-system-libraries flag in nokogiri when on TruffleRuby. Simple to do and convenient, but does not necessarily respect the user's intent.

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

No branches or pull requests

3 participants