Skip native extension building on non-CRuby

[andrykonchin]

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 ffi/ffi#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]

Looks good to me.

@andrykonchin ran the test suite locally and that gave:

214 tests, 287 assertions, 13 failures, 107 errors, 0 pendings, 8 omissions, 0 notifications
41.7476% passed

So it doesn't make sense to try to add TruffleRuby in CI at this stage, or at least not for running the test suite as it would just fail.

[eregon]

I think it would make sense to add truffleruby in

fiddle/.github/workflows/ci.yml

Line 28 in aad5a3b

include:

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]

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

[kou]

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

@headius Do you want to proceed #104 for JRuby instead of this approach?

[kou]

Why do we need this?

[eregon]

Because we need to load fiddle.rb from the stdlib in TruffleRuby/JRuby, which works (it's tested in the TruffleRuby/JRuby repos), unlike the version of Fiddle in this repo which only works on CRuby (because of the C extension).

[kou]

I don't think that changing $LOAD_PATH is a good approach because $LOAD_PATH is one of global variables.
Can we use another approach?

[eregon]

There is no other way (AFAIK). We need to make sure the Ruby files of this gem are not loaded because it's a different version than in stdlib. So we need to remove it from $LOAD_PATH.
It is the same approach as for older FFI versions: https://github.com/ffi/ffi/blob/85d0fabce6ab5ceb3848f7e09a265341672a272d/lib/ffi.rb#L19

[kou]

I feel that it's an ad-hoc approach.
Can we use https://github.com/oracle/truffleruby/blob/512427a658f29b222df9a6da1b3ffc12b46a3be6/lib/mri/fiddle.rb#L3-L4 ?

[eregon]

If this is a really short-term fix, we don't need require_relative changes. Because lib/fiddle/**/*.rb in ruby/fiddle and TruffleRuby will not be different. So existing TruffleRuby releases will work with lib/fiddle/**/*.rb in ruby/fiddle.

For completeness I tried and it doesn't work, the test suite fails during loading and just ruby -Ilib -e 'require "fiddle"; require "fiddle/struct"' fails with:

$ ruby -Ilib -e 'require "fiddle"; require "fiddle/struct"'
/home/eregon/code/fiddle/lib/fiddle/pack.rb:18:in `const_missing': uninitialized constant Fiddle::PackInfo::TYPE_BOOL (NameError)
Did you mean?  Fiddle::TYPE_LONG
               Fiddle::TYPE_VOID
	from /home/eregon/code/fiddle/lib/fiddle/pack.rb:18:in `<module:PackInfo>'
	from /home/eregon/code/fiddle/lib/fiddle/pack.rb:5:in `<module:Fiddle>'
	from /home/eregon/code/fiddle/lib/fiddle/pack.rb:4:in `<top (required)>'
	from <internal:core> core/kernel.rb:255:in `require'
	from /home/eregon/code/fiddle/lib/fiddle/struct.rb:4:in `<top (required)>'
	from <internal:core> core/kernel.rb:255:in `require'
	from -e:1:in `<main>'

OTOH $LOAD_PATH.delete(__dir__) works for that ruby command and the test suite loads and runs all tests (not all passing, but at least they all load).

[kou]

I said that I don't want to change any global variables by require "fiddle".

That wasn't clear to me, and I guess you mean change any global variable permanently, and that's why you prefer #146 (comment) to just $LOAD_PATH.delete(__dir__), correct?

Ah, I understand. Correct.

This is too brittle and as I already said it would break silently.

We can detect it by adding more checks to CI. For example, ruby -e 'require "fiddle"; require "fiddle/struct"' instead of ruby -e 'require "fiddle"; require "fiddle/struct"' as you showed.

The next TruffleRuby release is in 6 months, I don't think it's a good idea to require no changes in Fiddle Ruby files for 6 months.

We can use TruffleRuby compatible change something like if we have a CI job for TruffleRuby:

diff --git a/lib/fiddle/pack.rb b/lib/fiddle/pack.rb
index 81088f4..99e0742 100644
--- a/lib/fiddle/pack.rb
+++ b/lib/fiddle/pack.rb
@@ -15,8 +15,11 @@ module Fiddle
       TYPE_USHORT => ALIGN_SHORT,
       TYPE_UINT   => ALIGN_INT,
       TYPE_ULONG  => ALIGN_LONG,
-      TYPE_BOOL   => ALIGN_BOOL,
     }
+    # TruffleRuby doesn't have Fiddle::TYPE_BOOL yet.
+    if Fiddle.const_defined?(:TYPE_BOOL)
+      ALIGN_MAP[TYPE_BOOL] = ALIGN_BOOL
+    end
 
     PACK_MAP = {
       TYPE_VOIDP => "L!",
@@ -31,15 +34,18 @@ module Fiddle
       TYPE_UINT   => "I!",
       TYPE_ULONG  => "L!",
     }
