andrykonchin commented 2 months ago

Given https://bugs.ruby-lang.org/issues/20309 fiddle will become a bundled gem in Ruby 3.5+.

As a result people will add fiddle to their Gemfile where they previously wouldn't (and so there was no issue). Unfortunately this leads to failure for example on https://github.com/ffi/ffi/pull/1119:

https://github.com/ffi/ffi/actions/runs/10881406447/job/30190278321?pr=1119 JRuby fails when trying to build the C extension
TruffleRuby segfaults, probably because the fiddle C extension does something unexpected, and it's completely untested on TruffleRuby. It seems better to use the TruffleRuby Fiddle backend anyway for performance, as it avoids basically going twice through libffi (one used for the C extension API and the one used by Fiddle).

The fiddle gem (in this repo) currently only works on CRuby. So the best solution for now is to just use fiddle from stdlib on non-CRuby. That's properly tested and working.

Later it might be possible to upstream the Fiddle backends of JRuby / TruffleRuby in this repo. But that is a much larger undertaking and we need to fix the failure when people have gem "fiddle" in their Gemfile or through a gem's dependencies.

Close #145

eregon commented 2 months ago

I think it would make sense to add truffleruby in https://github.com/ruby/fiddle/blob/aad5a3bc797c7271bbc06258fe5cd99cd3f3d2bb/.github/workflows/ci.yml#L28 like - { os: ubuntu-latest , ruby: truffleruby } and then skip the two test steps. That way we'd still test that the gem builds and installs fine on truffleruby. @andrykonchin Could you try that?

eregon commented 2 months ago

Let's add - { os: ubuntu-latest , ruby: jruby } as well to make sure the gem installs fine on JRuby too.

eregon commented 2 months ago

Could you update the PR description to explain why this PR is needed?

Done.

@headius Do you want to proceed https://github.com/ruby/fiddle/issues/104 for JRuby instead of this approach?

That can always be done later/separately, as the updated description mentions. We need to fix the immediate issue.

headius commented 2 months ago

@kou We do still intend to import our Fiddle implementation into this gem, as with all other stdlib gems. That is not a high priority at the moment, so it's ok to move forward with this PR for now.

hsbt commented 2 months ago

Sorry, I read the detail of this thread today. I'm +1 to @kou 's direction. I prefer #147 and #148.

eregon commented 2 months ago

The issue is #148 will take some time. So I would like a short-term solution, this PR or equivalent functionality-wise, so existing TruffleRuby releases use their stdlib fiddle when the fiddle gem is installed, since the gem does not work on TruffleRuby as-is.

headius commented 2 months ago

@kou Thanks so much for helping to import the JRuby FFI-based version of fiddle! I will comment and help starting today.

kou commented 2 months ago

We can use #148 as a short-term solution by requiring only "existing TruffleRuby releases work with gem "fiddle" like built-in Fiddle". If we require many things such as faster implementation, better TruffleRuby compatibility and so on, it may not be a short-term solution. But it's your decision. (I think that we can improve upstreamed TruffleRuby implementation step-by-step after we release a new version with JRuby and TruffleRuby implementations.)

Note that I've implemented many "NotImplemented" methods in TruffleRuby's Fiddle. So #148 has better compatibility than TruffleRuby's Fiddle.

We don't use this as a short-term solution. I don't want to continue the "whether the $LOAD_PATH.delete approach is the best or not" discussion. I think that we can't reach consensus for it.

If you don't want to choose #148 as a short-term solution (you want to improve #148 as the best implementation), we just ship only JRuby implementation and release a new version for gem "fiddle" cases.

eregon commented 2 months ago

148 is not a short-term solution, we need to review and change every usage of private APIs there.

It's like if Fiddle would use internal CRuby functions not in headers (which might disappear/be renamed in the next version, etc), that would be unacceptable, it's the same here.

We had a similar situation with the FFI gem and what has worked very well is up to some TruffleRuby version use the stdlib (more precisely the version of ffi bundled with TruffleRuby), and for newer versions use the proper integration: https://github.com/ffi/ffi/blob/c128cede750242fe19945af8bd6c797728489ad5/lib/ffi.rb#L10-L27

Also note there the interface is "the methods the C extension exposes, no more, no less". So there is no concern of using private APIs in the FFI gem, the only "API" used there is require 'truffleruby/ffi_backend'.

https://github.com/ruby/fiddle/pull/147 for both JRuby and TruffleRuby could maybe be a shorter-term solution, but it will take some time to relicense. Also that may be less complete than the Fiddle support in TruffleRuby stdlib (I'm not sure).

