Merge pull request Homebrew#6329 from rolandwalker/system_command_refactor

rolandwalker · rolandwalker · commit 1abf0b99a9d4 · 2014-09-27T09:59:59.000-04:00
refactor Cask::SystemCommand
diff --git a/lib/cask/artifact/base.rb b/lib/cask/artifact/base.rb
@@ -45,7 +45,7 @@ def self.read_script_arguments(arguments, stanza, default_arguments={}, override
     end
 
     # key sanity
-    permitted_keys = [:args, :input, :executable, :must_succeed, :sudo, :print]
+    permitted_keys = [:args, :input, :executable, :must_succeed, :sudo, :print_stdout, :print_stderr]
     unknown_keys = arguments.keys - permitted_keys
     unless unknown_keys.empty?
       opoo %Q{Unknown arguments to #{description} -- #{unknown_keys.inspect} (ignored). Running "brew update && brew upgrade brew-cask && brew cleanup && brew cask cleanup" will likely fix it.}
diff --git a/lib/cask/artifact/install_script.rb b/lib/cask/artifact/install_script.rb
@@ -16,7 +16,7 @@ def install_phase
                                                                       artifact,
                                                                       "#{self.class.artifact_dsl_key}",
                                                                       {:must_succeed => true},
-                                                                      {:print => true}
+                                                                      {:print_stdout => true}
                                                                       )
       ohai "Running #{self.class.artifact_dsl_key} script #{executable}"
       raise CaskInvalidError.new(@cask, "#{self.class.artifact_dsl_key} missing executable") if executable.nil?
diff --git a/lib/cask/artifact/pkg.rb b/lib/cask/artifact/pkg.rb
@@ -48,6 +48,6 @@ def run_installer(pkg_description)
     ]
     args << '-verboseR' if ARGV.verbose?
     args << '-allowUntrusted' if pkg_install_opts :allow_untrusted
-    @command.run!('/usr/sbin/installer', {:sudo => true, :args => args, :print => true})
+    @command.run!('/usr/sbin/installer', {:sudo => true, :args => args, :print_stdout => true})
   end
 end
diff --git a/lib/cask/artifact/symlinked.rb b/lib/cask/artifact/symlinked.rb
@@ -22,14 +22,14 @@ def add_altname_metadata(source, target)
     odebug "Adding #{attribute} metadata"
     altnames = @command.run('/usr/bin/xattr',
                             :args => ['-p', attribute, target],
-                            :stderr => :silence).sub(/\A\((.*)\)\Z/, "#{$1}")
+                            :print_stderr => false).stdout.sub(/\A\((.*)\)\Z/, "#{$1}")
     odebug "Existing metadata is: '#{altnames}'"
     altnames.concat(', ') if altnames.length > 0
     altnames.concat(%Q{"#{target.basename}"})
     altnames = %Q{(#{altnames})}
     @command.run!('/usr/bin/xattr',
                   :args => ['-w', attribute, altnames, target],
-                  :stderr => :silence)
+                  :print_stderr => false)
   end
 
   def summary
diff --git a/lib/cask/artifact/uninstall_base.rb b/lib/cask/artifact/uninstall_base.rb
@@ -51,7 +51,7 @@ def dispatch_uninstall_directives(stanza, expand_tilde=false)
       executable, script_arguments = self.class.read_script_arguments(directives,
                                                                       'uninstall',
                                                                       {:must_succeed => true},
-                                                                      {:sudo => true, :print => true},
+                                                                      {:sudo => true, :print_stdout => true},
                                                                       :early_script)
 
       ohai "Running uninstall script #{executable}"
@@ -65,7 +65,7 @@ def dispatch_uninstall_directives(stanza, expand_tilde=false)
       Array(directives[:launchctl]).each do |service|
         ohai "Removing launchctl service #{service}"
         [false, true].each do |with_sudo|
-          xml_status = @command.run('/bin/launchctl', :args => ['list', '-x', service], :sudo => with_sudo)
+          xml_status = @command.run('/bin/launchctl', :args => ['list', '-x', service], :sudo => with_sudo).stdout
           if %r{^<\?xml}.match(xml_status)
             @command.run('/bin/launchctl',  :args => ['unload', '-w', '--', service], :sudo => with_sudo)
             sleep 1
@@ -80,7 +80,7 @@ def dispatch_uninstall_directives(stanza, expand_tilde=false)
     directives_set.select{ |h| h.key?(:quit) }.each do |directives|
       Array(directives[:quit]).each do |id|
         ohai "Quitting application ID #{id}"
-        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
+        num_running = @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application "System Events" to count processes whose bundle identifier is "#{id}"}], :sudo => true).stdout.to_i
         if num_running > 0
           @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application id "#{id}" to quit}], :sudo => true)
           sleep 3
