libvips / ruby-vips

Ruby extension for the libvips image processing library.
MIT License
829 stars 61 forks source link

Allow to opt-out of automatic msys2_mingw_dependency installation #332

Open tknerr opened 2 years ago

tknerr commented 2 years ago

Hi @jcupitt ,

as discussed in #331 (see comments), we have the need of more fine-grained control on the libvips library version to be used.

When you install the ruby-vips gem on Windows, it would automatically (and unconditionally) install the latest available libvips dependency to your msys2 ruby devkit environment (see here in ruby-vips.gemspec).

There is the %RUBY_DLL_PATH% environment variable which you can point to an external libvips installation (e.g. obtained from libvips/build-win64-mxe releases), but this only works when there is no libvips installation present in the msys2 ruby devkit environment.

So in order to make use of %RUBY_DLL_PATH% to get control of the libvips version being installed, you need to make sure that there is no parallel libvips installation in the msys2 ruby devkit environment. But here's the catch: every time you re-install or update the ruby-vips gem, you will get an msys2 libvips installation as a side effect, breaking the use of the external libvips version in %RUBY_DLL_PATH%.

One solution to this could be to provide a possibility to opt-out of the installation of "msys2_mingw_dependencies" in the gemspec (e.g. via an env var like SKIP_MSYS2_MINGW_DEPENDENCIES=1 or so).

What do you think? Does that sound feasible / sensible? Do you have other suggestions on how to enable better control over the libvips library version?

Cheers, Torben

jcupitt commented 2 years ago

Hi again,

Yes, this is a tricky one.

Another possibility would be to add some code to ruby-vips here:

https://github.com/libvips/ruby-vips/blob/master/lib/vips.rb#L13-L34

It could check for an env var called VIPSHOME and prepend that to the library name, perhaps:

def library_name(name, abi_number)
  lib = ''

  if ENV.has_key? 'VIPSHOME'
    lib += "#{ENV['VIPSHOME']}/lib/"
  end

  if FFI::Platform.windows?
    lib += "lib#{name}-#{abi_number}.dll"
  elsif FFI::Platform.mac?
    lib += "#{name}.#{abi_number}"
  else
    lib += "#{name}.so.#{abi_number}"
  end

  lib
end

Though some platforms use lib64 or lib/x86_64-linux-gnu/ which is annoying.

tknerr commented 2 years ago

Hi @jcupitt, thanks for the quick reply. That sounds even better! 👍🏻

  1. because having a dedicated VIPSHOME env var makes it a first-class feature of "ruby-vips" (vs RUBY_DLL_PATH which is a feature of RubyInstaller which is quirky and not under our control, and also works for Windows only)
  2. because that would then work cross-platform (i.e. can be used on Linux, Windows, MacOS in the same way), doesn't it?

If I get (or rather guess) it right on how ffi works, by explicitly loading the libraries from VIPSHOME via ffi_lib if present, there is also no chance this might interfere with an additionally installed libvips in msys2 / mingw, right? (and opting out of that would not even be necessary, then)

jcupitt commented 2 years ago

I'm honestly not sure :( We'd need to do some testing. Perhaps opening libvips-42.dll in c:\libvips-8.12.4\bin would load dependencies like libopenslide.dll from the mingw area by mistake? That could cause horrible crashes.

Thinking about it some more, really the best thing would be to fix the pacman package. Maybe open a bug report here?

https://github.com/msys2/MINGW-packages/issues?q=is%3Aissue+is%3Aopen+libvips

kleisauke commented 2 years ago

It seems that RubyInstaller honors the --ignore-dependencies option of gem: https://github.com/oneclick/rubyinstaller2/blob/ee44ad03f22cedfccb368f33dc9081daa84d0d81/resources/files/operating_system.rb#L8

So, to avoid installing libvips from MSYS2, you can just do:

$ gem install ruby-vips ffi --ignore-dependencies

FWIW, I tried to debug that crash with GDB using the mingw-w64-ucrt-x86_64-gdb package, but without debug symbols it's difficult to debug that further.

tknerr commented 2 years ago

Hi @jcupitt , thanks for the link to the MINGW-packages repo, I didn't know where to report this 👍🏻 but yes, fixing the pacman package would help for #331 (the crash), but not for #332 (how to control libvips version) though...

For sure, I can support with testing the VIPSHOME approach sketched above, also checking if it accidentally picks up libraries from the wrong location or not. Is the snippet from above all there is to it, or are there more places that would need to be adapted? (e.g. does ruby-vips exclusively use the libraries, or does it expect the correct vips.exe on the PATH or so?). If you want, I can also create a PR for that to start with.

