Merge pull request #2624 from vmrob/system-command

Process SystemCommand arguments in list format

lib/cask/artifact/pkg.rb

@@ -84,9 +84,9 @@ class Cask::Artifact::Pkg < Cask::Artifact::Base
     if uninstall_options.key? :quit
       [*uninstall_options[:quit]].each do |id|
         ohai "Quitting application ID #{id}"
-        num_running = @command.run!('/usr/bin/osascript', :args => ['-e', "tell application \"System Events\" to count processes whose bundle identifier is \"#{id}\""], :sudo => true).to_i
+        num_running = @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application "System Events" to count processes whose bundle identifier is "#{id}"}], :sudo => true).to_i
         if num_running > 0
-          @command.run!('/usr/bin/osascript', :args => ['-e', "tell application id \"#{id}\" to quit"], :sudo => true)
+          @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application id "#{id}" to quit}], :sudo => true)
         end
       end
     end

lib/cask/pkg.rb

@@ -1,6 +1,6 @@
 class Cask::Pkg
   def self.all_matching(regexp, command)
-    command.run(%Q(/usr/sbin/pkgutil --pkgs="#{regexp}")).split("\n").map do |package_id|
+    command.run('/usr/sbin/pkgutil', :args => [%Q{--pkgs=#{regexp}}]).split("\n").map do |package_id|
       new(package_id.chomp, command)
     end
   end

lib/cask/system_command.rb

@@ -1,14 +1,21 @@
+require 'open3'
+
 class Cask::SystemCommand
-  def self.run(command, options={})
-    command = _process_options(command, options)
-    odebug "Executing: #{command}"
+  def self.run(executable, options={})
+    command = _process_options(executable, options)
+    odebug "Executing: #{command.inspect}"
     output = ''
-    IO.popen(command, 'r+') do |pipe|
+    Open3.popen3(*command) do |stdin, stdout, stderr|
       if options[:input]
-        options[:input].each { |line| pipe.puts line }
+        options[:input].each { |line| stdin.puts line }
       end
-      pipe.close_write
-      while line = pipe.gets
+      stdin.close_write
+      while line = stdout.gets
+        output << line
+        ohai line.chomp if options[:print]
+      end
+      while line = stderr.gets
+        next if options[:stderr] == :silence
         output << line
         ohai line.chomp if options[:print]
       end
@@ -25,33 +32,20 @@ class Cask::SystemCommand
     run(command, options.merge(:must_succeed => true))
   end
 
-  def self._process_options(command, options)
+  def self._process_options(executable, options)
+    command = [executable]
     if options[:sudo]
-      command = "/usr/bin/sudo -E -- #{_quote(command)}"
+      command.unshift('/usr/bin/sudo', '-E', '--')
     end
-    if options[:args]
-      command = "#{command} #{options[:args].map { |arg| _quote(arg) }.join(' ')}"
-    end
-    case options[:stderr]
-    when :silence then
-      command = "#{command} 2>/dev/null"
-    when :merge, nil then
-      command = "#{command} 2>&1"
+    if ! options[:args].empty?
+      command.concat options[:args]
     end
     command
   end
 
   def self._assert_success(status, command, output)
     unless status.success?
-      raise CaskCommandFailedError.new(command, output)
-    end
-  end
-
-  def self._quote(string)
-    if %r{^(['"]).*\1$}.match(string)
-      string
-    else
-      %Q('#{string}')
+      raise CaskCommandFailedError.new(command.inspect, output)
     end
   end
 
@@ -62,7 +56,7 @@ class Cask::SystemCommand
       raise CaskError.new(<<-ERRMSG)
 Error parsing plist output from command.
   command was:
-  #{command}
+  #{command.inspect}
   output we attempted to parse:
   #{output}
         ERRMSG

test/cask/artifact/pkg_test.rb

@@ -12,14 +12,11 @@ describe Cask::Artifact::Pkg do
     it 'runs the system installer on the specified pkgs' do
       pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand)
 
-      expected_command = "/usr/bin/sudo -E -- '/usr/sbin/installer' '-pkg' '#{@cask.destination_path/'MyFancyPkg'/'Fancy.pkg'}' '-target' '/' 2>&1"
-      Cask::FakeSystemCommand.stubs_command(expected_command)
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/installer', '-pkg', @cask.destination_path/'MyFancyPkg'/'Fancy.pkg', '-target', '/'])
 
       shutup do
         pkg.install
       end
-
-      Cask::FakeSystemCommand.system_calls[expected_command].must_equal 1
     end
   end
 
@@ -27,17 +24,14 @@ describe Cask::Artifact::Pkg do
     it 'runs the specified uninstaller for the cask' do
       pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand)
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/bin/osascript' '-e' 'tell application "System Events" to count processes whose bundle identifier is "my.fancy.package.app"' 2>&1), '1')
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/bin/osascript' '-e' 'tell application id "my.fancy.package.app" to quit' 2>&1))
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/bin/osascript', '-e', 'tell application "System Events" to count processes whose bundle identifier is "my.fancy.package.app"'], '1')
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/bin/osascript', '-e', 'tell application id "my.fancy.package.app" to quit'])
 
-      expected_command = "/usr/bin/sudo -E -- '#{@cask.destination_path/'MyFancyPkg'/'FancyUninstaller.tool'}' '--please' 2>&1"
-      Cask::FakeSystemCommand.stubs_command(expected_command)
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', @cask.destination_path/'MyFancyPkg'/'FancyUninstaller.tool', '--please'])
 
       shutup do
         pkg.uninstall
       end
-
-      Cask::FakeSystemCommand.system_calls[expected_command].must_equal 1
     end
 
     it 'can uninstall using pkgutil, launchctl, and file lists' do
@@ -45,7 +39,7 @@ describe Cask::Artifact::Pkg do
       pkg = Cask::Artifact::Pkg.new(cask, Cask::FakeSystemCommand)
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil --pkgs="my.fancy.package.*" 2>&1),
+        ['/usr/sbin/pkgutil', '--pkgs=my.fancy.package.*'],
         [
           'my.fancy.package.main',
           'my.fancy.package.agent',
@@ -53,14 +47,14 @@ describe Cask::Artifact::Pkg do
       )
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-files' '--files' 'my.fancy.package.main' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fancy.package.main'],
         [
           'fancy/bin/fancy.exe',
           'fancy/var/fancy.data',
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fancy.package.main' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fancy.package.main'],
         [
           'fancy',
           'fancy/bin',
@@ -68,7 +62,7 @@ describe Cask::Artifact::Pkg do
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--pkg-info-plist' 'my.fancy.package.main' 2>&1),
+        ['/usr/sbin/pkgutil', '--pkg-info-plist', 'my.fancy.package.main'],
         <<-PLIST
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
@@ -84,11 +78,11 @@ describe Cask::Artifact::Pkg do
       )
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/bin/launchctl 'list' '-x' 'my.fancy.package.service' 2>&1),
+        ['/bin/launchctl', 'list', '-x', 'my.fancy.package.service'],
         "launchctl list returned unknown response\n"
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/sudo -E -- '/bin/launchctl' 'list' '-x' 'my.fancy.package.service' 2>&1),
+        ['/usr/bin/sudo', '-E', '--', '/bin/launchctl', 'list', '-x', 'my.fancy.package.service'],
         <<-PLIST
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
@@ -112,14 +106,15 @@ describe Cask::Artifact::Pkg do
 </plist>
         PLIST
       )
