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

Check JRuby version without accessing Gem #823

Merged
merged 1 commit into from Sep 23, 2020
Merged

Conversation

headius
Copy link
Contributor

@headius headius commented Sep 22, 2020

When running with --disable-gems, JRuby still needs to be able to load FFI from stdlib. Accessing the Gem namespace here requires that gems not be disabled.

This patch uses a simple split and integer comparison to compare the first two digits of the JRuby version with 9.3.

See #763 for the original code.

This patch is necessary for JRuby to fully source the FFI Ruby files from the gem (jruby/jruby#6150). Example failures here are due to the MRI suites running without RubyGems enabled, but they end up needing FFI for internal JRuby functionality.

https://travis-ci.org/github/jruby/jruby/builds/729197020

So close!

@headius
Copy link
Contributor Author

headius commented Sep 22, 2020

The TruffleRuby code might want to do the same.

@headius
Copy link
Contributor Author

headius commented Sep 22, 2020

The two specific failure cases I see in JRuby CI:

  • Our rbconfig/sizeof.rb requires FFI to enumerate native type widths.
org/jruby/RubyModule.java:3830:in `const_missing': uninitialized constant Gem (NameError)
	from /home/travis/build/jruby/jruby/lib/ruby/stdlib/ffi.rb:11:in `<main>'
	from org/jruby/RubyKernel.java:1043:in `require'
	from /home/travis/build/jruby/jruby/lib/ruby/stdlib/rbconfig/sizeof.rb:2:in `<main>'
	from org/jruby/RubyKernel.java:1043:in `require'
	from /home/travis/build/jruby/jruby/test/mri/lib/envutil.rb:11:in `<main>'
	from org/jruby/RubyKernel.java:1043:in `require'
	from org/jruby/RubyKernel.java:1071:in `require_relative'
	from /home/travis/build/jruby/jruby/test/mri/lib/test/unit.rb:8:in `<main>'
	from org/jruby/RubyKernel.java:1043:in `require'
	from test/mri/runner.rb:13:in `<main>'
  • FFI is loaded at boot and used on Windows to implement symlink support and on Solaris to implement flock.
NameError: uninitialized constant Gem
  const_missing at org/jruby/RubyModule.java:3830
         <main> at D:/a/jruby/jruby/lib/ruby/stdlib/ffi.rb:11
        require at org/jruby/RubyKernel.java:1043
         <main> at uri:classloader:/jruby/kernel/file.rb:6
           load at org/jruby/RubyKernel.java:1078
         <main> at file:///D:/a/jruby/jruby/lib/jruby.jar!/jruby/kernel.rb:27

The latter usage could possibly delay booting FFI, but either way once it's needed it needs to be loadable without RubyGems present.

lib/ffi.rb Outdated Show resolved Hide resolved
@eregon
Copy link
Collaborator

eregon commented Sep 22, 2020

In TruffleRuby I did not hit this issue because require "ffi" loads https://github.com/oracle/truffleruby/blob/4b5eda1a04e669d358bdd2eca6de5abba8fbb8cb/lib/truffle/ffi.rb if there was no gem "ffi", version before (or if RubyGems is disabled).
It has the drawback to not load the latest ffi gem with only require "ffi" (needs gem "ffi" or bundle exec).

Making FFI a default gem sounds nice.

lib/ffi.rb Show resolved Hide resolved
When running with --disable-gems, we still need to be able to
load FFI from stdlib. Accessing the Gem namespace here requires
that gems not be disabled.

This patch splits and compares the integer elements of version
directly rather than using Gem::Version to do that comparison.

See ffi#763 for the original code.
@headius
Copy link
Contributor Author

headius commented Sep 22, 2020

Patch has been updated to use a more terse version and remove remaining references to Gem.

Copy link
Collaborator

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks good to me.
@larskanis Could you merge? (maybe @headius needs a release too?)

@headius
Copy link
Contributor Author

headius commented Sep 22, 2020

Once there's a release with these changes I can flip the switch to make ffi a default gem in JRuby and delete our copies of the ruby files.

@larskanis larskanis merged commit 1459859 into ffi:master Sep 23, 2020
@headius headius deleted the no_rubygems branch September 23, 2020 22:03
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