@kleisauke yes, thanks for the hint regarding --ignore-dependencies. I found that too while trying to trick around it, but usually we freeze + install our gem dependencies via bundler, and there this approach won't work :-/

jcupitt commented 2 years ago

Yes, that's the only thing you'd need to change (I think!). But messing with paths and library resolution is extremely fragile and we're likely to trigger a lot of difficult to debug crashes.

I was thinking about this again -- I now think your original idea was right and we should have an env var like RUBYVIPS_EXTERNAL_LIBVIPS to turn off msys2_mingw_dependencies.

Perhaps (untested):

    "msys2_mingw_dependencies" => ENV.key?("RUBYVIPS_EXTERNAL_LIBVIPS") ? "" : "libvips"

So to use an external libvips you'd need to set RUBYVIPS_EXTERNAL_LIBVIPS to stop installation of the msys one, and set RUBY_DLL_PATH to include your external libvips bin area.

Any thoughts?

jcupitt commented 2 years ago

I came across this interesting article from a few years ago:

https://bibwild.wordpress.com/2015/09/09/optional-gem-dependencies/

Which (I think) advocates removing optional dependencies and checking at runtime instead, perhaps with a helpful message.

@jrochkind sorry to ping you, but I wonder if you have any insights on this issue?

jrochkind commented 2 years ago

You can ping me anytime @jcupitt, I love vips and appreciate and am impressed by your work! (Although I am a rubyist and use vips, I don't currently use ruby-vips -- my code calls out to a shell calling command line vips currently!)

There is no way to "opt out" of something in a ruby gemspec at runtime -- any conditional logic in the gemspec will be applied at gem build/release time, once when jcupitts builds and releases the gem, not at individual installation runtime. So definitely don't try that, which I think is unfortuantely what you were settling on -- either something is in a gemspec or is not, there's no way to make it conditional, with the conditions applied to the individual installation. Having something in the gemspec dependent on an ENV variable wont' work -- it'll be calculated at gem build/release time, and then fixed in place.

I don't develop or run ruby on Windows though, so don't totally understand the windows-specific issues. Looking at the current ruby-vips gemspec... I see in gem metadata, the key/value "msys2_mingw_dependencies" => "libvips" -- I have never seen this before, from the discussion here it sounds like you can express windows OS-level dependencies in gem metadata, that are actually enforced at gem install time? I am not familiar with this feature at all! So it's hard for me to judge the pros/cons of different ways of doing things.

With ordinary ruby gemspec dependencies (in gemspec as add_runtime_dependency) -- which this is not -- there are definitely pro's and con's of making an optional dependency only checked at runtime. Mainly the downside is that you are no making available ordinary ruby dependency features to do things like automatically install all dependencies at installation time, or compute the latest version(s) of all dependencies that are mutually satisfactory to each other. But if including the dependency in the ordinary way causes problems for some users -- whose usage isn't going to require the dependency at all, but where having it causes them problems (generally either long installation time/sizes, or conflicts with their desired dependency tree) -- it's kind of the only option to avoid the universal dependency.

If you do have an "optional dependency" which is only enforced at runtime -- I think it's important for the runtime checking to be very robust and the error message very clear, including (when it comes to rubygems) expressing and reporting the range of versions of the optional dependency that would be acceptable. (Rails' own runtime optional checking is not as good as it could be here when it comes to reporting).

That's my thinking on ordinary ruby gemspec dependencies. But this msys2_mingw_dependency is not that -- it's a windows feature I didn't even realize existed! (In non-Windows OSs, ruby gems generally have no formal way to express non-ruby-gem OS-level dependencies at all, as far as I know!). So I'm not totally sure how it applies here.

jcupitt commented 2 years ago

Hi! Thanks for the feedback.