@@ -94,7 +94,7 @@ def dispatch_uninstall_directives(stanza, expand_tilde=false)
         raise CaskInvalidError.new(@cask, "Each #{stanza} :signal must have 2 elements.") unless pair.length == 2
         signal, id = pair
         ohai "Signalling '#{signal}' to application ID '#{id}'"
-        pid_string = @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application "System Events" to get the unix id of every process whose bundle identifier is "#{id}"}], :sudo => true)
+        pid_string = @command.run!('/usr/bin/osascript', :args => ['-e', %Q{tell application "System Events" to get the unix id of every process whose bundle identifier is "#{id}"}], :sudo => true).stdout
         if pid_string.match(%r{\A\d+(?:\s*,\s*\d+)*\Z})    # sanity check
           pids = pid_string.split(%r{\s*,\s*}).map(&:strip).map(&:to_i)
           if pids.length > 0
@@ -116,7 +116,7 @@ def dispatch_uninstall_directives(stanza, expand_tilde=false)
     directives_set.select{ |h| h.key?(:kext) }.each do |directives|
       Array(directives[:kext]).each do |kext|
         ohai "Unloading kernel extension #{kext}"
-        is_loaded = @command.run!('/usr/sbin/kextstat', :args => ['-l', '-b', kext], :sudo => true)
+        is_loaded = @command.run!('/usr/sbin/kextstat', :args => ['-l', '-b', kext], :sudo => true).stdout
         if is_loaded.length > 1
           @command.run!('/sbin/kextunload', :args => ['-b', '--', kext], :sudo => true)
           sleep 1
@@ -129,7 +129,7 @@ def dispatch_uninstall_directives(stanza, expand_tilde=false)
       executable, script_arguments = self.class.read_script_arguments(directives,
                                                                       'uninstall',
                                                                       {:must_succeed => true},
-                                                                      {:sudo => true, :print => true},
+                                                                      {:sudo => true, :print_stdout => true},
                                                                       :script)
       raise CaskInvalidError.new(@cask, "#{stanza} :script without :executable.") if executable.nil?
       @command.run(@cask.destination_path.join(executable), script_arguments)
@@ -185,10 +185,10 @@ def dispatch_uninstall_directives(stanza, expand_tilde=false)
         next unless directory.exist?
         @command.run!('/bin/rm', :args => [ '-f', '--', directory.join('.DS_Store') ],
                                  :sudo => true,
-                                 :stderr => :silence)
+                                 :print_stderr => false)
         @command.run('/bin/rmdir', :args => [ '--', directory ],
                                    :sudo => true,
-                                   :stderr => :silence)
+                                   :print_stderr => false)
       end
     end
   end
diff --git a/lib/cask/cli/alfred.rb b/lib/cask/cli/alfred.rb
@@ -154,7 +154,7 @@ def self.alfred_primary_preference(key, value=nil)
       @system_command.run('/usr/bin/defaults', :args => ['write', PRIMARY_DOMAIN, key, value.to_s])
     else
       odebug 'Reading Alfred primary preferences'
-      @system_command.run('/usr/bin/defaults', :args => ['read', PRIMARY_DOMAIN, key])
+      @system_command.run('/usr/bin/defaults', :args => ['read', PRIMARY_DOMAIN, key]).stdout
     end
   end
 
@@ -168,7 +168,7 @@ def self.alfred_file_preference(files, key, value=nil)
         @system_command.run('/usr/bin/defaults', :args => ['write', file, key, value.to_s])
       else
         odebug 'Reading Alfred local preferences'
-        @system_command.run('/usr/bin/defaults', :args => ['read', file, key])
+        @system_command.run('/usr/bin/defaults', :args => ['read', file, key]).stdout
       end
     # hack/limitation: if there happen to be multiple local preference
     # files, we only return the value from the first one.
diff --git a/lib/cask/cli/doctor.rb b/lib/cask/cli/doctor.rb
@@ -65,7 +65,7 @@ def self.homebrew_origin
       HOMEBREW_REPOSITORY.cd do
         homebrew_origin = Cask::SystemCommand.run('git',
                                                   :args => %w{config --get remote.origin.url},
-                                                  :stderr => :silence).strip
+                                                  :print_stderr => false).stdout.strip
       end
       if homebrew_origin !~ %r{\S}
         homebrew_origin = "#{none_string} #{error_string}"
