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

Use JRuby implementation for TruffleRuby #149

Merged
merged 6 commits into from
Oct 10, 2024
Merged

Use JRuby implementation for TruffleRuby #149

merged 6 commits into from
Oct 10, 2024

Conversation

kou
Copy link
Member

@kou kou commented Oct 7, 2024

Fix GH-145

Rename lib/fiddle/jruby.rb to lib/fiddle/ffi_backend.rb as a generic ffi gem API based implementation.
JRuby and TruffleRuby use lib/fiddle/ffi_backend.rb.

Fix GH-145

lib/fiddle/truffleruby.rb uses lib/fiddle/jruby.rb.
lib/fiddle/jruby.rb uses FFI API. So we can use it for TruffleRuby
too.
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, I'd like to investigate a couple things.

lib/fiddle/jruby.rb Show resolved Hide resolved
@@ -6,6 +6,12 @@

module Fiddle
class TestClosure < Fiddle::TestCase
def setup
if RUBY_ENGINE == "truffleruby"
omit("FFI::Function don't accept #call-able object with TruffleRuby")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into this, TruffleRuby does pass most FFI gem tests so it's surprising, maybe this is not well tested or is not an officially supported API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 41c21f5 so all such tests pass now

@@ -158,6 +158,17 @@ def call(*args, &block);
end
Copy link
Member

@eregon eregon Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to rename this file to ffi_backend.rb or so and avoid a JRuby namespace since it is effectively independent from JRuby but it doesn't like that currently due to file naming and things like Fiddle::JRuby.
It's helpful to have a small diff at this point though, so I think that's best done later, maybe even in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we don't need to do it because this is a short-term workaround for TruffleRuby.
This file will be used by only JRuby eventually.

Copy link
Member

@eregon eregon Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the FFI backend seems to work well I think we should just use it long term, for both JRuby and TruffleRuby.
It's also nice to have 2 backends instead of 3.

* The issue is Fiddle structs have #to_ptr returning a CStructEntity and
  not a FFI::Pointer.
  So both Fiddle and FFI use #to_ptr and we need explicit coercion.
* TruffleRuby requires a Proc or Method as the callable.
under_gc_stress do
qsort.call(buff, buff.size, 1, callback)
end
assert_equal("1349", buff, bug4929)

# Ensure the test didn't mutate String literals
assert_equal("93" + "41", untouched)
Copy link
Member

@eregon eregon Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the added + above this was failing, which meant Fiddle tests were permanently changing the meaning of all "9341" frozen string literals.

Interestingly this sanity check actually fails on JRuby, I'll file an issue there: jruby/jruby#8365

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JRuby issue has been fixed on master and will be in JRuby 9.4.9.0.

We won't be updating 9.4 to use the fiddle gem in any case, but the test could be un-omitted now.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready, I am happy with this solution.

lib/fiddle/truffleruby.rb Outdated Show resolved Hide resolved
@kou kou merged commit f6d313a into master Oct 10, 2024
94 checks passed
@kou kou deleted the truffleruby-ffi branch October 10, 2024 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

The fiddle gem is broken on truffleruby and jruby
4 participants