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

Warn developers when uninstalling a dependency #1498

Merged
merged 4 commits into from
Nov 15, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Warn developers when uninstalling a dependency
Suggested in #1084.

Made the existing warning output entirely to STDERR, because
previously the first line went to STDERR and subsequent ones went
to STDOUT.
  • Loading branch information
alyssais committed Nov 14, 2016
commit 3c310b2e3dd7805b04f48507c65c2c0a856c2aa2
45 changes: 31 additions & 14 deletions Library/Homebrew/cmd/uninstall.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,8 @@ def uninstall
ARGV.kegs.group_by(&:rack)
end

if should_check_for_dependents?
all_kegs = kegs_by_rack.values.flatten(1)
return if check_for_dependents all_kegs
end
handle_unsatisfied_dependents(kegs_by_rack)
return if Homebrew.failed?

kegs_by_rack.each do |rack, kegs|
if ARGV.force?
Expand Down Expand Up @@ -78,28 +76,47 @@ def uninstall
end
end

def should_check_for_dependents?
# --ignore-dependencies, to be consistent with install
return false if ARGV.include?("--ignore-dependencies")
return false if ARGV.homebrew_developer?
true
def handle_unsatisfied_dependents(kegs_by_rack)
return if ARGV.include?("--ignore-dependencies")

all_kegs = kegs_by_rack.values.flatten(1)
check_for_dependents all_kegs
end

def check_for_dependents(kegs)
return false unless result = Keg.find_some_installed_dependents(kegs)

requireds, dependents = result
if ARGV.homebrew_developer?
dependents_output_for_developers(*result)
else
dependents_output_for_nondevelopers(*result)
end

true
end

def dependents_output_for_developers(requireds, dependents)
msg = requireds.join(", ")
msg << (requireds.count == 1 ? " is" : " are")
msg << " required by #{dependents.join(", ")}, which "
msg << (dependents.count == 1 ? "is" : "are")
msg << " currently installed."
msg << "\nYou can silence this warning with "
msg << "`brew uninstall --ignore-dependencies "
msg << "#{requireds.map(&:name).join(" ")}`."
Copy link
Member

Choose a reason for hiding this comment

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

Will merge this as-is but would be nice to refactor this to use EOS.undent and a bunch of variables declared before instead. Similarly below.

opoo msg
end

def dependents_output_for_nondevelopers(requireds, dependents)
msg = "Refusing to uninstall #{requireds.join(", ")} because "
msg << (requireds.count == 1 ? "it is" : "they are")
msg << " required by #{dependents.join(", ")}, which "
msg << (dependents.count == 1 ? "is" : "are")
msg << " currently installed."
msg << "\nYou can override this and force removal with "
msg << "`brew uninstall --ignore-dependencies "
msg << "#{requireds.map(&:name).join(" ")}`."
ofail msg
print "You can override this and force removal with "
puts "`brew uninstall --ignore-dependencies #{requireds.map(&:name).join(" ")}`."

true
end

def rm_pin(rack)
Expand Down
41 changes: 38 additions & 3 deletions Library/Homebrew/test/test_uninstall.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,55 @@
require "cmd/uninstall"

class UninstallTests < Homebrew::TestCase
def setup
@dependency = formula("dependency") { url "f-1" }
@dependent = formula("dependent") do
url "f-1"
depends_on "dependency"
end

[@dependency, @dependent].each { |f| f.installed_prefix.mkpath }

tab = Tab.empty
tab.tabfile = @dependent.installed_prefix/Tab::FILENAME
tab.runtime_dependencies = [
{ "full_name" => "dependency", "version" => "1" },
]
tab.write

stub_formula_loader @dependency
stub_formula_loader @dependent
end

def teardown
Homebrew.failed = false
[@dependency, @dependent].each { |f| f.rack.rmtree }
end

def handle_unsatisfied_dependents
capture_stderr do
opts = { @dependency.rack => [Keg.new(@dependency.installed_prefix)] }
Homebrew.handle_unsatisfied_dependents(opts)
end
end

def test_check_for_testball_f2s_when_developer
refute_predicate Homebrew, :should_check_for_dependents?
assert_match "Warning", handle_unsatisfied_dependents
refute_predicate Homebrew, :failed?
end

def test_check_for_dependents_when_not_developer
run_as_not_developer do
assert_predicate Homebrew, :should_check_for_dependents?
assert_match "Error", handle_unsatisfied_dependents
assert_predicate Homebrew, :failed?
end
end

def test_check_for_dependents_when_ignore_dependencies
ARGV << "--ignore-dependencies"
run_as_not_developer do
refute_predicate Homebrew, :should_check_for_dependents?
assert_empty handle_unsatisfied_dependents
refute_predicate Homebrew, :failed?
end
ensure
ARGV.delete("--ignore-dependencies")
Expand Down
4 changes: 4 additions & 0 deletions Library/Homebrew/test/test_utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ def test_gzip
end
end

def test_capture_stderr
assert_equal "test\n", capture_stderr { $stderr.puts "test" }
end

def test_shell_profile
ENV["SHELL"] = "/bin/sh"
assert_equal "~/.bash_profile", Utils::Shell.shell_profile
Expand Down
8 changes: 8 additions & 0 deletions Library/Homebrew/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,14 @@ def ignore_interrupts(opt = nil)
trap("INT", std_trap)
end

def capture_stderr
Copy link
Member

Choose a reason for hiding this comment

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

Think you can use shutup here. Again, won't let it block this PR but for post-merge refactoring that'd be good if it's possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think shutup returns the captured output. Would you be open to it being refactored to return the captured stdout and stderr?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, consolidation there makes sense although might want to tweak the name if we do that too. Definitely one for another PR 👍

old, $stderr = $stderr, StringIO.new
Copy link
Member

Choose a reason for hiding this comment

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

brew style complains about the double assignment.

yield
$stderr.string
ensure
$stderr = old
end

def nostdout
if ARGV.verbose?
yield
Expand Down