I don't want to continue the "whether the $LOAD_PATH.delete approach is the best or not" discussion. I think that we can't reach consensus for it.

Then maybe we should pick another approach, like the diff in https://github.com/ruby/fiddle/pull/146#discussion_r1777921796 + some testing in CI that basic things work (e.g. require all files, and some basic Fiddle usage to call a libc function). It is more fragile, but better than nothing working.

But it's your decision.

I disagree, you are very much forcing my hand here, by making me choose between:

Any application depending on fiddle (and there will be more since CRuby warns for it) does not work on the latest TruffleRuby release
Give up completely on what is private or public APIs of TruffleRuby and forever bear the cost of it

So I would like something which is not extreme as that. I think that's a reasonable request. IOW, I want a solution for https://github.com/ruby/fiddle/issues/145 that works on existing TruffleRuby releases and which does not involve giving up on what is private or public APIs of TruffleRuby.

Would you accept a PR using the diff in https://github.com/ruby/fiddle/pull/146#discussion_r1777921796 and adding basic tests to run on TruffleRuby releases? (we can also try to reuse existing tests but that will likely be a lot of skips)

kou commented 1 month ago

It's like if Fiddle would use internal CRuby functions not in headers (which might disappear/be renamed in the next version, etc), that would be unacceptable, it's the same here.

We'll use have_func in extconf.rb or something for the case. We may use respond_to? or something in .rb too.

147 for both JRuby and TruffleRuby could maybe be a shorter-term solution, but it will take some time to relicense.

147 isn't a short-term solution for JRuby. It's a real solution for JRuby.

Relicense has done.

Also that may be less complete than the Fiddle support in TruffleRuby stdlib (I'm not sure).

147 is more complete than the Fiddle in TruffleRuby. For example, #147 has variable arguments support but the Fiddle in TruffleRuby doesn't have.

I don't want to continue the "whether the $LOAD_PATH.delete approach is the best or not" discussion. I think that we can't reach consensus for it.

Then maybe we should pick another approach, like the diff in #146 (comment) + some testing in CI that basic things work (e.g. require all files, and some basic Fiddle usage to call a libc function). It is more fragile, but better than nothing working.

It was an option but it is not an option now because we have #147 and #148.

Give up completely on what is private or public APIs of TruffleRuby and forever bear the cost of it

You can defer it by just adding a comment something like the following:

diff --git a/lib/fiddle/truffleruby.rb b/lib/fiddle/truffleruby.rb
index f1c34c7..c49a72d 100644
--- a/lib/fiddle/truffleruby.rb
+++ b/lib/fiddle/truffleruby.rb
@@ -9,6 +9,9 @@
 # GNU General Public License version 2, or
 # GNU Lesser General Public License version 2.1.

+# CAUTION: This uses private APIs temporary. You should not do this in
+# other files.
+
 module Truffle::FiddleBackend

   SIZEOF_CHAR   = Primitive.pointer_find_type_size(:char)

If you want to use it as a long-term workaround, we can also use different file entirely for each TruffleRuby release to support multiple TruffleRuby releases something like:

diff --git a/lib/fiddle/truffleruby.rb b/lib/fiddle/truffleruby.rb
index f1c34c7..c2585ee 100644
--- a/lib/fiddle/truffleruby.rb
+++ b/lib/fiddle/truffleruby.rb
@@ -9,6 +9,9 @@
 # GNU General Public License version 2, or
 # GNU Lesser General Public License version 2.1.

+require_relative "truffleruby#{RUBY_ENGINE_VERSION.split('.', 2)[0]}"
+return
+
 module Truffle::FiddleBackend

   SIZEOF_CHAR   = Primitive.pointer_find_type_size(:char)

It's not difficult nor complex.

Or we can use #147 for TruffleRuby too instead of #148: #149

eregon commented 1 month ago

Or we can use #147 for TruffleRuby too instead of #148: #149

Yes, let's do that. The relicense was faster than expected, great.

ruby / fiddle

Skip native extension building on non-CRuby #146

148 is not a short-term solution, we need to review and change every usage of private APIs there.

147 for both JRuby and TruffleRuby could maybe be a shorter-term solution, but it will take some time to relicense.

147 isn't a short-term solution for JRuby. It's a real solution for JRuby.

147 is more complete than the Fiddle in TruffleRuby. For example, #147 has variable arguments support but the Fiddle in TruffleRuby doesn't have.