Merge pull request #5685 from reitermarkus/uninstall-rmdir

Run `uninstall rmdir:` after uninstalling artifacts.

Library/Homebrew/cask/artifact/abstract_uninstall.rb

@@ -2,6 +2,8 @@ require "timeout"
 
 require "utils/user"
 require "cask/artifact/abstract_artifact"
+require "extend/hash_validator"
+using HashValidator
 
 module Cask
   module Artifact
@@ -27,6 +29,8 @@ module Cask
       attr_reader :directives
 
       def initialize(cask, directives)
+        directives.assert_valid_keys!(*ORDERED_DIRECTIVES)
+
         super(cask)
         directives[:signal] = [*directives[:signal]].flatten.each_slice(2).to_a
         @directives = directives
@@ -49,30 +53,23 @@ module Cask
       private
 
       def dispatch_uninstall_directives(**options)
-        ohai "Running #{stanza} process for #{@cask}; your password may be necessary"
-
-        warn_for_unknown_directives(directives)
-
         ORDERED_DIRECTIVES.each do |directive_sym|
-          next unless directives.key?(directive_sym)
+          dispatch_uninstall_directive(directive_sym, **options)
-
-          args = directives[directive_sym]
-          send("uninstall_#{directive_sym}", *(args.is_a?(Hash) ? [args] : args), **options)
         end
       end
 
+      def dispatch_uninstall_directive(directive_sym, **options)
+        return unless directives.key?(directive_sym)
+
+        args = directives[directive_sym]
+
+        send("uninstall_#{directive_sym}", *(args.is_a?(Hash) ? [args] : args), **options)
+      end
+
       def stanza
         self.class.dsl_key
       end
 
-      def warn_for_unknown_directives(directives)
-        unknown_keys = directives.keys - ORDERED_DIRECTIVES
-        return if unknown_keys.empty?
-
-        opoo "Unknown arguments to #{stanza} -- #{unknown_keys.inspect}. " \
-             "Running \"brew update; brew cleanup\" will likely fix it."
-      end
-
       # Preserve prior functionality of script which runs first. Should rarely be needed.
       # :early_script should not delete files, better defer that to :script.
       # If Cask writers never need :early_script it may be removed in the future.

Library/Homebrew/cask/artifact/uninstall.rb

@@ -4,7 +4,14 @@ module Cask
   module Artifact
     class Uninstall < AbstractUninstall
       def uninstall_phase(**options)
-        dispatch_uninstall_directives(**options)
+        ORDERED_DIRECTIVES.reject { |directive_sym| directive_sym == :rmdir }
+                          .each do |directive_sym|
+                            dispatch_uninstall_directive(directive_sym, **options)
+                          end
+      end
+
+      def post_uninstall_phase(**options)
+        dispatch_uninstall_directive(:rmdir, **options)
       end
     end
   end

Library/Homebrew/cask/installer.rb

@@ -217,6 +217,13 @@ module Cask
           odebug "Reverting installation of artifact of class #{artifact.class}"
           artifact.uninstall_phase(command: @command, verbose: verbose?, force: force?)
         end
+
+        already_installed_artifacts.each do |artifact|
+          next unless artifact.respond_to?(:post_uninstall_phase)
+
+          odebug "Reverting installation of artifact of class #{artifact.class}"
+          artifact.post_uninstall_phase(command: @command, verbose: verbose?, force: force?)
+        end
       ensure
         purge_versioned_files
         raise e
@@ -425,9 +432,18 @@ module Cask
       artifacts.each do |artifact|
         next unless artifact.respond_to?(:uninstall_phase)
 
-        odebug "Un-installing artifact of class #{artifact.class}"
+        odebug "Uninstalling artifact of class #{artifact.class}"
         artifact.uninstall_phase(command: @command, verbose: verbose?, skip: clear, force: force?, upgrade: upgrade?)
       end
