Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Commit

Permalink
Re-work ARGV filtering to properly handle --HEAD
Browse files Browse the repository at this point in the history
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 #8668.
Closes #7724.
  • Loading branch information
Sharpie committed Nov 27, 2011
1 parent c3307a5 commit 3edc9d2
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 52 deletions.
4 changes: 4 additions & 0 deletions Library/Homebrew/cmd/install.rb
Expand Up @@ -84,6 +84,10 @@ def install_formulae formulae
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?

This comment has been minimized.

Copy link
@mislav

mislav Dec 2, 2011

Contributor

This introduces a bug where --force argument is rendered useless. Remember, we only use --force on formulae that are already installed. This conditional doesn't repect --force.

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Dec 2, 2011

Member

We're reaching a point where I really think this stuff needs to be either (more) heavily unit tested or have a CI system that tests these things more throughly before merging.

This comment has been minimized.

Copy link
@adamv

adamv Dec 2, 2011

Contributor

Or even a written doc (outside of the man page) for what behaviors we want to guarantee.


begin
fi = FormulaInstaller.new(f)
fi.install
Expand Down
18 changes: 18 additions & 0 deletions Library/Homebrew/extend/ARGV.rb
Expand Up @@ -96,6 +96,24 @@ def usage
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
Expand Down
10 changes: 10 additions & 0 deletions Library/Homebrew/formula.rb
Expand Up @@ -148,6 +148,16 @@ def installed?
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?
Expand Down
79 changes: 27 additions & 52 deletions Library/Homebrew/formula_installer.rb
Expand Up @@ -28,15 +28,17 @@ def install
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

This comment has been minimized.

Copy link
@kunaldes

kunaldes Dec 26, 2011

This assumes all dependencies are installed :from_name.

This comment has been minimized.

Copy link
@Sharpie

Sharpie Dec 26, 2011

Author Contributor

Good catch. Fixed in 487cb70.

install_dependency dep
end
end
end

# now show header as all the deps stuff has clouded the original issue
show_header = true
end
Expand All @@ -58,6 +60,16 @@ def install
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
Expand Down Expand Up @@ -105,7 +117,13 @@ def build
# 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
Expand Down Expand Up @@ -239,49 +257,6 @@ def check_m4
@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


Expand Down

0 comments on commit 3edc9d2

Please sign in to comment.