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

Import TruffleRuby implementation #148

Closed
wants to merge 2 commits into from
Closed

Import TruffleRuby implementation #148

wants to merge 2 commits into from

Conversation

kou
Copy link
Member

@kou kou commented Sep 29, 2024

Fix GH-145

lib/fiddle/truffleruby.rb is based on
https://github.com/oracle/truffleruby/blob/master/lib/truffle/truffle/fiddle_backend.rb
.

Here are changes for it:

  • Add Fiddle::Types::VARIADIC
  • Add Fiddle::Types::CONST_STRING
  • Add Fiddle::Types::BOOL
  • Add Fiddle::ALIGN_BOOL
  • Add Fiddle::SIZEOF_BOOL
  • Add Fiddle::SIZEOF_CONST_STRING
  • Add support for specifying type as not only Fiddle::Types::* but also
    Symbol like :int
  • Add Fiddle::Error as base the error class
  • Add support for Fiddle::Pointer.malloc {}
    Fiddle::Pointer
  • Add support for Fiddle.free(#to_int)
  • Accept Fiddle::Function(need_gvl:) but it's just ignored
  • Fiddle::Function#initialize: Add an argument validation
  • Fiddle::Function#initialize: Keep arguments as instance variables for
    getters
  • Add support for Fiddle::Handle.sym
  • Add support for Fiddle::Handle.[]
  • Add support for Fiddle::Handle.sym_defined?
  • Add support for Fiddle::Handle#sym
  • Add support for Fiddle::Handle#[]
  • Add support for Fiddle::Handle#sym_defined?
  • Add support for Fiddle::Pointer.malloc
  • Add support for Fiddle::Pointer.to_ptr(#to_ptr)
  • Add support for Fiddle::Pointer#free=
  • Add Fiddle::Pointer#freed?
  • Add support for Fiddle::Pointer#call_free
  • Add support for Fiddle::Pointer#to_i
  • Add support for Fiddle::Pointer#to_int
  • Add support for Fiddle::Pointer#ptr
  • Add support for Fiddle::Pointer#+@
  • Add support for Fiddle::Pointer#ref
  • Add support for Fiddle::Pointer#-@
  • Add support for Fiddle::Pointer#null?
  • Add support for Fiddle::Pointer#to_s
  • Add support for Fiddle::Pointer#to_str
  • Add support for Fiddle::Pointer#inspect
  • Add support for Fiddle::Pointer#<=>
  • Add support for Fiddle::Pointer#==
  • Add support for Fiddle::Pointer#eql?
  • Add support for Fiddle::Pointer#+
  • Add support for Fiddle::Pointer#-
  • Add support for Fiddle::Pointer#[]=
  • Add support for Fiddle::Pointer#size
  • Add support for Fiddle::Pointer#size=
  • Add Fiddle::ClearedReferenceError
  • Add no-op Fiddle::Pinned
  • Add Fiddle::NULL

Some features are still "not implemented". So there are some "omit"s
for TruffleRuby in tests.

@kou kou marked this pull request as draft September 29, 2024 09:28
@kou
Copy link
Member Author

kou commented Sep 29, 2024

This is based on GH-147. So we should not merge this before GH-147.

@eregon eregon changed the base branch from master to truffleruby-base October 1, 2024 12:20
@eregon
Copy link
Member

eregon commented Oct 1, 2024

I changed the target branch so this PR shows the relevant diff.


module Truffle::FiddleBackend

SIZEOF_CHAR = Primitive.pointer_find_type_size(:char)
Copy link
Member

@eregon eregon Oct 1, 2024

Choose a reason for hiding this comment

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

Primitive might be renamed or changed, etc.
So far they were only used inside TruffleRuby so there was no compatibility needed whatsoever.
Primitives so far have been an implementation detail (and in fact even the syntax to use them changed).

If a gem uses a Primitive then we need to make that Primitive stable.
One idea there is to annotate it in https://github.com/oracle/truffleruby (e.g. with @Stable, stable = true, stable = "fiddle" or usedBy = "fiddle") so we know we shouldn't change its name or behavior.
And also make sure exposing that behavior as a Primitive makes sense and is necessary.

We should probably also expose a predicate to find out if a Primitive is defined or not, since the set of Primitives changes with different TruffleRuby versions. Maybe TruffleRuby.primitive?(name).
That way if we want to use a new Primitive but it might not always be available it's possible.
And it's also nicer than comparing versions.

I don't want TruffleRuby to repeat the mistake of exposing all internal APIs as "effectively public API because used in some gem" (which was the case for CRuby C API and for JRuby Java API).

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do it or we can use ffi API like JRuby does.

LONG_NFI_TYPE = "SINT#{SIZEOF_LONG * 8}"
ULONG_NFI_TYPE = "UINT#{SIZEOF_LONG * 8}"

SIZE_T_TYPEDEF = Truffle::Config.lookup('platform.typedef.size_t')
Copy link
Member

Choose a reason for hiding this comment

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

Everything under Truffle:: is considered implementation details, with the exception of Truffle::Interop:
https://github.com/oracle/truffleruby/blob/master/doc/user/truffleruby-additions.md#unsupported-additional-functionality

It's a similar concern as with the Primitives, whatever is used in external gems needs to be made stable first.
Which might imply moving the method to maybe something like TruffleRuby::FiddleSupport, renaming the method or using a Primitive instead, etc.

Copy link
Member

@eregon eregon Oct 1, 2024

Choose a reason for hiding this comment

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

One trouble with this is e.g. if this is changed/moved/renamed then it won't work on TruffleRuby 24.1.0 and older versions.
Then we would need another solution for older versions.

For example a stable Primitive seems better here than exposing Truffle::Config.lookup.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can release a new version for newer TruffleRuby if it's needed.
Old TruffleRuby users can pin Fiddle gem to old version.

Copy link
Member

Choose a reason for hiding this comment

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

Old TruffleRuby users can pin Fiddle gem to old version.

The error for end users would be very unclear though (e.g. some NoMethodError), and they would likely have no idea how to solve this from the error message.
Also if for whatever reason it's a program run without Bundler then I don't think there is a way to pin the Fiddle version.

That's why I have always thought and still think the best solution is this PR for upcoming TruffleRuby versions, and using the stdlib for older TruffleRuby versions.
This is also what the FFI gem does for example:
https://github.com/ffi/ffi/blob/c128cede750242fe19945af8bd6c797728489ad5/lib/ffi.rb#L14-L27

# GNU General Public License version 2, or
# GNU Lesser General Public License version 2.1.

module Truffle::FiddleBackend
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't define Truffle or Truffle::FiddleBackend here but something like Fiddle::TruffleRubyBackend.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can change lib/fiddle/truffleruby.rb in this branch directly.

Comment on lines +4 to +10
# Copyright (c) 2019, 2024 Oracle and/or its affiliates. All rights reserved. This
# code is released under a tri EPL/GPL/LGPL license. You can use it,
# redistribute it and/or modify it under the terms of the:
#
# Eclipse Public License version 2.0, or
# GNU General Public License version 2, or
# GNU Lesser General Public License version 2.1.
Copy link
Member

@eregon eregon Oct 1, 2024

Choose a reason for hiding this comment

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

We will need to relicense this code to upstream it here (otherwise we would have to change the fiddle gem license).
From git shortlog -sne lib/truffle/truffle/fiddle_backend.rb it looks like it was only modified by Oracle employees, so then we should be able to relicense that, by relicensing https://github.com/oracle/truffleruby/blob/master/lib/truffle/truffle/fiddle_backend.rb under BSD-2-Clause.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. It's very helpful.

@kou
Copy link
Member Author

kou commented Oct 2, 2024

I've added truflleruby-20 and truffleruby-21 because:

ruby/setup-ruby don't provide them for macos-14 and ubuntu-24.04:

https://github.com/ruby/fiddle/actions/runs/11137969090/job/30952238981?pr=148#step:3:17

Error: Error: Unavailable version 20.3.0 for truffleruby on macos-14

https://github.com/ruby/fiddle/actions/runs/11137969090/job/30952239103?pr=148#step:3:17

Error: Error: Unavailable version 21.3.0 for truffleruby on macos-14

https://github.com/ruby/fiddle/actions/runs/11137969090/job/30952240601?pr=148#step:3:14

Error: Error: Unavailable version 20.3.0 for truffleruby on ubuntu-24.04

https://github.com/ruby/fiddle/actions/runs/11137969090/job/30952240753?pr=148#step:3:14

Error: Error: Unavailable version 21.3.0 for truffleruby on ubuntu-24.04

truffleruby-20 on macos-12 doesn't work because it doesn't work with recent Bundler:

https://github.com/ruby/fiddle/actions/runs/11137969090/job/30952235259?pr=148#step:3:27

   Using latest Bundler for truffleruby-20.3.0 because the default Bundler gem is too old for that Ruby version
  Ruby 2.6-2.7 only works with Bundler 2.4
  /Users/runner/.rubies/truffleruby-20.3.0/bin/gem install bundler --force -v ~> 2.4.0

truffleruby-21 on macos-12 doesn't work because of internal Bundler error:

https://github.com/ruby/fiddle/actions/runs/11137969090/job/30952235963?pr=148#step:4:10

NameError: method `search_by_dependency' not defined in Bundler::Index

Should we really care about old TruffleRuby releases?

@eregon
Copy link
Member

eregon commented Oct 2, 2024

I've added truffleruby-20 and truffleruby-21 because:

The versions at https://www.graalvm.org/release-calendar/#previous-releases are very confusing unfortunately, as they use Java versions, while Truffle/TruffleRuby versions use the year.
So truffleruby-20 and truffleruby-21 are ancient, from 2020 and 2021.

The current GraalVM LTS is GraalVM for JDK 21, which corresponds to TruffleRuby 23.1.0.
So it might make sense to test that and/or 24.0 as well.

I think we should solve it for all existing TruffleRuby releases if possible and #146 is one way to do that.
This PR is unlikely to work on older releases, and might not even work on the latest release, once we avoid depending on internal & changing APIs.

@eregon eregon changed the base branch from truffleruby-base to master October 8, 2024 20:37
Fix GH-145

lib/fiddle/truffleruby.rb is based on
https://github.com/oracle/truffleruby/blob/master/lib/truffle/truffle/fiddle_backend.rb
.

Here are changes for it:

* Add `Fiddle::Types::VARIADIC`
* Add `Fiddle::Types::CONST_STRING`
* Add `Fiddle::Types::BOOL`
* Add `Fiddle::ALIGN_BOOL`
* Add `Fiddle::SIZEOF_BOOL`
* Add `Fiddle::SIZEOF_CONST_STRING`
* Add support for specifying type as not only `Fiddle::Types::*` but also
  `Symbol` like `:int`
* Add `Fiddle::Error` as base the error class
* Add support for `Fiddle::Pointer.malloc {}`
  `Fiddle::Pointer`
* Add support for `Fiddle.free(#to_int)`
* Accept `Fiddle::Function(need_gvl:)` but it's just ignored
* `Fiddle::Function#initialize`: Add an argument validation
* `Fiddle::Function#initialize`: Keep arguments as instance variables for
  getters
* Add support for `Fiddle::Handle.sym`
* Add support for `Fiddle::Handle.[]`
* Add support for `Fiddle::Handle.sym_defined?`
* Add support for `Fiddle::Handle#sym`
* Add support for `Fiddle::Handle#[]`
* Add support for `Fiddle::Handle#sym_defined?`
* Add support for `Fiddle::Pointer.malloc`
* Add support for `Fiddle::Pointer.to_ptr(#to_ptr)`
* Add support for `Fiddle::Pointer#free=`
* Add `Fiddle::Pointer#freed?`
* Add support for `Fiddle::Pointer#call_free`
* Add support for `Fiddle::Pointer#to_i`
* Add support for `Fiddle::Pointer#to_int`
* Add support for `Fiddle::Pointer#ptr`
* Add support for `Fiddle::Pointer#+@`
* Add support for `Fiddle::Pointer#ref`
* Add support for `Fiddle::Pointer#-@`
* Add support for `Fiddle::Pointer#null?`
* Add support for `Fiddle::Pointer#to_s`
* Add support for `Fiddle::Pointer#to_str`
* Add support for `Fiddle::Pointer#inspect`
* Add support for `Fiddle::Pointer#<=>`
* Add support for `Fiddle::Pointer#==`
* Add support for `Fiddle::Pointer#eql?`
* Add support for `Fiddle::Pointer#+`
* Add support for `Fiddle::Pointer#-`
* Add support for `Fiddle::Pointer#[]=`
* Add support for `Fiddle::Pointer#size`
* Add support for `Fiddle::Pointer#size=`
* Add `Fiddle::ClearedReferenceError`
* Add no-op `Fiddle::Pinned`
* Add `Fiddle::NULL`

Some features are still "not implemented". So there are some "omit"s
for TruffleRuby in tests.
@kou
Copy link
Member Author

kou commented Oct 10, 2024

We use #149 instead of this.

@kou kou closed this Oct 10, 2024
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
2 participants