From 20a1199375417ba49cf435dccb0104c0ad8fe8d0 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 5 May 2020 12:50:41 +0100 Subject: [PATCH] Refactor CLI to remove `unless args_parsed` Refactor the CLI::Args module so it doesn't have different paths to check arguments depending on whether the arguments have been parsed or not. Instead, set the values we need from the global ARGV at first, global initialisation time where they will be thrown away when the actual arguments are parsed. To do this some other general refactoring was needed: - more methods made private when possible - e.g. `HEAD?` used consistently instead of `head` before arguments are parsed. - formula options are only parsed after named arguments are extracted --- Library/Homebrew/cli/args.rb | 146 +++++++++++--------------- Library/Homebrew/cli/parser.rb | 53 ++++++---- Library/Homebrew/cmd/install.rb | 2 +- Library/Homebrew/dev-cmd/irb.rb | 6 +- Library/Homebrew/extend/ENV/shared.rb | 2 +- Library/Homebrew/fetch.rb | 2 +- Library/Homebrew/formula_installer.rb | 4 +- Library/Homebrew/global.rb | 2 +- Library/Homebrew/reinstall.rb | 2 +- Library/Homebrew/software_spec.rb | 2 +- 10 files changed, 104 insertions(+), 117 deletions(-) diff --git a/Library/Homebrew/cli/args.rb b/Library/Homebrew/cli/args.rb index 09b3c14ff0..350767fd5c 100644 --- a/Library/Homebrew/cli/args.rb +++ b/Library/Homebrew/cli/args.rb @@ -5,56 +5,44 @@ require "ostruct" module Homebrew module CLI class Args < OpenStruct - attr_reader :processed_options, :args_parsed # undefine tap to allow --tap argument undef tap - def initialize - super + def initialize(argv = ARGV.dup.freeze, set_default_args: false) + super() - self[:remaining] = [] - self[:argv] = ARGV.dup.freeze - - @args_parsed = false @processed_options = [] + + # Set values needed before Parser#parse has been run. + return unless set_default_args + + self[:build_from_source?] = argv.include?("--build-from-source") || argv.include?("-s") + self[:build_bottle?] = argv.include?("--build-bottle") + self[:force_bottle?] = argv.include?("--force-bottle") + self[:HEAD?] = argv.include?("--HEAD") + self[:devel?] = argv.include?("--devel") + self[:universal?] = argv.include?("--universal") + self[:named_args] = argv.reject { |arg| arg.start_with?("-") } + end + + def freeze_named_args!(named_args) + self[:named_args] = named_args + self[:named_args].freeze end def freeze_processed_options!(processed_options) @processed_options += processed_options @processed_options.freeze - @args_parsed = true - end - - def option_to_name(option) - option.sub(/\A--?/, "") - .tr("-", "_") - end - - def cli_args - return @cli_args if @cli_args - - @cli_args = [] - processed_options.each do |short, long| - option = long || short - switch = "#{option_to_name(option)}?".to_sym - flag = option_to_name(option).to_sym - if @table[switch] == true || @table[flag] == true - @cli_args << option - elsif @table[flag].instance_of? String - @cli_args << option + "=" + @table[flag] - elsif @table[flag].instance_of? Array - @cli_args << option + "=" + @table[flag].join(",") - end - end - @cli_args end def options_only @options_only ||= cli_args.select { |arg| arg.start_with?("-") } + .freeze end def flags_only @flags_only ||= cli_args.select { |arg| arg.start_with?("--") } + .freeze end def passthrough @@ -62,7 +50,7 @@ module Homebrew end def named - remaining + named_args || [] end def no_named? @@ -74,16 +62,17 @@ module Homebrew def collect_build_args build_flags = [] - build_flags << "--HEAD" if head - build_flags << "--universal" if build_universal - build_flags << "--build-bottle" if build_bottle - build_flags << "--build-from-source" if build_from_source + build_flags << "--HEAD" if HEAD? + build_flags << "--universal" if build_universal? + build_flags << "--build-bottle" if build_bottle? + build_flags << "--build-from-source" if build_from_source? build_flags end def formulae require "formula" + @formulae ||= (downcased_unique_named - casks).map do |name| if name.include?("/") || File.exist?(name) Formulary.factory(name, spec) @@ -91,29 +80,35 @@ module Homebrew Formulary.find_with_priority(name, spec) end end.uniq(&:name) + .freeze end def resolved_formulae require "formula" + @resolved_formulae ||= (downcased_unique_named - casks).map do |name| Formulary.resolve(name, spec: spec(nil)) end.uniq(&:name) + .freeze end def formulae_paths @formulae_paths ||= (downcased_unique_named - casks).map do |name| Formulary.path(name) end.uniq + .freeze end def casks - @casks ||= downcased_unique_named.grep HOMEBREW_CASK_TAP_CASK_REGEX + @casks ||= downcased_unique_named.grep(HOMEBREW_CASK_TAP_CASK_REGEX) + .freeze end def kegs require "keg" require "formula" require "missing_formula" + @kegs ||= downcased_unique_named.map do |name| raise UsageError if name.empty? @@ -158,7 +153,7 @@ module Homebrew Please delete (with rm -rf!) all but one and then try again. EOS end - end + end.freeze end def build_stable? @@ -168,39 +163,40 @@ module Homebrew # Whether a given formula should be built from source during the current # installation run. def build_formula_from_source?(f) - return false if !build_from_source && !build_bottle + return false if !build_from_source? && !build_bottle? formulae.any? { |args_f| args_f.full_name == f.full_name } end - def build_from_source - return argv.include?("--build-from-source") || argv.include?("-s") unless args_parsed - - build_from_source? || s? - end - - def build_bottle - return argv.include?("--build-bottle") unless args_parsed - - build_bottle? - end - - def force_bottle - return argv.include?("--force-bottle") unless args_parsed - - force_bottle? - end - private + def option_to_name(option) + option.sub(/\A--?/, "") + .tr("-", "_") + end + + def cli_args + return @cli_args if @cli_args + + @cli_args = [] + @processed_options.each do |short, long| + option = long || short + switch = "#{option_to_name(option)}?".to_sym + flag = option_to_name(option).to_sym + if @table[switch] == true || @table[flag] == true + @cli_args << option + elsif @table[flag].instance_of? String + @cli_args << option + "=" + @table[flag] + elsif @table[flag].instance_of? Array + @cli_args << option + "=" + @table[flag].join(",") + end + end + @cli_args.freeze + end + def downcased_unique_named # Only lowercase names, not paths, bottle filenames or URLs - arguments = if args_parsed - named - else - argv.reject { |arg| arg.start_with?("-") } - end - arguments.map do |arg| + named.map do |arg| if arg.include?("/") || arg.end_with?(".tar.gz") || File.exist?(arg) arg else @@ -209,28 +205,10 @@ module Homebrew end.uniq end - def head - return argv.include?("--HEAD") unless args_parsed - - HEAD? - end - - def devel - return argv.include?("--devel") unless args_parsed - - devel? - end - - def build_universal - return argv.include?("--universal") unless args_parsed - - universal? - end - def spec(default = :stable) - if head + if HEAD? :head - elsif devel + elsif devel? :devel else default diff --git a/Library/Homebrew/cli/parser.rb b/Library/Homebrew/cli/parser.rb index b17c915af6..7cf3889edd 100644 --- a/Library/Homebrew/cli/parser.rb +++ b/Library/Homebrew/cli/parser.rb @@ -12,8 +12,8 @@ module Homebrew class Parser attr_reader :processed_options, :hide_from_man_page - def self.parse(args = ARGV, allow_no_named_args: false, &block) - new(args, &block).parse(args, allow_no_named_args: allow_no_named_args) + def self.parse(argv = ARGV.dup.freeze, allow_no_named_args: false, &block) + new(argv, &block).parse(allow_no_named_args: allow_no_named_args) end def self.from_cmd_path(cmd_path) @@ -37,9 +37,10 @@ module Homebrew } end - def initialize(&block) + def initialize(argv = ARGV.dup.freeze, &block) @parser = OptionParser.new - @args = Homebrew::CLI::Args.new + @argv = argv + @args = Homebrew::CLI::Args.new(@argv) @constraints = [] @conflicts = [] @@ -152,7 +153,7 @@ module Homebrew @parser.to_s end - def parse(argv = ARGV, allow_no_named_args: false) + def parse(argv = @argv, allow_no_named_args: false) raise "Arguments were already parsed!" if @args_parsed begin @@ -161,12 +162,14 @@ module Homebrew $stderr.puts generate_help_text raise e end + check_constraint_violations check_named_args(named_args, allow_no_named_args: allow_no_named_args) - @args[:remaining] = named_args + @args.freeze_named_args!(named_args) + parse_formula_options @args.freeze_processed_options!(@processed_options) Homebrew.args = @args - argv.freeze + @args_parsed = true @parser end @@ -186,21 +189,7 @@ module Homebrew end def formula_options - @args.formulae.each do |f| - next if f.options.empty? - - f.options.each do |o| - name = o.flag - description = "`#{f.name}`: #{o.description}" - if name.end_with? "=" - flag name, description: description - else - switch name, description: description - end - end - end - rescue FormulaUnavailableError - [] + @parse_formula_options = true end def max_named(count) @@ -239,6 +228,26 @@ module Homebrew private + def parse_formula_options + return unless @parse_formula_options + + @args.formulae.each do |f| + next if f.options.empty? + + f.options.each do |o| + name = o.flag + description = "`#{f.name}`: #{o.description}" + if name.end_with? "=" + flag name, description: description + else + switch name, description: description + end + end + end + rescue FormulaUnavailableError + [] + end + def enable_switch(*names, from:) names.each do |name| @switch_sources[option_to_name(name)] = from diff --git a/Library/Homebrew/cmd/install.rb b/Library/Homebrew/cmd/install.rb index 644dd6039e..7d60894b9b 100644 --- a/Library/Homebrew/cmd/install.rb +++ b/Library/Homebrew/cmd/install.rb @@ -152,7 +152,7 @@ module Homebrew end # --HEAD, fail with no head defined - raise "No head is defined for #{f.full_name}" if args.head? && f.head.nil? + raise "No head is defined for #{f.full_name}" if args.HEAD? && f.head.nil? # --devel, fail with no devel defined raise "No devel block is defined for #{f.full_name}" if args.devel? && f.devel.nil? diff --git a/Library/Homebrew/dev-cmd/irb.rb b/Library/Homebrew/dev-cmd/irb.rb index de6f6d6376..e319acd972 100644 --- a/Library/Homebrew/dev-cmd/irb.rb +++ b/Library/Homebrew/dev-cmd/irb.rb @@ -18,7 +18,8 @@ module Homebrew module_function def irb_args - Homebrew::CLI::Parser.new do + # work around IRB modifying ARGV. + Homebrew::CLI::Parser.new(ARGV.dup) do usage_banner <<~EOS `irb` [] @@ -33,8 +34,7 @@ module Homebrew end def irb - # work around IRB modifying ARGV. - irb_args.parse(ARGV.dup) + irb_args.parse if args.examples? puts "'v8'.f # => instance of the v8 formula" diff --git a/Library/Homebrew/extend/ENV/shared.rb b/Library/Homebrew/extend/ENV/shared.rb index 0f6fda16e4..fc422fa352 100644 --- a/Library/Homebrew/extend/ENV/shared.rb +++ b/Library/Homebrew/extend/ENV/shared.rb @@ -268,7 +268,7 @@ module SharedEnvExtension # @private def effective_arch - if Homebrew.args.build_bottle && ARGV.bottle_arch + if Homebrew.args.build_bottle? && ARGV.bottle_arch ARGV.bottle_arch else Hardware.oldest_cpu diff --git a/Library/Homebrew/fetch.rb b/Library/Homebrew/fetch.rb index b2e04554b0..6564aaf955 100644 --- a/Library/Homebrew/fetch.rb +++ b/Library/Homebrew/fetch.rb @@ -5,7 +5,7 @@ module Homebrew module_function def fetch_bottle?(f) - return true if Homebrew.args.force_bottle && f.bottle + return true if Homebrew.args.force_bottle? && f.bottle return false unless f.bottle && f.pour_bottle? return false if Homebrew.args.build_formula_from_source?(f) return false unless f.bottle.compatible_cellar? diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index da9135767d..cd757386cb 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -49,9 +49,9 @@ class FormulaInstaller @show_header = false @ignore_deps = false @only_deps = false - @build_from_source = Homebrew.args.build_from_source + @build_from_source = Homebrew.args.build_from_source? @build_bottle = false - @force_bottle = Homebrew.args.force_bottle + @force_bottle = Homebrew.args.force_bottle? @include_test = ARGV.include?("--include-test") @interactive = false @git = false diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index 6e9aefb675..b7067e7717 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -85,7 +85,7 @@ module Homebrew end def args - @args ||= CLI::Args.new + @args ||= CLI::Args.new(set_default_args: true) end def messages diff --git a/Library/Homebrew/reinstall.rb b/Library/Homebrew/reinstall.rb index 4a53a2a757..8fc5ed649d 100644 --- a/Library/Homebrew/reinstall.rb +++ b/Library/Homebrew/reinstall.rb @@ -25,7 +25,7 @@ module Homebrew fi = FormulaInstaller.new(f) fi.options = options - fi.build_bottle = Homebrew.args.build_bottle + fi.build_bottle = Homebrew.args.build_bottle? fi.interactive = Homebrew.args.interactive? fi.git = Homebrew.args.git? fi.link_keg ||= keg_was_linked if keg_had_linked_opt diff --git a/Library/Homebrew/software_spec.rb b/Library/Homebrew/software_spec.rb index 6258a18091..676e7e524f 100644 --- a/Library/Homebrew/software_spec.rb +++ b/Library/Homebrew/software_spec.rb @@ -95,7 +95,7 @@ class SoftwareSpec def bottled? bottle_specification.tag?(Utils::Bottles.tag) && \ - (bottle_specification.compatible_cellar? || Homebrew.args.force_bottle) + (bottle_specification.compatible_cellar? || Homebrew.args.force_bottle?) end def bottle(disable_type = nil, disable_reason = nil, &block)