From 2452b27866567da1c8a3767e6bfd462b90c0b821 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 16 Jul 2018 23:17:16 +0200 Subject: [PATCH] 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