diff --git a/lib/cask/cli/list.rb b/lib/cask/cli/list.rb
@@ -54,7 +54,7 @@ def self.list_installed
     if @options[:one]
       puts columns
     elsif @options[:long]
-      puts Cask::SystemCommand.run!("/bin/ls", :args => ["-l", Cask.caskroom])
+      puts Cask::SystemCommand.run!("/bin/ls", :args => ["-l", Cask.caskroom]).stdout
     else
       puts_columns columns
     end
diff --git a/lib/cask/container/criteria.rb b/lib/cask/container/criteria.rb
@@ -7,27 +7,25 @@ def initialize(path, command)
   end
 
   def file
-    @file ||= @command.run('/usr/bin/file', :args => ['-Izb', '--', path])
+    @file ||= @command.run('/usr/bin/file', :args => ['-Izb', '--', path]).stdout
   end
 
   def imageinfo
     @imageinfo ||= @command.run(
       '/usr/bin/hdiutil',
       # realpath is a failsafe against unusual filenames
       :args => ['imageinfo', Pathname.new(path).realpath],
-      :stderr => :silence,
-      :print => false
-    )
+      :print_stderr => false
+    ).stdout
   end
 
   def cabextract
     if HOMEBREW_PREFIX.join('bin/cabextract').exist?
       @cabextract ||= @command.run(
         HOMEBREW_PREFIX.join('bin/cabextract'),
         :args => ['-t', '--', path],
-        :stderr => :silence,
-        :print => false
-      )
+        :print_stderr => false
+      ).stdout
     end
   end
 
@@ -36,9 +34,8 @@ def lsar
       @lsar ||= @command.run(
         HOMEBREW_PREFIX.join('bin/lsar'),
         :args => ['-l', '-t', '--', path],
-        :stderr => :silence,
-        :print => false
-      )
+        :print_stderr => false
+      ).stdout
     end
   end
 
diff --git a/lib/cask/container/dmg.rb b/lib/cask/container/dmg.rb
@@ -23,9 +23,8 @@ def mount!
     plist = @command.run('/usr/bin/hdiutil',
       # realpath is a failsafe against unusual filenames
       :args => %w[mount -plist -nobrowse -readonly -noidme -mountrandom /tmp] + [Pathname.new(@path).realpath],
-      :input => %w[y],
-      :plist => true
-    )
+      :input => %w[y]
+    ).plist
     @mounts = mounts_from_plist(plist)
   end
 
@@ -38,7 +37,7 @@ def mounts_from_plist(plist)
 
   def assert_mounts_found
     if @mounts.empty?
-      raise CaskError.new "No mounts found in '#{@path}'; perhaps it is a bad DMG?"
+      raise CaskError.new %Q{No mounts found in '#{@path}'; perhaps it is a bad DMG?}
     end
   end
 
@@ -48,13 +47,13 @@ def eject!
       mountpath = Pathname.new(mount).realpath
       next unless mountpath.exist?
       @command.run('/usr/sbin/diskutil',
-                     :args => ['eject', mountpath],
-                     :stderr => :silence)
+                   :args => ['eject', mountpath],
+                   :print_stderr => false)
       next unless mountpath.exist?
       sleep 1
       @command.run('/usr/sbin/diskutil',
-                     :args => ['eject', mountpath],
-                     :stderr => :silence)
+                   :args => ['eject', mountpath],
+                   :print_stderr => false)
       next unless mountpath.exist?
       raise CaskError.new "Failed to eject #{mountpath}"
     end
diff --git a/lib/cask/download_strategy.rb b/lib/cask/download_strategy.rb
@@ -138,7 +138,7 @@ def fetch_repo target, url, revision=cask_url.revision, ignore_externals=false
     args << '--ignore-externals' if ignore_externals
     @command.run!('/usr/bin/svn',
                   :args => args,
-                  :stderr => :silence)
+                  :print_stderr => false)
   end
 
   def tarball_path
@@ -165,7 +165,7 @@ def tarball_path
   def compress
     Dir.chdir(cached_location) do
       @command.run!('/usr/bin/tar', :args => ['-s/^\.//', '--exclude', '.svn', '-cf', Pathname.new(tarball_path), '--', '.'],
-                                    :stderr => :silence)
+                                    :print_stderr => false)
     end
     clear_cache
   end