-    case SIZEOF_BOOL
-    when SIZEOF_CHAR
-      PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_UCHAR]
-    when SIZEOF_SHORT
-      PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_USHORT]
-    when SIZEOF_INT
-      PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_UINT]
-    when SIZEOF_LONG
-      PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_ULONG]
+    # TruffleRuby doesn't have Fiddle::SIZEOF_BOOL yet.
+    if Fiddle.const_defined?(:SIZEOF_BOOL)
+      case SIZEOF_BOOL
+      when SIZEOF_CHAR
+        PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_UCHAR]
+      when SIZEOF_SHORT
+        PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_USHORT]
+      when SIZEOF_INT
+        PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_UINT]
+      when SIZEOF_LONG
+        PACK_MAP[TYPE_BOOL] = PACK_MAP[TYPE_ULONG]
+      end
     end
 
     SIZE_MAP = {
@@ -54,8 +60,11 @@ module Fiddle
       TYPE_USHORT => SIZEOF_SHORT,
       TYPE_UINT   => SIZEOF_INT,
       TYPE_ULONG  => SIZEOF_LONG,
-      TYPE_BOOL   => SIZEOF_BOOL,
     }
+    # TruffleRuby doesn't have Fiddle::TYPE_BOOL yet.
+    if Fiddle.const_defined?(:TYPE_BOOL)
+      SIZE_MAP[TYPE_BOOL] = SIZEOF_BOOL
+    end
     if defined?(TYPE_LONG_LONG)
       ALIGN_MAP[TYPE_LONG_LONG] = ALIGN_MAP[TYPE_ULONG_LONG] = ALIGN_LONG_LONG
       PACK_MAP[TYPE_LONG_LONG] = "q"

Also it doesn't solve it for JRuby.

We can release an empty gem for JRuby like other gems did. (Or I may work on #104.)

Actually that approach is broken, as can be seen when trying to run the test suite, because fiddle.rb does not load all fiddle Ruby files but only some of them. For example fiddle/pack.rb isn't loaded eagerly. And so when the user or tests does require 'fiddle/struct' we'll end up with a mix of files from stdlib and from the gem:

How about requiring fiddle/struct in fiddle.rb?

  stdlib = RbConfig::CONFIG["rubylibdir"]
  $LOAD_PATH.prepend(stdlib)
  begin
    require 'fiddle'
    require 'fiddle/struct'
  ensure
    if $LOAD_PATH.first == stdlib
      $LOAD_PATH.shift
    else
      raise 'fiddle files unexpectedly modified the $LOAD_PATH'
    end
  end

Or we can update fiddle/pack.rb in ruby/fiddle to TruffleRuby compatible as I showed.

[eregon]

@kou I am not sure why you are suggesting more complex or brittle solutions.
(brittle because ruby -e 'require "fiddle"; require "fiddle/struct"' is not proper testing and only covers one example, there are other fiddle files that are not eagerly loaded)
I have shown a solution that works reliably and I have shown the various solutions your proposed so far do not work.
That solution is generic, i.e. it works for other gems too, not just fiddle, and we'll need to do something similar for some other default/bundled extension gems too.
So I have no interest to have highly-fiddle-specific solution here, it's just making things more complicated and less consistent (but if it would work reliably, fine, I'd be OK with it).

Also the solution I propose is effectively equivalent to what is already done in strscan and stringio, where there it works because there is no .rb file, which is conceptually equivalent to not adding the gem lib/ dir to $LOAD_PATH.

It is natural for language implementations to reimplement some of the core & standard library based on what makes sense for them, and possibly upstream that if it is stable enough and does not rely on too many internals. This upstreaming is a significant time investment, it does not happen in days but typically takes weeks or months (as we have already seen). It also significantly hinders the ability of language implementations to optimize these parts of stdlib better because then that upstreamed code needs to care about compatibility e.g. with older TruffleRuby versions, etc.

What is a concrete objective problem with $LOAD_PATH.delete(__dir__)?

It seems clear that "$LOAD_PATH is a global variable and shouldn't be changed" is not a good enough argument to reject that approach, $LOAD_PATH is part of the require API, it's the only way to influence where require looks. It is the natural way to solve this issue (short-term), where the gem is unexpectedly shadowing the standard library (which worked fine until now, as very few gems/Gemfile depended on fiddle).

We can release an empty gem for JRuby like other gems did.

How is this any better? Am I missing something?
As far as I understand, it would require you to do more work by building + pushing + testing the extra java-platform gem (if forgotten it breaks things) and it's effectively exactly the same as $LOAD_PATH.delete(__dir__).

And #104 would also be more work for you as then you'd need to maintain a Java implementation.

This PR, once merged & released should require no work or effort from you at all. I thought you would like that.

Also to give you my point of view:

• CRuby broke some stdlib/default/bundled gems for other Ruby implementations by making them bundled gems (or adding warnings for them)
• I filed the issue in The fiddle gem is broken on truffleruby and jruby #145
• @andrykonchin and I created a fix in this PR
• Now we are arguing for days about how it should be done, it is frustrating. It must be for you as well, I'm sorry for that.
• I don't have much time left to spend on this.

If there is still no progress on this PR, I guess one approach is to add some kind of patch in TruffleRuby to do $LOAD_PATH.delete(fiddle_gem_lib_dir) when require 'fiddle' to never load fiddle from the gem.
I don't think that's a good approach/solution though because:

