From bb291500968f04f4edacc99f0f8290fbe9599f4b Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 16 Jul 2018 22:46:02 +0200 Subject: [PATCH 1/7] Refactor `HashValidator`. --- Library/Homebrew/cask/lib/hbc/artifact/artifact.rb | 5 +++-- Library/Homebrew/cask/lib/hbc/artifact/installer.rb | 5 ++++- Library/Homebrew/cask/lib/hbc/artifact/pkg.rb | 11 +++++------ Library/Homebrew/cask/lib/hbc/artifact/relocated.rb | 5 +++-- Library/Homebrew/cask/lib/hbc/system_command.rb | 6 +++--- Library/Homebrew/cask/lib/hbc/utils/hash_validator.rb | 7 ------- Library/Homebrew/extend/hash_validator.rb | 9 +++++++++ 7 files changed, 27 insertions(+), 21 deletions(-) delete mode 100644 Library/Homebrew/cask/lib/hbc/utils/hash_validator.rb create mode 100644 Library/Homebrew/extend/hash_validator.rb diff --git a/Library/Homebrew/cask/lib/hbc/artifact/artifact.rb b/Library/Homebrew/cask/lib/hbc/artifact/artifact.rb index b7df4b0bd0..f61e997019 100644 --- a/Library/Homebrew/cask/lib/hbc/artifact/artifact.rb +++ b/Library/Homebrew/cask/lib/hbc/artifact/artifact.rb @@ -1,6 +1,7 @@ require "hbc/artifact/moved" -require "hbc/utils/hash_validator" +require "extend/hash_validator" +using HashValidator module Hbc module Artifact @@ -20,7 +21,7 @@ module Hbc raise CaskInvalidError.new(cask.token, "target required for #{english_name} '#{source_string}'") end - target_hash.extend(HashValidator).assert_valid_keys(:target) + target_hash.assert_valid_keys!(:target) new(cask, source_string, **target_hash) end diff --git a/Library/Homebrew/cask/lib/hbc/artifact/installer.rb b/Library/Homebrew/cask/lib/hbc/artifact/installer.rb index a3a84164e0..269e122f54 100644 --- a/Library/Homebrew/cask/lib/hbc/artifact/installer.rb +++ b/Library/Homebrew/cask/lib/hbc/artifact/installer.rb @@ -1,5 +1,8 @@ require "hbc/artifact/abstract_artifact" +require "extend/hash_validator" +using HashValidator + module Hbc module Artifact class Installer < AbstractArtifact @@ -60,7 +63,7 @@ module Hbc raise CaskInvalidError.new(cask, "invalid 'installer' stanza: Only one of #{VALID_KEYS.inspect} is permitted.") end - args.extend(HashValidator).assert_valid_keys(*VALID_KEYS) + args.assert_valid_keys!(*VALID_KEYS) new(cask, **args) end diff --git a/Library/Homebrew/cask/lib/hbc/artifact/pkg.rb b/Library/Homebrew/cask/lib/hbc/artifact/pkg.rb index 0fc9eac2bb..81b6e4d116 100644 --- a/Library/Homebrew/cask/lib/hbc/artifact/pkg.rb +++ b/Library/Homebrew/cask/lib/hbc/artifact/pkg.rb @@ -1,8 +1,9 @@ +require "vendor/plist/plist" + require "hbc/artifact/abstract_artifact" -require "hbc/utils/hash_validator" - -require "vendor/plist/plist" +require "extend/hash_validator" +using HashValidator module Hbc module Artifact @@ -10,9 +11,7 @@ module Hbc attr_reader :pkg_relative_path def self.from_args(cask, path, **stanza_options) - stanza_options.extend(HashValidator).assert_valid_keys( - :allow_untrusted, :choices - ) + stanza_options.assert_valid_keys!(:allow_untrusted, :choices) new(cask, path, **stanza_options) end diff --git a/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb b/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb index 9195d889a7..b9d897983b 100644 --- a/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb +++ b/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb @@ -1,6 +1,7 @@ require "hbc/artifact/abstract_artifact" -require "hbc/utils/hash_validator" +require "extend/hash_validator" +using HashValidator module Hbc module Artifact @@ -10,7 +11,7 @@ module Hbc if target_hash raise CaskInvalidError unless target_hash.respond_to?(:keys) - target_hash.extend(HashValidator).assert_valid_keys(:target) + target_hash.assert_valid_keys!(:target) end target_hash ||= {} diff --git a/Library/Homebrew/cask/lib/hbc/system_command.rb b/Library/Homebrew/cask/lib/hbc/system_command.rb index e1cbb3122a..a443e8d38b 100644 --- a/Library/Homebrew/cask/lib/hbc/system_command.rb +++ b/Library/Homebrew/cask/lib/hbc/system_command.rb @@ -3,8 +3,8 @@ require "vendor/plist/plist" require "shellwords" require "extend/io" - -require "hbc/utils/hash_validator" +require "extend/hash_validator" +using HashValidator module Hbc class SystemCommand @@ -45,7 +45,7 @@ module Hbc @print_stdout = print_stdout @print_stderr = print_stderr @must_succeed = must_succeed - options.extend(HashValidator).assert_valid_keys(:chdir) + options.assert_valid_keys!(:chdir) @options = options @env = env diff --git a/Library/Homebrew/cask/lib/hbc/utils/hash_validator.rb b/Library/Homebrew/cask/lib/hbc/utils/hash_validator.rb deleted file mode 100644 index f43bf173f4..0000000000 --- a/Library/Homebrew/cask/lib/hbc/utils/hash_validator.rb +++ /dev/null @@ -1,7 +0,0 @@ -module HashValidator - def assert_valid_keys(*valid_keys) - unknown_keys = keys - valid_keys - return if unknown_keys.empty? - raise %Q(Unknown keys: #{unknown_keys.inspect}. Running "brew update" will likely fix it.) - end -end diff --git a/Library/Homebrew/extend/hash_validator.rb b/Library/Homebrew/extend/hash_validator.rb new file mode 100644 index 0000000000..c5a046f9b6 --- /dev/null +++ b/Library/Homebrew/extend/hash_validator.rb @@ -0,0 +1,9 @@ +module HashValidator + refine Hash do + def assert_valid_keys!(*valid_keys) + unknown_keys = keys - valid_keys + return if unknown_keys.empty? + raise ArgumentError, "invalid keys: #{unknown_keys.map(&:inspect).join(", ")}" + end + end +end From 2452b27866567da1c8a3767e6bfd462b90c0b821 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 16 Jul 2018 23:17:16 +0200 Subject: [PATCH 2/7] Refactor `ErrorDuringExecution`. --- Library/Homebrew/cask/lib/hbc/exceptions.rb | 25 ------------------- .../Homebrew/cask/lib/hbc/system_command.rb | 5 +++- Library/Homebrew/download_strategy.rb | 2 +- Library/Homebrew/exceptions.rb | 21 +++++++++++++--- .../Homebrew/extend/os/linux/keg_relocate.rb | 4 ++- Library/Homebrew/os/linux/elf.rb | 9 +++---- Library/Homebrew/patch.rb | 3 +-- .../Homebrew/test/cask/system_command_spec.rb | 2 +- Library/Homebrew/test/exceptions_spec.rb | 5 ++-- Library/Homebrew/utils.rb | 2 +- Library/Homebrew/utils/popen.rb | 6 ++--- 11 files changed, 38 insertions(+), 46 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/exceptions.rb b/Library/Homebrew/cask/lib/hbc/exceptions.rb index 392e34f982..243dc458dd 100644 --- a/Library/Homebrew/cask/lib/hbc/exceptions.rb +++ b/Library/Homebrew/cask/lib/hbc/exceptions.rb @@ -53,31 +53,6 @@ module Hbc end end - class CaskCommandFailedError < CaskError - def initialize(cmd, stdout, stderr, status) - @cmd = cmd - @stdout = stdout - @stderr = stderr - @status = status - end - - def to_s - s = "Command failed to execute!\n" - s.concat("\n") - s.concat("==> Failed command:\n") - s.concat(@cmd.join(" ")).concat("\n") - s.concat("\n") - s.concat("==> Standard Output of failed command:\n") - s.concat(@stdout).concat("\n") - s.concat("\n") - s.concat("==> Standard Error of failed command:\n") - s.concat(@stderr).concat("\n") - s.concat("\n") - s.concat("==> Exit status of failed command:\n") - s.concat(@status.inspect).concat("\n") - end - end - class CaskX11DependencyError < AbstractCaskErrorWithToken def to_s <<~EOS diff --git a/Library/Homebrew/cask/lib/hbc/system_command.rb b/Library/Homebrew/cask/lib/hbc/system_command.rb index a443e8d38b..a46ce662f9 100644 --- a/Library/Homebrew/cask/lib/hbc/system_command.rb +++ b/Library/Homebrew/cask/lib/hbc/system_command.rb @@ -84,7 +84,10 @@ module Hbc def assert_success return if processed_status&.success? - raise CaskCommandFailedError.new(command, processed_output[:stdout], processed_output[:stderr], processed_status) + raise ErrorDuringExecution.new(command, + stdout: processed_output[:stdout], + stderr: processed_output[:stderr], + status: processed_status) end def expanded_args diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 0ed3554501..9e4dbc57bb 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -69,7 +69,7 @@ class AbstractDownloadStrategy def safe_system(*args) if @shutup - quiet_system(*args) || raise(ErrorDuringExecution.new(args.shift, args)) + quiet_system(*args) || raise(ErrorDuringExecution.new(args, status: $CHILD_STATUS)) else super(*args) end diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index 63dc29b54f..8db68ba978 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -1,3 +1,5 @@ +require "shellwords" + class UsageError < RuntimeError attr_reader :reason @@ -524,9 +526,22 @@ end # raised by safe_system in utils.rb class ErrorDuringExecution < RuntimeError - def initialize(cmd, args = []) - args = args.map { |a| a.to_s.gsub " ", "\\ " }.join(" ") - super "Failure while executing: #{cmd} #{args}" + def initialize(cmd, status:, stdout: nil, stderr: nil) + s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}." + + if stdout + s << "==> Standard Output of failed command:\n" + s << stdout + s << "\n" unless stdout.end_with?("\n") + end + + if stderr + s << "==> Standard Error of failed command:\n" + s << stderr + s << "\n" unless stderr.end_with?("\n") + end + + super s end end diff --git a/Library/Homebrew/extend/os/linux/keg_relocate.rb b/Library/Homebrew/extend/os/linux/keg_relocate.rb index cdaa2de973..8219d73c3e 100644 --- a/Library/Homebrew/extend/os/linux/keg_relocate.rb +++ b/Library/Homebrew/extend/os/linux/keg_relocate.rb @@ -20,7 +20,9 @@ class Keg # patchelf requires that the ELF file have a .dynstr section. # Skip ELF files that do not have a .dynstr section. return if ["cannot find section .dynstr", "strange: no string table"].include?(old_rpath) - raise ErrorDuringExecution, "#{cmd_rpath}\n#{old_rpath}" unless $CHILD_STATUS.success? + unless $CHILD_STATUS.success? + raise ErrorDuringExecution.new(cmd_rpath, stdout: old_rpath, status: $CHILD_STATUS) + end rpath = old_rpath .split(":") diff --git a/Library/Homebrew/os/linux/elf.rb b/Library/Homebrew/os/linux/elf.rb index 1a53e50f3e..3865ef644c 100644 --- a/Library/Homebrew/os/linux/elf.rb +++ b/Library/Homebrew/os/linux/elf.rb @@ -118,12 +118,10 @@ module ELFShim patchelf = DevelopmentTools.locate "patchelf" if path.dylib? command = [patchelf, "--print-soname", path.expand_path.to_s] - soname = Utils.popen_read(*command).chomp - raise ErrorDuringExecution, command unless $CHILD_STATUS.success? + soname = Utils.safe_popen_read(*command).chomp end command = [patchelf, "--print-needed", path.expand_path.to_s] - needed = Utils.popen_read(*command).split("\n") - raise ErrorDuringExecution, command unless $CHILD_STATUS.success? + needed = Utils.safe_popen_read(*command).split("\n") [soname, needed] end @@ -131,8 +129,7 @@ module ELFShim soname = nil needed = [] command = ["readelf", "-d", path.expand_path.to_s] - lines = Utils.popen_read(*command).split("\n") - raise ErrorDuringExecution, command unless $CHILD_STATUS.success? + lines = Utils.safe_popen_read(*command).split("\n") lines.each do |s| filename = s[/\[(.*)\]/, 1] next if filename.nil? diff --git a/Library/Homebrew/patch.rb b/Library/Homebrew/patch.rb index 9150be25e7..9d07e21c35 100644 --- a/Library/Homebrew/patch.rb +++ b/Library/Homebrew/patch.rb @@ -67,8 +67,7 @@ class EmbeddedPatch def apply data = contents.gsub("HOMEBREW_PREFIX", HOMEBREW_PREFIX) args = %W[-g 0 -f -#{strip}] - Utils.popen_write("patch", *args) { |p| p.write(data) } - raise ErrorDuringExecution.new("patch", args) unless $CHILD_STATUS.success? + Utils.safe_popen_write("patch", *args) { |p| p.write(data) } end def inspect diff --git a/Library/Homebrew/test/cask/system_command_spec.rb b/Library/Homebrew/test/cask/system_command_spec.rb index f08193e652..b589e979ec 100644 --- a/Library/Homebrew/test/cask/system_command_spec.rb +++ b/Library/Homebrew/test/cask/system_command_spec.rb @@ -69,7 +69,7 @@ describe Hbc::SystemCommand, :cask do it "throws an error" do expect { described_class.run!(command) - }.to raise_error(Hbc::CaskCommandFailedError) + }.to raise_error(ErrorDuringExecution) end end diff --git a/Library/Homebrew/test/exceptions_spec.rb b/Library/Homebrew/test/exceptions_spec.rb index 1a09693a22..0b87c0ef0a 100644 --- a/Library/Homebrew/test/exceptions_spec.rb +++ b/Library/Homebrew/test/exceptions_spec.rb @@ -183,9 +183,10 @@ describe CurlDownloadStrategyError do end describe ErrorDuringExecution do - subject { described_class.new("badprg", %w[arg1 arg2]) } + subject { described_class.new(["badprg", "arg1", "arg2"], status: status) } + let(:status) { instance_double(Process::Status, exitstatus: 17) } - its(:to_s) { is_expected.to eq("Failure while executing: badprg arg1 arg2") } + its(:to_s) { is_expected.to eq("Failure while executing; `badprg arg1 arg2` exited with 17.") } end describe ChecksumMismatchError do diff --git a/Library/Homebrew/utils.rb b/Library/Homebrew/utils.rb index 7421fb5c73..dc9c33dd3c 100644 --- a/Library/Homebrew/utils.rb +++ b/Library/Homebrew/utils.rb @@ -296,7 +296,7 @@ end # Kernel.system but with exceptions def safe_system(cmd, *args, **options) - Homebrew.system(cmd, *args, **options) || raise(ErrorDuringExecution.new(cmd, args)) + Homebrew.system(cmd, *args, **options) || raise(ErrorDuringExecution.new([cmd, *args], status: $CHILD_STATUS)) end # prints no output diff --git a/Library/Homebrew/utils/popen.rb b/Library/Homebrew/utils/popen.rb index dc7a1c2b15..6a7addcd5d 100644 --- a/Library/Homebrew/utils/popen.rb +++ b/Library/Homebrew/utils/popen.rb @@ -5,7 +5,7 @@ module Utils def self.safe_popen_read(*args, **options, &block) output = popen_read(*args, **options, &block) - raise ErrorDuringExecution, args unless $CHILD_STATUS.success? + raise ErrorDuringExecution(args, stdout: output, status: $CHILD_STATUS) unless $CHILD_STATUS.success? output end @@ -14,8 +14,8 @@ module Utils end def self.safe_popen_write(*args, **options, &block) - output = popen_write(args, **options, &block) - raise ErrorDuringExecution, args unless $CHILD_STATUS.success? + output = popen_write(*args, **options, &block) + raise ErrorDuringExecution(args, stdout: output, status: $CHILD_STATUS) unless $CHILD_STATUS.success? output end From 01b2be755c99f6bab6e773fc7f3e3186b5505cb5 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Tue, 17 Jul 2018 00:30:34 +0200 Subject: [PATCH 3/7] Move `odebug`. --- Library/Homebrew/cask/lib/hbc/utils.rb | 8 -------- Library/Homebrew/utils.rb | 6 ++++++ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/utils.rb b/Library/Homebrew/cask/lib/hbc/utils.rb index 00bc2a4b11..275165b646 100644 --- a/Library/Homebrew/cask/lib/hbc/utils.rb +++ b/Library/Homebrew/cask/lib/hbc/utils.rb @@ -4,14 +4,6 @@ require "stringio" BUG_REPORTS_URL = "https://github.com/Homebrew/homebrew-cask#reporting-bugs".freeze -# global methods - -def odebug(title, *sput) - return unless ARGV.debug? - puts Formatter.headline(title, color: :magenta) - puts sput unless sput.empty? -end - module Hbc module Utils def self.gain_permissions_remove(path, command: SystemCommand) diff --git a/Library/Homebrew/utils.rb b/Library/Homebrew/utils.rb index dc9c33dd3c..bea9b75aa7 100644 --- a/Library/Homebrew/utils.rb +++ b/Library/Homebrew/utils.rb @@ -28,6 +28,12 @@ def ohai(title, *sput) puts sput end +def odebug(title, *sput) + return unless ARGV.debug? + puts Formatter.headline(title, color: :magenta) + puts sput unless sput.empty? +end + def oh1(title, options = {}) if $stdout.tty? && !ARGV.verbose? && options.fetch(:truncate, :auto) == :auto title = Tty.truncate(title) From 3ff9c5335d8c397c2257a4e59c49ad095074b73e Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Tue, 17 Jul 2018 00:31:19 +0200 Subject: [PATCH 4/7] Canonicalize `input` in `initialize`. --- Library/Homebrew/cask/lib/hbc/system_command.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/system_command.rb b/Library/Homebrew/cask/lib/hbc/system_command.rb index a46ce662f9..0bbbd4275c 100644 --- a/Library/Homebrew/cask/lib/hbc/system_command.rb +++ b/Library/Homebrew/cask/lib/hbc/system_command.rb @@ -41,7 +41,7 @@ module Hbc @executable = executable @args = args @sudo = sudo - @input = input + @input = [*input] @print_stdout = print_stdout @print_stderr = print_stderr @must_succeed = must_succeed @@ -116,7 +116,7 @@ module Hbc end def write_input_to(raw_stdin) - [*input].each(&raw_stdin.method(:print)) + input.each(&raw_stdin.method(:write)) end def each_line_from(sources) From 2712fcaa6728a5bfbf9a2113bdfd93b52370615b Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Thu, 19 Jul 2018 13:31:40 +0200 Subject: [PATCH 5/7] Use interleaved output for `ErrorDuringExecution`. --- .../Homebrew/cask/lib/hbc/system_command.rb | 8 ++++--- Library/Homebrew/exceptions.rb | 22 ++++++++++--------- .../Homebrew/extend/os/linux/keg_relocate.rb | 2 +- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/system_command.rb b/Library/Homebrew/cask/lib/hbc/system_command.rb index 0bbbd4275c..e86fa8f6c7 100644 --- a/Library/Homebrew/cask/lib/hbc/system_command.rb +++ b/Library/Homebrew/cask/lib/hbc/system_command.rb @@ -19,6 +19,7 @@ module Hbc end def run! + @merged_output = [] @processed_output = { stdout: "", stderr: "" } odebug command.shelljoin @@ -27,9 +28,11 @@ module Hbc when :stdout puts line.chomp if print_stdout? processed_output[:stdout] << line + @merged_output << [:stdout, line] when :stderr $stderr.puts Formatter.error(line.chomp) if print_stderr? processed_output[:stderr] << line + @merged_output << [:stderr, line] end end @@ -85,9 +88,8 @@ module Hbc def assert_success return if processed_status&.success? raise ErrorDuringExecution.new(command, - stdout: processed_output[:stdout], - stderr: processed_output[:stderr], - status: processed_status) + status: processed_status, + output: @merged_output) end def expanded_args diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index 8db68ba978..0b37cbaa71 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -526,19 +526,21 @@ end # raised by safe_system in utils.rb class ErrorDuringExecution < RuntimeError - def initialize(cmd, status:, stdout: nil, stderr: nil) + def initialize(cmd, status:, output: nil) s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}." - if stdout - s << "==> Standard Output of failed command:\n" - s << stdout - s << "\n" unless stdout.end_with?("\n") - end + unless [*output].empty? + format_output_line = lambda do |type, line| + if type == :stderr + Formatter.error(line) + else + line + end + end - if stderr - s << "==> Standard Error of failed command:\n" - s << stderr - s << "\n" unless stderr.end_with?("\n") + s << " Here's the output:\n" + s << output.map(&format_output_line).join + s << "\n" unless s.end_with?("\n") end super s diff --git a/Library/Homebrew/extend/os/linux/keg_relocate.rb b/Library/Homebrew/extend/os/linux/keg_relocate.rb index 8219d73c3e..568e707d16 100644 --- a/Library/Homebrew/extend/os/linux/keg_relocate.rb +++ b/Library/Homebrew/extend/os/linux/keg_relocate.rb @@ -21,7 +21,7 @@ class Keg # Skip ELF files that do not have a .dynstr section. return if ["cannot find section .dynstr", "strange: no string table"].include?(old_rpath) unless $CHILD_STATUS.success? - raise ErrorDuringExecution.new(cmd_rpath, stdout: old_rpath, status: $CHILD_STATUS) + raise ErrorDuringExecution.new(cmd_rpath, status: $CHILD_STATUS, output: [:stdout, old_rpath]) end rpath = old_rpath From fec884d8dac727e4537b2e2f1fae2b5569855b66 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Thu, 19 Jul 2018 13:31:48 +0200 Subject: [PATCH 6/7] Add tests for `ErrorDuringExecution`. --- .../test/error_during_execution_spec.rb | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 Library/Homebrew/test/error_during_execution_spec.rb diff --git a/Library/Homebrew/test/error_during_execution_spec.rb b/Library/Homebrew/test/error_during_execution_spec.rb new file mode 100644 index 0000000000..4dd6d1ec3f --- /dev/null +++ b/Library/Homebrew/test/error_during_execution_spec.rb @@ -0,0 +1,63 @@ +describe ErrorDuringExecution do + subject(:error) { described_class.new(command, status: status, output: output) } + let(:command) { ["false"] } + let(:status) { instance_double(Process::Status, exitstatus: exitstatus) } + let(:exitstatus) { 1 } + let(:output) { nil } + + describe "#initialize" do + it "fails when only given a command" do + expect { + described_class.new(command) + }.to raise_error(ArgumentError) + end + + it "fails when only given a status" do + expect { + described_class.new(status: status) + }.to raise_error(ArgumentError) + end + + it "does not raise an error when given both a command and a status" do + expect { + described_class.new(command, status: status) + }.not_to raise_error + end + end + + describe "#to_s" do + context "when only given a command and a status" do + its(:to_s) { is_expected.to eq "Failure while executing; `false` exited with 1." } + end + + context "when additionally given the output" do + let(:output) { + [ + [:stdout, "This still worked.\n"], + [:stderr, "Here something went wrong.\n"], + ] + } + + before do + allow($stdout).to receive(:tty?).and_return(true) + end + + its(:to_s) { + is_expected.to eq <<~EOS + Failure while executing; `false` exited with 1. Here's the output: + This still worked. + #{Formatter.error("Here something went wrong.\n")} + EOS + } + end + + context "when command arguments contain special characters" do + let(:command) { ["env", "PATH=/bin", "cat", "with spaces"] } + + its(:to_s) { + is_expected + .to eq 'Failure while executing; `env PATH=/bin cat with\ spaces` exited with 1.' + } + end + end +end From ad7be58655e260adda6a9e4d33365148d22463db Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Thu, 19 Jul 2018 13:36:27 +0200 Subject: [PATCH 7/7] Use guard clause instead of `||`. --- Library/Homebrew/download_strategy.rb | 3 ++- Library/Homebrew/utils.rb | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 9e4dbc57bb..ddd38f3d79 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -69,7 +69,8 @@ class AbstractDownloadStrategy def safe_system(*args) if @shutup - quiet_system(*args) || raise(ErrorDuringExecution.new(args, status: $CHILD_STATUS)) + return if quiet_system(*args) + raise(ErrorDuringExecution.new(args, status: $CHILD_STATUS)) else super(*args) end diff --git a/Library/Homebrew/utils.rb b/Library/Homebrew/utils.rb index bea9b75aa7..6e04ea27b8 100644 --- a/Library/Homebrew/utils.rb +++ b/Library/Homebrew/utils.rb @@ -302,7 +302,8 @@ end # Kernel.system but with exceptions def safe_system(cmd, *args, **options) - Homebrew.system(cmd, *args, **options) || raise(ErrorDuringExecution.new([cmd, *args], status: $CHILD_STATUS)) + return if Homebrew.system(cmd, *args, **options) + raise(ErrorDuringExecution.new([cmd, *args], status: $CHILD_STATUS)) end # prints no output