Exception when requiring after 'mkmf' library

[botandrose]

Steps to reproduce:

$ irb
2.3.0 :001 > require 'scrypt'
 => true 
2.3.0 :002 > require 'mkmf'
 => true 
2.3.0 :003 > exit

$ irb
2.3.0 :001 > require 'mkmf'
 => true 
2.3.0 :002 > require 'scrypt'
ArgumentError: wrong number of arguments (given 1, expected 2+)
    from /home/micah/.rvm/rubies/ruby-2.3.0/lib/ruby/2.3.0/mkmf.rb:1237:in `find_type'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/ffi-1.9.10/lib/ffi/struct.rb:332:in `find_type'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/ffi-1.9.10/lib/ffi/struct.rb:326:in `find_field_type'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/ffi-1.9.10/lib/ffi/struct.rb:368:in `array_layout'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/ffi-1.9.10/lib/ffi/struct.rb:278:in `layout'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/scrypt-2.1.1/lib/scrypt.rb:140:in `<class:Calibration>'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/scrypt-2.1.1/lib/scrypt.rb:139:in `<class:Engine>'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/scrypt-2.1.1/lib/scrypt.rb:24:in `<module:SCrypt>'
    from /home/micah/.rvm/gems/ruby-2.3.0/gems/scrypt-2.1.1/lib/scrypt.rb:10:in `<top (required)>'
    from /home/micah/.rvm/rubies/ruby-2.3.0/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:127:in `require'
    from /home/micah/.rvm/rubies/ruby-2.3.0/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:127:in `rescue in require'
    from /home/micah/.rvm/rubies/ruby-2.3.0/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:40:in `require'
    from (irb):2
    from /home/micah/.rvm/rubies/ruby-2.3.0/bin/irb:11:in `<main>'

Also reproducible on ruby-2.2.0. Haven't tried others.

[febeling]

mkmf is polluting the toplevel namespace here with it's defintion of find_type.

And ffi is tripping over it here.

[botandrose]

Hmm, so it seems to me the "right" thing to here is to fix mkmf to not pollute the global namespace. Since mkmf is in core, and this would have backwards-compatibility ramifications, this might take a while, and existing Rubies will still have this problem, so a workaround might be a better short-term solution. I'll see if I can hack something together... maybe something that allows one to use the functionality in mkmf without polluting the top-level namespace.

[botandrose]

Thanks for doing the detective work on this, btw, @febeling !

[botandrose]

https://bugs.ruby-lang.org/issues/12370

[febeling]

Yes, the right fix would be to isolate mkmf cleanly into a module. But FFI is also a bit casual about grabbing any find_type it sees and calling it.

For the first fix, a major version bump would be exactly the right tool. Too bad this is a core module, and not a gem. But there must be an equivalent to a major version bump in core? What is generally done to break an API in such cases in ruby?

I don't know if it's a bad idea, but maybe define the whole module just without the include when loaded as require 'mkmf/isolated' and start to write nagging deprecation warnings when used with require 'mkmf' going forward?

[botandrose]

Yes, that was exactly my thought... extracting MakeMakeFile to its own file, and mkmf would just load that file and include the module. I think it would probably be nice to also use module_function or extend self within MakeMakeFile, so that one could use its functions in a nice qualified manner, e.g. MakeMakeFile.find_type(...).

Looking into more short-term workarounds, I found the https://github.com/rosylilly/uninclude gem, which, in theory, would let one immediately uninclude MakeMakeFile after requiring mkmf, but I can't get it to work on the top-level namespace. :/

[botandrose]

Top-level namespace stuff is weird. This works:

require 'uninclude'
require 'mkmf'
Object.send :uninclude, MakeMakefile

After which you can call methods directly on MakeMakefile. FWIW.

[febeling]

Yes, it looks like a welcome workaround for anyone who's blocked by this problem. It also neatly fits the description you gave a few days back - good job :)

Not sure if you agree, maybe this issue could even be closed now?

One can argue that the main issue is poor practice in mkmf, also ffi is a bit too cavalier about type definition discovery. And scrypt is only showing symptoms of this, as possibly many libs would that rely on both.

[stakach]

I think we should leave the issue open until the workaround is no longer needed with the latest gems versions