sup-heliotrope / sup

A curses threads-with-tags style email client (mailing list: supmua@googlegroups.com)
http://sup-heliotrope.github.io
GNU General Public License v2.0
889 stars 96 forks source link

mkrf_conf_xapian.rb: Fallback to Gem.user_dir #576

Closed IPv2 closed 4 years ago

IPv2 commented 4 years ago

ext/mkrf_conf_xapian.rb didn't check writability of the requested gem installation directory before attempting to install the xapian-ruby gem.

In typical default configurations, this would cause installation of the sup gem to fail for non-root users (because of no write permission by non-root users to the default system-wide gem directory).

Examples of gem installation workarounds that were being used due to this:

In this commit, we modify ext/mkrf_conf_xapian.rb to:

  1. Check that the requested gem installation directory is writable. If not, then fallback to installing xapian-ruby into the user gem directory (Gem.user_dir) instead.

  2. Improve error handling: output a useful error message to STDERR if the attempted installation of xapian-ruby fails for any reason.

danc86 commented 4 years ago

I don't really like that we have this weird fake extension and that it tries to implicitly install stuff using gem install... in the Fedora package this is just patched out because xapian bindings are not installed as a gem.

But anyway, I get that this is just providing a more convenient installation method for people who aren't getting xapian from their distro packages so this seems like a nice improvement.

IPv2 commented 4 years ago

That's an interesting approach in Fedora - and I wonder if there is a possible further improvement we could make.

In Debian-based distributions, the packaged version of Xapian (Debian package ruby-xapian) isn't installed as a gem.

This means that in mkrf_conf_xapian.rb, the existing check doesn't detect that the distro package is installed - as it is explicitly looking for a gem:

# try to load gem
gem name, version

Perhaps in a future PR, we should modify this check to instead (say) try to require "xapian" - which would detect the existing installation.