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

Ensure compatibility with a single shared libvips library #390

Merged
merged 4 commits into from
Mar 7, 2024

Conversation

kleisauke
Copy link
Member

@kleisauke kleisauke commented Mar 4, 2024

See: #372.

@jcupitt
Copy link
Member

jcupitt commented Mar 4, 2024

Hiya,

Hmmm I'm a little unsure about this approach, it means we have the library name hidden in the library_name method, when it's supposed to be a parameter.

Instead, how adding an new method for detecting a semi-static libvips? Something like (untested):

def library_name(name, abi_number)
  if FFI::Platform.windows?
    "lib#{name}-#{abi_number}.dll"
  elsif FFI::Platform.mac?
    "#{name}.#{abi_number}"
  else
    "#{name}.so.#{abi_number}"
  end
end

module Vips
  extend FFI::Library

  ffi_lib library_name("vips", 42)

  # see if we can get glib functions from the libvips library
  #
  # if we can, we are dealing with a semi-static libvips binary or a platform 
  # which handles library dependencies automatically, and we can
  # fetch everything from that ... if it fails, we will need to open separate
  # glib and gobject libraries
  begin
    attach_function :g_malloc, [:size_t], :pointer
    is_semistatic = true
  rescue => e
    is_semistatic = false
  end
  
  def semistatic?
    is_semistatic
  end
end

module GLib
  class << self
    attr_accessor :logger
  end
  @logger = Logger.new($stdout)
  @logger.level = Logger::WARN

  extend FFI::Library

  if Vips::semistatic?
    ffi_lib library_name("glib-2.0", 0)
  else
    ffi_lib library_name("vips", 42)
  end

...

Hmm or something like that anyway. I've not thought about this very carefully!

pyvips could maybe use something similar, I think at the moment it'll try to load glib as a test for semistatic, but that could break on windows if the user has glib from another project on their PATH. Seeing if we can get g_free or whatever from libvips ought to be safer.

Or I've badly misunderstood!

@jcupitt
Copy link
Member

jcupitt commented Mar 4, 2024

Hmm semistatic? is a bad name, it should maybe be separate_libs?. Anyway, it controls whether we need to open libraries separately.

@kleisauke
Copy link
Member Author

It looks like this can be simplified by passing an array of library names to ffi_lib, see:
https://github.com/ffi/ffi/wiki/Using-Multiple-and-Alternate-Libraries#using-one-of-three-alternate-libraries

I just tested this on Windows, both the -static and -static-ffi binaries seems to work! 🎉

PS> git clone -b single-shared-vips-compat https://github.com/kleisauke/ruby-vips.git
PS> cd ruby-vips
PS> gem build ruby-vips.gemspec
PS> gem install ruby-vips-2.2.1.gem ffi --ignore-dependencies
PS> $env:RUBY_DLL_PATH = "C:\vips-dev-8.15\bin";
PS> irb
irb(main):001:0> require "vips"
=> true
irb(main):002:0> im = Vips::Image.black 100, 100
=> #<Image 100x100 uchar, 1 bands, multiband>

@kleisauke
Copy link
Member Author

pyvips could maybe use something similar, I think at the moment it'll try to load glib as a test for semistatic, but that could break on windows if the user has glib from another project on their PATH. Seeing if we can get g_free or whatever from libvips ought to be safer.

Ah, that would indeed be problematic, but only for Windows, so maybe we shouldn't spend too much time on this.

Wrapping all GLib and GObject symbols would indeed solve this (this was suggested in libvips/libvips#2788), but it's a lot of work, and I'm not sure if it's worth it. :(

@kleisauke
Copy link
Member Author

I restarted the flaky tests. This is ready for review now.

@kleisauke kleisauke marked this pull request as ready for review March 4, 2024 18:15
we must fetch `g_*()` funcs from libvips if we can
@jcupitt
Copy link
Member

jcupitt commented Mar 5, 2024

Nice! I didn't know you could pass sets of library names to ffi_lib, that's great.

But .... wouldn't it still be better to try to fetch g_*() funcs from libvips? I think it's not too hard, I tried:

2c55d8b

And it seems to work, though I've not checked windows.

@kleisauke
Copy link
Member Author

Ah, I see what you meant. Indeed, that would work. It also fixes the PATH issue described above, nice!

@kleisauke
Copy link
Member Author

I merged commit 2c55d8b within this branch, additionally I had to apply commit 491d7d9 to fix this error on Windows:

irb(main):001:0>  require "vips"
C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/ffi-1.16.3-x64-mingw-ucrt/lib/ffi/library.rb:216:in `attach_function': Function 'g_malloc' not found in [libvips-42.dll] (FFI::NotFoundError)
        from C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/ruby-vips-2.2.1/lib/vips.rb:49:in `<module:Vips>'
        from C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/ruby-vips-2.2.1/lib/vips.rb:43:in `<top (required)>'
        from <internal:C:/Ruby32-x64/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:160:in `require'
        from <internal:C:/Ruby32-x64/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:160:in `rescue in require'
        from <internal:C:/Ruby32-x64/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:40:in `require'
        from (irb):1:in `<main>'
        from C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/irb-1.6.2/exe/irb:11:in `<top (required)>'
        from C:/Ruby32-x64/bin/irb:33:in `load'
        from C:/Ruby32-x64/bin/irb:33:in `<main>'
<internal:C:/Ruby32-x64/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:86:in `require': cannot load such file -- vips (LoadError)
        from <internal:C:/Ruby32-x64/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:86:in `require'
        from (irb):1:in `<main>'
        from C:/Ruby32-x64/lib/ruby/gems/3.2.0/gems/irb-1.6.2/exe/irb:11:in `<top (required)>'
        from C:/Ruby32-x64/bin/irb:33:in `load'
        from C:/Ruby32-x64/bin/irb:33:in `<main>'

It seems to work properly now.

@jcupitt jcupitt merged commit 74c20e6 into libvips:master Mar 7, 2024
26 checks passed
@jcupitt
Copy link
Member

jcupitt commented Mar 7, 2024

Great! Thank you for fixing this Kleis.

@jcupitt
Copy link
Member

jcupitt commented Mar 7, 2024

I really like ruby, but I still get confused about when to use . and :: :(

@kleisauke kleisauke deleted the single-shared-vips-compat branch March 7, 2024 10:26
@Nakilon
Copy link
Contributor

Nakilon commented Mar 7, 2024

I really like ruby, but I still get confused about when to use . and :: :(

It's like a matter of personal preference.

Though for example I had an issue with :: recently. In my code that utilizes AWS sdk there was a line of code:

Aws::Sigv4::Signer.new(

Then for the same program testing I used a poor-man's stubbing for third-party libraries like this:

Aws = Struct.new(:Sigv4).new(Struct.new(:new).new(...

and it obviously failed because Struct does not respond to ::. But it works once you rewrite it as:

Aws.Sigv4.Signer.new(

My current choice is to use . everywhere and use the leading :: to access the global modules. I.e.:

::Aws.Sigv4.Signer.new(

and the same for ::File, ::Dir, ::Struct, ::ENV, etc. (It only sucks you can't do ::Integer(iteger_string))

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.

3 participants