+
+      artifacts.each do |artifact|
+        next unless artifact.respond_to?(:post_uninstall_phase)
+
+        odebug "Post-uninstalling artifact of class #{artifact.class}"
+        artifact.post_uninstall_phase(
+          command: @command, verbose: verbose?, skip: clear, force: force?, upgrade: upgrade?,
+        )
+      end
     end
 
     def zap

Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb

@@ -218,32 +218,6 @@ shared_examples "#uninstall_phase or #zap_phase" do
     end
   end
 
-  context "using :rmdir" do
-    let(:fake_system_command) { NeverSudoSystemCommand }
-    let(:cask) { Cask::CaskLoader.load(cask_path("with-#{artifact_dsl_key}-rmdir")) }
-    let(:empty_directory) { Pathname.new("#{TEST_TMPDIR}/empty_directory_path") }
-    let(:ds_store) { empty_directory.join(".DS_Store") }
-
-    before do
-      empty_directory.mkdir
-      FileUtils.touch ds_store
-    end
-
-    after do
-      FileUtils.rm_rf empty_directory
-    end
-
-    it "is supported" do
-      expect(empty_directory).to exist
-      expect(ds_store).to exist
-
-      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
-
-      expect(ds_store).not_to exist
-      expect(empty_directory).not_to exist
-    end
-  end
-
   [:script, :early_script].each do |script_type|
     context "using #{script_type.inspect}" do
       let(:fake_system_command) { NeverSudoSystemCommand }

Library/Homebrew/test/cask/artifact/uninstall_spec.rb

@@ -4,4 +4,34 @@ describe Cask::Artifact::Uninstall, :cask do
   describe "#uninstall_phase" do
     include_examples "#uninstall_phase or #zap_phase"
   end
+
+  describe "#post_uninstall_phase" do
+    subject(:artifact) { cask.artifacts.find { |a| a.is_a?(described_class) } }
+
+    context "using :rmdir" do
+      let(:fake_system_command) { NeverSudoSystemCommand }
+      let(:cask) { Cask::CaskLoader.load(cask_path("with-uninstall-rmdir")) }
+      let(:empty_directory) { Pathname.new("#{TEST_TMPDIR}/empty_directory_path") }
+      let(:ds_store) { empty_directory.join(".DS_Store") }
+
+      before do
+        empty_directory.mkdir
+        FileUtils.touch ds_store
+      end
+
+      after do
+        FileUtils.rm_rf empty_directory
+      end
+
+      it "is supported" do
+        expect(empty_directory).to exist
+        expect(ds_store).to exist
+
+        artifact.post_uninstall_phase(command: fake_system_command)
+
+        expect(ds_store).not_to exist
+        expect(empty_directory).not_to exist
+      end
+    end
+  end
 end

Library/Homebrew/test/cask/artifact/zap_spec.rb

@@ -3,5 +3,33 @@ require_relative "shared_examples/uninstall_zap"
 describe Cask::Artifact::Zap, :cask do
   describe "#zap_phase" do
     include_examples "#uninstall_phase or #zap_phase"
+
+    context "using :rmdir" do
+      subject(:artifact) { cask.artifacts.find { |a| a.is_a?(described_class) } }
+
+      let(:fake_system_command) { NeverSudoSystemCommand }
+      let(:cask) { Cask::CaskLoader.load(cask_path("with-zap-rmdir")) }
+      let(:empty_directory) { Pathname.new("#{TEST_TMPDIR}/empty_directory_path") }
+      let(:ds_store) { empty_directory.join(".DS_Store") }
+
+      before do
+        empty_directory.mkdir
+        FileUtils.touch ds_store
+      end
+
+      after do
+        FileUtils.rm_rf empty_directory
+      end
+
+      it "is supported" do
+        expect(empty_directory).to exist
+        expect(ds_store).to exist
+
+        artifact.zap_phase(command: fake_system_command)
+
+        expect(ds_store).not_to exist
+        expect(empty_directory).not_to exist
+      end
+    end
   end
 end