• It won't fix existing TruffleRuby & JRuby releases.
• It's hidden vs being clear here in the gem.

[kou]

I am not sure why you are suggesting more complex or brittle solutions.

I can't understand why I need to say the same thing again and again:
I don't want to change any global variables by require "fiddle".

I can't understand why you say that it's not clear.

(brittle because ruby -e 'require "fiddle"; require "fiddle/struct"' is not proper testing and only covers one example, there are other fiddle files that are not eagerly loaded) I have shown a solution that works reliably and I have shown the various solutions your proposed so far do not work.

I already showed a solution for it. We can use it for other files.

That solution is generic, i.e. it works for other gems too, not just fiddle, and we'll need to do something similar for some other default/bundled extension gems too. So I have no interest to have highly-fiddle-specific solution here, it's just making things more complicated and less consistent (but if it would work reliably, fine, I'd be OK with it).

Also the solution I propose is effectively equivalent to what is already done in strscan and stringio, where there it works because there is no .rb file, which is conceptually equivalent to not adding the gem lib/ dir to $LOAD_PATH.

In principal, your solution (ignore gem) isn't a good solution. If an user wants to use the specific version of A gem, the version information is just ignored with TruffleRuby. It just avoids require error. I pointed out this 2 years ago: ruby/strscan#35 (comment)

It seems that you say that this is a "short-term" workaround but you don't think that this is a "short-term" workaround. At least strscan still uses this workaround. I don't think that it's "short-term".

Anyway, here are PRs that import JRuby and TruffleRuby implementations:

• Import JRuby implementation #147
• Import TruffleRuby implementation #148

[eregon]

I don't want to change any global variables by require "fiddle".

But without that, there is AFAIK no short-term solution to make it work (on existing TruffleRuby releases).
Hence while I understand you don't like to change $LOAD_PATH, since it is the only solution to fix existing releases (broken indirectly by the fiddle warning added in CRuby), I think it is a reasonable short-term solution.

I already showed a solution for it. We can use it for other files.

It does feel brittle as if a new file is added it could be missed there. I suppose we could use Dir.glob but anyway that snippet of code is just more complicated and feels more risky than $LOAD_PATH.delete(__dir__).
With $LOAD_PATH.delete(__dir__) the behavior is clear, with the alternatives it's far more subtle and therefore prone to bugs.

In principal, your solution (ignore gem) isn't a good solution. If an user wants to use the specific version of A gem, the version information is just ignored with TruffleRuby.

Yes that's a valid point.
However, a subset of the functionality working is much better than nothing working or even require 'fiddle' failing.
The tension here is with:

It also significantly hinders the ability of language implementations to optimize these parts of stdlib better because then that upstreamed code needs to care about compatibility e.g. with older TruffleRuby versions, etc.

And for example we found that synchronization for stringio is a huge overhead, and optimizing that would be much harder if TruffleRuby's stringio implementation lives in ruby/stringio and needs to support multiple truffleruby versions.

It seems that you say that this is a "short-term" workaround but you don't think that this is a "short-term" workaround. At least strscan still uses this workaround. I don't think that it's "short-term".

Actually on September 12 I wrote to @andrykonchin in a private chat:

mmh strscan is adding some new features and they are not added for truffleruby since we have strscan.rb in the truffleruby repo: ruby/strscan#106
Probably we should consider moving that upstream. We need to look at the stability of the Primitive's used there though, and Truffle:: methods

And there is the same concern with moving the code here.

Anyway, here are PRs that import JRuby and TruffleRuby implementations:

Thank you, I didn't expect that.
There are some concerns we need to discuss before that can be merged, I'll reply on that PR.

[eregon]

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

Done.

@headius Do you want to proceed #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]

@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.

[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.

OK. Thanks for sharing your thought.

[kou]

I don't think that changing $LOAD_PATH is a good approach because $LOAD_PATH is one of global variables.
Can we use another approach?

[kou]

I feel that it's an ad-hoc approach.
Can we use https://github.com/oracle/truffleruby/blob/512427a658f29b222df9a6da1b3ffc12b46a3be6/lib/mri/fiddle.rb#L3-L4 ?

[kou]

Can we revert this?
We don't need to use rake default with TruffleRuby.

[eregon]

On one side you seem to be asking a clean integration by upstraming the Fiddle TruffleRuby backend here soon, and on the other side you seem to dislike any change making incremental progress towards that (the changes in test/run.rb is similar, it makes it easier to run tests in order to make them pass).
I don't see any point to revert this, it's useful to run bundle exec rake on TruffleRuby, and that's the standard way to run a gem's test suite.

It's also for consistency with the logic in extconf.rb, this extension is a CRuby-only extension.

[kou]

I just want to keep 1 logical change in 1 PR. Is it strange?
I think that this PR focus on "workaround for bundled gem" not "upstreaming TruffleRuby implementation".

the changes in test/run.rb is similar, it makes it easier to run tests in order to make them pass

It's needless when we don't use $LOAD_PATH.delete approach.

[hsbt]

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

[eregon]

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]

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

[kou]

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]

#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'.

#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 #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.

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 #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 #146 (comment) 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]

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]

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

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