From 9393b16930740f16a8749d006e5acf3f52d8e5fb Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Thu, 9 Mar 2017 22:01:46 +0100 Subject: [PATCH] Fix `uninstall :pkgutil` leaving empty `.app` directories. --- Library/Homebrew/cask/lib/hbc/pkg.rb | 106 ++++++++---------- .../artifact/uninstall_zap_shared_examples.rb | 93 ++------------- Library/Homebrew/test/cask/pkg_spec.rb | 100 ++++++++++++----- 3 files changed, 130 insertions(+), 169 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/pkg.rb b/Library/Homebrew/cask/lib/hbc/pkg.rb index 87a87c3cc8..102755c534 100644 --- a/Library/Homebrew/cask/lib/hbc/pkg.rb +++ b/Library/Homebrew/cask/lib/hbc/pkg.rb @@ -16,29 +16,32 @@ module Hbc def uninstall unless pkgutil_bom_files.empty? odebug "Deleting pkg files" - @command.run("/usr/bin/xargs", args: ["-0", "--", "/bin/rm", "-f", "--"], input: pkgutil_bom_files.join("\0"), sudo: true) + @command.run("/usr/bin/xargs", args: ["-0", "--", "/bin/rm", "--"], input: pkgutil_bom_files.join("\0"), sudo: true) end unless pkgutil_bom_specials.empty? odebug "Deleting pkg symlinks and special files" - @command.run("/usr/bin/xargs", args: ["-0", "--", "/bin/rm", "-f", "--"], input: pkgutil_bom_specials.join("\0"), sudo: true) + @command.run("/usr/bin/xargs", args: ["-0", "--", "/bin/rm", "--"], input: pkgutil_bom_specials.join("\0"), sudo: true) end unless pkgutil_bom_dirs.empty? odebug "Deleting pkg directories" - _deepest_path_first(pkgutil_bom_dirs).each do |dir| + deepest_path_first(pkgutil_bom_dirs).each do |dir| next if MacOS.undeletable?(dir) - next unless dir.exist? - _with_full_permissions(dir) do - _delete_broken_file_dir(dir) && next - _clean_broken_symlinks(dir) - _clean_ds_store(dir) - _rmdir(dir) + with_full_permissions(dir) do + clean_broken_symlinks(dir) + clean_ds_store(dir) + rmdir(dir) end end end + if root.directory? && !MacOS.undeletable?(root) + clean_ds_store(root) + rmdir(root) + end + forget end @@ -47,39 +50,38 @@ module Hbc @command.run!("/usr/sbin/pkgutil", args: ["--forget", package_id], sudo: true) end - def pkgutil_bom(*type) - @command.run!("/usr/sbin/pkgutil", args: [*type, "--files", package_id].compact) - .stdout - .split("\n") - .map { |path| root.join(path) } - end - def pkgutil_bom_files - @pkgutil_bom_files ||= pkgutil_bom("--only-files") - end - - def pkgutil_bom_dirs - @pkgutil_bom_dirs ||= pkgutil_bom("--only-dirs") - end - - def pkgutil_bom_all - @pkgutil_bom_all ||= pkgutil_bom + @pkgutil_bom_files ||= pkgutil_bom_all.select(&:file?) - pkgutil_bom_specials end def pkgutil_bom_specials - pkgutil_bom_all - pkgutil_bom_files - pkgutil_bom_dirs + @pkgutil_bom_specials ||= pkgutil_bom_all.select(&method(:special?)) + end + + def pkgutil_bom_dirs + @pkgutil_bom_dirs ||= pkgutil_bom_all.select(&:directory?) - pkgutil_bom_specials + end + + def pkgutil_bom_all + @pkgutil_bom_all ||= info.fetch("paths").keys.map { |p| root.join(p) } end def root - @root ||= Pathname(info.fetch("volume")).join(info.fetch("install-location")) + @root ||= Pathname.new(info.fetch("volume")).join(info.fetch("install-location")) end def info - @command.run!("/usr/sbin/pkgutil", args: ["--pkg-info-plist", package_id]) - .plist + @info ||= @command.run!("/usr/sbin/pkgutil", args: ["--export-plist", package_id]) + .plist end - def _rmdir(path) + private + + def special?(path) + path.symlink? || path.chardev? || path.blockdev? + end + + def rmdir(path) return unless path.children.empty? if path.symlink? @command.run!("/bin/rm", args: ["-f", "--", path], sudo: true) @@ -88,7 +90,7 @@ module Hbc end end - def _with_full_permissions(path) + def with_full_permissions(path) original_mode = (path.stat.mode % 01000).to_s(8) # TODO: similarly read and restore macOS flags (cf man chflags) @command.run!("/bin/chmod", args: ["--", "777", path], sudo: true) @@ -99,36 +101,24 @@ module Hbc 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 + def deepest_path_first(paths) + paths.sort_by { |path| -path.to_s.split(File::SEPARATOR).count } + end + + def clean_ds_store(dir) + return unless (ds_store = dir.join(".DS_Store")).exist? + @command.run!("/bin/rm", args: ["--", ds_store], sudo: true) + end + + # Some packages leave broken symlinks around; we clean them out before + # attempting to `rmdir` to prevent extra cruft from lying around. + def clean_broken_symlinks(dir) + dir.children.select(&method(:broken_symlink?)).each do |path| + @command.run!("/bin/rm", args: ["--", path], sudo: true) end end - # Some pkgs incorrectly report files (generally nibs) - # as directories; we remove these as files instead. - def _delete_broken_file_dir(path) - return unless path.file? && !path.symlink? - @command.run!("/bin/rm", args: ["-f", "--", path], sudo: true) - end - - # 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| - if _broken_symlink?(child) - @command.run!("/bin/rm", args: ["--", child], sudo: true) - end - end - end - - def _clean_ds_store(dir) - ds_store = dir.join(".DS_Store") - @command.run!("/bin/rm", args: ["--", ds_store], sudo: true) if ds_store.exist? - end - - def _broken_symlink?(path) + def broken_symlink?(path) path.symlink? && !path.exist? end end diff --git a/Library/Homebrew/test/cask/artifact/uninstall_zap_shared_examples.rb b/Library/Homebrew/test/cask/artifact/uninstall_zap_shared_examples.rb index 2b69a06a83..69e0154899 100644 --- a/Library/Homebrew/test/cask/artifact/uninstall_zap_shared_examples.rb +++ b/Library/Homebrew/test/cask/artifact/uninstall_zap_shared_examples.rb @@ -66,94 +66,23 @@ shared_examples "#uninstall_phase or #zap_phase" do let(:fake_system_command) { class_double(Hbc::SystemCommand) } let(:cask) { Hbc::CaskLoader.load_from_file(TEST_FIXTURE_DIR/"cask/Casks/with-#{artifact_name}-pkgutil.rb") } + let(:main_pkg_id) { "my.fancy.package.main" } let(:agent_pkg_id) { "my.fancy.package.agent" } - let(:main_files) do - %w[ - fancy/bin/fancy.exe - fancy/var/fancy.data - ] - end - let(:main_dirs) do - %w[ - fancy - fancy/bin - fancy/var - ] - end - let(:agent_files) do - %w[ - fancy/agent/fancy-agent.exe - fancy/agent/fancy-agent.pid - fancy/agent/fancy-agent.log - ] - end - let(:agent_dirs) do - %w[ - fancy - fancy/agent - ] - end - let(:pkg_info_plist) do - <<-EOS.undent - - - - - install-location - tmp - volume - / - - - EOS - end it "is supported" do - allow(fake_system_command).to receive(:run).with( - "/usr/sbin/pkgutil", - args: ["--pkgs=my.fancy.package.*"], - ).and_return(double(stdout: "#{main_pkg_id}\n#{agent_pkg_id}")) + main_pkg = Hbc::Pkg.new(main_pkg_id, fake_system_command) + agent_pkg = Hbc::Pkg.new(agent_pkg_id, fake_system_command) - [ - [main_pkg_id, main_files, main_dirs], - [agent_pkg_id, agent_files, agent_dirs], - ].each do |pkg_id, pkg_files, pkg_dirs| + expect(Hbc::Pkg).to receive(:all_matching).and_return( + [ + main_pkg, + agent_pkg, + ], + ) - allow(fake_system_command).to receive(:run!).with( - "/usr/sbin/pkgutil", - args: ["--only-files", "--files", pkg_id.to_s], - ).and_return(double(stdout: pkg_files.join("\n"))) - - allow(fake_system_command).to receive(:run!).with( - "/usr/sbin/pkgutil", - args: ["--only-dirs", "--files", pkg_id.to_s], - ).and_return(double(stdout: pkg_dirs.join("\n"))) - - allow(fake_system_command).to receive(:run!).with( - "/usr/sbin/pkgutil", - args: ["--files", pkg_id.to_s], - ).and_return(double(stdout: (pkg_files + pkg_dirs).join("\n"))) - - result = Hbc::SystemCommand::Result.new(nil, pkg_info_plist, nil, 0) - allow(fake_system_command).to receive(:run!).with( - "/usr/sbin/pkgutil", - args: ["--pkg-info-plist", pkg_id.to_s], - ).and_return(result) - - expect(fake_system_command).to receive(:run).with( - "/usr/bin/xargs", - args: ["-0", "--", "/bin/rm", "-f", "--"], - input: pkg_files.map { |path| "/tmp/#{path}" }.join("\0"), - sudo: true, - ) - - expect(fake_system_command).to receive(:run!).with( - "/usr/sbin/pkgutil", - args: ["--forget", pkg_id.to_s], - sudo: true, - ) - end + expect(main_pkg).to receive(:uninstall) + expect(agent_pkg).to receive(:uninstall) subject end diff --git a/Library/Homebrew/test/cask/pkg_spec.rb b/Library/Homebrew/test/cask/pkg_spec.rb index a77ce85b99..9185331fbe 100644 --- a/Library/Homebrew/test/cask/pkg_spec.rb +++ b/Library/Homebrew/test/cask/pkg_spec.rb @@ -1,7 +1,7 @@ describe Hbc::Pkg, :cask do describe "#uninstall" do let(:fake_system_command) { Hbc::NeverSudoSystemCommand } - let(:empty_response) { double(stdout: "") } + let(:empty_response) { double(stdout: "", plist: {"volume" => "/", "install-location" => "", "paths" => {}}) } let(:pkg) { described_class.new("my.fake.pkg", fake_system_command) } it "removes files and dirs referenced by the pkg" do @@ -14,6 +14,9 @@ describe Hbc::Pkg, :cask do some_dirs = Array.new(3) { Pathname.new(Dir.mktmpdir) } allow(pkg).to receive(:pkgutil_bom_dirs).and_return(some_dirs) + root_dir = Pathname.new(Dir.mktmpdir) + allow(pkg).to receive(:root).and_return(root_dir) + allow(pkg).to receive(:forget) pkg.uninstall @@ -25,25 +28,15 @@ describe Hbc::Pkg, :cask do some_dirs.each do |dir| expect(dir).not_to exist end + + expect(root_dir).not_to exist end context "pkgutil" do - let(:fake_system_command) { class_double(Hbc::SystemCommand) } - it "forgets the pkg" do allow(fake_system_command).to receive(:run!).with( "/usr/sbin/pkgutil", - args: ["--only-files", "--files", "my.fake.pkg"], - ).and_return(empty_response) - - allow(fake_system_command).to receive(:run!).with( - "/usr/sbin/pkgutil", - args: ["--only-dirs", "--files", "my.fake.pkg"], - ).and_return(empty_response) - - allow(fake_system_command).to receive(:run!).with( - "/usr/sbin/pkgutil", - args: ["--files", "my.fake.pkg"], + args: ["--export-plist", "my.fake.pkg"], ).and_return(empty_response) expect(fake_system_command).to receive(:run!).with( @@ -58,6 +51,7 @@ describe Hbc::Pkg, :cask do it "removes broken symlinks" do fake_dir = Pathname.new(Dir.mktmpdir) + fake_root = Pathname.new(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) } @@ -66,6 +60,7 @@ describe Hbc::Pkg, :cask do allow(pkg).to receive(:pkgutil_bom_specials).and_return([]) allow(pkg).to receive(:pkgutil_bom_files).and_return([]) allow(pkg).to receive(:pkgutil_bom_dirs).and_return([fake_dir]) + allow(pkg).to receive(:root).and_return(fake_root) allow(pkg).to receive(:forget) pkg.uninstall @@ -73,24 +68,11 @@ describe Hbc::Pkg, :cask do expect(intact_symlink).to exist expect(broken_symlink).not_to exist expect(fake_dir).to exist - end - - it "removes files incorrectly reportes as directories" do - fake_dir = Pathname.new(Dir.mktmpdir) - fake_file = fake_dir.join("ima_file_pretending_to_be_a_dir").tap { |path| FileUtils.touch(path) } - - allow(pkg).to receive(:pkgutil_bom_specials).and_return([]) - allow(pkg).to receive(:pkgutil_bom_files).and_return([]) - allow(pkg).to receive(:pkgutil_bom_dirs).and_return([fake_file, fake_dir]) - allow(pkg).to receive(:forget) - - pkg.uninstall - - expect(fake_file).not_to exist - expect(fake_dir).not_to exist + expect(fake_root).not_to exist end it "snags permissions on ornery dirs, but returns them afterwards" do + fake_root = Pathname.new(Dir.mktmpdir) fake_dir = Pathname.new(Dir.mktmpdir) fake_file = fake_dir.join("ima_installed_file").tap { |path| FileUtils.touch(path) } fake_dir.chmod(0000) @@ -98,6 +80,7 @@ describe Hbc::Pkg, :cask do allow(pkg).to receive(:pkgutil_bom_specials).and_return([]) allow(pkg).to receive(:pkgutil_bom_files).and_return([fake_file]) allow(pkg).to receive(:pkgutil_bom_dirs).and_return([fake_dir]) + allow(pkg).to receive(:root).and_return(fake_root) allow(pkg).to receive(:forget) shutup do @@ -109,4 +92,63 @@ describe Hbc::Pkg, :cask do expect((fake_dir.stat.mode % 01000).to_s(8)).to eq("0") end end + + describe "#info" do + let(:fake_system_command) { class_double(Hbc::SystemCommand) } + + let(:volume) { "/" } + let(:install_location) { "tmp" } + + let(:pkg_id) { "my.fancy.package.main" } + + let(:pkg_files) do + %w[ + fancy/bin/fancy.exe + fancy/var/fancy.data + ] + end + let(:pkg_directories) do + %w[ + fancy + fancy/bin + fancy/var + ] + end + + let(:pkg_info_plist) do + <<-EOS.undent + + + + + install-location + #{install_location} + volume + #{volume} + paths + + #{(pkg_files + pkg_directories).map { |f| "#{f}" }.join("")} + + + + EOS + end + + it "correctly parses a Property List" do + pkg = Hbc::Pkg.new(pkg_id, fake_system_command) + + expect(fake_system_command).to receive(:run!).with( + "/usr/sbin/pkgutil", + args: ["--export-plist", pkg_id], + ).and_return( + Hbc::SystemCommand::Result.new(nil, pkg_info_plist, nil, 0), + ) + + info = pkg.info + + expect(info["install-location"]).to eq(install_location) + expect(info["volume"]).to eq(volume) + expect(info["paths"].keys).to eq(pkg_files + pkg_directories) + end + end end