fixes to pkg uninstaller to prevent madness

- do not remove *all* symlinks from referenced dirs, only broken ones
 - restore dir permission after use so we don't leave behind 777 dirs
 - add some better testing around `Cask::Pkg`
 - clean up `FakeSystemCommand` interface in tests

refs #1274
This commit is contained in:
phinze 2013-10-24 14:52:55 -05:00
parent 852dd46f9e
commit 7ae7a18932
9 changed files with 164 additions and 51 deletions

View File

@ -7,7 +7,7 @@ class Cask::Pkg
attr_reader :package_id
def initialize(package_id, command)
def initialize(package_id, command=Cask::SystemCommand)
@package_id = package_id
@command = command
end
@ -17,10 +17,13 @@ class Cask::Pkg
@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)
_clean_symlinks(dir)
_clean_ds_store(dir)
@command.run!('rmdir', :args => [dir], :sudo => true) if dir.exist? && dir.children.empty?
if dir.exist?
_with_full_permissions(dir) do
_clean_broken_symlinks(dir)
_clean_ds_store(dir)
dir.rmdir if dir.children.empty?
end
end
end
forget
end
@ -52,19 +55,30 @@ class Cask::Pkg
)
end
def _with_full_permissions(path, &block)
original_mode = (path.stat.mode % 01000).to_s(8)
@command.run!('chmod', :args => ['777', path], :sudo => true)
block.call
ensure
if path.exist? # block may have removed dir
@command.run!('chmod', :args => [original_mode, path], :sudo => true)
end
end
def _deepest_path_first(paths)
paths.sort do |path_a, path_b|
path_b.to_s.split('/').count <=> path_a.to_s.split('/').count
end
end
def _clean_symlinks(dir)
# Some pkgs leave broken symlinks hanging around; we clean them out before
# attempting to rmdir to prevent extra cruft from lying around after
# uninstall
return unless dir.exist?
# Some pkgs leave broken symlinks hanging around; we clean them out before
# attempting to rmdir to prevent extra cruft from lying around after
# uninstall
def _clean_broken_symlinks(dir)
dir.children.each do |child|
@command.run!('rm', :args => [child], :sudo => true) if child.symlink?
if _broken_symlink?(child)
@command.run!('rm', :args => [child], :sudo => true)
end
end
end
@ -72,4 +86,8 @@ class Cask::Pkg
ds_store = dir.join('.DS_Store')
@command.run!('rm', :args => [ds_store], :sudo => true) if ds_store.exist?
end
def _broken_symlink?(path)
path.symlink? && !path.readlink.exist?
end
end

View File

@ -13,7 +13,7 @@ describe Cask::Artifact::Pkg do
pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand)
expected_command = "sudo -E 'installer' '-pkg' '#{@cask.destination_path/'MyFancyPkg'/'Fancy.pkg'}' '-target' '/' 2>&1"
Cask::FakeSystemCommand.fake_response_for(expected_command)
Cask::FakeSystemCommand.stubs_command(expected_command)
shutup do
pkg.install
@ -28,7 +28,7 @@ describe Cask::Artifact::Pkg do
pkg = Cask::Artifact::Pkg.new(@cask, Cask::FakeSystemCommand)
expected_command = "sudo -E '#{@cask.destination_path/'MyFancyPkg'/'FancyUninstaller.tool'}' '--please' 2>&1"
Cask::FakeSystemCommand.fake_response_for(expected_command)
Cask::FakeSystemCommand.stubs_command(expected_command)
shutup do
pkg.uninstall
@ -41,7 +41,7 @@ describe Cask::Artifact::Pkg do
cask = Cask.load('with-pkgutil-uninstall')
pkg = Cask::Artifact::Pkg.new(cask, Cask::FakeSystemCommand)
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(pkgutil --pkgs="my.fancy.package.*" 2>&1),
[
'my.fancy.package.main',
@ -49,14 +49,14 @@ describe Cask::Artifact::Pkg do
].join("\n")
)
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(pkgutil '--only-files' '--files' 'my.fancy.package.main' 2>&1),
[
'fancy/bin/fancy.exe',
'fancy/var/fancy.data',
].join("\n")
)
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(pkgutil '--only-dirs' '--files' 'my.fancy.package.main' 2>&1),
[
'fancy',
@ -64,7 +64,7 @@ describe Cask::Artifact::Pkg do
'fancy/var',
].join("\n")
)
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(pkgutil '--pkg-info-plist' 'my.fancy.package.main' 2>&1),
<<-PLIST
<?xml version="1.0" encoding="UTF-8"?>
@ -79,10 +79,10 @@ describe Cask::Artifact::Pkg do
</plist>
PLIST
)
Cask::FakeSystemCommand.fake_response_for(%Q(sudo -E 'kextunload' '-b' 'my.fancy.package.kernelextension' 2>&1))
Cask::FakeSystemCommand.fake_response_for(%Q(sudo -E 'pkgutil' '--forget' 'my.fancy.package.main' 2>&1))
Cask::FakeSystemCommand.stubs_command(%Q(sudo -E 'kextunload' '-b' 'my.fancy.package.kernelextension' 2>&1))
Cask::FakeSystemCommand.stubs_command(%Q(sudo -E 'pkgutil' '--forget' 'my.fancy.package.main' 2>&1))
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(pkgutil '--only-files' '--files' 'my.fancy.package.agent' 2>&1),
[
'fancy/agent/fancy-agent.exe',
@ -90,14 +90,14 @@ describe Cask::Artifact::Pkg do
'fancy/agent/fancy-agent.log',
].join("\n")
)
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(pkgutil '--only-dirs' '--files' 'my.fancy.package.agent' 2>&1),
[
'fancy',
'fancy/agent',
].join("\n")
)
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(pkgutil '--pkg-info-plist' 'my.fancy.package.agent' 2>&1),
<<-PLIST
<?xml version="1.0" encoding="UTF-8"?>
@ -119,10 +119,10 @@ describe Cask::Artifact::Pkg do
/tmp/fancy/bin
/tmp/fancy/var
].each do |dir|
Cask::FakeSystemCommand.fake_response_for(%Q(sudo -E 'chmod' '777' '#{dir}' 2>&1))
Cask::FakeSystemCommand.stubs_command(%Q(sudo -E 'chmod' '777' '#{dir}' 2>&1))
end
Cask::FakeSystemCommand.fake_response_for(%Q(sudo -E 'pkgutil' '--forget' 'my.fancy.package.agent' 2>&1))
Cask::FakeSystemCommand.stubs_command(%Q(sudo -E 'pkgutil' '--forget' 'my.fancy.package.agent' 2>&1))
# No assertions after call since all assertions are implicit from the interactions setup above.
# TODO: verify rmdir / rm commands (requires setting up actual file tree or faking out .exists?

