Fix regression where some CLI commands break on Ruby 1.8

[claui]

This fixes a regression where for example, brew cask list would break on Ruby 1.8.7.

Cause

I feel the regression might have been introduced in the merge commit 62c1ce5, which adds code to path_base.rb which depends on Gem::Version, but doesn’t require the rubygems gem.

A Google search showed that rubygems is built into 1.9 and later but not into 1.8 (I learn something new every day!). This seems to cause brew cask list (and other commands) to break under Ruby 1.8 since 62c1ce5.

Steps to reproduce

Assuming rbenv and Ruby 1.8.7-p375 are installed, follow these steps:

1. Make sure homebrew-cask is in development mode.
2. In case you don’t have 1.8.7-p375 installed, change the command below to the patch level you have.
3. Run brew cask list under Ruby 1.8 like so:

RBENV_VERSION=1.8.7-p375 HOMEBREW_BREW_FILE=/usr/local/bin/brew ruby /usr/local/Library/brew.rb /usr/local/bin/brew-cask list

Result

Error: uninitialized constant Cask::Source::PathBase::Gem while loading '/usr/local/Library/Taps/caskroom/homebrew-cask/Casks/clarify.rb'
Please report this bug:
    https://github.com/caskroom/homebrew-cask/issues
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/source/path_base.rb:51:in `load'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask.rb:78:in `load'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/scopes.rb:87:in `installed'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/scopes.rb:81:in `map'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/scopes.rb:81:in `installed'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli/list.rb:52:in `list_installed'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli/list.rb:10:in `run'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli.rb:81:in `run_command'
/usr/local/Cellar/brew-cask/0.48.0/rubylib/cask/cli.rb:121:in `process'
/usr/local/bin/brew-cask.rb:42
/usr/local/Library/brew.rb:59:in `require'
/usr/local/Library/brew.rb:59:in `require?'
/usr/local/Library/brew.rb:118

CLI::Alfred

• There appears to be a similar constellation in the CLI::Alfred class (since 0f664ca). That commit has been around since 0.41.1 so I guess it doesn’t cause any issues after all. I couldn’t produce any problem with this either but I added the missing require here as well just in case.

Test coverage

Given that brew cask list breaks, I would have expected list_test.rb to break as well. However, for some strange reason all the unit tests are still green:

cd "$(brew --repository)"/Library/Taps/caskroom/homebrew-cask/lib/..

RBENV_VERSION=1.8.7-p375 bundle exec rake test TEST=test/cask/cli/list_test.rb

Strangely, the result is:

...

Finished in 7.214952s, 0.4158 runs/s, 0.4158 assertions/s.

3 runs, 3 assertions, 0 failures, 0 errors, 0 skips

Please review

Pinging one of @federicobond @ndr-qef @phinze @rolandwalker to please have a look. 😊

Thank you!

[phinze]

First of all, hello and welcome @claui 👋 😀

And second - wow great work researching and documenting this issue!

However, for some strange reason all the unit tests are still green

I suspect this is the case because 'rubygems' ends up getting required somewhere in the mountain of setup that happens in test_helper.rb. I think the effort to arrange a test scenario that would cover probably outweighs the value it would provide, so I'm okay with this just being documented here and in the commit message.

As a nitpick - I generally lean towards a bit more verbosity in the commit message. There's great documentation work done here in the PR, but it would be more difficult for our Future Selves running git blame and reading the commit message for the require 'rubygems' line to answer the question "why does this fix it?". But like a say - just a nitpick. 🐒

So 👍 from me - happy to have you on the team!

[claui]

Thanks @phinze! 😄

By the way, I just found #7529 and #7530 where requiring (or not requiring) rubygems has already been discussed. Does this imply that the plan is to lose the dependency on rubygems altogether?

[rolandwalker]

Those PRs are just me not realizing that rubygems was being used for more than the transitional Tap migration code. It also turns out I need Gem::Version for #7696.

[claui]

Thank you @rolandwalker for clarifying this.
So would you prefer me to move that require back to the central location where it used to be?

[phinze]

Does this imply that the plan is to lose the dependency on rubygems altogether?

Yeah we will probably never drop the dependency on rubygems - we're mostly just focused on killing the dependency on homebrew code (see #5080).

FWIW I think keeping the requires closer to the Gem::Version usages is nicer, so I'm +1 for merge as-is. Will let @rolandwalker comment tho.

[rolandwalker]

+1 for merge as-is. I did the same in lib/cask/installer.rb (#7696), which merged while this was pending.