Merge pull request #4819 from woodruffw/error-pipe

utils: Use JSON to marshal child errors

Library/Homebrew/build.rb

@@ -190,7 +190,20 @@ begin
   build   = Build.new(formula, options)
   build.install
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_hash = JSON.parse e.to_json
+
+  # Special case: need to recreate BuildErrors in full
+  # for proper analytics reporting and error messages.
+  # BuildErrors are specific to build processses and not other
+  # children, which is why we create the necessary state here
+  # and not in Utils.safe_fork.
+  if error_hash["json_class"] == "BuildError"
+    error_hash["cmd"] = e.cmd
+    error_hash["args"] = e.args
+    error_hash["env"] = e.env
+  end
+
+  error_pipe.puts error_hash.to_json
   error_pipe.close
   exit! 1
 end

Library/Homebrew/dev-cmd/test.rb

@@ -93,9 +93,6 @@ module Homebrew
             exec(*args)
           end
         end
-      rescue ::Test::Unit::AssertionFailedError => e
-        ofail "#{f.full_name}: failed"
-        puts e.message
       rescue Exception => e # rubocop:disable Lint/RescueException
         ofail "#{f.full_name}: failed"
         puts e, e.backtrace

Library/Homebrew/exceptions.rb

@@ -352,14 +352,16 @@ class FormulaAmbiguousPythonError < RuntimeError
 end
 
 class BuildError < RuntimeError
-  attr_reader :formula, :env
+  attr_reader :cmd, :args, :env
-  attr_accessor :options
+  attr_accessor :formula, :options
 
   def initialize(formula, cmd, args, env)
     @formula = formula
+    @cmd = cmd
+    @args = args
     @env = env
-    args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ")
+    pretty_args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ")
-    super "Failed executing: #{cmd} #{args}"
+    super "Failed executing: #{cmd} #{pretty_args}"
   end
 
   def issues
@@ -526,12 +528,22 @@ end
 
 # raised by safe_system in utils.rb
 class ErrorDuringExecution < RuntimeError
+  attr_reader :cmd
   attr_reader :status
+  attr_reader :output
 
   def initialize(cmd, status:, output: nil)
+    @cmd = cmd
     @status = status
+    @output = output
 
-    s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}."
+    exitstatus = if status.respond_to?(:exitstatus)
+      status.exitstatus
+    else
+      status
+    end
+
+    s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{exitstatus}."
 
     unless [*output].empty?
       format_output_line = lambda do |type, line|
@@ -596,3 +608,22 @@ class BottleFormulaUnavailableError < RuntimeError
     EOS
   end
 end
+
+# Raised when a child process sends us an exception over its error pipe.
+class ChildProcessError < RuntimeError
+  attr_reader :inner
+  attr_reader :inner_class
+
+  def initialize(inner)
+    @inner = inner
+    @inner_class = Object.const_get inner["json_class"]
+
+    super <<~EOS
+      An exception occured within a build process:
+        #{inner_class}: #{inner["m"]}
+    EOS
+
+    # Clobber our real (but irrelevant) backtrace with that of the inner exception.
+    set_backtrace inner["b"]
+  end
+end

Library/Homebrew/formula_installer.rb

@@ -744,14 +744,18 @@ class FormulaInstaller
       raise "Empty installation"
     end
   rescue Exception => e # rubocop:disable Lint/RescueException
-    e.options = display_options(formula) if e.is_a?(BuildError)
+    if e.is_a? BuildError
+      e.formula = formula
+      e.options = options
+    end
+
     ignore_interrupts do
       # any exceptions must leave us with nothing installed
       formula.update_head_version
       formula.prefix.rmtree if formula.prefix.directory?
       formula.rack.rmdir_if_possible
     end
-    raise
+    raise e
   end
 
   def link(keg)

Library/Homebrew/global.rb

@@ -1,5 +1,7 @@
 require "pathname"
 require "English"
+require "json"
+require "json/add/exception"
 
 HOMEBREW_LIBRARY_PATH = Pathname.new(__FILE__).realpath.parent
 

Library/Homebrew/postinstall.rb

@@ -15,7 +15,7 @@ begin
   formula.extend(Debrew::Formula) if ARGV.debug?
   formula.run_post_install
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_pipe.puts e.to_json
   error_pipe.close
   exit! 1
 end

Library/Homebrew/test.rb

@@ -28,7 +28,7 @@ begin
     raise "test returned false" if formula.run_test == false
   end
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_pipe.puts e.to_json
   error_pipe.close
   exit! 1
 end

Library/Homebrew/test/formula_installer_spec.rb

@@ -4,6 +4,7 @@ require "keg"
 require "tab"
 require "test/support/fixtures/testball"
 require "test/support/fixtures/testball_bottle"
+require "test/support/fixtures/failball"
 
 describe FormulaInstaller do
   define_negated_matcher :need_bottle, :be_bottle_unneeded
@@ -28,7 +29,7 @@ describe FormulaInstaller do
       Tab.clear_cache
       expect(Tab.for_keg(keg)).not_to be_poured_from_bottle
 
-      yield formula
+      yield formula if block_given?
     ensure
       Tab.clear_cache
       keg.unlink
@@ -132,4 +133,21 @@ describe FormulaInstaller do
       fi.check_install_sanity
     }.to raise_error(CannotInstallFormulaError)
   end
