Upstream truffleruby patches needed when using system libraries

[eregon]

• Adapt tests for that configuration.

What problem is this PR intended to solve?

#2491

Have you included adequate test coverage?

Yes.

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

Only C, no need to change Java code.

[eregon]

This should fix this currently-failing CI job: https://github.com/sparklemotion/nokogiri/actions/runs/3217744062/jobs/5261075281

[flavorjones]

Thanks for shipping this! I'll take a look shortly.

[flavorjones]

Normally we don't run the suite against TR for every commit (because it takes so long we opt for a cron job), so I've just pushed a commit that tries to make sure CI will run TR when branch names have "truffle" in them.

[eregon]

Normally we don't run the suite against TR for every commit (because it takes so long we opt for a cron job), so I've just pushed a commit that tries to make sure CI will run TR when branch names have "truffle" in them.

Right. Since there is workflow_dispatch I should be able to schedule it on my fork manually.

BTW for test speed, the main cost is the default NOKOGIRI_TEST_GC_LEVEL=major. Without that tests execute in just 1 minute for me locally. So maybe we should set NOKOGIRI_TEST_GC_LEVEL=normal for TruffleRuby, or GC less often and then we could run it in the gate/more often?

[eregon]

It passed in https://github.com/eregon/nokogiri/actions/runs/3237310347/jobs/5304216696

[eregon]

Given these test times over 1 hour yeah I think we should definitely force GC less often 😅
We can do that separately though, I can try it on a branch tomorrow with NOKOGIRI_TEST_GC_LEVEL=normal for comparison.

[eregon]

I've asked the TruffleNFI/Sulong team about that (@rschatz), and it sounds like a limitation of libffi (can't read varargs from a libffi callback), so probably nothing we can do about it in the short term.

[eregon]

Links I've found about that from a quick search:

• Feature Request: support variadic callbacks libffi/libffi#388
• Support variadic function for NativeCallback  frida/frida-gum#364 (comment)

[eregon]

We can do that separately though, I can try it on a branch tomorrow with NOKOGIRI_TEST_GC_LEVEL=normal for comparison.

It runs in ~3min (with --enable-system-libraries) and ~8-9min (without): https://github.com/eregon/nokogiri/actions/runs/3241791089/jobs/5314201500
I'll push that on this PR too, but feel free to remove it if you want to keep it for these jobs.

I'm unsure how useful it is to GC between tests on TruffleRuby. I would say not worth it given the huge overhead.
Marking actually happens when leaving a C ext method in TruffleRuby, not during the JVM GC, and to detect a problem we need to execute code using e.g. a freed handle. The GC between tests makes it rather unlikely to reuse such a handle, so for TruffleRuby I'd think GCs happening during the tests (which already occur naturally) are more useful to detect this kind of bugs.

[flavorjones]

Yes, the GC settings are there primarily to exercise CRuby and I wasn't even aware that TruffleRuby supported those settings. Please do feel free to add a commit that restores normal GC behavior for TR.

I have a terribly busy day today, but I will circle back to this by the weekend. I'm glad we're green and look forward to staying green!

[flavorjones]

Thank you, @eregon!

[eregon]

Thank you for merging!

I plan to remove the patches in TruffleRuby after the next nokogiri release, because they shouldn't be needed anymore, they are currently only needed when using system libraries, and with this PR won't be necessary for future nokogiri versions (when using system libraries).
When using packaged libraries (the default) there is already no patching.

[flavorjones]

OK. If it's helpful, for managing upstream version checks, these changes will show up in Nokogiri 1.14.0.