diff --git a/lib/cask/fetcher.rb b/lib/cask/fetcher.rb
@@ -8,7 +8,7 @@ def self.head(url)
       googlecode_fake_head(url)
     else
       Cask::SystemCommand.run("curl",
-        :args => ["--max-time", TIMEOUT, "--silent", "--location", "--head", url])
+                              :args => ["--max-time", TIMEOUT, "--silent", "--location", "--head", url])
     end
   end
 
diff --git a/lib/cask/installer.rb b/lib/cask/installer.rb
@@ -106,7 +106,7 @@ def formula_dependencies
         print "#{dep_name} ... "
         installed = @command.run(HOMEBREW_BREW_FILE,
                                  :args => ['list', '--versions', dep_name],
-                                 :stderr => :silence).include?(dep_name)
+                                 :print_stderr => false).stdout.include?(dep_name)
         if installed
           puts "already installed"
         else
diff --git a/lib/cask/pkg.rb b/lib/cask/pkg.rb
@@ -227,7 +227,7 @@ class Cask::Pkg
                 ].map{|x| Pathname.new(x)}
 
   def self.all_matching(regexp, command)
-    command.run('/usr/sbin/pkgutil', :args => [%Q{--pkgs=#{regexp}}]).split("\n").map do |package_id|
+    command.run('/usr/sbin/pkgutil', :args => [%Q{--pkgs=#{regexp}}]).stdout.split("\n").map do |package_id|
       new(package_id.chomp, command)
     end
   end
@@ -269,19 +269,19 @@ def forget
   def pkgutil_bom_files
     @command.run!('/usr/sbin/pkgutil',
       :args => ['--only-files', '--files', package_id]
-    ).split("\n").map { |path| root.join(path) }
+    ).stdout.split("\n").map { |path| root.join(path) }
   end
 
   def pkgutil_bom_dirs
     @command.run!('/usr/sbin/pkgutil',
       :args => ['--only-dirs', '--files', package_id]
-    ).split("\n").map { |path| root.join(path) }
+    ).stdout.split("\n").map { |path| root.join(path) }
   end
 
   def pkgutil_bom_all
     @command.run!('/usr/sbin/pkgutil',
       :args => ['--files', package_id]
-    ).split("\n").map { |path| root.join(path) }
+    ).stdout.split("\n").map { |path| root.join(path) }
   end
 
   def pkgutil_bom_specials
@@ -294,9 +294,8 @@ def root
 
   def info
     @command.run!('/usr/sbin/pkgutil',
-      :args => ['--pkg-info-plist', package_id],
-      :plist => true
-    )
+                  :args => ['--pkg-info-plist', package_id]
+                 ).plist
   end
 
   def _rmdir(path)
diff --git a/lib/cask/system_command.rb b/lib/cask/system_command.rb
diff --git a/lib/cask/utils.rb b/lib/cask/utils.rb
diff --git a/test/cask/artifact/alt_target_test.rb b/test/cask/artifact/alt_target_test.rb
diff --git a/test/cask/dsl/after_install_test.rb b/test/cask/dsl/after_install_test.rb
diff --git a/test/cask/pkg_test.rb b/test/cask/pkg_test.rb
diff --git a/test/support/fake_system_command.rb b/test/support/fake_system_command.rb
diff --git a/test/test_helper.rb b/test/test_helper.rb

Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ def install_phase`
`16`	`16`	`artifact,`
`17`	`17`	`"#{self.class.artifact_dsl_key}",`
`18`	`18`	`{:must_succeed => true},`
`19`		`- {:print => true}`
	`19`	`+ {:print_stdout => true}`
`20`	`20`	`)`
`21`	`21`	`ohai "Running #{self.class.artifact_dsl_key} script #{executable}"`
`22`	`22`	`raise CaskInvalidError.new(@cask, "#{self.class.artifact_dsl_key} missing executable") if executable.nil?`
Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,6 @@ def run_installer(pkg_description)`
`48`	`48`	`]`
`49`	`49`	`args << '-verboseR' if ARGV.verbose?`
`50`	`50`	`args << '-allowUntrusted' if pkg_install_opts :allow_untrusted`
`51`		`- @command.run!('/usr/sbin/installer', {:sudo => true, :args => args, :print => true})`
	`51`	`+ @command.run!('/usr/sbin/installer', {:sudo => true, :args => args, :print_stdout => true})`
`52`	`52`	`end`
`53`	`53`	`end`