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/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 e1cbb3122a..e86fa8f6c7 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 @@ -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 @@ -41,11 +44,11 @@ module Hbc @executable = executable @args = args @sudo = sudo - @input = input + @input = [*input] @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 @@ -84,7 +87,9 @@ 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, + status: processed_status, + output: @merged_output) end def expanded_args @@ -113,7 +118,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) 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/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/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 0ed3554501..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.shift, args)) + return if 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..0b37cbaa71 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,24 @@ 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:, output: nil) + s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}." + + unless [*output].empty? + format_output_line = lambda do |type, line| + if type == :stderr + Formatter.error(line) + else + line + end + end + + s << " Here's the output:\n" + s << output.map(&format_output_line).join + s << "\n" unless s.end_with?("\n") + end + + super s 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 diff --git a/Library/Homebrew/extend/os/linux/keg_relocate.rb b/Library/Homebrew/extend/os/linux/keg_relocate.rb index cdaa2de973..568e707d16 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, status: $CHILD_STATUS, output: [:stdout, old_rpath]) + 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/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 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..6e04ea27b8 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) @@ -296,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)) + return if 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