It sounds like the best thing is to remove msys2_mingw_dependencies (as you guessed, it's way for windows ruby users to install gems with binary components, which otherwise can be extremely difficult) and display the best error message we can instead.

Presumably a try / catch around the first ffi import, maybe here:

https://github.com/libvips/ruby-vips/blob/master/lib/vips.rb#L45

Does anyone fancy taking a stab at a prototype PR?

jcupitt commented 2 years ago

Although I am a rubyist and use vips, I don't currently use ruby-vips -- my code calls out to a shell calling command line vips currently!

Oh, interesting. Do you use rails? Does ActiveRecord not fit your use case well?

jrochkind commented 2 years ago

Oh, interesting. Do you use rails? Does ActiveRecord not fit your use case well?

I do use rails. I think maybe you're asking about "ActiveStorage"? It's true I don't use ActiveStorage (I do use ActiveRecord, but I don't think it's relevant here), I do my own file handling and transformation with shrine. I could still use the image_processing gem that ActiveStorage uses (which is actually by janko, the same author as shrine) -- but I don't. Partially because some of this code pre-dates ActiveStorage/ImageProcessing, partially because file handling is central to my app and I just want complete control, including of things that may or may not be available via ImageProcessing.

I definitely could still use ruby-vips but... I just don't? I just felt more comfortable not relying on anything but the command line, and not having to worry about any possible concurrency or ruby memory bugs. it's possible i'll switch to ruby-vips in the future but it's working fine.

jrochkind commented 2 years ago

which otherwise can be extremely difficult

I now worry your proposal would make things "extremely difficult" for the common case, for the benefit of making something more convenient that's an edge case.

But I don't use ruby on Windows, I don't fully understand the case here (as far as I know, on non-windows there isn't even a facility to formally express non-ruby dependencies like this, it's unique to windows!), so I'll stay out of any further discussion!

jcupitt commented 2 years ago

Ah! I had an idea!

How about removing the mingw dependency from ruby-vips, and making a tiny new gem called ruby-vips-windows (or maybe ruby-vips-mingw? something like that?) which depends on ruby-vips, and which has the mingw dependency.

Now windows users who want the mingw binary can put ruby-vips-mingw in their gemspec, and everyone else can use plain ruby-vips.

I suppose the downside is that the ruby-vips-mingw version numbers will need to exactly match the ruby-vips ones so people can still pin etc. Can we have two gems in one repo and generate the second programmatically?

jrochkind commented 2 years ago

@jcupitt

That works to do what you say, and works for people who are including ruby-vips in an app.

The problem is -- what if you are writing a gem, and want to declare ruby-vips as a dependency of that gem? You are writing a gem that works on any platform, so you aren't going to declare ruby-vips-windows as a dependency. Or wait, would you? I mean, it would work fine mostly just as it is now -- except for the problem at the top of this ticket, which would be introduced via an intermediate dependency. But a gem author probably wouldn't even notice the windows-specific gem, in which case...

Someone using this gem that has ordinary ruby-vips as a intermediate dependency, on windows... would not get the special windows dependency. Which might mean things wouldn't work for them, unless they knew how to take care of it (say, by adding the windows version of the dependency to their app gemfile manually, after only getting the platform-independent one as an indirect dependency? Or just by manually installing windows libvips, that's what the special windows dependency is doing right?).

Seems confusing to me. Whether it's worth the confusion I couldn't say, I still don't totally understand the problem case (or how these windows-only gem metadata dependencies work. I wonder if we can find anyone who is an expert in how these windows metadata dependencies work, to suggest alternate solutions?).

jcupitt commented 2 years ago

Hmmm yes that's true.

I was imagining downstream projects would continue to just have just ruby-vips as a dependency, and then at the final step anyone deploying on windows would add ruby-vips-mingw to their gemspec. But that's not really any better than asking people to use pacman to install libvips if they are running on windows.

I'm now thinking this is unfixable. You can't safely mix the official libvips binaries with a mingw install (DLLs could crash into each other). If you want to use ruby-vips on windows with the libvips binaries, you really need to manage the whole binary stack yourself.

Let's just fix the pacman libvips. It needs fixing anyway, and that'll resolve most of these immediate issues.

jcupitt commented 2 years ago

I meant to add:

jrochkind commented 2 years ago

Interesting, thanks for the background.

In other OS, with some gems you responsible for installing your own C or other binary components, usually with a package manager. The most you will get from the gem is a nice warning message telling you you need to install it if it isn't present (and more often a cryptic warning message). Or in some cases on other OSs with ruby, the gem will install the a binary library itself at install time (the message you see at gem install time when this happens is Building native extensions. This could take a while...).

So rubygems on Windows, though, unique to rubygems on Windows, has a mechanism to trigger automatic install of binary dependencies from an external package manager. By listing something in the gemspec metadata -- which I can't think of anything else in metadata that has an actual automatic effect on gem install or run, all the rest of them are either just description, or in some cases possibly have an effect on rubygems.org hosting at release-time.

I'm not sure what aspect of Windows makes this more desirable such that they've rigged up this windows-ony feature -- maybe that it's harder to supply your own binary library with the gem on Windows, as is sometimes done on other OSs? Or it's just harder to install binary libraries yourself on Windows in general, in a linkable way?

