Improve loading extension

[jonbarlo]

In RubyGems, we are rubygems/rubygems#8921 to stop installing extensions into the lib directory of the installed gem.

If we made such change, this gem would break because it relies on the extension being present in the same directory as lib code.

This PR changes things to not make such assumption and instead loading from extensions directory whenever if possible then fallback to legacy search and paths

[davispuh]

I don't think this is good idea. Why would it ever fail? To me it seems only way it would fail if either this code is buggy (that should just be fixed) or if Ruby/RubyGems is broken for user in which case user should fix that.

[jonbarlo]

thanks i have updated the code a little bit, wondering what do you think of raising exception adding a actionable description message

[davispuh]

This is worse than how it was before, I guess you didn't understood my comment. I meant only that additional "begin / rescue" block which seemed to not be necessary.

Now with this current implementation it will break all current ffi-compiler users, so all old gems won't work.
And what about people who want to use script directly without installing any gems? eg. ruby myscript.rb, this won't be found.

Another thing now I thought is that you assume that library name and gem name will have same name but I don't think will always be case. It's probably not even too common when that actually happens.
For example there is scrypt gem that loads scrypt_ext library - https://github.com/pbhogan/scrypt/blob/master/lib/scrypt/scrypt_ext.rb#L11

[jonbarlo]

@davispuh yes i understood something totally different i have pushed the code with the removal of the extra block of code (begin/rescue)

regarding the name as long as the extension file is present in the expected location it should grab it, but yes you are right! i am not sure if override and expand params in here? something among these lines:

def self.find(library_name, start_path = nil, gem_name: nil)
  library = Platform.system.map_library_name(library_name)

  # If gem_name is provided, try RubyGems extension_dir for that gem
  if gem_name && defined?(Gem::Specification)
    begin
      spec = Gem::Specification.find_by_name(gem_name)
      if spec && spec.respond_to?(:extension_dir)
        ext_dir = spec.extension_dir
        ext_path = File.join(ext_dir, library)
        return ext_path if File.exist?(ext_path)
      end
    rescue Gem::MissingSpecError
      # Ignore and fall back to legacy search
    end
  else
    #  try RubyGems extension_dir using library_name as gem name
    if defined?(Gem::Specification)
      begin
        spec = Gem::Specification.find_by_name(library_name)
        if spec && spec.respond_to?(:extension_dir)
          ext_dir = spec.extension_dir
          ext_path = File.join(ext_dir, library)
          return ext_path if File.exist?(ext_path)
        end
      rescue Gem::MissingSpecError
        # Ignore and fall back to legacy search
      end
    end
  end

  # Legacy recursive search
  root = false
  Pathname.new(start_path || caller_path(caller[0])).ascend do |path|
    Dir.glob("#{path}/**/#{FFI::Platform::ARCH}-#{FFI::Platform::OS}/#{library}") do |f|
      return f
    end
    Dir.glob("#{path}/**/#{library}") do |f|
      return f
    end
    break if root
    root = File.basename(path) == 'lib'
  end
  raise LoadError, "cannot find '#{library_name}' library"
end

Thanks for your help, much appreciate it

[davispuh]

Besides that, seems fine to me.

[deivid-rodriguez]

I did not look at this patch closely, but I feel my original approach is simpler, although maybe not correct?

In any case, even if FFI::Compiler::Loader.find usages will break if we make this change in RubyGems, there are other extensions not using ffi-compiler that will break, too, like ohler55/ox#383.

So even if ffi-compiler should eventually be changed too, we should focus on an approach that warns every case that would eventually break, but keeps everything working other than that. Sounds tricky though, so I'm unsure we want to make this change at all in RubyGems.