Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
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...
commit f6d54c00281d0b8d3eb9f6779fbb8ff1b71117c2 1 parent 64a4c0f
@jacknagel authored
View
16 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 @@ 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
View
2  Library/Homebrew/dependencies.rb
@@ -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
View
2  Library/Homebrew/formula.rb
@@ -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
View
27 Library/Homebrew/formula_installer.rb
@@ -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
View
34 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 @@ 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
4 Library/Homebrew/tab.rb
@@ -73,11 +73,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
View
2  Library/Homebrew/test/test_build_options.rb
@@ -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")
View
53 Library/Homebrew/test/test_options.rb
@@ -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
Please sign in to comment.
Something went wrong with that request. Please try again.