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.

Sign up for GitHub

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

Jump to bottom

Skip native extension building on non-CRuby #146

Closed
wants to merge 3 commits into from
Closed

Skip native extension building on non-CRuby #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
341265c
Skip building on TruffleRuby
andrykonchin Sep 17, 2024
3f6ac2b
Update CI workflow and build and install gems on TruffleRuby and JRuby
andrykonchin Sep 17, 2024
58193bd
Avoid adding a non-normalized path to $LOAD_PATH in tests
eregon Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,31 @@ jobs:
cd tmp
ruby test/run.rb

# Don't run tests on TruffleRuby and JRuby because tests don't pass on them yet.
# So check only gem installing.
install:
name: >-
Install ${{ matrix.ruby }}
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
ruby:
- truffleruby
- jruby
steps:
- uses: actions/checkout@v4

- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby }}
bundler-cache: true

- run: rake install
andrykonchin marked this conversation as resolved.
Show resolved Hide resolved

- run: ruby -e 'require "fiddle"'

docker:
name: >-
${{ matrix.service }}
Expand Down
10 changes: 7 additions & 3 deletions Rakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@ namespace :version do
end
end

require 'rake/extensiontask'
Rake::ExtensionTask.new("fiddle")
Rake::ExtensionTask.new("-test-/memory_view")
if RUBY_ENGINE == 'ruby'
require 'rake/extensiontask'
Rake::ExtensionTask.new("fiddle")
Rake::ExtensionTask.new("-test-/memory_view")
else
task :compile
end
Comment on lines -20 to +26
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@eregon eregon Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


task :default => [:compile, :test]
5 changes: 5 additions & 0 deletions ext/fiddle/extconf.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# frozen_string_literal: true
require 'mkmf'

if RUBY_ENGINE != 'ruby'
File.write('Makefile', dummy_makefile("").join)
return
end

# :stopdoc:

def gcc?
Expand Down
9 changes: 8 additions & 1 deletion lib/fiddle.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
# frozen_string_literal: true

require 'fiddle.so'
if RUBY_ENGINE == "ruby"
require 'fiddle.so'
else
$LOAD_PATH.delete(__dir__)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Member

@eregon eregon Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@kou kou Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@kou kou Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eregon eregon Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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:

Copy link
Member

@eregon eregon Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

require 'fiddle' # load from stdlib
return
end

require 'fiddle/closure'
require 'fiddle/function'
require 'fiddle/version'
Expand Down
2 changes: 1 addition & 1 deletion test/run.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

$VERBOSE = true

source_dir = "#{__dir__}/.."
source_dir = File.dirname(__dir__)
$LOAD_PATH.unshift("#{source_dir}/test")
$LOAD_PATH.unshift("#{source_dir}/lib")

Expand Down