View File

@ -1,7 +1,7 @@
require 'test_helper'
def fake_alfred_preference(key, response)
Cask::FakeSystemCommand.fake_response_for("defaults read com.runningwithcrayons.Alfred-Preferences #{key} 2>&1", response)
Cask::FakeSystemCommand.stubs_command("defaults read com.runningwithcrayons.Alfred-Preferences #{key} 2>&1", response)
end
def fake_alfred_installed(installed=true)
@ -77,7 +77,7 @@ describe Cask::CLI::Alfred do
)
SCOPE_RESPONSE
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(defaults write com.runningwithcrayons.Alfred-Preferences features.defaultresults.scope "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes','#{Cask.caskroom}')" 2>&1)
)
@ -94,7 +94,7 @@ describe Cask::CLI::Alfred do
SCOPE_RESPONSE
expected_scopes = (Cask::CLI::Alfred::DEFAULT_SCOPES + [Cask.caskroom]).map { |s| "'#{s}'" }
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(defaults write com.runningwithcrayons.Alfred-Preferences features.defaultresults.scope "(#{expected_scopes.join(',')})" 2>&1)
)
@ -139,7 +139,7 @@ describe Cask::CLI::Alfred do
)
SCOPE_RESPONSE
Cask::FakeSystemCommand.fake_response_for(
Cask::FakeSystemCommand.stubs_command(
%Q(defaults write com.runningwithcrayons.Alfred-Preferences features.defaultresults.scope "('/Applications','/Library/PreferencePanes','/System/Library/PreferencePanes')" 2>&1)
)

View File

@ -12,7 +12,7 @@ describe Cask::Container::Naked do
path = '/tmp/downloads/kevin-spacey-1.2.pkg'
expected_destination = cask.destination_path.join('kevin spacey.pkg')
expected_command = %Q(ditto '#{path}' '#{expected_destination}' 2>&1)
Cask::FakeSystemCommand.fake_response_for(expected_command)
Cask::FakeSystemCommand.stubs_command(expected_command)
container = Cask::Container::Naked.new(cask, path, Cask::FakeSystemCommand)
container.extract

View File

@ -1,6 +1,6 @@
require 'test_helper'
describe Cask::Installer do
describe Cask::LinkChecker do
describe "request processing" do
it "adds an error if response is empty" do
cask = TestHelper.test_cask

76
test/cask/pkg_test.rb Normal file
View File

@ -0,0 +1,76 @@
require 'test_helper'
describe Cask::Pkg do
describe 'uninstall' do
it 'removes files and dirs referenced by the pkg' do
pkg = Cask::Pkg.new('my.fake.pkg', Cask::NeverSudoSystemCommand)
some_files = Array.new(3) { Pathname(Tempfile.new('testfile').path) }
pkg.stubs(:files).returns some_files
some_dirs = Array.new(3) { Pathname(Dir.mktmpdir) }
pkg.stubs(:dirs).returns some_dirs
pkg.stubs(:forget)
pkg.uninstall
some_files.each { |file| file.wont_be :exist? }
some_dirs.each { |dir| dir.wont_be :exist? }
end
it 'forgets the pkg' do
pkg = Cask::Pkg.new('my.fake.pkg', Cask::FakeSystemCommand)
pkg.stubs(:files).returns([])
pkg.stubs(:dirs).returns([])
Cask::FakeSystemCommand.expects_command(
%q(sudo -E 'pkgutil' '--forget' 'my.fake.pkg' 2>&1)
)
pkg.uninstall
end
it 'cleans broken symlinks, but leaves AOK symlinks' do
pkg = Cask::Pkg.new('my.fake.pkg', Cask::NeverSudoSystemCommand)
fake_dir = Pathname(Dir.mktmpdir)
fake_file = fake_dir.join('ima_file').tap { |path| FileUtils.touch(path) }
intact_symlink = fake_dir.join('intact_symlink').tap { |path| path.make_symlink(fake_file) }
broken_symlink = fake_dir.join('broken_symlink').tap { |path| path.make_symlink('im_nota_file') }
pkg.stubs(:files).returns([])
pkg.stubs(:dirs).returns([fake_dir])
pkg.stubs(:forget)
pkg.uninstall
intact_symlink.must_be :exist?
broken_symlink.wont_be :exist?
fake_dir.must_be :exist?
end
it 'snags permissions on ornery dirs, but returns them afterwords' do
pkg = Cask::Pkg.new('my.fake.pkg', Cask::NeverSudoSystemCommand)
fake_dir = Pathname(Dir.mktmpdir)
fake_file = fake_dir.join('ima_installed_file').tap { |path| FileUtils.touch(path) }
other_file = fake_dir.join('ima_other_file').tap { |path| FileUtils.touch(path) }
fake_dir.chmod(0000)
pkg.stubs(:files).returns([fake_file])
pkg.stubs(:dirs).returns([fake_dir])
pkg.stubs(:forget)
pkg.uninstall
fake_dir.must_be :directory?
fake_file.wont_be :file?
(fake_dir.stat.mode % 01000).to_s(8).must_equal '0'
end
end
end

View File

@ -1,34 +1,49 @@
class Cask::FakeSystemCommand
def self.fake_response_for(command, response='')
def self.responses
@responses ||= {}
@responses[command] = response
end
def self.expectations
@expectations ||= {}
end
def self.system_calls
@system_calls
end
def self.init
@responses = {}
@system_calls ||= Hash.new(0)
end
def self.clear
@responses = {}
@system_calls = Hash.new(0)
@responses = nil
@expectations = nil
@system_calls = nil
end
def self.stubs_command(command, response='')
responses[command] = response
end
def self.expects_command(command, response='', times=1)
stubs_command(command, response)
expectations[command] = times
end
def self.verify_expectations!
expectations.each do |command, times|
unless system_calls[command] == times
fail("expected #{command} to be run #{times} times, but got #{system_calls[command]}")
end
end
end
def self.run(command, options={})
command = Cask::SystemCommand._process_options(command, options)
@responses ||= {}
@system_calls ||= Hash.new(0)
unless @responses.key?(command)
fail("no response faked for #{command.inspect}, faked responses are: #{@responses.inspect}")
unless responses.key?(command)
fail("no response faked for #{command.inspect}, faked responses are: #{responses.inspect}")
end
@system_calls[command] += 1
system_calls[command] += 1
if options[:plist]
Plist::parse_xml(@responses[command])
Plist::parse_xml(responses[command])
else
@responses[command]
responses[command]
end
end
@ -38,13 +53,10 @@ class Cask::FakeSystemCommand
end
module FakeSystemCommandHooks
def before_setup
Cask::FakeSystemCommand.init
super
end
def after_teardown
super
Cask::FakeSystemCommand.verify_expectations!
ensure
Cask::FakeSystemCommand.clear
end
end

View File

@ -0,0 +1,5 @@
class Cask::NeverSudoSystemCommand < Cask::SystemCommand
def self.run(command, options={})
super(command, options.merge(:sudo => false))
end
end

View File

@ -85,7 +85,9 @@ require 'support/fake_fetcher'
require 'support/fake_appdir'
require 'support/fake_system_command'
require 'support/cleanup'
require 'support/never_sudo_system_command'
require 'tmpdir'
require 'tempfile'
# pretend like we installed the cask tap
project_root = Pathname.new(File.expand_path("#{File.dirname(__FILE__)}/../"))