FormulaInstaller: construct new ARGV from an Options collection

The array of options that is passed to the spawned build process is a
combination of the current ARGV, options passed in by a dependent
formula, and an existing install receipt. The objects that are
interacting here each expect the resulting collection to have certain
properties, and the expectations are not consistent.

Clear up this confusing mess by only dealing with Options collections.
This keeps our representation of options uniform across the codebase.

We can remove BuildOptions dependency on HomebrewArgvExtension, which
allows us to pass any Array-like collection to Tab.create. The only
other site inside of FormulaInstaller that uses the array is the #exec
call, and there it is splatted and thus we can substitute our Options
collection there as well.

8 files changed, 112 insertions, 28 deletions

diff --git a/Library/Homebrew/build_options.rb b/Library/Homebrew/build_options.rb
index 1718bc4a0..24c49931f 100644
--- a/Library/Homebrew/build_options.rb
+++ b/Library/Homebrew/build_options.rb
@@ -7,7 +7,7 @@ class BuildOptions
   include Enumerable
 
   def initialize args
-    @args = Array.new(args).extend(HomebrewArgvExtension)
+    @args = Options.coerce(args)
     @options = Options.new
   end
 
@@ -37,7 +37,7 @@ class BuildOptions
   end
 
   def include? name
-    @args.include? '--' + name
+    args.include? '--' + name
   end
 
   def with? name
@@ -55,11 +55,11 @@ class BuildOptions
   end
 
   def head?
-    @args.flag? '--HEAD'
+    args.include? '--HEAD'
   end
 
   def devel?
-    @args.include? '--devel'
+    args.include? '--devel'
   end
 
   def stable?
@@ -68,21 +68,21 @@ class BuildOptions
 
   # True if the user requested a universal build.
   def universal?
-    @args.include?('--universal') && has_option?('universal')
+    args.include?('--universal') && has_option?('universal')
   end
 
   # Request a 32-bit only build.
   # This is needed for some use-cases though we prefer to build Universal
   # when a 32-bit version is needed.
   def build_32_bit?
-    @args.include?('--32-bit') && has_option?('32-bit')
+    args.include?('--32-bit') && has_option?('32-bit')
   end
 
   def used_options
-    Options.new((as_flags & @args.options_only).map { |o| Option.new(o) })
+    Options.new(@options & @args)
   end
 
   def unused_options
-    Options.new((as_flags - @args.options_only).map { |o| Option.new(o) })
+    Options.new(@options - @args)
   end
 end
diff --git a/Library/Homebrew/dependencies.rb b/Library/Homebrew/dependencies.rb
index 404a34903..561294fb1 100644
--- a/Library/Homebrew/dependencies.rb
+++ b/Library/Homebrew/dependencies.rb
@@ -138,7 +138,7 @@ module Dependable
   end
 
   def options
-    Options.new((tags - RESERVED_TAGS).map { |o| Option.new(o) })
+    Options.coerce(tags - RESERVED_TAGS)
   end
 end
 
diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb
index e5b5d58fc..c877adb84 100644
--- a/Library/Homebrew/formula.rb
+++ b/Library/Homebrew/formula.rb
@@ -690,7 +690,7 @@ private
     end
 
     def build
-      @build ||= BuildOptions.new(ARGV)
+      @build ||= BuildOptions.new(ARGV.options_only)
     end
 
     def url val=nil, specs=nil
diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb
index 1e44b18e4..7e4418241 100644
--- a/Library/Homebrew/formula_installer.rb
+++ b/Library/Homebrew/formula_installer.rb
@@ -17,7 +17,7 @@ class FormulaInstaller
     @f = ff
     @show_header = false
     @ignore_deps = ARGV.ignore_deps? || ARGV.interactive?
-    @options = []
+    @options = Options.new
 
     @@attempted ||= Set.new
 
@@ -247,6 +247,17 @@ class FormulaInstaller
     @build_time ||= Time.now - @start_time unless pour_bottle? or ARGV.interactive? or @start_time.nil?
   end
 
+  def build_argv
+    @build_argv ||= begin
+      opts = Options.coerce(ARGV.options_only)
+      unless opts.include? '--fresh'
+        opts.concat(options) # from a dependent formula
+        opts.concat((tab.used_options rescue [])) # from a previous install
+      end
+      opts << '--build-from-source' # don't download bottle
+    end
+  end
+
   def build
     FileUtils.rm Dir["#{HOMEBREW_LOGS}/#{f}/*"]
 
@@ -261,16 +272,6 @@ 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 = ARGV.clone
-    args.concat(tab.used_options.as_flags) unless tab.nil? or args.include? '--fresh'
-    # FIXME: enforce the download of the non-bottled package
-    # in the spawned Ruby process.
-    args << '--build-from-source'
-    # Add any options that were passed into this FormulaInstaller instance.
-    # Usually this represents options being passed by a dependent formula.
-    args.concat options
-    args.uniq!
-
     fork do
       begin
         read.close
@@ -281,7 +282,7 @@ class FormulaInstaller
              '-rbuild',
              '--',
              f.path,
-             *args.options_only
+             *build_argv
       rescue Exception => e
         Marshal.dump(e, write)
         write.close
@@ -300,7 +301,7 @@ class FormulaInstaller
 
     raise "Empty installation" if Dir["#{f.prefix}/*"].empty?
 
