From 367fdb971839c0b3686ca23a6de76da75cc80580 Mon Sep 17 00:00:00 2001 From: Joshua McKinney Date: Sat, 18 Mar 2017 17:48:20 -0500 Subject: [PATCH 1/6] Refactor brew cask reinstall The implementation of the reinstall command was the same as Installer#install, aside from the uninstall of the existing cask. Moved this within the class to DRY up the implementation. --- .../Homebrew/cask/lib/hbc/cli/reinstall.rb | 29 ++++--------------- Library/Homebrew/cask/lib/hbc/installer.rb | 19 ++++++++++-- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb b/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb index b52c433282..c27fa9f2fd 100644 --- a/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb +++ b/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb @@ -7,30 +7,11 @@ module Hbc begin cask = CaskLoader.load(cask_token) - installer = Installer.new(cask, - force: force, - skip_cask_deps: skip_cask_deps, - require_sha: require_sha) - installer.print_caveats - installer.fetch - - if cask.installed? - # use copy of cask for uninstallation to avoid 'No such file or directory' bug - installed_cask = cask - - # use the same cask file that was used for installation, if possible - if (installed_caskfile = installed_cask.installed_caskfile).exist? - installed_cask = CaskLoader.load_from_file(installed_caskfile) - end - - # Always force uninstallation, ignore method parameter - Installer.new(installed_cask, force: true).uninstall - end - - installer.stage - installer.install_artifacts - installer.enable_accessibility_access - puts installer.summary + Installer.new(cask, + force: force, + skip_cask_deps: skip_cask_deps, + require_sha: require_sha, + reinstall: true).install count += 1 rescue CaskUnavailableError => e diff --git a/Library/Homebrew/cask/lib/hbc/installer.rb b/Library/Homebrew/cask/lib/hbc/installer.rb index 824c1b1beb..6f6957574a 100644 --- a/Library/Homebrew/cask/lib/hbc/installer.rb +++ b/Library/Homebrew/cask/lib/hbc/installer.rb @@ -18,12 +18,13 @@ module Hbc PERSISTENT_METADATA_SUBDIRS = ["gpg"].freeze - def initialize(cask, command: SystemCommand, force: false, skip_cask_deps: false, require_sha: false) + def initialize(cask, command: SystemCommand, force: false, skip_cask_deps: false, require_sha: false, reinstall: false) @cask = cask @command = command @force = force @skip_cask_deps = skip_cask_deps @require_sha = require_sha + @reinstall = reinstall end def self.print_caveats(cask) @@ -76,13 +77,14 @@ module Hbc def install odebug "Hbc::Installer#install" - if @cask.installed? && !force + if @cask.installed? && !force && !@reinstall raise CaskAlreadyInstalledAutoUpdatesError, @cask if @cask.auto_updates raise CaskAlreadyInstalledError, @cask end print_caveats fetch + uninstall_if_neccessary stage install_artifacts enable_accessibility_access @@ -90,6 +92,19 @@ module Hbc puts summary end + def uninstall_if_neccessary + return unless @cask.installed? && @reinstall + installed_cask = @cask + + # use the same cask file that was used for installation, if possible + if (installed_caskfile = installed_cask.installed_caskfile).exist? + installed_cask = CaskLoader.load_from_file(installed_caskfile) + end + + # Always force uninstallation, ignore method parameter + Installer.new(installed_cask, force: true).uninstall + end + def summary s = "" s << "#{Emoji.install_badge} " if Emoji.enabled? From 3703ef1885ba4afce1ed4ae531dcb7ddc573b3c2 Mon Sep 17 00:00:00 2001 From: Joshua McKinney Date: Sat, 18 Mar 2017 18:57:04 -0500 Subject: [PATCH 2/6] Show messages when (un)installing Casks Addresses an issue where it can be unclear at times exactly which part of the (un|re)installation processes is reporting an error. See https://github.com/caskroom/homebrew-cask/issues/30968 --- Library/Homebrew/cask/lib/hbc/installer.rb | 4 +++- .../Homebrew/test/cask/cli/install_spec.rb | 14 +++++++++++ .../Homebrew/test/cask/cli/reinstall_spec.rb | 23 +++++++++++++++++++ .../Homebrew/test/cask/cli/uninstall_spec.rb | 17 ++++++++++++++ 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/lib/hbc/installer.rb b/Library/Homebrew/cask/lib/hbc/installer.rb index 6f6957574a..69113e9fc8 100644 --- a/Library/Homebrew/cask/lib/hbc/installer.rb +++ b/Library/Homebrew/cask/lib/hbc/installer.rb @@ -85,6 +85,8 @@ module Hbc print_caveats fetch uninstall_if_neccessary + + oh1 "Installing Cask #{@cask}" stage install_artifacts enable_accessibility_access @@ -322,7 +324,7 @@ module Hbc end def uninstall - odebug "Hbc::Installer#uninstall" + oh1 "Uninstalling Cask #{@cask}" disable_accessibility_access uninstall_artifacts purge_versioned_files diff --git a/Library/Homebrew/test/cask/cli/install_spec.rb b/Library/Homebrew/test/cask/cli/install_spec.rb index 5a40017e83..fef0b28247 100644 --- a/Library/Homebrew/test/cask/cli/install_spec.rb +++ b/Library/Homebrew/test/cask/cli/install_spec.rb @@ -1,4 +1,18 @@ describe Hbc::CLI::Install, :cask do + it "displays the installation progress" do + output = Regexp.new <<-EOS.undent + ==> Downloading file:.*/caffeine.zip + ==> Verifying checksum for Cask local-caffeine + ==> Installing Cask local-caffeine + ==> Moving App 'Caffeine.app' to '.*/Caffeine.app'. + 🍺 local-caffeine was successfully installed! + EOS + + expect { + Hbc::CLI::Install.run("local-caffeine") + }.to output(output).to_stdout + end + it "allows staging and activation of multiple Casks at once" do shutup do Hbc::CLI::Install.run("local-transmission", "local-caffeine") diff --git a/Library/Homebrew/test/cask/cli/reinstall_spec.rb b/Library/Homebrew/test/cask/cli/reinstall_spec.rb index e573a3470c..7f3c60bc4a 100644 --- a/Library/Homebrew/test/cask/cli/reinstall_spec.rb +++ b/Library/Homebrew/test/cask/cli/reinstall_spec.rb @@ -1,4 +1,27 @@ describe Hbc::CLI::Reinstall, :cask do + it "displays the reinstallation progress" do + caffeine = Hbc::CaskLoader.load_from_file(TEST_FIXTURE_DIR/"cask/Casks/local-caffeine.rb") + + shutup do + Hbc::Installer.new(caffeine).install + end + + output = Regexp.new <<-EOS.undent + ==> Downloading file:.*/caffeine.zip + Already downloaded: .*/local-caffeine--1.2.3.zip + ==> Verifying checksum for Cask local-caffeine + ==> Uninstalling Cask local-caffeine + ==> Removing App '.*/Caffeine.app'. + ==> Installing Cask local-caffeine + ==> Moving App 'Caffeine.app' to '.*/Caffeine.app'. + 🍺 local-caffeine was successfully installed! + EOS + + expect { + Hbc::CLI::Reinstall.run("local-caffeine") + }.to output(output).to_stdout + end + it "allows reinstalling a Cask" do shutup do Hbc::CLI::Install.run("local-transmission") diff --git a/Library/Homebrew/test/cask/cli/uninstall_spec.rb b/Library/Homebrew/test/cask/cli/uninstall_spec.rb index fb196ee726..714e652c7e 100644 --- a/Library/Homebrew/test/cask/cli/uninstall_spec.rb +++ b/Library/Homebrew/test/cask/cli/uninstall_spec.rb @@ -1,4 +1,21 @@ describe Hbc::CLI::Uninstall, :cask do + it "displays the uninstallation progress" do + caffeine = Hbc::CaskLoader.load_from_file(TEST_FIXTURE_DIR/"cask/Casks/local-caffeine.rb") + + shutup do + Hbc::Installer.new(caffeine).install + end + + output = Regexp.new <<-EOS.undent + ==> Uninstalling Cask local-caffeine + ==> Removing App '.*/Caffeine.app'. + EOS + + expect { + Hbc::CLI::Uninstall.run("local-caffeine") + }.to output(output).to_stdout + end + it "shows an error when a bad Cask is provided" do expect { Hbc::CLI::Uninstall.run("notacask") From d11e417105c04a1c21edfb4481bc26e21f1c94f9 Mon Sep 17 00:00:00 2001 From: Joshua McKinney Date: Sat, 18 Mar 2017 18:59:28 -0500 Subject: [PATCH 3/6] Hide output from brew cask uninstall test This test showed extraneous info in the test output --- Library/Homebrew/test/cask/cli/uninstall_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/test/cask/cli/uninstall_spec.rb b/Library/Homebrew/test/cask/cli/uninstall_spec.rb index 714e652c7e..036c47b5cc 100644 --- a/Library/Homebrew/test/cask/cli/uninstall_spec.rb +++ b/Library/Homebrew/test/cask/cli/uninstall_spec.rb @@ -30,7 +30,9 @@ describe Hbc::CLI::Uninstall, :cask do it "tries anyway on a non-present Cask when --force is given" do expect { - Hbc::CLI::Uninstall.run("local-caffeine", "--force") + shutup do + Hbc::CLI::Uninstall.run("local-caffeine", "--force") + end }.not_to raise_error end From 437db065caad8cb607373c41c3dc7bb2e3c58a66 Mon Sep 17 00:00:00 2001 From: Joshua McKinney Date: Sun, 19 Mar 2017 19:56:41 -0500 Subject: [PATCH 4/6] Be a little less specific in cask output tests These tests seemed a little over-specified and were failing on the CI server. Reducing the specificity a little to try to get them to pass. --- Library/Homebrew/test/cask/cli/install_spec.rb | 6 +++--- Library/Homebrew/test/cask/cli/reinstall_spec.rb | 10 +++++----- Library/Homebrew/test/cask/cli/uninstall_spec.rb | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Library/Homebrew/test/cask/cli/install_spec.rb b/Library/Homebrew/test/cask/cli/install_spec.rb index fef0b28247..219b9522e4 100644 --- a/Library/Homebrew/test/cask/cli/install_spec.rb +++ b/Library/Homebrew/test/cask/cli/install_spec.rb @@ -1,11 +1,11 @@ describe Hbc::CLI::Install, :cask do it "displays the installation progress" do output = Regexp.new <<-EOS.undent - ==> Downloading file:.*/caffeine.zip + ==> Downloading file:.*caffeine.zip ==> Verifying checksum for Cask local-caffeine ==> Installing Cask local-caffeine - ==> Moving App 'Caffeine.app' to '.*/Caffeine.app'. - 🍺 local-caffeine was successfully installed! + ==> Moving App 'Caffeine.app' to '.*Caffeine.app'. + .*local-caffeine was successfully installed! EOS expect { diff --git a/Library/Homebrew/test/cask/cli/reinstall_spec.rb b/Library/Homebrew/test/cask/cli/reinstall_spec.rb index 7f3c60bc4a..8885fa1990 100644 --- a/Library/Homebrew/test/cask/cli/reinstall_spec.rb +++ b/Library/Homebrew/test/cask/cli/reinstall_spec.rb @@ -7,14 +7,14 @@ describe Hbc::CLI::Reinstall, :cask do end output = Regexp.new <<-EOS.undent - ==> Downloading file:.*/caffeine.zip - Already downloaded: .*/local-caffeine--1.2.3.zip + ==> Downloading file:.*caffeine.zip + Already downloaded: .*local-caffeine--1.2.3.zip ==> Verifying checksum for Cask local-caffeine ==> Uninstalling Cask local-caffeine - ==> Removing App '.*/Caffeine.app'. + ==> Removing App '.*Caffeine.app'. ==> Installing Cask local-caffeine - ==> Moving App 'Caffeine.app' to '.*/Caffeine.app'. - 🍺 local-caffeine was successfully installed! + ==> Moving App 'Caffeine.app' to '.*Caffeine.app'. + .*local-caffeine was successfully installed! EOS expect { diff --git a/Library/Homebrew/test/cask/cli/uninstall_spec.rb b/Library/Homebrew/test/cask/cli/uninstall_spec.rb index 036c47b5cc..4089c47b46 100644 --- a/Library/Homebrew/test/cask/cli/uninstall_spec.rb +++ b/Library/Homebrew/test/cask/cli/uninstall_spec.rb @@ -8,7 +8,7 @@ describe Hbc::CLI::Uninstall, :cask do output = Regexp.new <<-EOS.undent ==> Uninstalling Cask local-caffeine - ==> Removing App '.*/Caffeine.app'. + ==> Removing App '.*Caffeine.app'. EOS expect { From a90d1e169939cea7006f7465fe8c04370005223d Mon Sep 17 00:00:00 2001 From: Joshua McKinney Date: Mon, 27 Mar 2017 01:31:29 -0500 Subject: [PATCH 5/6] Installer#reinstall instead of #install :reinstall Call an explicit method on Installer to reinstall rather than using a flag to indicate when we're reinstalling a cask --- Library/Homebrew/cask/lib/hbc/cli/reinstall.rb | 3 +-- Library/Homebrew/cask/lib/hbc/installer.rb | 10 ++++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb b/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb index c27fa9f2fd..c2ed8f4625 100644 --- a/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb +++ b/Library/Homebrew/cask/lib/hbc/cli/reinstall.rb @@ -10,8 +10,7 @@ module Hbc Installer.new(cask, force: force, skip_cask_deps: skip_cask_deps, - require_sha: require_sha, - reinstall: true).install + require_sha: require_sha).reinstall count += 1 rescue CaskUnavailableError => e diff --git a/Library/Homebrew/cask/lib/hbc/installer.rb b/Library/Homebrew/cask/lib/hbc/installer.rb index 69113e9fc8..c9b6b4e538 100644 --- a/Library/Homebrew/cask/lib/hbc/installer.rb +++ b/Library/Homebrew/cask/lib/hbc/installer.rb @@ -18,13 +18,13 @@ module Hbc PERSISTENT_METADATA_SUBDIRS = ["gpg"].freeze - def initialize(cask, command: SystemCommand, force: false, skip_cask_deps: false, require_sha: false, reinstall: false) + def initialize(cask, command: SystemCommand, force: false, skip_cask_deps: false, require_sha: false) @cask = cask @command = command @force = force @skip_cask_deps = skip_cask_deps @require_sha = require_sha - @reinstall = reinstall + @reinstall = false end def self.print_caveats(cask) @@ -94,6 +94,12 @@ module Hbc puts summary end + def reinstall + odebug "Hbc::Installer#reinstall" + @reinstall = true + install + end + def uninstall_if_neccessary return unless @cask.installed? && @reinstall installed_cask = @cask From cb28ab640e191500d480308ca262366184518022 Mon Sep 17 00:00:00 2001 From: Joshua McKinney Date: Mon, 17 Apr 2017 17:21:02 -0700 Subject: [PATCH 6/6] Refactor uninstall existing cask --- Library/Homebrew/cask/lib/hbc/installer.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/installer.rb b/Library/Homebrew/cask/lib/hbc/installer.rb index c9b6b4e538..51b0490f5e 100644 --- a/Library/Homebrew/cask/lib/hbc/installer.rb +++ b/Library/Homebrew/cask/lib/hbc/installer.rb @@ -84,7 +84,7 @@ module Hbc print_caveats fetch - uninstall_if_neccessary + uninstall_existing_cask if @reinstall oh1 "Installing Cask #{@cask}" stage @@ -100,14 +100,12 @@ module Hbc install end - def uninstall_if_neccessary - return unless @cask.installed? && @reinstall - installed_cask = @cask + def uninstall_existing_cask + return unless @cask.installed? # use the same cask file that was used for installation, if possible - if (installed_caskfile = installed_cask.installed_caskfile).exist? - installed_cask = CaskLoader.load_from_file(installed_caskfile) - end + installed_caskfile = @cask.installed_caskfile + installed_cask = installed_caskfile.exist? ? CaskLoader.load_from_file(installed_caskfile) : @cask # Always force uninstallation, ignore method parameter Installer.new(installed_cask, force: true).uninstall