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

Load JRuby ext under JRuby #747

Merged
merged 1 commit into from Mar 30, 2020
Merged

Load JRuby ext under JRuby #747

merged 1 commit into from Mar 30, 2020

Conversation

headius
Copy link
Contributor

@headius headius commented Feb 16, 2020

JRuby's FFI is almost aligned with the gem via jruby/jruby#5948. This change will allow us to use ffi.rb unmodified.

@larskanis
Copy link
Member

This is awesome Charles! I'm very glad, that MRI and JRuby FFI are aligned now. I'm sorry I can't help with JRuby, but I don't use Java at all. It takes me too much time to do serious work. Anyway I'd like to work together if there are issues with C or Ruby parts.

Just to clarify: This change allows to sync lib and spec directories without modifications between both repositories? Using the ffi-platform-ruby-gem on JRuby is still not intended, is it?

There seems to be no code for synchronization in jruby/jruby#5948 so far. Is this something you plan to add?

@larskanis
Copy link
Member

@headius Can you answer the questions above? I'd like to merge this PR.

@headius
Copy link
Contributor Author

headius commented Mar 29, 2020

@larskanis Sorry I didn't see this comment!

Yes, JRuby's FFI lib and spec should now be exactly the same as from the gem. There's no syncing code in jruby/jruby#5948 because my hope is that we can source the gem contents directly, as a default gem.

I have created jruby/jruby#6150 to track that change. It doesn't work properly at the moment because the "java" version of the gem contains no sources. I suggest either we include the sources there, or release a single gem that just skips the extension build on JRuby.

@larskanis larskanis merged commit a5bc0f3 into ffi:master Mar 30, 2020
larskanis added a commit to larskanis/ffi that referenced this pull request Mar 30, 2020
Related to ffi#747 .
JRuby-9.3 is expected to be fully compatible to this gem's Ruby code.
This allows to ship the Ruby library code per platform java gem and add it as a default gem to JRuby.
@larskanis
Copy link
Member

I suggest either we include the sources there, or release a single gem that just skips the extension build on JRuby.

@headius I merged this manually. A platform=java gem with only ruby sources is much smaller than the ordinary platform=ruby gem. So I changed the java gem accordingly in #763 .

@eregon
Copy link
Collaborator

eregon commented Mar 30, 2020

How much smaller is it?

For TruffleRuby we'll want to use the platform=ruby gem at some point to reuse the Ruby files from the gem, so I think longer term it would be simpler to only have a platform=ruby gem (also in terms of complexity of the Rakefile, etc).

@headius headius deleted the jruby_ext branch March 30, 2020 13:35
@headius
Copy link
Contributor Author

headius commented Mar 30, 2020

I do not have any preference between using the ruby gem or the java gem, so I'll leave that up to you.

@larskanis
Copy link
Member

With #763 both platform gems (ruby and java) are equally usable. extconf.rb already compiles the extension conditionally and building a platform java gem is just a few lines of simple code in the Rakefile. So I'm OK with both of them.

I also wouldn't omit the platform=java gem since the java version has been explicit requested several times, although ffi-1.12.2 installs pretty fine on JRuby-9.2 for me (it runs a dummy makefile and falls through to the internal JRuby impl at runtime).

Sizes of the gems are as following:

$ ll pkg/*.gem
-rw-r--r-- 1 lars lars 898048 Mär 30 21:07 pkg/ffi-1.12.2.gem
-rw-r--r-- 1 lars lars  51712 Mär 30 21:07 pkg/ffi-1.12.2-java.gem

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