From 0fcc62db311bb279cf7069b15ddafa13a121d383 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 6 Feb 2019 19:04:29 +0100 Subject: [PATCH] Run `uninstall rmdir:` after uninstalling artifacts. --- .../cask/artifact/abstract_uninstall.rb | 27 ++++++++--------- Library/Homebrew/cask/artifact/uninstall.rb | 9 +++++- Library/Homebrew/cask/installer.rb | 18 ++++++++++- .../artifact/shared_examples/uninstall_zap.rb | 26 ---------------- .../test/cask/artifact/uninstall_spec.rb | 30 +++++++++++++++++++ .../Homebrew/test/cask/artifact/zap_spec.rb | 28 +++++++++++++++++ 6 files changed, 95 insertions(+), 43 deletions(-) diff --git a/Library/Homebrew/cask/artifact/abstract_uninstall.rb b/Library/Homebrew/cask/artifact/abstract_uninstall.rb index 64e909b6663e5..531e20c653fe4 100644 --- a/Library/Homebrew/cask/artifact/abstract_uninstall.rb +++ b/Library/Homebrew/cask/artifact/abstract_uninstall.rb @@ -2,6 +2,8 @@ require "utils/user" require "cask/artifact/abstract_artifact" +require "extend/hash_validator" +using HashValidator module Cask module Artifact @@ -27,6 +29,8 @@ def self.from_args(cask, **directives) 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 @@ def summarize private def dispatch_uninstall_directives(**options) - ohai "Running #{stanza} process for #{@cask}; your password may be necessary" + ORDERED_DIRECTIVES.each do |directive_sym| + dispatch_uninstall_directive(directive_sym, **options) + end + end - warn_for_unknown_directives(directives) + def dispatch_uninstall_directive(directive_sym, **options) + return unless directives.key?(directive_sym) - ORDERED_DIRECTIVES.each do |directive_sym| - next unless directives.key?(directive_sym) + args = directives[directive_sym] - args = directives[directive_sym] - send("uninstall_#{directive_sym}", *(args.is_a?(Hash) ? [args] : args), **options) - end + 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. diff --git a/Library/Homebrew/cask/artifact/uninstall.rb b/Library/Homebrew/cask/artifact/uninstall.rb index 6e2b12098cbbc..93b6ada85c325 100644 --- a/Library/Homebrew/cask/artifact/uninstall.rb +++ b/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 diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index 407f5a9b398c4..5c2b58ebd21cb 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -217,6 +217,13 @@ def install_artifacts 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 @@ def uninstall_artifacts(clear: false) 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 diff --git a/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb b/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb index dd034e041ec00..b77f9599a8566 100644 --- a/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb +++ b/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb @@ -218,32 +218,6 @@ 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 } diff --git a/Library/Homebrew/test/cask/artifact/uninstall_spec.rb b/Library/Homebrew/test/cask/artifact/uninstall_spec.rb index cfee17fb858df..5d5a70c7042d3 100644 --- a/Library/Homebrew/test/cask/artifact/uninstall_spec.rb +++ b/Library/Homebrew/test/cask/artifact/uninstall_spec.rb @@ -4,4 +4,34 @@ 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 diff --git a/Library/Homebrew/test/cask/artifact/zap_spec.rb b/Library/Homebrew/test/cask/artifact/zap_spec.rb index a5cef6c530bf6..15122dda8125d 100644 --- a/Library/Homebrew/test/cask/artifact/zap_spec.rb +++ b/Library/Homebrew/test/cask/artifact/zap_spec.rb @@ -3,5 +3,33 @@ 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