Merge pull request #2299 from reitermarkus/cask-refactor-artifacts

Refactor artifacts.

7 files changed, 78 insertions, 102 deletions

diff --git a/Library/Homebrew/cask/lib/hbc/artifact/base.rb b/Library/Homebrew/cask/lib/hbc/artifact/base.rb
index a8a17c081..924493fc0 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/base.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/base.rb
@@ -32,10 +32,7 @@ module Hbc
       def self.read_script_arguments(arguments, stanza, default_arguments = {}, override_arguments = {}, key = nil)
         # TODO: when stanza names are harmonized with class names,
         #       stanza may not be needed as an explicit argument
-        description = stanza.to_s
-        if key
-          description.concat(" #{key.inspect}")
-        end
+        description = key ? "#{stanza} #{key.inspect}" : stanza.to_s
 
         # backward-compatible string value
         arguments = { executable: arguments } if arguments.is_a?(String)
diff --git a/Library/Homebrew/cask/lib/hbc/artifact/binary.rb b/Library/Homebrew/cask/lib/hbc/artifact/binary.rb
index 395ab5c5c..06bdfe157 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/binary.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/binary.rb
@@ -7,8 +7,8 @@ module Hbc
         super if CLI.binaries?
       end
 
-      def link(artifact_spec)
-        super(artifact_spec)
+      def link
+        super
         FileUtils.chmod "+x", source
       end
     end
diff --git a/Library/Homebrew/cask/lib/hbc/artifact/moved.rb b/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
index 64756fd93..01e98ac35 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/moved.rb
@@ -8,61 +8,38 @@ module Hbc
       end
 
       def install_phase
-        each_artifact do |artifact|
-          load_specification(artifact)
-          next unless preflight_checks
-          delete if Utils.path_occupied?(target) && force
-          move
-        end
+        each_artifact(&method(:move))
       end
 
       def uninstall_phase
-        each_artifact do |artifact|
-          load_specification(artifact)
-          next unless File.exist?(target)
-          delete
-        end
+        each_artifact(&method(:delete))
       end
 
       private
 
-      def each_artifact
-        # the sort is for predictability between Ruby versions
-        @cask.artifacts[self.class.artifact_dsl_key].sort.each do |artifact|
-          yield artifact
-        end
-      end
-
       def move
-        ohai "Moving #{self.class.artifact_english_name} '#{source.basename}' to '#{target}'"
-        target.dirname.mkpath
-        FileUtils.move(source, target)
-        add_altname_metadata target, source.basename.to_s
-      end
-
-      def preflight_checks
         if Utils.path_occupied?(target)
-          raise CaskError, warning_target_exists << "." unless force
-          opoo(warning_target_exists { |s| s << "overwriting." })
+          message = "It seems there is already #{self.class.artifact_english_article} #{self.class.artifact_english_name} at '#{target}'"
+          raise CaskError, "#{message}." unless force
+          opoo "#{message}; overwriting."
+          delete
         end
+
         unless source.exist?
-          message = "It seems the #{self.class.artifact_english_name} source is not there: '#{source}'"
-          raise CaskError, message
+          raise CaskError, "It seems the #{self.class.artifact_english_name} source '#{source}' is not there."
         end
-        true
-      end
 
-      def warning_target_exists
-        message_parts = [
-          "It seems there is already #{self.class.artifact_english_article} #{self.class.artifact_english_name} at '#{target}'",
-        ]
-        yield(message_parts) if block_given?
-        message_parts.join("; ")
+        ohai "Moving #{self.class.artifact_english_name} '#{source.basename}' to '#{target}'."
+        target.dirname.mkpath
+        FileUtils.move(source, target)
+        add_altname_metadata target, source.basename.to_s
       end
 
       def delete
-        ohai "Removing #{self.class.artifact_english_name}: '#{target}'"
-        raise CaskError, "Cannot remove undeletable #{self.class.artifact_english_name}" if MacOS.undeletable?(target)
+        ohai "Removing #{self.class.artifact_english_name} '#{target}'."
+        return unless Utils.path_occupied?(target)
+
+        raise CaskError, "Cannot remove undeletable #{self.class.artifact_english_name}." if MacOS.undeletable?(target)
 
         if force
           Utils.gain_permissions_remove(target, command: @command)
diff --git a/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb b/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb
index bd68ccad0..e92b16227 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/relocated.rb
@@ -42,6 +42,14 @@ module Hbc
                       print_stderr: false)
       end
 
+      def each_artifact
+        # the sort is for predictability between Ruby versions
+        @cask.artifacts[self.class.artifact_dsl_key].sort.each do |artifact|
+          load_specification(artifact)
+          yield
+        end
+      end
+
       def load_specification(artifact_spec)
         source_string, target_hash = artifact_spec
         raise CaskInvalidError if source_string.nil?