+
+  specify "install fails with BuildError when a system() call fails" do
+    ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"] = "1"
+    ENV["FAILBALL_BUILD_ERROR"] = "1"
+
+    expect {
+      temporary_install(Failball.new)
+    }.to raise_error(BuildError)
+  end
+
+  specify "install fails with a RuntimeError when #install raises" do
+    ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"] = "1"
+
+    expect {
+      temporary_install(Failball.new)
+    }.to raise_error(RuntimeError)
+  end
 end

Library/Homebrew/test/support/fixtures/failball.rb

@@ -0,0 +1,20 @@
+class Failball < Formula
+  def initialize(name = "failball", path = Pathname.new(__FILE__).expand_path, spec = :stable, alias_path: nil)
+    self.class.instance_eval do
+      stable.url "file://#{TEST_FIXTURE_DIR}/tarballs/testball-0.1.tbz"
+      stable.sha256 TESTBALL_SHA256
+    end
+    super
+  end
+
+  def install
+    prefix.install "bin"
+    prefix.install "libexec"
+
+    # This should get marshalled into a BuildError.
+    system "/usr/bin/false" if ENV["FAILBALL_BUILD_ERROR"]
+
+    # This should get marshalled into a RuntimeError.
+    raise "something that isn't a build error happened!"
+  end
+end

Library/Homebrew/test/support/lib/config.rb

@@ -8,7 +8,11 @@ HOMEBREW_BREW_FILE = Pathname.new(ENV["HOMEBREW_BREW_FILE"])
 
 TEST_TMPDIR = ENV.fetch("HOMEBREW_TEST_TMPDIR") do |k|
   dir = Dir.mktmpdir("homebrew-tests-", ENV["HOMEBREW_TEMP"] || "/tmp")
-  at_exit { FileUtils.remove_entry(dir) }
+  at_exit do
+    # Child processes inherit this at_exit handler, but we don't want them
+    # to clean TEST_TMPDIR up prematurely (i.e., when they exit early for a test).
+    FileUtils.remove_entry(dir) unless ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"]
+  end
   ENV[k] = dir
 end
 

Library/Homebrew/test/utils/fork_spec.rb

@@ -0,0 +1,21 @@
+require "utils/fork"
+
+describe Utils do
+  describe "#safe_fork" do
+    it "raises a RuntimeError on an error that isn't ErrorDuringExecution" do
+      expect {
+        described_class.safe_fork do
+          raise "this is an exception in the child"
+        end
+      }.to raise_error(RuntimeError)
+    end
+
+    it "raises an ErrorDuringExecution on one in the child" do
+      expect {
+        described_class.safe_fork do
+          safe_system "/usr/bin/false"
+        end
+      }.to raise_error(ErrorDuringExecution)
+    end
+  end
+end

Library/Homebrew/utils/fork.rb

@@ -2,6 +2,26 @@ require "fcntl"
 require "socket"
 
 module Utils
+  def self.rewrite_child_error(child_error)
+    error = if child_error.inner_class == ErrorDuringExecution
+      ErrorDuringExecution.new(child_error.inner["cmd"],
+                               status: child_error.inner["status"],
+                               output: child_error.inner["output"])
+    elsif child_error.inner_class == BuildError
+      # We fill `BuildError#formula` and `BuildError#options` in later,
+      # when we rescue this in `FormulaInstaller#build`.
+      BuildError.new(nil, child_error.inner["cmd"],
+                     child_error.inner["args"], child_error.inner["env"])
+    else
+      # Everything other error in the child just becomes a RuntimeError.
+      RuntimeError.new(child_error.message)
+    end
+
+    error.set_backtrace child_error.backtrace
+
+    error
+  end
+
   def self.safe_fork(&_block)
     Dir.mktmpdir("homebrew", HOMEBREW_TEMP) do |tmpdir|
       UNIXServer.open("#{tmpdir}/socket") do |server|
@@ -15,7 +35,18 @@ module Utils
             write.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC)
             yield
           rescue Exception => e # rubocop:disable Lint/RescueException
-            Marshal.dump(e, write)
+            error_hash = JSON.parse e.to_json
+
+            # Special case: We need to recreate ErrorDuringExecutions
+            # for proper error messages and because other code expects
+            # to rescue them further down.
+            if e.is_a?(ErrorDuringExecution)
+              error_hash["cmd"] = e.cmd
+              error_hash["status"] = e.status.exitstatus
+              error_hash["output"] = e.output
+            end
+
+            write.puts error_hash.to_json
             write.close
             exit!
           else
@@ -33,10 +64,20 @@ module Utils
             socket.close
           end
           write.close
-          data = read.read
+          # Each line on the error pipe contains a JSON-serialized exception.
+          # We read the first, since only one is necessary for a failure.
+          data = read.gets
           read.close
           Process.wait(pid) unless socket.nil?
-          raise Marshal.load(data) unless data.nil? || data.empty? # rubocop:disable Security/MarshalLoad
+
+          if data && !data.empty?
+            error_hash = JSON.parse(data) unless data.nil? || data.empty?
+
+            e = ChildProcessError.new(error_hash)
+
+            raise rewrite_child_error(e)
+          end
+
           raise Interrupt if $CHILD_STATUS.exitstatus == 130
           raise "Forked child process failed: #{$CHILD_STATUS}" unless $CHILD_STATUS.success?
         end