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 the Ruby files of the FFI gem on TruffleRuby >= 20.1.0 #768

Merged
merged 3 commits into from Apr 14, 2020

Conversation

eregon
Copy link
Collaborator

@eregon eregon commented Apr 11, 2020

Similar logic as for JRuby.
This will only apply for TruffleRuby >= 20.1.0 (not yet released), as versions before don't expect that.

I ran it locally with this and changes in TruffleRuby and all specs passed.

Ref #660

@eregon eregon requested a review from larskanis April 11, 2020 12:15
@eregon eregon force-pushed the use-ffi-ruby-files-for-truffleruby branch from 99f1764 to 7c577eb Compare April 11, 2020 12:18
Copy link
Member

@larskanis larskanis left a comment

Choose a reason for hiding this comment

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

Nice! I think we'll need TruffleRuby and JRuby in CI of this project.

lib/ffi/pointer.rb Outdated Show resolved Hide resolved
@eregon eregon force-pushed the use-ffi-ruby-files-for-truffleruby branch from 7c577eb to 16f221a Compare April 11, 2020 17:19
@eregon
Copy link
Collaborator Author

eregon commented Apr 11, 2020

Nice! I think we'll need TruffleRuby and JRuby in CI of this project.

truffleruby-head is already tested in GitHub Actions.
But indeed once TruffleRuby 20.1.0 is released we should test against truffleruby too to make sure the latest release works.

Regarding adding JRuby in CI I'll leave that to @headius or to you (it should be trivial to add it to .github/workflows/ci.yml but not sure it's green).

lib/ffi.rb Outdated
JRuby::Util.load_ext("org.jruby.ext.ffi.FFIService")
require 'ffi/ffi'

elsif RUBY_ENGINE == 'truffleruby' && Gem::Version.new(RUBY_ENGINE_VERSION) >= Gem::Version.new("20.1.0")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could tweak this condition to >= Gem::Version.new("20.1.0-dev-a") once the related changes are merged in TruffleRuby (nightly versions look like 20.1.0-dev-374d84d2).
That way the CI running on truffleruby-head would return true for this condition.
(Potentially we could use RUBY_RELEASE_DATE too)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've done this, the latest TruffleRuby nightly now has the changes needed.

@headius
Copy link
Contributor

headius commented Apr 13, 2020

Regarding adding JRuby in CI I'll leave that to @headius or to you (it should be trivial to add it to .github/workflows/ci.yml but not sure it's green).

Along with updating the FFI .rb files, I also made sure all specs are green on JRuby.

@eregon
Copy link
Collaborator Author

eregon commented Apr 13, 2020

@headius Could you make a PR adding jruby-head in CI then?
(Which has a version of 9.3.0.0-SNAPSHOT which is >= 9.3.pre)

@headius
Copy link
Contributor

headius commented Apr 13, 2020

@eregon I just checked and there's a handful of failures in recent spec updates related to the fixnum/bignum/long threshold.

I would prefer to wait on adding JRuby to CI until we can use the gem for all FFI ruby sources (i.e. delete our copies of libs and specs), so I know there's no conflict between FFI head and JRuby head.

@eregon
Copy link
Collaborator Author

eregon commented Apr 14, 2020

I would prefer to wait on adding JRuby to CI until we can use the gem for all FFI ruby sources (i.e. delete our copies of libs and specs), so I know there's no conflict between FFI head and JRuby head.

That wouldn't work with older versions of the FFI gem though, so I'm not sure how feasible is that.

@eregon
Copy link
Collaborator Author

eregon commented Apr 14, 2020

@larskanis Looks like it's working just fine with truffleruby-head, OK to merge?
(I verified locally the correct branch is taken)

@larskanis larskanis merged commit 9e6892c into ffi:master Apr 14, 2020
@headius
Copy link
Contributor

headius commented Apr 14, 2020

That wouldn't work with older versions of the FFI gem though

I don't know what you mean here. Why would I test against an older version of the gem?

@eregon
Copy link
Collaborator Author

eregon commented Apr 14, 2020

I don't know what you mean here. Why would I test against an older version of the gem?

Users might install an older version of the gem on the future JRuby release without FFI Ruby files. That would then break. So I think we need to keep Ruby files also in Ruby implementations as long as we want to support older FFI releases.

@headius
Copy link
Contributor

headius commented Apr 14, 2020

@eregon I think you misunderstand... we would still include FFI as a default gem, like most of the other standard libraries. Installing an older FFI gem would not install any new files, so the ones in stdlib would still be used. Newer FFI gems would be picked up and used. I don't see a problem.

@headius
Copy link
Contributor

headius commented Apr 14, 2020

FWIW I don't really intend to "support" older FFI gems in JRuby because they're all stub gems with no content.

@eregon
Copy link
Collaborator Author

eregon commented Apr 15, 2020

Right, that should work if that version of the default gem is recent or includes the JRuby version check in ffi.rb. A default gem is not very different than a copy in stdlib + a .gemspec anyway.

@eregon
Copy link
Collaborator Author

eregon commented Apr 15, 2020

@larskanis Could you make a release of FFI?

This PR will help us address an issue happening with aruba + bundle install --standalone adding paths with .. inside $LOAD_PATH (rubygems/rubygems#3505), making

ffi/lib/ffi.rb

Line 21 in 9e6892c

$LOAD_PATH.delete(File.dirname(__FILE__))
not working as expected.
We could also try to adapt the code there to compare against expanded (File.expand_path) entries of $LOAD_PATH but I think that would be kind of slow and I think it's mostly a Bundler issue. Also, with this PR, TruffleRuby no longer uses that part of the code anyway, which is cleaner and avoids the "require gem but remove it and go to stdlib" part.

@headius
Copy link
Contributor

headius commented Apr 15, 2020

A default gem is not very different than a copy in stdlib + a .gemspec anyway.

Isn't that exactly what a default gem is?

@eregon
Copy link
Collaborator Author

eregon commented Apr 15, 2020

Yeah, exactly so in the end whether there is a copy of the lib files in JRuby or a default gem is pretty much the same, that's why I mentioned that.

@larskanis
Copy link
Member

@eregon

Could you make a release of FFI?

I would like to fix #751 first, so that a :string callback return value is properly raised. The rest is fine for ffi-1.13.0 IMHO. I'll release it in the next days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants