Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip quitting applications when not logged into GUI. #5010

Merged
merged 1 commit into from
Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion Library/Homebrew/cask/artifact/abstract_uninstall.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require "timeout"

require "utils/user"
require "cask/artifact/abstract_artifact"

module Cask
Expand Down Expand Up @@ -124,9 +125,14 @@ def running_processes(bundle_id, command: nil)
# :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|
ohai "Quitting application ID #{bundle_id}"
next if running_processes(bundle_id, command: command).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}'."
command.run!("/usr/bin/osascript", args: ["-e", %Q(tell application id "#{bundle_id}" to quit)], sudo: true)

begin
Expand Down
8 changes: 4 additions & 4 deletions Library/Homebrew/cask/artifact/pkg.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require "plist"

require "utils/user"
require "cask/artifact/abstract_artifact"

require "extend/hash_validator"
Expand Down Expand Up @@ -50,11 +51,10 @@ def run_installer(command: nil, verbose: false, **_options)
end
with_choices_file do |choices_path|
args << "-applyChoiceChangesXML" << choices_path if choices_path
logged_in_user = Utils.current_user
env = {
"LOGNAME" => logged_in_user,
"USER" => logged_in_user,
"USERNAME" => logged_in_user,
"LOGNAME" => User.current,
"USER" => User.current,
"USERNAME" => User.current,
}
command.run!("/usr/sbin/installer", sudo: true, args: args, print_stdout: true, env: env)
end
Expand Down
4 changes: 3 additions & 1 deletion Library/Homebrew/cask/caskroom.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require "utils/user"

module Cask
module Caskroom
module_function
Expand All @@ -18,7 +20,7 @@ def ensure_caskroom_exists

SystemCommand.run("/bin/mkdir", args: ["-p", path], sudo: sudo)
SystemCommand.run("/bin/chmod", args: ["g+rwx", path], sudo: sudo)
SystemCommand.run("/usr/sbin/chown", args: [Utils.current_user, path], sudo: sudo)
SystemCommand.run("/usr/sbin/chown", args: [User.current, path], sudo: sudo)
SystemCommand.run("/usr/bin/chgrp", args: ["admin", path], sudo: sudo)
end

Expand Down
8 changes: 3 additions & 5 deletions Library/Homebrew/cask/staged.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require "utils/user"

module Cask
module Staged
def info_plist_file(index = 0)
Expand Down Expand Up @@ -31,7 +33,7 @@ def set_permissions(paths, permissions_str)
sudo: false)
end

def set_ownership(paths, user: current_user, group: "staff")
def set_ownership(paths, user: User.current, group: "staff")
full_paths = remove_nonexistent(paths)
return if full_paths.empty?

Expand All @@ -40,10 +42,6 @@ def set_ownership(paths, user: current_user, group: "staff")
sudo: true)
end

def current_user
Utils.current_user
end

private

def remove_nonexistent(paths)
Expand Down
7 changes: 2 additions & 5 deletions Library/Homebrew/cask/utils.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require "utils/user"
require "yaml"
require "open3"
require "stringio"
Expand Down Expand Up @@ -52,7 +53,7 @@ def self.gain_permissions(path, command_args, command)
# before using sudo+chown
ohai "Using sudo to gain ownership of path '#{path}'"
command.run("/usr/sbin/chown",
args: command_args + ["--", current_user, path],
args: command_args + ["--", User.current, path],
sudo: true)
tried_ownership = true
# retry chflags/chmod after chown
Expand All @@ -62,10 +63,6 @@ def self.gain_permissions(path, command_args, command)
end
end

def self.current_user
Etc.getpwuid(Process.euid).name
end

def self.path_occupied?(path)
File.exist?(path) || File.symlink?(path)
end
Expand Down
12 changes: 7 additions & 5 deletions Library/Homebrew/system_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@
using HashValidator
require "extend/predicable"

def system_command(*args)
SystemCommand.run(*args)
end
module Kernel
def system_command(*args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the move?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason it cannot be called inside a DelegateClass, so it needs to be defined inside Kernel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird!

SystemCommand.run(*args)
end

def system_command!(*args)
SystemCommand.run!(*args)
def system_command!(*args)
SystemCommand.run!(*args)
end
end

class SystemCommand
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/test/cask/artifact/app_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
let(:force) { true }

before do
allow(Cask::Utils).to receive(:current_user).and_return("fake_user")
allow(User).to receive(:current).and_return(User.new("fake_user"))
end

describe "target is both writable and user-owned" do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
it "can set the ownership of a file" do
fake_pathname = existing_path

allow(staged).to receive(:current_user).and_return("fake_user")
allow(User).to receive(:current).and_return(User.new("fake_user"))
allow(staged).to receive(:Pathname).and_return(fake_pathname)

FakeSystemCommand.expects_command(
Expand All @@ -89,7 +89,7 @@
it "can set the ownership of multiple files" do
fake_pathname = existing_path

allow(staged).to receive(:current_user).and_return("fake_user")
allow(User).to receive(:current).and_return(User.new("fake_user"))
allow(staged).to receive(:Pathname).and_return(fake_pathname)

FakeSystemCommand.expects_command(
Expand All @@ -112,7 +112,7 @@
end

it "cannot set the ownership of a file that does not exist" do
allow(staged).to receive(:current_user).and_return("fake_user")
allow(User).to receive(:current).and_return(User.new("fake_user"))
fake_pathname = non_existent_path
allow(staged).to receive(:Pathname).and_return(fake_pathname)

Expand Down
36 changes: 36 additions & 0 deletions Library/Homebrew/test/utils/user_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
require "utils/user"

describe User do
subject { described_class.current }

it { is_expected.to eq ENV["USER"] }

describe "#gui?" do
before do
allow(SystemCommand).to receive(:run).with("who")
.and_return([who_output, "", instance_double(Process::Status, success?: true)])
end

context "when the current user is in a console session" do
let(:who_output) {
<<~EOS
#{ENV["USER"]} console Oct 1 11:23
#{ENV["USER"]} ttys001 Oct 1 11:25
EOS
}

its(:gui?) { is_expected.to be true }
end

context "when the current user is not in a console session" do
let(:who_output) {
<<~EOS
#{ENV["USER"]} ttys001 Oct 1 11:25
fake_user ttys002 Oct 1 11:27
EOS
}

its(:gui?) { is_expected.to be false }
end
end
end
18 changes: 18 additions & 0 deletions Library/Homebrew/utils/user.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
require "delegate"
require "etc"

require "system_command"

class User < DelegateClass(String)
def gui?
out, _, status = system_command "who"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks whether the user has a logged in GUI session. It does not check whether the current session is part of that GUI session.

For example, if you are logged in via the GUI and the Homebrew session is via SSH, this will return true, but AppleScript commands will still fail.

Have you tested any cases where this will return false? What are those cases?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reitermarkus let me know what you think of the above!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it should check the current session, not any session. I'm not sure how to do that, however. Feel free to send a PR to improve this.

return false unless status.success?
out.lines
.map(&:split)
.any? { |user, type,| user == self && type == "console" }
end

def self.current
@current ||= new(Etc.getpwuid(Process.euid).name)
end
end