From 088730c6f191c5d15e0e99a73d406a40fb899cff Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 4 Sep 2018 08:37:08 +0100 Subject: [PATCH] Revert "Use JSON to marshal errors from children" --- Library/Homebrew/build.rb | 15 +----- Library/Homebrew/dev-cmd/test.rb | 3 ++ Library/Homebrew/exceptions.rb | 41 ++-------------- Library/Homebrew/formula_installer.rb | 8 +--- Library/Homebrew/global.rb | 2 - Library/Homebrew/postinstall.rb | 2 +- Library/Homebrew/test.rb | 2 +- .../Homebrew/test/formula_installer_spec.rb | 20 +------- .../test/support/fixtures/failball.rb | 20 -------- Library/Homebrew/test/support/lib/config.rb | 6 +-- Library/Homebrew/test/utils/fork_spec.rb | 21 --------- Library/Homebrew/utils/fork.rb | 47 ++----------------- 12 files changed, 18 insertions(+), 169 deletions(-) delete mode 100644 Library/Homebrew/test/support/fixtures/failball.rb delete mode 100644 Library/Homebrew/test/utils/fork_spec.rb diff --git a/Library/Homebrew/build.rb b/Library/Homebrew/build.rb index 3d72481687..fe4225384e 100644 --- a/Library/Homebrew/build.rb +++ b/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 - - # 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 + Marshal.dump(e, error_pipe) error_pipe.close exit! 1 end diff --git a/Library/Homebrew/dev-cmd/test.rb b/Library/Homebrew/dev-cmd/test.rb index 8e8ceb9824..d21a62cde5 100644 --- a/Library/Homebrew/dev-cmd/test.rb +++ b/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 diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index 51d85d19ca..dc3f5f9e36 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -352,16 +352,14 @@ class FormulaAmbiguousPythonError < RuntimeError end class BuildError < RuntimeError - attr_reader :cmd, :args, :env - attr_accessor :formula, :options + attr_reader :formula, :env + 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(" ") - super "Failed executing: #{cmd} #{pretty_args}" + args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ") + 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) - status.exitstatus - else - status - end - - s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{exitstatus}." + s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.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 diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index 8eeea7c160..cb03e5c9cc 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/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.formula = formula - e.options = options - end - + e.options = display_options(formula) if e.is_a?(BuildError) 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) diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index 78f7bd30ba..ce38bddd59 100644 --- a/Library/Homebrew/global.rb +++ b/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 diff --git a/Library/Homebrew/postinstall.rb b/Library/Homebrew/postinstall.rb index 94c56e48fc..53a5b7e751 100644 --- a/Library/Homebrew/postinstall.rb +++ b/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 diff --git a/Library/Homebrew/test.rb b/Library/Homebrew/test.rb index fbe6fa64e7..3d5e62a884 100644 --- a/Library/Homebrew/test.rb +++ b/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 diff --git a/Library/Homebrew/test/formula_installer_spec.rb b/Library/Homebrew/test/formula_installer_spec.rb index de6008c6b4..932738742b 100644 --- a/Library/Homebrew/test/formula_installer_spec.rb +++ b/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 diff --git a/Library/Homebrew/test/support/fixtures/failball.rb b/Library/Homebrew/test/support/fixtures/failball.rb deleted file mode 100644 index 125452feca..0000000000 --- a/Library/Homebrew/test/support/fixtures/failball.rb +++ /dev/null @@ -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 diff --git a/Library/Homebrew/test/support/lib/config.rb b/Library/Homebrew/test/support/lib/config.rb index 68030ad9a5..5f0b69313c 100644 --- a/Library/Homebrew/test/support/lib/config.rb +++ b/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 - # 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 + at_exit { FileUtils.remove_entry(dir) } ENV[k] = dir end diff --git a/Library/Homebrew/test/utils/fork_spec.rb b/Library/Homebrew/test/utils/fork_spec.rb deleted file mode 100644 index 0af9b8f3ae..0000000000 --- a/Library/Homebrew/test/utils/fork_spec.rb +++ /dev/null @@ -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 diff --git a/Library/Homebrew/utils/fork.rb b/Library/Homebrew/utils/fork.rb index d0ecbb4ded..5087ca716c 100644 --- a/Library/Homebrew/utils/fork.rb +++ b/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 - - # 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 + Marshal.dump(e, write) 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. - # We read the first, since only one is necessary for a failure. - data = read.gets + data = read.read read.close Process.wait(pid) unless socket.nil? - - 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 Marshal.load(data) unless data.nil? || data.empty? # rubocop:disable Security/MarshalLoad raise Interrupt if $CHILD_STATUS.exitstatus == 130 raise "Forked child process failed: #{$CHILD_STATUS}" unless $CHILD_STATUS.success? end