diff --git a/Library/Homebrew/build.rb b/Library/Homebrew/build.rb index fe4225384e..3d72481687 100644 --- a/Library/Homebrew/build.rb +++ b/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 diff --git a/Library/Homebrew/dev-cmd/test.rb b/Library/Homebrew/dev-cmd/test.rb index d21a62cde5..8e8ceb9824 100644 --- a/Library/Homebrew/dev-cmd/test.rb +++ b/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 diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index dc3f5f9e36..51d85d19ca 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -352,14 +352,16 @@ class FormulaAmbiguousPythonError < RuntimeError end class BuildError < RuntimeError - attr_reader :formula, :env - attr_accessor :options + attr_reader :cmd, :args, :env + 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(" ") - super "Failed executing: #{cmd} #{args}" + pretty_args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ") + 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 diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index cb03e5c9cc..8eeea7c160 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/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) diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index ce38bddd59..78f7bd30ba 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -1,5 +1,7 @@ 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 53a5b7e751..94c56e48fc 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 - Marshal.dump(e, error_pipe) + error_pipe.puts e.to_json error_pipe.close exit! 1 end diff --git a/Library/Homebrew/test.rb b/Library/Homebrew/test.rb index 3d5e62a884..fbe6fa64e7 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 - Marshal.dump(e, error_pipe) + error_pipe.puts e.to_json 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 932738742b..de6008c6b4 100644 --- a/Library/Homebrew/test/formula_installer_spec.rb +++ b/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 diff --git a/Library/Homebrew/test/support/fixtures/failball.rb b/Library/Homebrew/test/support/fixtures/failball.rb new file mode 100644 index 0000000000..125452feca --- /dev/null +++ b/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 diff --git a/Library/Homebrew/test/support/lib/config.rb b/Library/Homebrew/test/support/lib/config.rb index 5f0b69313c..68030ad9a5 100644 --- a/Library/Homebrew/test/support/lib/config.rb +++ b/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 diff --git a/Library/Homebrew/test/utils/fork_spec.rb b/Library/Homebrew/test/utils/fork_spec.rb new file mode 100644 index 0000000000..0af9b8f3ae --- /dev/null +++ b/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 diff --git a/Library/Homebrew/utils/fork.rb b/Library/Homebrew/utils/fork.rb index 5087ca716c..d0ecbb4ded 100644 --- a/Library/Homebrew/utils/fork.rb +++ b/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