This repository has been archived by the owner. It is now read-only.
Permalink
Browse files

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.
  • Loading branch information...
1 parent 7a89354 commit 775184240d039973f3cd0751eb648d8840f70276 @jacknagel jacknagel committed Jan 23, 2013
@@ -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 @@ def as_flags
end
def include? name
- @args.include? '--' + name
+ args.include? '--' + name
end
def with? name
@@ -55,11 +55,11 @@ def without? name
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 @@ def stable?
# 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
@@ -138,7 +138,7 @@ def recommended?
end
def options
- Options.new((tags - RESERVED_TAGS).map { |o| Option.new(o) })
+ Options.coerce(tags - RESERVED_TAGS)
end
end
@@ -690,7 +690,7 @@ def #{cksum}(val=nil)
end
def build
- @build ||= BuildOptions.new(ARGV)
+ @build ||= BuildOptions.new(ARGV.options_only)
end
def url val=nil, specs=nil
@@ -17,7 +17,7 @@ def initialize ff
@f = ff
@show_header = false
@ignore_deps = ARGV.ignore_deps? || ARGV.interactive?
- @options = []
+ @options = Options.new
@@attempted ||= Set.new
@@ -247,6 +247,17 @@ def build_time
@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 @@ 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 = 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 @@ def build
'-rbuild',
'--',
f.path,
- *args.options_only
+ *build_argv
rescue Exception => e
Marshal.dump(e, write)
write.close
@@ -300,7 +301,7 @@ def build
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
@@ -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 @@ def eql?(other)
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 @@ def -(o)
Options.new(@options - o)
end
+ def &(o)
+ Options.new(@options & o)
+ end
+
def *(arg)
@options.to_a * arg
end
@@ -71,8 +87,22 @@ def include?(o)
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
View
@@ -77,11 +77,11 @@ def universal?
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
@@ -3,7 +3,7 @@
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")
@@ -38,6 +38,12 @@ def test_description
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 @@ def test_to_ary
@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

0 comments on commit 7751842

Please sign in to comment.