Merge pull request #5247 from reitermarkus/quit

Improve application quitting.

Library/Homebrew/cask/artifact/abstract_uninstall.rb

@@ -94,8 +94,10 @@ module Cask
               command.run!("/bin/launchctl", args: ["remove", service], sudo: with_sudo)
               sleep 1
             end
-            paths = ["/Library/LaunchAgents/#{service}.plist",
+            paths = [
-                     "/Library/LaunchDaemons/#{service}.plist"]
+              "/Library/LaunchAgents/#{service}.plist",
+              "/Library/LaunchDaemons/#{service}.plist",
+            ]
             paths.each { |elt| elt.prepend(ENV["HOME"]) } unless with_sudo
             paths = paths.map { |elt| Pathname(elt) }.select(&:exist?)
             paths.each do |path|
@@ -105,46 +107,78 @@ module Cask
             next unless Pathname(service).exist?
 
             command.run!("/bin/launchctl", args: ["unload", "-w", "--", service], sudo: with_sudo)
-            command.run!("/bin/rm",        args: ["-f", "--", service], sudo: with_sudo)
+            command.run!("/bin/rm", args: ["-f", "--", service], sudo: with_sudo)
             sleep 1
           end
         end
       end
 
-      def running_processes(bundle_id, command: nil)
+      def running_processes(bundle_id)
-        command.run!("/bin/launchctl", args: ["list"]).stdout.lines
+        system_command!("/bin/launchctl", args: ["list"])
-               .map { |line| line.chomp.split("\t") }
+          .stdout.lines
-               .map { |pid, state, id| [pid.to_i, state.to_i, id] }
+          .map { |line| line.chomp.split("\t") }
-               .select do |(pid, _, id)|
+          .map { |pid, state, id| [pid.to_i, state.to_i, id] }
-                 pid.nonzero? && id.match?(/^#{Regexp.escape(bundle_id)}($|\.\d+)/)
+          .select do |(pid, _, id)|
-               end
+            pid.nonzero? && id.match?(/^#{Regexp.escape(bundle_id)}($|\.\d+)/)
+          end
       end
 
       # :quit/:signal must come before :kext so the kext will not be in use by a running process
       def uninstall_quit(*bundle_ids, command: nil, **_)
         bundle_ids.each do |bundle_id|
-          next if running_processes(bundle_id, command: command).empty?
+          next if running_processes(bundle_id).empty?
 
           unless User.current.gui?
             ohai "Not logged into a GUI; skipping quitting application ID '#{bundle_id}'."
             next
           end
 
-          ohai "Quitting application ID '#{bundle_id}'."
+          ohai "Quitting application '#{bundle_id}'..."
-          command.run!("/usr/bin/osascript", args: ["-e", %Q(tell application id "#{bundle_id}" to quit)], sudo: true)
 
           begin
-            Timeout.timeout(3) do
+            Timeout.timeout(10) do
               Kernel.loop do
-                break if running_processes(bundle_id, command: command).empty?
+                next unless quit(bundle_id).success?
+                if running_processes(bundle_id).empty?
+                  puts "Application '#{bundle_id}' quit successfully."
+                  break
+                end
               end
             end
           rescue Timeout::Error
+            opoo "Application '#{bundle_id}' did not quit."
             next
           end
         end
       end
 
+      def quit(bundle_id)
+        script = <<~JAVASCRIPT
+          'use strict';
+
+          ObjC.import('stdlib')
+
+          function run(argv) {
+            var app = Application(argv[0])
+
+            try {
+              app.quit()
+            } catch (err) {
+              if (app.running()) {
+                $.exit(1)
+              }
+            }
+
+            $.exit(0)
+          }
+        JAVASCRIPT
+
+        system_command "osascript", args:         ["-l", "JavaScript", "-e", script, bundle_id],
+                                    print_stderr: false,
+                                    sudo:         true
+      end
+      private :quit
+
       # :signal should come after :quit so it can be used as a backup when :quit fails
       def uninstall_signal(*signals, command: nil, **_)
         signals.each do |pair|
@@ -154,7 +188,7 @@ module Cask
 
           signal, bundle_id = pair
           ohai "Signalling '#{signal}' to application ID '#{bundle_id}'"
-          pids = running_processes(bundle_id, command: command).map(&:first)
+          pids = running_processes(bundle_id).map(&:first)
           next unless pids.any?
 
           # Note that unlike :quit, signals are sent from the current user (not
@@ -174,13 +208,12 @@ module Cask
 
         login_items.each do |name|
           ohai "Removing login item #{name}"
-          command.run!(
+          system_command!(
-            "/usr/bin/osascript",
+            "osascript",
             args: [
               "-e",
               %Q(tell application "System Events" to delete every login item whose name is "#{name}"),
             ],
-            sudo: false,
           )
           sleep 1
         end
@@ -190,14 +223,14 @@ module Cask
       def uninstall_kext(*kexts, command: nil, **_)
         kexts.each do |kext|
           ohai "Unloading kernel extension #{kext}"
-          is_loaded = command.run!("/usr/sbin/kextstat", args: ["-l", "-b", kext], sudo: true).stdout
+          is_loaded = system_command!("/usr/sbin/kextstat", args: ["-l", "-b", kext], sudo: true).stdout
           if is_loaded.length > 1
-            command.run!("/sbin/kextunload", args: ["-b", kext], sudo: true)
+            system_command!("/sbin/kextunload", args: ["-b", kext], sudo: true)
             sleep 1
           end
-          command.run!("/usr/sbin/kextfind", args: ["-b", kext], sudo: true).stdout.chomp.lines.each do |kext_path|
+          system_command!("/usr/sbin/kextfind", args: ["-b", kext], sudo: true).stdout.chomp.lines.each do |kext_path|
             ohai "Removing kernel extension #{kext_path}"
-            command.run!("/bin/rm", args: ["-rf", kext_path], sudo: true)
+            system_command!("/bin/rm", args: ["-rf", kext_path], sudo: true)
           end
         end
       end
@@ -287,7 +320,7 @@ module Cask
       end
 
       def trash_paths(*paths, command: nil, **_)
-        result = command.run!("/usr/bin/osascript", args: ["-e", <<~APPLESCRIPT, *paths])
+        result = command.run!("osascript", args: ["-e", <<~APPLESCRIPT, *paths])
           on run argv
             repeat with i from 1 to (count argv)
               set item i of argv to (item i of argv as POSIX file)

Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb

@@ -1,10 +1,12 @@
+require "benchmark"
+
 shared_examples "#uninstall_phase or #zap_phase" do
+  subject { artifact }
+
   let(:artifact_dsl_key) { described_class.dsl_key }
   let(:artifact) { cask.artifacts.find { |a| a.is_a?(described_class) } }
   let(:fake_system_command) { FakeSystemCommand }
 
-  subject { artifact.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command) }
-
   context "using :launchctl" do
     let(:cask) { Cask::CaskLoader.load(cask_path("with-#{artifact_dsl_key}-launchctl")) }
     let(:launchctl_list_cmd) { %w[/bin/launchctl list my.fancy.package.service] }
@@ -38,7 +40,7 @@ shared_examples "#uninstall_phase or #zap_phase" do
 
       FakeSystemCommand.expects_command(launchctl_remove_cmd)
 
-      subject
+      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
     end
 
     it "works when job is owned by system" do
@@ -54,7 +56,7 @@ shared_examples "#uninstall_phase or #zap_phase" do
 
       FakeSystemCommand.expects_command(sudo(launchctl_remove_cmd))
 
-      subject
+      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
     end
   end
 
@@ -80,7 +82,7 @@ shared_examples "#uninstall_phase or #zap_phase" do
       expect(main_pkg).to receive(:uninstall)
       expect(agent_pkg).to receive(:uninstall)
 
-      subject
+      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
     end
   end
 
@@ -89,43 +91,65 @@ shared_examples "#uninstall_phase or #zap_phase" do
     let(:kext_id) { "my.fancy.package.kernelextension" }
 
     it "is supported" do
-      FakeSystemCommand.stubs_command(
+      allow(subject).to receive(:system_command!)
-        sudo(%W[/usr/sbin/kextstat -l -b #{kext_id}]), "loaded"
+        .with("/usr/sbin/kextstat", args: ["-l", "-b", kext_id], sudo: true)
-      )
+        .and_return(instance_double("SystemCommand::Result", stdout: "loaded"))
 
-      FakeSystemCommand.expects_command(
+      expect(subject).to receive(:system_command!)
-        sudo(%W[/sbin/kextunload -b #{kext_id}]),
+        .with("/sbin/kextunload", args: ["-b", kext_id], sudo: true)
-      )
+        .and_return(instance_double("SystemCommand::Result"))
 
-      FakeSystemCommand.expects_command(
+      expect(subject).to receive(:system_command!)
-        sudo(%W[/usr/sbin/kextfind -b #{kext_id}]), "/Library/Extensions/FancyPackage.kext\n"
+        .with("/usr/sbin/kextfind", args: ["-b", kext_id], sudo: true)
-      )
+        .and_return(instance_double("SystemCommand::Result", stdout: "/Library/Extensions/FancyPackage.kext\n"))
 
-      FakeSystemCommand.expects_command(
+      expect(subject).to receive(:system_command!)
-        sudo(["/bin/rm", "-rf", "/Library/Extensions/FancyPackage.kext"]),
+        .with("/bin/rm", args: ["-rf", "/Library/Extensions/FancyPackage.kext"], sudo: true)
-      )
 
-      subject
+      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
     end
   end
 
   context "using :quit" do
     let(:cask) { Cask::CaskLoader.load(cask_path("with-#{artifact_dsl_key}-quit")) }
     let(:bundle_id) { "my.fancy.package.app" }
-    let(:quit_application_script) do
+
-      %Q(tell application id "#{bundle_id}" to quit)
+    it "is skipped when the user is not a GUI user" do
+      allow(User.current).to receive(:gui?).and_return false
+      allow(subject).to receive(:running_processes).with(bundle_id).and_return([[0, "", bundle_id]])
+
+      expect {
+        subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
+      }.to output(/Not logged into a GUI; skipping quitting application ID 'my.fancy.package.app'\./).to_stdout
     end
 
-    it "is supported" do
+    it "quits a running application" do
-      FakeSystemCommand.stubs_command(
+      allow(User.current).to receive(:gui?).and_return true
-        %w[/bin/launchctl list], "999\t0\t#{bundle_id}\n"
-      )
 
-      FakeSystemCommand.stubs_command(
+      expect(subject).to receive(:running_processes).with(bundle_id).ordered.and_return([[0, "", bundle_id]])
-        %w[/bin/launchctl list],
+      expect(subject).to receive(:quit).with(bundle_id)
-      )
+                                       .and_return(instance_double("SystemCommand::Result", success?: true))
+      expect(subject).to receive(:running_processes).with(bundle_id).ordered.and_return([])
 
-      subject
+      expect {
+        subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
+      }.to output(/Application 'my.fancy.package.app' quit successfully\./).to_stdout
+    end
+
+    it "tries to quit the application for 10 seconds" do
+      allow(User.current).to receive(:gui?).and_return true
+
+      allow(subject).to receive(:running_processes).with(bundle_id).and_return([[0, "", bundle_id]])
+      allow(subject).to receive(:quit).with(bundle_id)
+                                      .and_return(instance_double("SystemCommand::Result", success?: false))
+
+      time = Benchmark.measure do
+        expect {
+          subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
+        }.to output(/Application 'my.fancy.package.app' did not quit\./).to_stderr
+      end
+
+      expect(time.real).to be_within(3).of(10)
     end
   end
 
@@ -136,15 +160,14 @@ shared_examples "#uninstall_phase or #zap_phase" do
     let(:unix_pids) { [12_345, 67_890] }
 
     it "is supported" do
-      FakeSystemCommand.stubs_command(
+      allow(subject).to receive(:running_processes).with(bundle_id)
-        %w[/bin/launchctl list], unix_pids.map { |pid| [pid, 0, bundle_id].join("\t") }.join("\n")
+                                                   .and_return(unix_pids.map { |pid| [pid, 0, bundle_id] })
-      )
 
       signals.each do |signal|
         expect(Process).to receive(:kill).with(signal, *unix_pids)
       end
 
-      subject
+      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
     end
   end
 
@@ -161,7 +184,7 @@ shared_examples "#uninstall_phase or #zap_phase" do
       let(:fake_system_command) { NeverSudoSystemCommand }
       let(:cask) { Cask::CaskLoader.load(cask_path("with-#{artifact_dsl_key}-#{directive}")) }
 
-      around(:each) do |example|
+      around do |example|
         begin
           ENV["HOME"] = dir
 
@@ -173,18 +196,21 @@ shared_examples "#uninstall_phase or #zap_phase" do
         end
       end
 
-      before(:each) do
+      before do
+        # rubocop:disable RSpec/AnyInstance
         allow_any_instance_of(Cask::Artifact::AbstractUninstall).to receive(:trash_paths)
           .and_wrap_original do |method, *args|
-            result = method.call(*args)
+            method.call(*args).tap do |result|
-            FileUtils.rm_rf result.stdout.split("\0")
+              FileUtils.rm_rf result.stdout.split("\0")
+            end
           end
+        # rubocop:enable RSpec/AnyInstance
       end
 
       it "is supported" do
         expect(paths).to all(exist)
 
-        subject
+        subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
 
         paths.each do |path|
           expect(path).not_to exist
@@ -199,12 +225,12 @@ shared_examples "#uninstall_phase or #zap_phase" do
     let(:empty_directory) { Pathname.new("#{TEST_TMPDIR}/empty_directory_path") }
     let(:ds_store) { empty_directory.join(".DS_Store") }
 
-    before(:each) do
+    before do
       empty_directory.mkdir
       FileUtils.touch ds_store
     end
 
-    after(:each) do
+    after do
       FileUtils.rm_rf empty_directory
     end
 
@@ -212,7 +238,7 @@ shared_examples "#uninstall_phase or #zap_phase" do
       expect(empty_directory).to exist
       expect(ds_store).to exist
 
-      subject
+      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
 
       expect(ds_store).not_to exist
       expect(empty_directory).not_to exist
@@ -243,7 +269,7 @@ shared_examples "#uninstall_phase or #zap_phase" do
         )
 
         InstallHelper.install_without_artifacts(cask)
-        subject
+        subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
       end
     end
   end
@@ -252,12 +278,14 @@ shared_examples "#uninstall_phase or #zap_phase" do
     let(:cask) { Cask::CaskLoader.load(cask_path("with-#{artifact_dsl_key}-login-item")) }
 
     it "is supported" do
-      FakeSystemCommand.expects_command(
+      expect(subject).to receive(:system_command!)
-        ["/usr/bin/osascript", "-e", 'tell application "System Events" to delete every login ' \
+        .with(
-                                     'item whose name is "Fancy"'],
+          "osascript",
+        args: ["-e", 'tell application "System Events" to delete every login item whose name is "Fancy"'],
       )
+        .and_return(instance_double("SystemCommand::Result"))
 
-      subject
+      subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command)
     end
   end
 end

Library/Homebrew/test/cask/artifact/uninstall_spec.rb

@@ -1,4 +1,4 @@
-require_relative "uninstall_zap_shared_examples"
+require_relative "shared_examples/uninstall_zap"
 
 describe Cask::Artifact::Uninstall, :cask do
   describe "#uninstall_phase" do

Library/Homebrew/test/cask/artifact/zap_spec.rb

@@ -1,4 +1,4 @@
-require_relative "uninstall_zap_shared_examples"
+require_relative "shared_examples/uninstall_zap"
 
 describe Cask::Artifact::Zap, :cask do
   describe "#zap_phase" do