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

Update FFI scripts and other files from FFI gem #5947

Closed
headius opened this issue Oct 28, 2019 · 9 comments
Closed

Update FFI scripts and other files from FFI gem #5947

headius opened this issue Oct 28, 2019 · 9 comments
Milestone

Comments

@headius
Copy link
Member

headius commented Oct 28, 2019

Ok, it's time we got JRuby's FFI and the FFI gem on the same page, so we can share updates.

Currently a large number of files we have in common with the FFI gem are not up-to-date. There are many more type configs in the gem and I'm sure there's fixes and tweaks on the JRuby side that have not been propagated to the gem.

I'll start a PR and we can get this stuff synced up. Once we match, I'd like to get the ffi "java" gem populated with the shared files so we can preinstall it and allow users to update it between JRuby releases (which is not possible right now).

cc @larskanis @ahorek @olleolleolle @eregon

@headius headius added this to the JRuby 9.2.10.0 milestone Oct 28, 2019
This was referenced Oct 28, 2019
@larskanis
Copy link

Hi @headius ! It's been some months since this comment, but to that time I migrated jruby's ffi java files to the ffi gem as a jruby extension here. It already worked pretty well, but a bunch of tests failed. I didn't push this attempt forward, since I wasn't sure, whether moving the ffi code out of the jruby repository into the ffi gem repository is the direction you would like to go.

This way we could make sure, that every addition to the ffi gem also pass on JRuby. But I'm not sure, how backward compatible the JRuby files are, since the ffi gem should be usable on various JRuby versions.

@eregon
Copy link
Member

eregon commented Oct 28, 2019

Agreed it would be good to share more of the FFI code between implementations: ffi/ffi#660

Currently TruffleRuby has a copy of FFI Ruby files (= frontend), and reimplements the C extension methods with Ruby + Truffle NFI code (= backend).
We have a script to update the frontend files as well as specs easily in TruffleRuby:
https://github.com/oracle/truffleruby/blob/853c3ab0194ed04cf4b3fb7b528bf360e66407fe/tool/import-ffi.sh

TruffleRuby passes almost all specs of FFI, there are just a few exclusions.
It would be a good idea to add TruffleRuby in FFI's CI and just exclude these not-yet-working specs.

I'm not sure what's best for the future.
Maybe we should have the TruffleRuby backend in the FFI gem?
Or just use the frontend Ruby code from the FFI gem, but use the backend in TruffleRuby?
Or just keep the current situation for TruffleRuby since it's already very easy to synchronize?

If we move the TruffleRuby backend in the FFI gem, I would want push access to ffi/ffi so I can maintain and fix the TruffleRuby backend conveniently.
Currently the TruffleRuby backend code is not that stable across TruffleRuby versions yet (e.g., we now use a different method to get access to Truffle NFI), so I'm a bit hesitant to move it to an external repository.

@headius
Copy link
Member Author

headius commented Oct 28, 2019

@eregon We have a similar compatibility situation across versions with the JRuby extension (based on jnr) so I agree it would be simpler for now if we kept our own custom extensions in our respective projects. My hope with this PR is that we'll get to a point where the remaining .rb and .conf files are identical, so that JRuby's build would just install the ffi gem as a default gem and it could be updated independent of JRuby releases.

See most recent commit to this PR which just tweaks the ffi.rb ext-loading logic to load JRuby's already-present extension.

No objections to giving you or someone else from TruffleRuby push access to FFI.

On the flip side, the code in our built-in FFI extension does not change frequently, so I think keeping out out of the ffi/ffi repository would be best.

@larskanis Thank you for that work! It may become relevant again but I think the best option is to keep the other impl-specific bits out of the gem, since we (JRuby and TruffleRuby) don't need a build step for the ffi gem and it would be simpler to get the other bits in sync without also dealing with the extension.

In JRuby's case, moving the extension to the gem would also lead to some tricky problems with shipping JRuby in other forms, such as the jruby-complete jar. The extension would move out to a separate jar file, that depends on jnr-ffi and other libraries...which are also dependencies of JRuby itself. JRuby also uses FFI to implement many Ruby features, which means we'll almost always be loading the extension. It's just simpler to keep it in jruby/jruby for now.

@headius
Copy link
Member Author

headius commented Oct 28, 2019

@eregon @larskanis I am having trouble sorting out which branches/commits from the various FFI unification PRs apply to JRuby. The PR in #5948 is now at least running specs, but there's a lot of minor failures and some features missing (e.g. my __union! impl is only partial because I could not figure out how the code maps to the JRuby ext).

Can you help me sort out changes either of you made to align the JRuby FFI ext with the recent changes to the gem? Almost none of those changes made it back into JRuby, so there's a lot of work over the past couple years that I need to catch up with.

@eregon
Copy link
Member

eregon commented Oct 28, 2019

@headius My changes shouldn't change any API. Here is the list:
https://github.com/ffi/ffi/pulls?q=is%3Apr+author%3Aeregon+is%3Aclosed
So new specs, one bug fix (ffi/ffi#692), moved a couple things to Ruby and fixing FFI's internal test and benchmark setup.

@ankane
Copy link

ankane commented Nov 5, 2019

This is great! Hopefully it'll take care of ffi/ffi#717 as well.

@headius
Copy link
Member Author

headius commented Jan 9, 2020

Pinging this bug since #5948 is looking good. Largest changes so far are removing Java logic that moved to Ruby. Other changes are minor. Help wanted!

@headius
Copy link
Member Author

headius commented Feb 12, 2020

This one is a little risky to do for a minor, and we're releasing 9.3 in just a few weeks, so I'm retargeting.

@headius headius modified the milestones: JRuby 9.2.10.0, JRuby 9.3.0.0 Feb 12, 2020
@headius
Copy link
Member Author

headius commented Mar 28, 2020

This has been completed by #5948 and will be in Ruby 9.3.

@headius headius closed this as completed Mar 28, 2020
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

No branches or pull requests

4 participants