From b5f942764a58c1a04e04de83e94eb59a915ee3cb Mon Sep 17 00:00:00 2001 From: Charlie Sharpsteen Date: Sat, 26 Nov 2011 21:03:46 -0800 Subject: [PATCH] Re-work ARGV filtering to properly handle --HEAD Previously, stripping arguments like `--HEAD` for dependencies failed because that flag affects the installation prefix encoded into formula objects. The previous implementation of `ARGV` filtering tried to contain all changes to a single method call before the `FormulaInstaller` forks. This update spreads things out a bit: - The Homebrew `ARGV` extension adds a new method, `filter_for_dependencies` which strips flags like `--HEAD`, yields to a block, then restores the original contents of ARGV. - The `explicitly_requested?` test, which returns true or false depending on if a formula object is a member of `ARGV.formulae`, is now a method of `Formula` objects. - `FormulaInstaller` objects now execute the installation of dependencies inside an `ARGV.filter_for_dependencies` block if the dependency was `explicitly_requested?`. Fixes Homebrew/homebrew#8668. Closes Homebrew/homebrew#7724. --- Library/Homebrew/cmd/install.rb | 4 ++ Library/Homebrew/extend/ARGV.rb | 18 ++++++ Library/Homebrew/formula.rb | 10 ++++ Library/Homebrew/formula_installer.rb | 79 +++++++++------------------ 4 files changed, 59 insertions(+), 52 deletions(-) diff --git a/Library/Homebrew/cmd/install.rb b/Library/Homebrew/cmd/install.rb index 7142a5fcbd..2848986d4f 100644 --- a/Library/Homebrew/cmd/install.rb +++ b/Library/Homebrew/cmd/install.rb @@ -84,6 +84,10 @@ module Homebrew extend self unless formulae.empty? perform_preinstall_checks formulae.each do |f| + # Check formula status and skip if necessary---a formula passed on the + # command line may have been installed to satisfy a dependency. + next if f.installed? + begin fi = FormulaInstaller.new(f) fi.install diff --git a/Library/Homebrew/extend/ARGV.rb b/Library/Homebrew/extend/ARGV.rb index e46c0b0386..9a86e86ece 100644 --- a/Library/Homebrew/extend/ARGV.rb +++ b/Library/Homebrew/extend/ARGV.rb @@ -96,6 +96,24 @@ module HomebrewArgvExtension Homebrew.help_s end + def filter_for_dependencies + # Clears some flags that affect installation, yields to a block, then + # restores to original state. + old_args = clone + + %w[ + --debug -d + --fresh + --interactive -i + --verbose -v + --HEAD + ].each {|flag| delete flag} + + yield + + replace old_args + end + private def downcased_unique_named diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index bf930f5297..9c63a35ffb 100644 --- a/Library/Homebrew/formula.rb +++ b/Library/Homebrew/formula.rb @@ -148,6 +148,16 @@ class Formula return false end + def explicitly_requested? + + # `ARGV.formulae` will throw an exception if it comes up with an empty + # list. + # + # FIXME: `ARGV.formulae` shouldn't be throwing exceptions, see issue #8823 + return false if ARGV.named.empty? + ARGV.formulae.include? self + end + def installed_prefix head_prefix = HOMEBREW_CELLAR+@name+'HEAD' if @version == 'HEAD' || head_prefix.directory? diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index 61b2b608c4..bd7c672c32 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -28,15 +28,17 @@ class FormulaInstaller needed_deps = f.recursive_deps.reject{ |d| d.installed? } unless needed_deps.empty? needed_deps.each do |dep| - fi = FormulaInstaller.new(dep) - fi.ignore_deps = true - fi.show_header = false - oh1 "Installing #{f} dependency: #{dep}" - fi.install - fi.caveats - fi.finish + if dep.explicitly_requested? + install_dependency dep + else + ARGV.filter_for_dependencies do + # Re-create the formula object so that args like `--HEAD` won't + # affect properties like the installation prefix. + dep = Formula.factory dep.name + install_dependency dep + end + end end - # now show header as all the deps stuff has clouded the original issue show_header = true end @@ -58,6 +60,16 @@ class FormulaInstaller raise "Nothing was installed to #{f.prefix}" unless f.installed? end + def install_dependency dep + fi = FormulaInstaller.new dep + fi.ignore_deps = true + fi.show_header = false + oh1 "Installing #{f} dependency: #{dep}" + fi.install + fi.caveats + fi.finish + end + def caveats if f.caveats ohai "Caveats", f.caveats @@ -105,7 +117,13 @@ class FormulaInstaller # I'm guessing this is not a good way to do this, but I'm no UNIX guru ENV['HOMEBREW_ERROR_PIPE'] = write.to_i.to_s - args = filtered_args + args = ARGV.clone + unless args.include? '--fresh' + previous_install = Tab.for_formula f + args.concat previous_install.used_options + args.uniq! # Just in case some dupes were added + end + fork do begin read.close @@ -239,49 +257,6 @@ class FormulaInstaller @show_summary_heading = true end end - - private - - # This method gives us a chance to pre-process command line arguments before the - # installer forks and `Formula.install` kicks in. - def filtered_args - # Returns true if the formula attached to this installer was explicitly - # passed on the command line by the user as opposed to being automatically - # added to satisfy a dependency. - def explicitly_requested? - # `ARGV.formulae` will throw an exception if it comes up with an empty - # list. - # - # FIXME: - # `ARGV.formulae` probably shouldn't be throwing exceptions, it should be - # the caller's responsibility to check `ARGV.formulae.empty?`. - return false if ARGV.named.empty? - ARGV.formulae.include? f - end - - args = ARGV.clone - - # FIXME: Also need to remove `--HEAD`, however there is a problem in that - # the properties of formula objects, such as `prefix`, are influenced by - # `--HEAD` and get set when the object is created. - # - # See issue #8668 - %w[ - --debug -d - --fresh - --interactive -i - --verbose -v - ].each {|flag| args.delete flag} unless explicitly_requested? - - unless args.include? '--fresh' - previous_install = Tab.for_formula f - args.concat previous_install.used_options - end - - args.uniq! # Just in case some dupes slipped by - - return args - end end