brew-test-bot: make test commands shell-safe

Library/Contributions/cmd/brew-test-bot.rb

@@ -36,7 +36,7 @@ class Step
     @category = test.category
     @command = command
     @puts_output_on_success = options[:puts_output_on_success]
-    @name = command.split[1].delete '-'
+    @name = command[1].delete("-")
     @status = :running
     @repository = HOMEBREW_REPOSITORY
     @time = 0
@@ -61,7 +61,7 @@ class Step
   end
 
   def command_short
-    @command.gsub(/(brew|--force|--retry|--verbose|--build-bottle|--rb) /, '').strip.squeeze ' '
+    (@command - %w[brew --force --retry --verbose --build-bottle --rb]).join(" ")
   end
 
   def passed?
@@ -73,8 +73,9 @@ class Step
   end
 
   def puts_command
-    print "#{Tty.blue}==>#{Tty.white} #{@command}#{Tty.reset}"
+    cmd = @command.join(" ")
-    tabs = (80 - "PASSED".length + 1 - @command.length) / 8
+    print "#{Tty.blue}==>#{Tty.white} #{cmd}#{Tty.reset}"
+    tabs = (80 - "PASSED".length + 1 - cmd.length) / 8
     tabs.times{ print "\t" }
     $stdout.flush
   end
@@ -91,14 +92,15 @@ class Step
     puts_command
 
     start_time = Time.now
-    run_command = "#{@command} &>'#{log_file_path}'"
+
-    if run_command.start_with? 'git '
+    pid = fork do
-      Dir.chdir @repository do
+      STDOUT.reopen(log_file_path, "wb")
-        `#{run_command}`
+      STDERR.reopen(log_file_path, "wb")
-      end
+      Dir.chdir(@repository) if @command.first == "git"
-    else
+      exec(*@command)
-      `#{run_command}`
     end
+    Process.wait(pid)
+
     end_time = Time.now
     @time = end_time - start_time
 
@@ -177,10 +179,10 @@ class Test
        and not ENV['ghprbPullId']
       diff_start_sha1 = shorten_revision ENV['GIT_PREVIOUS_COMMIT']
       diff_end_sha1 = shorten_revision ENV['GIT_COMMIT']
-      test "brew update" if current_branch == "master"
+      test "brew", "update" if current_branch == "master"
     elsif @hash or @url
       diff_start_sha1 = current_sha1
-      test "brew update" if current_branch == "master"
+      test "brew", "update" if current_branch == "master"
       diff_end_sha1 = current_sha1
     end
 
@@ -206,13 +208,13 @@ class Test
         @name = "#{diff_start_sha1}-#{diff_end_sha1}"
       end
     elsif @hash
-      test "git checkout #{@hash}"
+      test "git", "checkout", @hash
       diff_start_sha1 = "#{@hash}^"
       diff_end_sha1 = @hash
       @name = @hash
     elsif @url
-      test "git checkout #{current_sha1}"
+      test "git", "checkout", current_sha1
-      test "brew pull --clean #{@url}"
+      test "brew", "pull", "--clean", @url
       diff_end_sha1 = current_sha1
       @short_url = @url.gsub('https://github.com/', '')
       if @short_url.include? '/commit/'
@@ -274,18 +276,17 @@ class Test
   def setup
     @category = __method__
     return if ARGV.include? "--skip-setup"
-    test "brew doctor"
+    test "brew", "doctor"
-    test "brew --env"
+    test "brew", "--env"
-    test "brew config"
+    test "brew", "config"
   end
 
   def formula formula
     @category = __method__.to_s + ".#{formula}"
 
-    test "brew uses #{formula}"
+    test "brew", "uses", formula
     dependencies = `brew deps #{formula}`.split("\n")
     dependencies -= `brew list`.split("\n")
-    dependencies = dependencies.join(' ')
     formula_object = Formula.factory(formula)
     return unless satisfied_requirements? formula_object
 
@@ -294,7 +295,7 @@ class Test
       CompilerSelector.new(formula_object).compiler
     rescue CompilerSelectionError => e
       unless installed_gcc
-        test "brew install gcc"
+        test "brew", "install", "gcc"
         installed_gcc = true
         retry
       end
@@ -303,52 +304,54 @@ class Test
       return
     end
 