-    Tab.create(f, args).write # INSTALL_RECEIPT.json
+    Tab.create(f, build_argv).write # INSTALL_RECEIPT.json
 
   rescue Exception => e
     ignore_interrupts do
diff --git a/Library/Homebrew/options.rb b/Library/Homebrew/options.rb
index 10abc1fae..e10d9483e 100644
--- a/Library/Homebrew/options.rb
+++ b/Library/Homebrew/options.rb
@@ -4,9 +4,8 @@ class Option
   attr_reader :name, :description, :flag
 
   def initialize(name, description=nil)
-    @name = name.to_s[/^(?:--)?(.+)$/, 1]
+    @name, @flag = split_name(name)
     @description = description.to_s
-    @flag = "--#{@name}"
   end
 
   def to_s
@@ -29,6 +28,19 @@ class Option
   def hash
     name.hash
   end
+
+  private
+
+  def split_name(name)
+    case name
+    when /^-[a-zA-Z]$/
+      [name[1..1], name]
+    when /^--(.+)$/
+      [$1, name]
+    else
+      [name, "--#{name}"]
+    end
+  end
 end
 
 class Options
@@ -55,6 +67,10 @@ class Options
     Options.new(@options - o)
   end
 
+  def &(o)
+    Options.new(@options & o)
+  end
+
   def *(arg)
     @options.to_a * arg
   end
@@ -71,8 +87,22 @@ class Options
     any? { |opt| opt == o || opt.name == o || opt.flag == o }
   end
 
+  def concat(o)
+    o.each { |opt| @options << opt }
+    self
+  end
+
   def to_a
     @options.to_a
   end
   alias_method :to_ary, :to_a
+
+  def self.coerce(arg)
+    case arg
+    when self then arg
+    when Option then new << arg
+    when Array then new(arg.map { |a| Option.new(a.to_s) })
+    else raise TypeError, "Cannot convert #{arg.inspect} to Options"
+    end
+  end
 end
diff --git a/Library/Homebrew/tab.rb b/Library/Homebrew/tab.rb
index 20f7b729b..15b0b59ea 100644
--- a/Library/Homebrew/tab.rb
+++ b/Library/Homebrew/tab.rb
@@ -77,11 +77,11 @@ class Tab < OpenStruct
   end
 
   def used_options
-    Options.new(super.map { |o| Option.new(o) })
+    Options.coerce(super)
   end
 
   def unused_options
-    Options.new(super.map { |o| Option.new(o) })
+    Options.coerce(super)
   end
 
   def options
diff --git a/Library/Homebrew/test/test_build_options.rb b/Library/Homebrew/test/test_build_options.rb
index cc73b28b1..daf5ef96b 100644
--- a/Library/Homebrew/test/test_build_options.rb
+++ b/Library/Homebrew/test/test_build_options.rb
@@ -3,7 +3,7 @@ require 'build_options'
 
 class BuildOptionsTests < Test::Unit::TestCase
   def setup
-    args = %w{--with-foo --with-bar --without-qux}.extend(HomebrewArgvExtension)
+    args = %w{--with-foo --with-bar --without-qux}
     @build = BuildOptions.new(args)
     @build.add("with-foo")
     @build.add("with-bar")
diff --git a/Library/Homebrew/test/test_options.rb b/Library/Homebrew/test/test_options.rb
index 065c00a82..78edb408c 100644
--- a/Library/Homebrew/test/test_options.rb
+++ b/Library/Homebrew/test/test_options.rb
@@ -38,6 +38,12 @@ class OptionTests < Test::Unit::TestCase
     assert_empty @option.description
     assert_equal "foo", Option.new("foo", "foo").description
   end
+
+  def test_preserves_short_options
+    option = Option.new("-d")
+    assert_equal "-d", option.flag
+    assert_equal "d", option.name
+  end
 end
 
 class OptionsTests < Test::Unit::TestCase
@@ -87,4 +93,51 @@ class OptionsTests < Test::Unit::TestCase
     @options << option
     assert_equal [option], @options.to_ary
   end
+
+  def test_concat_array
+    option = Option.new("foo")
+    @options.concat([option])
+    assert @options.include?(option)
+    assert_equal [option], @options.to_a
+  end
+
+  def test_concat_options
+    option = Option.new("foo")
+    opts = Options.new
+    opts << option
+    @options.concat(opts)
+    assert @options.include?(option)
+    assert_equal [option], @options.to_a
+  end
+
+  def test_concat_returns_self
+    assert_same @options, (@options.concat([]))
+  end
+
+  def test_intersection
+    foo, bar, baz = %w{foo bar baz}.map { |o| Option.new(o) }
+    options = Options.new << foo << bar
+    @options << foo << baz
+    assert_equal [foo], (@options & options).to_a
+  end
+
+  def test_coerce_with_options
+    assert_same @options, Options.coerce(@options)
+  end
+
+  def test_coerce_with_option
+    option = Option.new("foo")
+    assert_equal option, Options.coerce(option).to_a.first
+  end
+
+  def test_coerce_with_array
+    array = %w{--foo --bar}
+    option1 = Option.new("foo")
+    option2 = Option.new("bar")
+    assert_equal [option1, option2].sort, Options.coerce(array).to_a.sort
+  end
+
+  def test_coerce_raises_for_inappropriate_types
+    assert_raises(TypeError) { Options.coerce(1) }
+  end
 end