-      Cask::FakeSystemCommand.expects_command(%Q(/usr/bin/sudo -E -- '/bin/launchctl' 'remove' '--' 'my.fancy.package.service' 2>&1))
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/kextstat' '-l' '-b' 'my.fancy.package.kernelextension' 2>&1), 'loaded')
-      Cask::FakeSystemCommand.expects_command(%Q(/usr/bin/sudo -E -- '/sbin/kextunload' '-b' '--' 'my.fancy.package.kernelextension' 2>&1))
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fancy.package.main' 2>&1))
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/bin/launchctl', 'remove', '--', 'my.fancy.package.service'])
+
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/kextstat', '-l', '-b', 'my.fancy.package.kernelextension'], 'loaded')
+      Cask::FakeSystemCommand.expects_command(['/usr/bin/sudo', '-E', '--', '/sbin/kextunload', '-b', '--', 'my.fancy.package.kernelextension'])
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fancy.package.main'])
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-files' '--files' 'my.fancy.package.agent' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fancy.package.agent'],
         [
           'fancy/agent/fancy-agent.exe',
           'fancy/agent/fancy-agent.pid',
@@ -127,14 +122,14 @@ describe Cask::Artifact::Pkg do
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fancy.package.agent' 2>&1),
+        ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fancy.package.agent'],
         [
           'fancy',
           'fancy/agent',
         ].join("\n")
       )
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/sbin/pkgutil '--pkg-info-plist' 'my.fancy.package.agent' 2>&1),
+        ['/usr/sbin/pkgutil', '--pkg-info-plist', 'my.fancy.package.agent'],
         <<-PLIST
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
@@ -155,13 +150,18 @@ describe Cask::Artifact::Pkg do
         /tmp/fancy/bin
         /tmp/fancy/var
       ].each do |dir|
