Merge pull request #2303 from reitermarkus/fix-pkg-not-uninstalling-apps

Fix `uninstall :pkgutil` leaving empty `.app` directories.

3 files changed, 127 insertions, 164 deletions

diff --git a/Library/Homebrew/cask/lib/hbc/pkg.rb b/Library/Homebrew/cask/lib/hbc/pkg.rb
index 87a87c3cc..902d56449 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) }
+    def pkgutil_bom_files
+      @pkgutil_bom_files ||= pkgutil_bom_all.select(&:file?) - pkgutil_bom_specials
     end
 
-    def pkgutil_bom_files
-      @pkgutil_bom_files ||= pkgutil_bom("--only-files")
+    def pkgutil_bom_specials
+      @pkgutil_bom_specials ||= pkgutil_bom_all.select(&method(:special?))
     end
 
     def pkgutil_bom_dirs
-      @pkgutil_bom_dirs ||= pkgutil_bom("--only-dirs")
+      @pkgutil_bom_dirs ||= pkgutil_bom_all.select(&:directory?) - pkgutil_bom_specials
     end
 
     def pkgutil_bom_all
-      @pkgutil_bom_all ||= pkgutil_bom
-    end
-
-    def pkgutil_bom_specials
-      pkgutil_bom_all - pkgutil_bom_files - pkgutil_bom_dirs
+      @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,47 +90,37 @@ 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)
+      original_flags = @command.run!("/usr/bin/stat", args: ["-f", "%Of", "--", path]).stdout.chomp
+
       @command.run!("/bin/chmod", args: ["--", "777", path], sudo: true)
       yield
     ensure
       if path.exist? # block may have removed dir
         @command.run!("/bin/chmod", args: ["--", original_mode, path], sudo: true)
+        @command.run!("/usr/bin/chflags", args: ["--", original_flags, 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
+    def deepest_path_first(paths)
+      paths.sort_by { |path| -path.to_s.split(File::SEPARATOR).count }
     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)
+    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 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
+    # 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
 
-    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 2b69a06a8..69e015489 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
-        <?xml version="1.0" encoding="UTF-8"?>
-        <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
-        <plist version="1.0">
-        <dict>
-                <key>install-location</key>
-                <string>tmp</string>
-                <key>volume</key>
-                <string>/</string>
-        </dict>
-        </plist>
-      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_id, main_files, main_dirs],
-        [agent_pkg_id, agent_files, agent_dirs],
-      ].each do |pkg_id, pkg_files, pkg_dirs|
-
-        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
+      main_pkg = Hbc::Pkg.new(main_pkg_id, fake_system_command)
+      agent_pkg = Hbc::Pkg.new(agent_pkg_id, fake_system_command)
+
+      expect(Hbc::Pkg).to receive(:all_matching).and_return(
+        [
+          main_pkg,
+          agent_pkg,
+        ],
+      )
+
+      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 a77ce85b9..9930cd00f 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
+        <?xml version="1.0" encoding="UTF-8"?>
+        <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+        <plist version="1.0">
+        <dict>
+          <key>install-location</key>
+          <string>#{install_location}</string>
+          <key>volume</key>
+          <string>#{volume}</string>
+          <key>paths</key>
+          <dict>
+            #{(pkg_files + pkg_directories).map { |f| "<key>#{f}</key><dict></dict>" }.join("")}
+          </dict>
+        </dict>
+        </plist>
+      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