-    test "brew fetch --retry #{dependencies}" unless dependencies.empty?
+    test "brew", "fetch", "--retry", *dependencies unless dependencies.empty?
-    formula_fetch_options = " "
+    formula_fetch_options = []
-    formula_fetch_options << " --build-bottle" unless ARGV.include? '--no-bottle'
+    formula_fetch_options << "--build-bottle" unless ARGV.include? "--no-bottle"
-    formula_fetch_options << " --force" if ARGV.include? '--cleanup'
+    formula_fetch_options << "--force" if ARGV.include? "--cleanup"
-    test "brew fetch --retry#{formula_fetch_options} #{formula}"
+    formula_fetch_options << formula
-    test "brew uninstall --force #{formula}" if formula_object.installed?
+    test "brew", "fetch", formula, "--retry", *formula_fetch_options
-    install_args = '--verbose'
+    test "brew", "uninstall", "--force", formula if formula_object.installed?
-    install_args << ' --build-bottle' unless ARGV.include? '--no-bottle'
+    install_args = %w[--verbose]
-    install_args << ' --HEAD' if ARGV.include? '--HEAD'
+    install_args << "--build-bottle" unless ARGV.include? "--no-bottle"
-    test "brew install --only-dependencies #{install_args} #{formula}" unless dependencies.empty?
+    install_args << "--HEAD" if ARGV.include? "--HEAD"
-    test "brew install #{install_args} #{formula}"
+    install_args << formula
+    test "brew", "install", "--only-dependencies", *install_args unless dependencies.empty?
+    test "brew", "install", *install_args
     install_passed = steps.last.passed?
-    test "brew audit #{formula}"
+    test "brew", "audit", formula
     if install_passed
       unless ARGV.include? '--no-bottle'
-        test "brew bottle --rb #{formula}", :puts_output_on_success => true
+        test "brew", "bottle", "--rb", formula, :puts_output_on_success => true
         bottle_step = steps.last
         if bottle_step.passed? and bottle_step.has_output?
           bottle_filename =
             bottle_step.output.gsub(/.*(\.\/\S+#{bottle_native_regex}).*/m, '\1')
-          test "brew uninstall --force #{formula}"
+          test "brew", "uninstall", "--force", formula
-          test "brew install #{bottle_filename}"
+          test "brew", "install", bottle_filename
         end
       end
-      test "brew test --verbose #{formula}" if formula_object.test_defined?
+      test "brew", "test", "--verbose", formula if formula_object.test_defined?
-      test "brew uninstall --force #{formula}"
+      test "brew", "uninstall", "--force", formula
     end
 
     if formula_object.devel && !ARGV.include?('--HEAD') \
        && satisfied_requirements?(formula_object, :devel)
-      test "brew fetch --retry --devel#{formula_fetch_options} #{formula}"
+      test "brew", "fetch", "--retry", "--devel", *formula_fetch_options
-      test "brew install --devel --verbose #{formula}"
+      test "brew", "install", "--devel", "--verbose", formula
       devel_install_passed = steps.last.passed?
-      test "brew audit --devel #{formula}"
+      test "brew", "audit", "--devel", formula
       if devel_install_passed
-        test "brew test --devel --verbose #{formula}" if formula_object.test_defined?
+        test "brew", "test", "--devel", "--verbose", formula if formula_object.test_defined?
-        test "brew uninstall --devel --force #{formula}"
+        test "brew", "uninstall", "--devel", "--force", formula
       end
     end
-    test "brew uninstall --force #{dependencies}" unless dependencies.empty?
+    test "brew", "uninstall", "--force", *dependencies unless dependencies.empty?
   end
 
   def homebrew
     @category = __method__
-    test "brew tests"
+    test "brew", "tests"
-    test "brew readall"
+    test "brew", "readall"
   end
 
   def cleanup_before
@@ -364,27 +367,30 @@ class Test
 
   def cleanup_after
     @category = __method__
-    force_flag = ''
+    checkout_args = []
     if ARGV.include? '--cleanup'
-      test 'git clean --force -dx'
+      test "git", "clean", "--force", "-dx"
-      force_flag = '-f'
+      checkout_args << "-f"
     end
 
+    checkout_args << @start_branch
+
     if ARGV.include? '--cleanup' or @url or @hash
-      test "git checkout #{force_flag} #{@start_branch}"
+      test "git", "checkout", *checkout_args
     end
 
     if ARGV.include? '--cleanup'
-      test 'git reset --hard'
+      test "git", "reset", "--hard"
       git 'stash pop 2>/dev/null'
-      test 'brew cleanup'
+      test "brew", "cleanup"
     end
 
     FileUtils.rm_rf @brewbot_root unless ARGV.include? "--keep-logs"
   end
 
-  def test cmd, options={}
+  def test(*args)
-    step = Step.new self, cmd, options
+    options = Hash === args.last ? args.pop : {}
+    step = Step.new self, args, options
     step.run
     steps << step
     step
@@ -524,7 +530,7 @@ if ARGV.include? "--junit"
           system_out = testcase.add_element 'system-out'
           system_out.text = output
         else
-          failure.attributes['message'] = "#{step.status}: #{step.command}"
+          failure.attributes["message"] = "#{step.status}: #{step.command.join(" ")}"
           failure.text = output
         end
       end