diff --git a/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb b/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb
index 2cd172ad3..16715fe81 100644
--- a/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb
+++ b/Library/Homebrew/cask/lib/hbc/artifact/symlinked.rb
@@ -11,55 +11,48 @@ module Hbc
         "#{artifact_english_name} #{link_type_english_name}s"
       end
 
-      def self.islink?(path)
-        path.symlink?
-      end
-
-      def link(artifact_spec)
-        load_specification artifact_spec
-        return unless preflight_checks(source, target)
-        ohai "#{self.class.link_type_english_name}ing #{self.class.artifact_english_name} '#{source.basename}' to '#{target}'"
-        create_filesystem_link(source, target)
-      end
-
-      def unlink(artifact_spec)
-        load_specification artifact_spec
-        return unless self.class.islink?(target)
-        ohai "Removing #{self.class.artifact_english_name} #{self.class.link_type_english_name.downcase}: '#{target}'"
-        target.delete
-      end
-
       def install_phase
-        @cask.artifacts[self.class.artifact_dsl_key].each(&method(:link))
+        each_artifact(&method(:link))
       end
 
       def uninstall_phase
-        @cask.artifacts[self.class.artifact_dsl_key].each(&method(:unlink))
+        each_artifact(&method(:unlink))
       end
 
-      def preflight_checks(source, target)
-        if target.exist? && !self.class.islink?(target)
-          raise CaskError, "It seems there is already #{self.class.artifact_english_article} #{self.class.artifact_english_name} at '#{target}'; not linking."
-        end
+      private
+
+      def link
         unless source.exist?
-          raise CaskError, "It seems the #{self.class.link_type_english_name.downcase} source is not there: '#{source}'"
+          raise CaskError, "It seems the #{self.class.link_type_english_name.downcase} source '#{source}' is not there."
+        end
+
+        if target.exist? && !target.symlink?
+          raise CaskError, "It seems there is already #{self.class.artifact_english_article} #{self.class.artifact_english_name} at '#{target}'; not linking."
         end
-        true
+
+        ohai "Linking #{self.class.artifact_english_name} '#{source.basename}' to '#{target}'."
+        create_filesystem_link(source, target)
+      end
+
+      def unlink
+        return unless target.symlink?
+        ohai "Unlinking #{self.class.artifact_english_name} '#{target}'."
+        target.delete
       end
 
       def create_filesystem_link(source, target)
-        Pathname.new(target).dirname.mkpath
-        @command.run!("/bin/ln", args: ["-hfs", "--", source, target])
+        target.dirname.mkpath
+        @command.run!("/bin/ln", args: ["-h", "-f", "-s", "--", source, target])
         add_altname_metadata source, target.basename.to_s
       end
 
       def summarize_artifact(artifact_spec)
         load_specification artifact_spec
 
-        if self.class.islink?(target) && target.exist? && target.readlink.exist?
+        if target.symlink? && target.exist? && target.readlink.exist?
           "#{printable_target} -> #{target.readlink} (#{target.readlink.abv})"
         else
-          string = if self.class.islink?(target)
+          string = if target.symlink?
             "#{printable_target} -> #{target.readlink}"
           else
             printable_target
diff --git a/Library/Homebrew/test/cask/artifact/app_spec.rb b/Library/Homebrew/test/cask/artifact/app_spec.rb
index bfd2d5cd4..54e6ae5bf 100644
--- a/Library/Homebrew/test/cask/artifact/app_spec.rb
+++ b/Library/Homebrew/test/cask/artifact/app_spec.rb
@@ -7,8 +7,8 @@ describe Hbc::Artifact::App, :cask do
   let(:source_path) { cask.staged_path.join("Caffeine.app") }
   let(:target_path) { Hbc.appdir.join("Caffeine.app") }
 
-  let(:install_phase) { -> { app.install_phase } }
-  let(:uninstall_phase) { -> { app.uninstall_phase } }
+  let(:install_phase) { app.install_phase }
+  let(:uninstall_phase) { app.uninstall_phase }
 
   before(:each) do
     InstallHelper.install_without_artifacts(cask)
@@ -17,7 +17,7 @@ describe Hbc::Artifact::App, :cask do
   describe "install_phase" do
     it "installs the given app using the proper target directory" do
       shutup do
-        install_phase.call
+        install_phase
       end
 
       expect(target_path).to be_a_directory
@@ -40,7 +40,7 @@ describe Hbc::Artifact::App, :cask do
         FileUtils.mv(source_path, appsubdir)
 
         shutup do
-          install_phase.call
+          install_phase
         end
 
         expect(target_path).to be_a_directory
@@ -53,7 +53,7 @@ describe Hbc::Artifact::App, :cask do
       FileUtils.cp_r source_path, staged_app_copy
 
       shutup do
-        install_phase.call
+        install_phase
       end
 
       expect(target_path).to be_a_directory
@@ -69,7 +69,7 @@ describe Hbc::Artifact::App, :cask do
       end
 
       it "avoids clobbering an existing app" do