Some other gems use OS-provided binaries instead; if the latest standard package-manager-supplied binary library on another OS had problems with it (which seems to be the problem here), that would probably cause problems in that non-Windows situation too. They rely on the the package-manager-supplied binary library building -- or if it doesn't they have the option of supplying/compiling the binary library themselves at gem install time, which is an option not present or less feasible on Windows.

jrochkind commented 2 years ago

Sorry for so many words, I'm just procesing, you don't have to read all this if it's not helpful!

If your problem is that the pacman-managed libvips has problems with it, so you want to let people manually install a libvips... what if you just remove the "msys2_mingw_dependencies" from the gemspec, so manually installing libvips is what a Windows user has to to do?

The alternative (other than fixing the pacman one) is that naive users get a Windows ruby-vips install that initially appears to work, but has bugs/raises in some paths, but if they care/know, they can use a special install process to install their own libvips instead. That's the thing we're having trouble figuring out a good way to do, but... is that actually desirable?

There are occasional ruby gems that on unix/MacOS just make you install the binary dependency yourself, although it does make users grumble.

Then we're back to your second-gem idea.... as an alternative to just making the user install libvips themselves, they can optionally just drop in ruby-vips-windows to do it... this makes more sense to me now that I understand the situation. You could put a note in the ruby-vips-windows saying this should NEVER be used as an intermediate dependency in a gem (at least not a multi-platform one), it should only be dropped into apps to automatically install libvips via "pacman" if you want... and there could also be a note there saying if you choose this option (the ruby-vips-windows gem, that will automatically install pacman libvips for you)... as of this writing you're going to get a buggy libvips which will cause errors to raise in some cases with ruby-vips-windows... I guess if you're doing this, you think it's because some users would want that option?

jcupitt commented 2 years ago

... I made a PR to update the libvips version: https://github.com/msys2/MINGW-packages/pull/11078

It seems to build OK, but I've not tested to see if it fixes the bug you were seeing, I wasn't sure how to make a ucrt package.

tknerr commented 2 years ago

Hey @jcupitt , @jrochkind ,

first of all, sorry for my slow response time this week, and dropping back in to the conversation only that late again...

Re-reading the whole discussion I think there were many valuable aspects and new insights that have been brought up, thanks for that 👍🏻

@jcupitt special thanks for putting up the PR for pacman (I think I would have managed to increase the pkgver version, but would probably have missed all the new dependencies... thanks!!)

Regarding this issue: I still think the problem that is still unsolved is that there is currently no way to control which version of the underlying libvips library is getting installed. That's can also happen on Linux, e.g. when you install libvips via apt package manager and where the apt repositories may not retain older versions forever.

That's why I especially liked the $VIPSHOME env var approach you sketched above, as this would provide a cross-platform possibility for providing your own version-pinned libvips libraries. I still see the potential issues with accidentally loading a DLLs from the wrong location though, and that would definitely need more testing (if that's even approach you would be willing to follow)... at least it would be backwards-compatible, i.e. only people who explicitly opt-in for the specific use case of using a custom libvips by setting the VIPSHOME env var would be affected.

Considering the suggestion of publishing two variants of the ruby-vips gem, that would solve our issue as well, at least on windows where you can then provide your own version-pinned libvips library via RUBY_DLL_PATH (which is a feature that ships with rubyinstaller and is thus windows only), and might even work on Linux when you set LD_LIBRARY_PATH (haven't checked that though). If that is an approach you would want to follow, then we could also keep ruby-vips as-is (to ensure backwards compatibility!) and co-publish an additional ruby-vips-no-ming-dependencies or similar which would be the same except for the "msys2_mingw_dependencies" being excluded from the gemspec.

Imho backwards-compatibility for the existing user base is super important, otherwise this will end up in confusion and additional support efforts. I think both approaches could be followed in a backwards-compatible way though.

What do you think?

jcupitt commented 2 years ago

I think my current thinking is that we can't safely mix binaries between the libvips win builds we make and the win builds made by the msys team. Many of the DLLs will clash and it's very likely to cause mysterious crashes. I'm often wrong though, so if I'm mixed up there, please do correct me!

This means there's no safe way for people who use rubyinstaller to patch in a different libvips binary themselves. They must go via pacman and the msys toolchain. It's not actually that hard: you just make some edits to the PKGBUILD file and rerun it to make a new .pkg which you can drop in with pacman.

So I think that's probably the solution. You can stick with the libvips that ruby gives you automatically, or if you're determined, you can make your own package.

Hopefully the updated libvips will fix the original crash too, though I've not been able to test it yet.

jrochkind commented 2 years ago

I just want to emphasize that im pretty sure you can't put a conditional in the gemspec on an env var that is different for different users. If you want to try something like that please test it thoroughly to confirm.