-        Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/chmod' '--' '777' '#{dir}' 2>&1))
+        Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/chmod', '--', '777', '#{dir}'])
       end
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fancy.package.agent' 2>&1))
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fancy.package.agent'])
 
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/rm' '-f' '--' '/tmp/fancy/bin/fancy.exe' '/tmp/fancy/var/fancy.data' 2>&1))
-      Cask::FakeSystemCommand.stubs_command(%Q(/usr/bin/sudo -E -- '/bin/rm' '-f' '--' '/tmp/fancy/agent/fancy-agent.exe' '/tmp/fancy/agent/fancy-agent.pid' '/tmp/fancy/agent/fancy-agent.log' 2>&1))
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/rm', '-f', '--',
+                                             Pathname.new('/tmp/fancy/bin/fancy.exe'),
+                                             Pathname.new('/tmp/fancy/var/fancy.data')])
+      Cask::FakeSystemCommand.stubs_command(['/usr/bin/sudo', '-E', '--', '/bin/rm', '-f', '--',
+                                             Pathname.new('/tmp/fancy/agent/fancy-agent.exe'),
+                                             Pathname.new('/tmp/fancy/agent/fancy-agent.pid'),
+                                             Pathname.new('/tmp/fancy/agent/fancy-agent.log')])
 
       # No assertions after call since all assertions are implicit from the interactions setup above.
       # TODO: verify rmdir commands (requires setting up actual file tree or faking out .exists?

test/cask/cli/alfred_test.rb

@@ -1,7 +1,7 @@
 require 'test_helper'
 
 def fake_alfred_preference(key, response)
-  Cask::FakeSystemCommand.stubs_command("/usr/bin/defaults 'read' 'com.runningwithcrayons.Alfred-Preferences' '#{key}' 2>&1", response)
+  Cask::FakeSystemCommand.stubs_command(['/usr/bin/defaults', 'read', 'com.runningwithcrayons.Alfred-Preferences', key], response)
 end
 
 def fake_alfred_installed(installed=true)
@@ -78,7 +78,7 @@ describe Cask::CLI::Alfred do
       SCOPE_RESPONSE
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes','#{Cask.caskroom}')" 2>&1)
+        ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes','#{Cask.caskroom}')"}]
       )
 
       TestHelper.must_output(self, lambda {
@@ -95,7 +95,7 @@ describe Cask::CLI::Alfred do
 
       expected_scopes = (Cask::CLI::Alfred::DEFAULT_SCOPES + [Cask.caskroom]).map { |s| "'#{s}'" }
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "(#{expected_scopes.join(',')})" 2>&1)
+        ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"(#{expected_scopes.join(',')})"}]
       )
 
       TestHelper.must_output(self, lambda {
@@ -140,7 +140,7 @@ describe Cask::CLI::Alfred do
       SCOPE_RESPONSE
 
       Cask::FakeSystemCommand.stubs_command(
-        %Q(/usr/bin/defaults 'write' 'com.runningwithcrayons.Alfred-Preferences' 'features.defaultresults.scope' "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes')" 2>&1)
+        ['/usr/bin/defaults', 'write', 'com.runningwithcrayons.Alfred-Preferences', 'features.defaultresults.scope', %Q{"('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes')"}]
       )
 
       TestHelper.must_output(self, lambda {

test/cask/container/naked_test.rb

@@ -11,7 +11,7 @@ describe Cask::Container::Naked do
     cask                 = SpaceyCask.new
     path                 = '/tmp/downloads/kevin-spacey-1.2.pkg'
     expected_destination = cask.destination_path.join('kevin spacey.pkg')
-    expected_command     = %Q(/usr/bin/ditto '--' '#{path}' '#{expected_destination}' 2>&1)
+    expected_command     = ['/usr/bin/ditto', '--', path, expected_destination]
     Cask::FakeSystemCommand.stubs_command(expected_command)
 
     container = Cask::Container::Naked.new(cask, path, Cask::FakeSystemCommand)

test/cask/pkg_test.rb

@@ -23,14 +23,14 @@ describe Cask::Pkg do
       pkg = Cask::Pkg.new('my.fake.pkg', Cask::FakeSystemCommand)
 
       Cask::FakeSystemCommand.stubs_command(
-        "/usr/sbin/pkgutil '--only-files' '--files' 'my.fake.pkg' 2>&1"
+        ['/usr/sbin/pkgutil', '--only-files', '--files', 'my.fake.pkg']
       )
       Cask::FakeSystemCommand.stubs_command(
-        "/usr/sbin/pkgutil '--only-dirs' '--files' 'my.fake.pkg' 2>&1"
+        ['/usr/sbin/pkgutil', '--only-dirs', '--files', 'my.fake.pkg']
       )
 
       Cask::FakeSystemCommand.expects_command(
-        %q(/usr/bin/sudo -E -- '/usr/sbin/pkgutil' '--forget' 'my.fake.pkg' 2>&1)
+        ['/usr/bin/sudo', '-E', '--', '/usr/sbin/pkgutil', '--forget', 'my.fake.pkg']
       )
 
       pkg.uninstall

test/support/fake_system_command.rb

@@ -29,7 +29,7 @@ class Cask::FakeSystemCommand
   def self.verify_expectations!
     expectations.each do |command, times|
       unless system_calls[command] == times
-        fail("expected #{command} to be run #{times} times, but got #{system_calls[command]}")
+        fail("expected #{command.inspect} to be run #{times} times, but got #{system_calls[command]}")
       end
     end
   end