-        expect(install_phase).to raise_error(Hbc::CaskError, "It seems there is already an App at '#{target_path}'.")
+        expect { install_phase }.to raise_error(Hbc::CaskError, "It seems there is already an App at '#{target_path}'.")
 
         expect(source_path).to be_a_directory
         expect(target_path).to be_a_directory
@@ -89,17 +89,17 @@ describe Hbc::Artifact::App, :cask do
         describe "target is both writable and user-owned" do
           it "overwrites the existing app" do
             stdout = <<-EOS.undent
-              ==> Removing App: '#{target_path}'
-              ==> Moving App 'Caffeine.app' to '#{target_path}'
+              ==> Removing App '#{target_path}'.
+              ==> Moving App 'Caffeine.app' to '#{target_path}'.
             EOS
 
             stderr = <<-EOS.undent
               Warning: It seems there is already an App at '#{target_path}'; overwriting.
             EOS
 
-            expect {
-              expect(install_phase).to output(stdout).to_stdout
-            }.to output(stderr).to_stderr
+            expect { install_phase }
+              .to output(stdout).to_stdout
+              .and output(stderr).to_stderr
 
             expect(source_path).not_to exist
             expect(target_path).to be_a_directory
@@ -124,17 +124,17 @@ describe Hbc::Artifact::App, :cask do
               .and_call_original
 
             stdout = <<-EOS.undent
-              ==> Removing App: '#{target_path}'
-              ==> Moving App 'Caffeine.app' to '#{target_path}'
+              ==> Removing App '#{target_path}'.
+              ==> Moving App 'Caffeine.app' to '#{target_path}'.
             EOS
 
             stderr = <<-EOS.undent
               Warning: It seems there is already an App at '#{target_path}'; overwriting.
             EOS
 
-            expect {
-              expect(install_phase).to output(stdout).to_stdout
-            }.to output(stderr).to_stderr
+            expect { install_phase }
+              .to output(stdout).to_stdout
+              .and output(stderr).to_stderr
 
             expect(source_path).not_to exist
             expect(target_path).to be_a_directory
@@ -160,7 +160,7 @@ describe Hbc::Artifact::App, :cask do
       end
 
       it "leaves the target alone" do
-        expect(install_phase).to raise_error(Hbc::CaskError, "It seems there is already an App at '#{target_path}'.")
+        expect { install_phase }.to raise_error(Hbc::CaskError, "It seems there is already an App at '#{target_path}'.")
         expect(target_path).to be_a_symlink
       end
 
@@ -169,17 +169,17 @@ describe Hbc::Artifact::App, :cask do
 
         it "overwrites the existing app" do
           stdout = <<-EOS.undent
-            ==> Removing App: '#{target_path}'
-            ==> Moving App 'Caffeine.app' to '#{target_path}'
+            ==> Removing App '#{target_path}'.
+            ==> Moving App 'Caffeine.app' to '#{target_path}'.
           EOS
 
           stderr = <<-EOS.undent
             Warning: It seems there is already an App at '#{target_path}'; overwriting.
           EOS
 
-          expect {
-            expect(install_phase).to output(stdout).to_stdout
-          }.to output(stderr).to_stderr
+          expect { install_phase }
+            .to output(stdout).to_stdout
+            .and output(stderr).to_stderr
 
           expect(source_path).not_to exist
           expect(target_path).to be_a_directory
@@ -193,22 +193,22 @@ describe Hbc::Artifact::App, :cask do
     it "gives a warning if the source doesn't exist" do
       source_path.rmtree
 
-      message = "It seems the App source is not there: '#{source_path}'"
+      message = "It seems the App source '#{source_path}' is not there."
 
-      expect(install_phase).to raise_error(Hbc::CaskError, message)
+      expect { install_phase }.to raise_error(Hbc::CaskError, message)
     end
   end
 
   describe "uninstall_phase" do
     it "deletes managed apps" do
       shutup do
-        install_phase.call
+        install_phase
       end
 
       expect(target_path).to exist
 
       shutup do
-        uninstall_phase.call
+        uninstall_phase
       end
 
       expect(target_path).not_to exist
@@ -226,7 +226,7 @@ describe Hbc::Artifact::App, :cask do
     describe "app is correctly installed" do
       it "returns the path to the app" do
         shutup do
-          install_phase.call
+          install_phase
         end
 
         expect(contents).to eq(["#{target_path} (#{target_path.abv})"])
diff --git a/Library/Homebrew/test/cask/cli/uninstall_spec.rb b/Library/Homebrew/test/cask/cli/uninstall_spec.rb
index cbfb3e237..fb196ee72 100644
--- a/Library/Homebrew/test/cask/cli/uninstall_spec.rb
+++ b/Library/Homebrew/test/cask/cli/uninstall_spec.rb
@@ -47,6 +47,7 @@ describe Hbc::CLI::Uninstall, :cask do
     end
 
     expect(cask).to be_installed
+    expect(Hbc.appdir.join("MyFancyApp.app")).to exist
 
     expect {
       shutup do