Merge pull request #4816 from Homebrew/revert-4752-error-pipe

Revert "Use JSON to marshal errors from children"

Library/Homebrew/build.rb

@@ -190,20 +190,7 @@ begin
   build   = Build.new(formula, options)
   build.install
 rescue Exception => e # rubocop:disable Lint/RescueException
-  error_hash = JSON.parse e.to_json
+  Marshal.dump(e, error_pipe)
-
-  # 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,6 +93,9 @@ 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,16 +352,14 @@ class FormulaAmbiguousPythonError < RuntimeError
 end
 
 class BuildError < RuntimeError
-  attr_reader :cmd, :args, :env
+  attr_reader :formula, :env
-  attr_accessor :formula, :options
+  attr_accessor :options
 
   def initialize(formula, cmd, args, env)
     @formula = formula
-    @cmd = cmd
-    @args = args
     @env = env
-    pretty_args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ")
+    args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ")
-    super "Failed executing: #{cmd} #{pretty_args}"
+    super "Failed executing: #{cmd} #{args}"
   end
 
   def issues
@@ -528,22 +526,12 @@ 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
 
-    exitstatus = if status.respond_to?(:exitstatus)
+    s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.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|
@@ -608,22 +596,3 @@ 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,18 +744,14 @@ class FormulaInstaller
       raise "Empty installation"
     end
   rescue Exception => e # rubocop:disable Lint/RescueException
-    if e.is_a? BuildError
+    e.options = display_options(formula) 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 e
+    raise
   end
 
   def link(keg)

Library/Homebrew/global.rb

@@ -1,7 +1,5 @@
 require "pathname"
 require "English"
-require "json"
-require "json/add/core"
 
 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
-  error_pipe.puts e.to_json
+  Marshal.dump(e, error_pipe)
   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
-  error_pipe.puts e.to_json
+  Marshal.dump(e, error_pipe)
   error_pipe.close
   exit! 1
 end

Library/Homebrew/test/formula_installer_spec.rb

@@ -4,7 +4,6 @@ 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
@@ -29,7 +28,7 @@ describe FormulaInstaller do
       Tab.clear_cache
       expect(Tab.for_keg(keg)).not_to be_poured_from_bottle
 
-      yield formula if block_given?
+      yield formula
     ensure
       Tab.clear_cache
       keg.unlink
@@ -133,21 +132,4 @@ 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

@@ -1,20 +0,0 @@
-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,11 +8,7 @@ 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 do
+  at_exit { FileUtils.remove_entry(dir) }
-    # 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

@@ -1,21 +0,0 @@
-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,26 +2,6 @@ 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|
@@ -35,18 +15,7 @@ module Utils
             write.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC)
             yield
           rescue Exception => e # rubocop:disable Lint/RescueException
-            error_hash = JSON.parse e.to_json
+            Marshal.dump(e, write)
-
-            # 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
@@ -64,20 +33,10 @@ module Utils
             socket.close
           end
           write.close
-          # Each line on the error pipe contains a JSON-serialized exception.
+          data = read.read
-          # 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