require most commands to succeed; cleanup on install failure

- add `run!` method which raises if command does not succeed
- use `run!` when the command we are running must succeed for things to
  move forward. this should help produce clearer error messages in
  failure scenarios.
- move caveats earlier in the install process so reports can be made
  about potential failures
- remove the destination tree on cask install failure, so the cask will
  not be considered installed

lib/cask/artifact/app.rb

@@ -17,7 +17,7 @@ class Cask::Artifact::App < Cask::Artifact::Base
 
     return unless preflight_checks(source, target)
     ohai "Linking #{source.basename} to #{target}"
-    @command.run('/bin/ln', :args => ['-hfs', source, target])
+    @command.run!('/bin/ln', :args => ['-hfs', source, target])
   end
 
   def preflight_checks(source, target)

lib/cask/artifact/pkg.rb

@@ -13,7 +13,7 @@ class Cask::Artifact::Pkg < Cask::Artifact::Base
 
   def run_installer(pkg_relative_path)
     ohai "Running installer for #{@cask}; your password may be necessary."
-    @command.run("installer", {
+    @command.run!("installer", {
       :sudo => true,
       :args => %W[-pkg #{@cask.destination_path.join(pkg_relative_path)} -target /],
       :print => true
@@ -23,7 +23,7 @@ class Cask::Artifact::Pkg < Cask::Artifact::Base
   def manually_uninstall(uninstall_options)
     ohai "Running uninstall process for #{@cask}; your password may be necessary."
     if uninstall_options.key? :script
-      @command.run(
+      @command.run!(
         @cask.destination_path.join(uninstall_options[:script]),
         uninstall_options.merge(:sudo => true, :print => true)
       )
@@ -32,7 +32,7 @@ class Cask::Artifact::Pkg < Cask::Artifact::Base
     if uninstall_options.key? :kext
       [*uninstall_options[:kext]].each do |kext|
         ohai "Unloading kernel extension #{kext}"
-        @command.run('kextunload', :args => ['-b', kext], :sudo => true)
+        @command.run!('kextunload', :args => ['-b', kext], :sudo => true)
       end
     end
 
@@ -44,14 +44,14 @@ class Cask::Artifact::Pkg < Cask::Artifact::Base
     if uninstall_options.key? :launchctl
       [*uninstall_options[:launchctl]].each do |service|
         ohai "Removing launchctl service #{service}"
-        @command.run('launchctl', :args => ['remove', service], :sudo => true)
+        @command.run!('launchctl', :args => ['remove', service], :sudo => true)
       end
     end
 
     if uninstall_options.key? :files
       uninstall_options[:files].each do |file|
         ohai "Removing file #{file}"
-        @command.run('rm', :args => ['-rf', file], :sudo => true)
+        @command.run!('rm', :args => ['-rf', file], :sudo => true)
       end
     end
   end

lib/cask/artifact/prefpane.rb

@@ -17,7 +17,7 @@ class Cask::Artifact::Prefpane < Cask::Artifact::Base
 
     return unless preflight_checks(source, target)
     ohai "Linking prefPane #{source.basename} to #{target}"
-    @command.run('/bin/ln', :args => ['-hfs', source, target])
+    @command.run!('/bin/ln', :args => ['-hfs', source, target])
   end
 
   def preflight_checks(source, target)

lib/cask/container/dmg.rb

@@ -20,11 +20,11 @@ class Cask::Container::Dmg < Cask::Container::Base
   end
 
   def mount!
-    plist = @command.run('hdiutil', :args => [
-      'mount',
-      '-plist', '-nobrowse', '-readonly', '-noidme', '-mountrandom',
-      '/tmp', @path
-    ], :plist => true, :input => ['y'])
+    plist = @command.run!('hdiutil',
+      :args => %w[mount -plist -nobrowse -readonly -noidme -mountrandom /tmp] + [@path],
+      :input => %w[y],
+      :plist => true
+    )
     @mounts = mounts_from_plist(plist)
   end
 
@@ -42,7 +42,7 @@ class Cask::Container::Dmg < Cask::Container::Base
 
   def eject!
     @mounts.each do |mount|
-      @command.run('hdiutil', :args => ['eject', mount])
+      @command.run!('hdiutil', :args => ['eject', mount])
     end
   end
 end

lib/cask/container/naked.rb

@@ -6,7 +6,7 @@ class Cask::Container::Naked < Cask::Container::Base
   end
 
   def extract
-    @command.run('ditto', :args => [@path, @cask.destination_path.join(target_file)])
+    @command.run!('ditto', :args => [@path, @cask.destination_path.join(target_file)])
   end
 
   def target_file

lib/cask/container/tar.rb

@@ -7,11 +7,8 @@ class Cask::Container::Tar < Cask::Container::Base
 
   def extract
     Dir.mktmpdir do |staging_dir|
-      @command.run('tar', :args => [
-        'xf', @path,
-        '-C', staging_dir,
-      ])
-      @command.run('ditto', :args => [staging_dir, @cask.destination_path])
+      @command.run!('tar', :args => ['xf', @path, '-C', staging_dir])
+      @command.run!('ditto', :args => [staging_dir, @cask.destination_path])
     end
   end
 end

lib/cask/container/zip.rb

@@ -7,13 +7,8 @@ class Cask::Container::Zip < Cask::Container::Base
 
   def extract
     Dir.mktmpdir do |staging_dir|
-      @command.run('unzip', :args => [
-        '-qq',
-        '-d', staging_dir,
-        @path,
-        '-x', '__MACOSX/*'
-      ])
-      @command.run('ditto', :args => [staging_dir, @cask.destination_path])
+      @command.run!('unzip', :args => ['-qq', '-d', staging_dir, @path, '-x', '__MACOSX/*'])
+      @command.run!('ditto', :args => [staging_dir, @cask.destination_path])
     end
   end
 end

lib/cask/exceptions.rb

@@ -43,3 +43,22 @@ class CaskAlreadyInstalledError < CaskError
     "Cask for #{name} is already installed. Use `--force` to install anyways."
   end
 end
+
+class CaskCommandFailedError < CaskError
+  def initialize cmd, output
+    @cmd = cmd
+    @output = output
+  end
+
+  def to_s;
+    <<-EOS.undent
+      Command failed to execute!
+
+      ==> Failed command:
+      #{@cmd}
+
+      ==> Output of failed command:
+      #{@output}
+    EOS
+  end
+end

lib/cask/installer.rb

@@ -11,13 +11,16 @@ class Cask::Installer
       raise CaskAlreadyInstalledError.new(@cask)
     end
 
+    print_caveats
+
     download
     extract_primary_container
     install_artifacts
 
     ohai "Success! #{@cask} installed to #{@cask.destination_path}"
-
-    print_caveats
+  rescue
+    purge_files
+    raise
   end
 
 
@@ -61,6 +64,6 @@ class Cask::Installer
   end
 
   def purge_files
-    @cask.destination_path.rmtree
+    @cask.destination_path.rmtree if @cask.destination_path.exist?
   end
 end

lib/cask/pkg.rb

@@ -14,29 +14,29 @@ class Cask::Pkg
 
   def uninstall
     files.each do |file|
-      @command.run('rm', :args => [file], :sudo => true) if file.exist?
+      @command.run!('rm', :args => [file], :sudo => true) if file.exist?
     end
     _deepest_path_first(dirs).each do |dir|
-      @command.run('chmod', :args => ['777', dir], :sudo => true)
+      @command.run!('chmod', :args => ['777', dir], :sudo => true)
       _clean_symlinks(dir)
       _clean_ds_store(dir)
-      @command.run('rmdir', :args => [dir], :sudo => true) if dir.exist? && dir.children.empty?
+      @command.run!('rmdir', :args => [dir], :sudo => true) if dir.exist? && dir.children.empty?
     end
     forget
   end
 
   def forget
-    @command.run('pkgutil', :args => ['--forget', package_id], :sudo => true)
+    @command.run!('pkgutil', :args => ['--forget', package_id], :sudo => true)
   end
 
   def dirs
-    @command.run('pkgutil',
+    @command.run!('pkgutil',
       :args => ['--only-dirs', '--files', package_id]
     ).split("\n").map { |path| root.join(path) }
   end
 
   def files
-    @command.run('pkgutil',
+    @command.run!('pkgutil',
       :args => ['--only-files', '--files', package_id]
     ).split("\n").map { |path| root.join(path) }
   end
@@ -46,7 +46,7 @@ class Cask::Pkg
   end
 
   def info
-    @command.run('pkgutil',
+    @command.run!('pkgutil',
       :args => ['--pkg-info-plist', package_id],
       :plist => true
     )
@@ -64,14 +64,12 @@ class Cask::Pkg
     # uninstall
     return unless dir.exist?
     dir.children.each do |child|
-      @command.run('rm', :args => [child], :sudo => true) if child.symlink?
+      @command.run!('rm', :args => [child], :sudo => true) if child.symlink?
     end
   end
 
   def _clean_ds_store(dir)
-    # Clean .DS_Store files:
-    # https://en.wikipedia.org/wiki/.DS_Store
     ds_store = dir.join('.DS_Store')
-    @command.run('rm', :args => [ds_store], :sudo => true) if ds_store.exist?
+    @command.run!('rm', :args => [ds_store], :sudo => true) if ds_store.exist?
   end
 end

lib/cask/system_command.rb

@@ -12,6 +12,7 @@ class Cask::SystemCommand
         ohai line.chomp if options[:print]
       end
     end
+    _assert_success($?, command, output) if options[:must_succeed]
     if options[:plist]
       Plist::parse_xml(output)
     else
@@ -19,6 +20,10 @@ class Cask::SystemCommand
     end
   end
 
+  def self.run!(command, options)
+    run(command, options.merge(:must_succeed => true))
+  end
+
   def self._process_options(command, options)
     if options[:sudo]
       command = "sudo -E #{_quote(command)}"
@@ -35,6 +40,12 @@ class Cask::SystemCommand
     command
   end
 
+  def self._assert_success(status, command, output)
+    unless status.success?
+      raise CaskCommandFailedError.new(command, output)
+    end
+  end
+
   def self._quote(string)
     %Q('#{string}')
   end

test/support/fake_system_command.rb

@@ -31,6 +31,10 @@ class Cask::FakeSystemCommand
       @responses[command]
     end
   end
+
+  def self.run!(*args)
+    run(*args)
+  end
 end
 
 module FakeSystemCommandHooks