In SystemCommand, fix success? and exit_status

[claui]

For the upcoming brew cask search by name, I’m going to have to invoke a Cask::SystemCommand with the option :must_succeed => false and then look at the exit status.

Like this:

result = Cask::SystemCommand.run(command, :must_succeed => false)
if (result.exit_status >= 2)
  # …
else
  # …
end

The issue is that the SystemCommand::Result#exit_status method returns the whole Process::Status object instead of a Fixnum.

As a workaround, I could have done result.exit_status.exitstatus but I think this would be very ugly.

Cause

When invoking a SystemCommand with the option :must_succeed => false, the code in SystemCommand feeds a Process.Status object to SystemCommand::Result.new instead of the Fixnum it expects.

Impact

Apart from the usage in the upcoming PR, why didn’t this issue ever come up?

I think this is because:

1. Usually, everyone uses :must_succeed => true so there is no need to care about exit_status, and
2. In the rare usages of :must_succeed => false (only the Casks private-eye and soundflower do this), no one cares about exit_status.

Please review

Pinging @federicobond @ndr-qef @phinze @rolandwalker to ask if one of you could please have a look. Thanks! 😊

[claui]

Hmm. Travis says syntax_test fails because of system_command_test.
But on my machine, that test passes for all of 1.8.7, 1.9.3, and 2.0.0.

Could anyone please give me a hint what I did wrong?

[rolandwalker]

Good catch.

This probably never came up because Cask::SystemCommand::Result is very new (#6329). It was added for GPG support. The GpgCheck class was only merged a week ago (#6184) and as yet has no public interface.

:must_succeed => true has also been working fine because it only needs this == to work. Just plain == would be the same as @exit_status.to_i == 0, which works as expected. (Either the child processed exited with 0 or it did not.)

But an implicit to_i is not the right way to read various non-zero exit codes, as you found.

I'll look into the test failures shortly.

[claui]

I have finally managed to reproduce the failing test.

The problem seems to be that on my machine, syntax_test.rb thinks I only have Ruby 2.0 installed. So it decided to skip all the syntax tests for 1.8, 1.9, and 2.1.

When I run the tests using the -v option like this:

cd -P "$(brew --repository)"/Library/Taps/caskroom/homebrew-cask/lib/..
bundle exec rake test TEST=test/syntax_test.rb TESTOPTS="-v" | grep -A2 -B1 'system_command_test.*:$'

it says the tests have been skipped:

161) Skipped:
Syntax check::under Ruby 1.9#test_0170_/Users/claudia/Documents/dev/homebrew-cask-dev/test/cask/system_command_test.rb is valid Ruby [/Users/claudia/Documents/dev/homebrew-cask-dev/test/syntax_test.rb:17]:
Skipped, no message given

--
--
352) Skipped:
Syntax check::under Ruby 2.1#test_0170_/Users/claudia/Documents/dev/homebrew-cask-dev/test/cask/system_command_test.rb is valid Ruby [/Users/claudia/Documents/dev/homebrew-cask-dev/test/syntax_test.rb:17]:
Skipped, no message given

--
--
400) Skipped:
Syntax check::under Ruby 1.8#test_0170_/Users/claudia/Documents/dev/homebrew-cask-dev/test/cask/system_command_test.rb is valid Ruby [/Users/claudia/Documents/dev/homebrew-cask-dev/test/syntax_test.rb:17]:
Skipped, no message given

Turns out that while syntax_test.rb is in fact rbenv-aware, it looks for it in ~/.rbenv while my local rbenv installation lives in /usr/local/var/rbenv instead.

As a quick workaround, I just did:

ln -s $(rbenv root) ~/.rbenv

Now the 1.8 and 1.9 tests actually get executed:

182) Error:
Syntax check::under Ruby 1.9#test_0170_/Users/claudia/Documents/dev/homebrew-cask-dev/test/cask/system_command_test.rb is valid Ruby:
SyntaxError: /Users/claudia/Documents/dev/homebrew-cask-dev/test/cask/system_command_test.rb failed syntax check
    /Users/claudia/Documents/dev/homebrew-cask-dev/test/syntax_test.rb:25:in `test_0170_/Users/claudia/Documents/dev/homebrew-cask-dev/test/cask/system_command_test.rb is valid Ruby'
rake aborted!
Command failed with status (1): [ruby -I"lib:test" -I"/usr/local/var/rbenv/versions/1.8.7-p302/lib/ruby/gems/1.8/gems/rake-10.0.4/lib" "/usr/local/var/rbenv/versions/1.8.7-p302/lib/ruby/gems/1.8/gems/rake-10.0.4/lib/rake/rake_test_loader.rb" "test/syntax_test.rb" -v]

[claui]

… which in turn helped me see what my actual mistake was:

test/cask/system_command_test.rb:27: invalid multibyte char (US-ASCII)
test/cask/system_command_test.rb:27: invalid multibyte char (US-ASCII)
test/cask/system_command_test.rb:27: syntax error, unexpected $end, expecting ')'

Turns out that in one of the string literals, I used an apostrophe (’) which is a non-ASCII character. 🙈

I have now replaced it with a quotation mark. 😬

[rolandwalker]

Looks good.

I checked that $? returns a Process::Status instance under Ruby 1.8 (where raw_wait_thr will be nil) and thus $?.exitstatus is OK.

[claui]

Thanks for the review!