Merge pull request #4488 from reitermarkus/system-command

Refactor `Hbc::SystemCommand`.

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

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
 

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 "extend/hash_validator"
-
+using HashValidator
-require "vendor/plist/plist"
 
 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(
+        stanza_options.assert_valid_keys!(:allow_untrusted, :choices)
-          :allow_untrusted, :choices
-        )
         new(cask, path, **stanza_options)
       end
 

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 ||= {}

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

Library/Homebrew/cask/lib/hbc/system_command.rb

@@ -3,8 +3,8 @@ require "vendor/plist/plist"
 require "shellwords"
 
 require "extend/io"
-
+require "extend/hash_validator"
-require "hbc/utils/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)

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)

Library/Homebrew/cask/lib/hbc/utils/hash_validator.rb

@@ -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

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

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 = [])
+  def initialize(cmd, status:, output: nil)
-    args = args.map { |a| a.to_s.gsub " ", "\\ " }.join(" ")
+    s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}."
-    super "Failure while executing: #{cmd} #{args}"
+
+    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
 

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

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(":")

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
+        soname = Utils.safe_popen_read(*command).chomp
-        raise ErrorDuringExecution, command unless $CHILD_STATUS.success?
       end
       command = [patchelf, "--print-needed", path.expand_path.to_s]
-      needed = Utils.popen_read(*command).split("\n")
+      needed = Utils.safe_popen_read(*command).split("\n")
-      raise ErrorDuringExecution, command unless $CHILD_STATUS.success?
       [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")
+      lines = Utils.safe_popen_read(*command).split("\n")
-      raise ErrorDuringExecution, command unless $CHILD_STATUS.success?
       lines.each do |s|
         filename = s[/\[(.*)\]/, 1]
         next if filename.nil?

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) }
+    Utils.safe_popen_write("patch", *args) { |p| p.write(data) }
-    raise ErrorDuringExecution.new("patch", args) unless $CHILD_STATUS.success?
   end
 
   def inspect

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
 

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

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

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

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)
+    output = popen_write(*args, **options, &block)
-    raise ErrorDuringExecution, args unless $CHILD_STATUS.success?
+    raise ErrorDuringExecution(args, stdout: output, status: $CHILD_STATUS) unless $CHILD_STATUS.success?
     output
   end