From 20155c8df9569e78a16ea47136a711469a29e5b7 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Sun, 25 Mar 2018 23:49:54 +0530 Subject: [PATCH 1/3] bump-formula-pr: Use Parser to parse args --- Library/Homebrew/dev-cmd/bump-formula-pr.rb | 562 ++++++++++---------- 1 file changed, 291 insertions(+), 271 deletions(-) diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index b5c1dccdd7..daaf10e01a 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -42,16 +42,300 @@ #: the preexisting formula already uses. require "formula" +require "cli_parser" module Homebrew module_function + def bump_formula_pr + @args = Homebrew::CLI::Parser.parse do + switch "--devel" + switch "-n", "--dry-run" + switch "--write" + switch "--audit" + switch "--strict" + switch "--no-browse" + switch :quiet + switch :force + switch :debug + flag "--url", required: true + flag "--sha256", required: true + flag "--mirror", required: true + flag "--tag", required: true + flag "--revision", required: true + flag "--version", required: true + flag "--message", required: true + end + + # As this command is simplifying user run commands then let's just use a + # user path, too. + ENV["PATH"] = ENV["HOMEBREW_PATH"] + + # Use the user's browser, too. + ENV["BROWSER"] = ENV["HOMEBREW_BROWSER"] + + # Setup GitHub environment variables + %w[GITHUB_USER GITHUB_PASSWORD GITHUB_TOKEN].each do |env| + homebrew_env = ENV["HOMEBREW_#{env}"] + next unless homebrew_env + next if homebrew_env.empty? + ENV[env] = homebrew_env + end + + gh_api_errors = [GitHub::AuthenticationFailedError, GitHub::HTTPNotFoundError, + GitHub::RateLimitExceededError, GitHub::Error, JSON::ParserError].freeze + + formula = ARGV.formulae.first + + if formula + check_for_duplicate_pull_requests(formula) + checked_for_duplicates = true + end + + new_url = @args.url + if new_url && !formula + # Split the new URL on / and find any formulae that have the same URL + # except for the last component, but don't try to match any more than the + # first five components since sometimes the last component isn't the only + # one to change. + new_url_split = new_url.split("/") + maximum_url_components_to_match = 5 + components_to_match = [new_url_split.count - 1, maximum_url_components_to_match].min + base_url = new_url_split.first(components_to_match).join("/") + base_url = /#{Regexp.escape(base_url)}/ + is_devel = @args.devel? + guesses = [] + Formula.each do |f| + if is_devel && f.devel && f.devel.url && f.devel.url.match(base_url) + guesses << f + elsif f.stable&.url && f.stable.url.match(base_url) + guesses << f + end + end + if guesses.count == 1 + formula = guesses.shift + elsif guesses.count > 1 + odie "Couldn't guess formula for sure: could be one of these:\n#{guesses}" + end + end + odie "No formula found!" unless formula + + check_for_duplicate_pull_requests(formula) unless checked_for_duplicates + + requested_spec, formula_spec = if @args.devel? + devel_message = " (devel)" + [:devel, formula.devel] + else + [:stable, formula.stable] + end + odie "#{formula}: no #{requested_spec} specification found!" unless formula_spec + + hash_type, old_hash = if (checksum = formula_spec.checksum) + [checksum.hash_type, checksum.hexdigest] + end + + new_hash = @args[hash_type] + new_tag = @args.tag + new_revision = @args.revision + new_mirror = @args.mirror + forced_version = @args.version + new_url_hash = if new_url && new_hash + true + elsif new_tag && new_revision + false + elsif !hash_type + odie "#{formula}: no --tag=/--revision= arguments specified!" + elsif !new_url + odie "#{formula}: no --url= argument specified!" + else + new_mirror = case new_url + when requested_spec != :devel && %r{.*ftp.gnu.org/gnu.*} + new_url.sub "ftp.gnu.org/gnu", "ftpmirror.gnu.org" + when %r{.*mirrors.ocf.berkeley.edu/debian.*} + new_url.sub "mirrors.ocf.berkeley.edu/debian", "mirrorservice.org/sites/ftp.debian.org/debian" + end + resource = Resource.new { @url = new_url } + resource.download_strategy = DownloadStrategyDetector.detect_from_url(new_url) + resource.owner = Resource.new(formula.name) + resource.version = forced_version if forced_version + odie "No --version= argument specified!" unless resource.version + resource_path = resource.fetch + tar_file_extensions = %w[.tar .tb2 .tbz .tbz2 .tgz .tlz .txz .tZ] + if tar_file_extensions.any? { |extension| new_url.include? extension } + gnu_tar_gtar_path = HOMEBREW_PREFIX/"opt/gnu-tar/bin/gtar" + gnu_tar_gtar = gnu_tar_gtar_path if gnu_tar_gtar_path.executable? + tar = which("gtar") || gnu_tar_gtar || which("tar") + if Utils.popen_read(tar, "-tf", resource_path) =~ %r{/.*\.} + new_hash = resource_path.sha256 + else + odie "#{resource_path} is not a valid tar file!" + end + else + new_hash = resource_path.sha256 + end + end + + if @args.dry_run? + ohai "brew update" + else + safe_system "brew", "update" + end + + old_formula_version = formula_version(formula, requested_spec) + + replacement_pairs = [] + if requested_spec == :stable && formula.revision.nonzero? + replacement_pairs << [/^ revision \d+\n(\n( head "))?/m, "\\2"] + end + + replacement_pairs += formula_spec.mirrors.map do |mirror| + [/ +mirror \"#{Regexp.escape(mirror)}\"\n/m, ""] + end + + replacement_pairs += if new_url_hash + [ + [/#{Regexp.escape(formula_spec.url)}/, new_url], + [old_hash, new_hash], + ] + else + [ + [formula_spec.specs[:tag], new_tag], + [formula_spec.specs[:revision], new_revision], + ] + end + + backup_file = File.read(formula.path) unless @args.dry_run? + + if new_mirror + replacement_pairs << [/^( +)(url \"#{Regexp.escape(new_url)}\"\n)/m, "\\1\\2\\1mirror \"#{new_mirror}\"\n"] + end + + if forced_version && forced_version != "0" + if requested_spec == :stable + if File.read(formula.path).include?("version \"#{old_formula_version}\"") + replacement_pairs << [old_formula_version.to_s, forced_version] + elsif new_mirror + replacement_pairs << [/^( +)(mirror \"#{new_mirror}\"\n)/m, "\\1\\2\\1version \"#{forced_version}\"\n"] + else + replacement_pairs << [/^( +)(url \"#{new_url}\"\n)/m, "\\1\\2\\1version \"#{forced_version}\"\n"] + end + elsif requested_spec == :devel + replacement_pairs << [/( devel do.+?version \")#{old_formula_version}(\"\n.+?end\n)/m, "\\1#{forced_version}\\2"] + end + elsif forced_version && forced_version == "0" + if requested_spec == :stable + replacement_pairs << [/^ version \"[\w\.\-\+]+\"\n/m, ""] + elsif requested_spec == :devel + replacement_pairs << [/( devel do.+?)^ +version \"[^\n]+\"\n(.+?end\n)/m, "\\1\\2"] + end + end + new_contents = inreplace_pairs(formula.path, replacement_pairs) + + new_formula_version = formula_version(formula, requested_spec, new_contents) + + if new_formula_version < old_formula_version + formula.path.atomic_write(backup_file) unless @args.dry_run? + odie <<~EOS + You probably need to bump this formula manually since changing the + version from #{old_formula_version} to #{new_formula_version} would be a downgrade. + EOS + elsif new_formula_version == old_formula_version + formula.path.atomic_write(backup_file) unless @args.dry_run? + odie <<~EOS + You probably need to bump this formula manually since the new version + and old version are both #{new_formula_version}. + EOS + end + + if @args.dry_run? + if @args.strict? + ohai "brew audit --strict #{formula.path.basename}" + elsif @args.audit? + ohai "brew audit #{formula.path.basename}" + end + else + failed_audit = false + if @args.strict? + system HOMEBREW_BREW_FILE, "audit", "--strict", formula.path + failed_audit = !$CHILD_STATUS.success? + elsif @args.audit? + system HOMEBREW_BREW_FILE, "audit", formula.path + failed_audit = !$CHILD_STATUS.success? + end + if failed_audit + formula.path.atomic_write(backup_file) + odie "brew audit failed!" + end + end + + formula.path.parent.cd do + branch = "#{formula.name}-#{new_formula_version}" + git_dir = Utils.popen_read("git rev-parse --git-dir").chomp + shallow = !git_dir.empty? && File.exist?("#{git_dir}/shallow") + + if @args.dry_run? + ohai "fork repository with GitHub API" + ohai "git fetch --unshallow origin" if shallow + ohai "git checkout --no-track -b #{branch} origin/master" + ohai "git commit --no-edit --verbose --message='#{formula.name} #{new_formula_version}#{devel_message}' -- #{formula.path}" + ohai "git push --set-upstream $HUB_REMOTE #{branch}:#{branch}" + ohai "create pull request with GitHub API" + ohai "git checkout -" + else + + begin + response = GitHub.create_fork(formula.tap.full_name) + # GitHub API responds immediately but fork takes a few seconds to be ready. + sleep 3 + rescue *gh_api_errors => e + formula.path.atomic_write(backup_file) unless @args.dry_run? + odie "Unable to fork: #{e.message}!" + end + + remote_url = response.fetch("clone_url") + username = response.fetch("owner").fetch("login") + + safe_system "git", "fetch", "--unshallow", "origin" if shallow + safe_system "git", "checkout", "--no-track", "-b", branch, "origin/master" + safe_system "git", "commit", "--no-edit", "--verbose", + "--message=#{formula.name} #{new_formula_version}#{devel_message}", + "--", formula.path + safe_system "git", "push", "--set-upstream", remote_url, "#{branch}:#{branch}" + safe_system "git", "checkout", "--quiet", "-" + pr_message = <<~EOS + Created with `brew bump-formula-pr`. + EOS + user_message = @args.message + if user_message + pr_message += "\n" + <<~EOS + --- + + #{user_message} + EOS + end + pr_title = "#{formula.name} #{new_formula_version}#{devel_message}" + + begin + url = GitHub.create_pull_request(formula.tap.full_name, pr_title, + "#{username}:#{branch}", "master", pr_message)["html_url"] + if @args.no_browse? + puts url + else + exec_browser url + end + rescue *gh_api_errors => e + odie "Unable to open pull request: #{e.message}!" + end + end + end + end + def inreplace_pairs(path, replacement_pairs) - if ARGV.dry_run? + if @args.dry_run? contents = path.open("r") { |f| Formulary.ensure_utf8_encoding(f).read } contents.extend(StringInreplaceExtension) replacement_pairs.each do |old, new| - unless ARGV.flag?("--quiet") + unless Homebrew.args.quiet? ohai "replace #{old.inspect} with #{new.inspect}" end contents.gsub!(old, new) @@ -59,12 +343,12 @@ module Homebrew unless contents.errors.empty? raise Utils::InreplaceError, path => contents.errors end - path.atomic_write(contents) if ARGV.include?("--write") + path.atomic_write(contents) if @args.write? contents else Utils::Inreplace.inreplace(path) do |s| replacement_pairs.each do |old, new| - unless ARGV.flag?("--quiet") + unless Homebrew.args.quiet? ohai "replace #{old.inspect} with #{new.inspect}" end s.gsub!(old, new) @@ -103,279 +387,15 @@ module Homebrew #{pull_requests.map { |pr| "#{pr["title"]} #{pr["html_url"]}" }.join("\n")} EOS error_message = "Duplicate PRs should not be opened. Use --force to override this error." - if ARGV.force? && !ARGV.flag?("--quiet") + if Homebrew.args.force? && !Homebrew.args.quiet? opoo duplicates_message - elsif !ARGV.force? && ARGV.flag?("--quiet") + elsif !Homebrew.args.force? && Homebrew.args.quiet? odie error_message - elsif !ARGV.force? + elsif !Homebrew.args.force? odie <<~EOS #{duplicates_message.chomp} #{error_message} EOS end end - - def bump_formula_pr - # As this command is simplifying user run commands then let's just use a - # user path, too. - ENV["PATH"] = ENV["HOMEBREW_PATH"] - - # Use the user's browser, too. - ENV["BROWSER"] = ENV["HOMEBREW_BROWSER"] - - # Setup GitHub environment variables - %w[GITHUB_USER GITHUB_PASSWORD GITHUB_TOKEN].each do |env| - homebrew_env = ENV["HOMEBREW_#{env}"] - next unless homebrew_env - next if homebrew_env.empty? - ENV[env] = homebrew_env - end - - gh_api_errors = [GitHub::AuthenticationFailedError, GitHub::HTTPNotFoundError, - GitHub::RateLimitExceededError, GitHub::Error, JSON::ParserError].freeze - - formula = ARGV.formulae.first - - if formula - check_for_duplicate_pull_requests(formula) - checked_for_duplicates = true - end - - new_url = ARGV.value("url") - if new_url && !formula - # Split the new URL on / and find any formulae that have the same URL - # except for the last component, but don't try to match any more than the - # first five components since sometimes the last component isn't the only - # one to change. - new_url_split = new_url.split("/") - maximum_url_components_to_match = 5 - components_to_match = [new_url_split.count - 1, maximum_url_components_to_match].min - base_url = new_url_split.first(components_to_match).join("/") - base_url = /#{Regexp.escape(base_url)}/ - is_devel = ARGV.include?("--devel") - guesses = [] - Formula.each do |f| - if is_devel && f.devel && f.devel.url && f.devel.url.match(base_url) - guesses << f - elsif f.stable&.url && f.stable.url.match(base_url) - guesses << f - end - end - if guesses.count == 1 - formula = guesses.shift - elsif guesses.count > 1 - odie "Couldn't guess formula for sure: could be one of these:\n#{guesses}" - end - end - odie "No formula found!" unless formula - - check_for_duplicate_pull_requests(formula) unless checked_for_duplicates - - requested_spec, formula_spec = if ARGV.include?("--devel") - devel_message = " (devel)" - [:devel, formula.devel] - else - [:stable, formula.stable] - end - odie "#{formula}: no #{requested_spec} specification found!" unless formula_spec - - hash_type, old_hash = if (checksum = formula_spec.checksum) - [checksum.hash_type.to_s, checksum.hexdigest] - end - - new_hash = ARGV.value(hash_type) - new_tag = ARGV.value("tag") - new_revision = ARGV.value("revision") - new_mirror = ARGV.value("mirror") - forced_version = ARGV.value("version") - new_url_hash = if new_url && new_hash - true - elsif new_tag && new_revision - false - elsif !hash_type - odie "#{formula}: no --tag=/--revision= arguments specified!" - elsif !new_url - odie "#{formula}: no --url= argument specified!" - else - new_mirror = case new_url - when requested_spec != :devel && %r{.*ftp.gnu.org/gnu.*} - new_url.sub "ftp.gnu.org/gnu", "ftpmirror.gnu.org" - when %r{.*mirrors.ocf.berkeley.edu/debian.*} - new_url.sub "mirrors.ocf.berkeley.edu/debian", "mirrorservice.org/sites/ftp.debian.org/debian" - end - resource = Resource.new { @url = new_url } - resource.download_strategy = DownloadStrategyDetector.detect_from_url(new_url) - resource.owner = Resource.new(formula.name) - resource.version = forced_version if forced_version - odie "No --version= argument specified!" unless resource.version - resource_path = resource.fetch - tar_file_extensions = %w[.tar .tb2 .tbz .tbz2 .tgz .tlz .txz .tZ] - if tar_file_extensions.any? { |extension| new_url.include? extension } - gnu_tar_gtar_path = HOMEBREW_PREFIX/"opt/gnu-tar/bin/gtar" - gnu_tar_gtar = gnu_tar_gtar_path if gnu_tar_gtar_path.executable? - tar = which("gtar") || gnu_tar_gtar || which("tar") - if Utils.popen_read(tar, "-tf", resource_path) =~ %r{/.*\.} - new_hash = resource_path.sha256 - else - odie "#{resource_path} is not a valid tar file!" - end - else - new_hash = resource_path.sha256 - end - end - - if ARGV.dry_run? - ohai "brew update" - else - safe_system "brew", "update" - end - - old_formula_version = formula_version(formula, requested_spec) - - replacement_pairs = [] - if requested_spec == :stable && formula.revision.nonzero? - replacement_pairs << [/^ revision \d+\n(\n( head "))?/m, "\\2"] - end - - replacement_pairs += formula_spec.mirrors.map do |mirror| - [/ +mirror \"#{Regexp.escape(mirror)}\"\n/m, ""] - end - - replacement_pairs += if new_url_hash - [ - [/#{Regexp.escape(formula_spec.url)}/, new_url], - [old_hash, new_hash], - ] - else - [ - [formula_spec.specs[:tag], new_tag], - [formula_spec.specs[:revision], new_revision], - ] - end - - backup_file = File.read(formula.path) unless ARGV.dry_run? - - if new_mirror - replacement_pairs << [/^( +)(url \"#{Regexp.escape(new_url)}\"\n)/m, "\\1\\2\\1mirror \"#{new_mirror}\"\n"] - end - - if forced_version && forced_version != "0" - if requested_spec == :stable - if File.read(formula.path).include?("version \"#{old_formula_version}\"") - replacement_pairs << [old_formula_version.to_s, forced_version] - elsif new_mirror - replacement_pairs << [/^( +)(mirror \"#{new_mirror}\"\n)/m, "\\1\\2\\1version \"#{forced_version}\"\n"] - else - replacement_pairs << [/^( +)(url \"#{new_url}\"\n)/m, "\\1\\2\\1version \"#{forced_version}\"\n"] - end - elsif requested_spec == :devel - replacement_pairs << [/( devel do.+?version \")#{old_formula_version}(\"\n.+?end\n)/m, "\\1#{forced_version}\\2"] - end - elsif forced_version && forced_version == "0" - if requested_spec == :stable - replacement_pairs << [/^ version \"[\w\.\-\+]+\"\n/m, ""] - elsif requested_spec == :devel - replacement_pairs << [/( devel do.+?)^ +version \"[^\n]+\"\n(.+?end\n)/m, "\\1\\2"] - end - end - new_contents = inreplace_pairs(formula.path, replacement_pairs) - - new_formula_version = formula_version(formula, requested_spec, new_contents) - - if new_formula_version < old_formula_version - formula.path.atomic_write(backup_file) unless ARGV.dry_run? - odie <<~EOS - You probably need to bump this formula manually since changing the - version from #{old_formula_version} to #{new_formula_version} would be a downgrade. - EOS - elsif new_formula_version == old_formula_version - formula.path.atomic_write(backup_file) unless ARGV.dry_run? - odie <<~EOS - You probably need to bump this formula manually since the new version - and old version are both #{new_formula_version}. - EOS - end - - if ARGV.dry_run? - if ARGV.include? "--strict" - ohai "brew audit --strict #{formula.path.basename}" - elsif ARGV.include? "--audit" - ohai "brew audit #{formula.path.basename}" - end - else - failed_audit = false - if ARGV.include? "--strict" - system HOMEBREW_BREW_FILE, "audit", "--strict", formula.path - failed_audit = !$CHILD_STATUS.success? - elsif ARGV.include? "--audit" - system HOMEBREW_BREW_FILE, "audit", formula.path - failed_audit = !$CHILD_STATUS.success? - end - if failed_audit - formula.path.atomic_write(backup_file) - odie "brew audit failed!" - end - end - - formula.path.parent.cd do - branch = "#{formula.name}-#{new_formula_version}" - git_dir = Utils.popen_read("git rev-parse --git-dir").chomp - shallow = !git_dir.empty? && File.exist?("#{git_dir}/shallow") - - if ARGV.dry_run? - ohai "fork repository with GitHub API" - ohai "git fetch --unshallow origin" if shallow - ohai "git checkout --no-track -b #{branch} origin/master" - ohai "git commit --no-edit --verbose --message='#{formula.name} #{new_formula_version}#{devel_message}' -- #{formula.path}" - ohai "git push --set-upstream $HUB_REMOTE #{branch}:#{branch}" - ohai "create pull request with GitHub API" - ohai "git checkout -" - else - - begin - response = GitHub.create_fork(formula.tap.full_name) - # GitHub API responds immediately but fork takes a few seconds to be ready. - sleep 3 - rescue *gh_api_errors => e - formula.path.atomic_write(backup_file) unless ARGV.dry_run? - odie "Unable to fork: #{e.message}!" - end - - remote_url = response.fetch("clone_url") - username = response.fetch("owner").fetch("login") - - safe_system "git", "fetch", "--unshallow", "origin" if shallow - safe_system "git", "checkout", "--no-track", "-b", branch, "origin/master" - safe_system "git", "commit", "--no-edit", "--verbose", - "--message=#{formula.name} #{new_formula_version}#{devel_message}", - "--", formula.path - safe_system "git", "push", "--set-upstream", remote_url, "#{branch}:#{branch}" - safe_system "git", "checkout", "--quiet", "-" - pr_message = <<~EOS - Created with `brew bump-formula-pr`. - EOS - user_message = ARGV.value("message") - if user_message - pr_message += "\n" + <<~EOS - --- - - #{user_message} - EOS - end - pr_title = "#{formula.name} #{new_formula_version}#{devel_message}" - - begin - url = GitHub.create_pull_request(formula.tap.full_name, pr_title, - "#{username}:#{branch}", "master", pr_message)["html_url"] - if ARGV.include?("--no-browse") - puts url - else - exec_browser url - end - rescue *gh_api_errors => e - odie "Unable to open pull request: #{e.message}!" - end - end - end - end end From 36c1ad9f64cfa11265e504bea7fe69fdb69e9fb2 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Sun, 1 Apr 2018 22:01:06 +0530 Subject: [PATCH 2/3] cli_parser: Add depends, conflicts option constraints --- Library/Homebrew/cli_parser.rb | 62 +++++++++++++++++++++ Library/Homebrew/dev-cmd/bump-formula-pr.rb | 4 ++ Library/Homebrew/test/cli_parser_spec.rb | 37 ++++++++++++ 3 files changed, 103 insertions(+) diff --git a/Library/Homebrew/cli_parser.rb b/Library/Homebrew/cli_parser.rb index 5ce769343b..d92a2a4ff4 100644 --- a/Library/Homebrew/cli_parser.rb +++ b/Library/Homebrew/cli_parser.rb @@ -13,6 +13,8 @@ module Homebrew @parsed_args = OpenStruct.new # undefine tap to allow --tap argument @parsed_args.instance_eval { undef tap } + @depends = [] + @conflicts = [] instance_eval(&block) end @@ -47,6 +49,14 @@ module Homebrew end end + def depends(primary, secondary, mandatory: false) + @depends << [primary, secondary, mandatory] + end + + def conflicts(primary, secondary) + @conflicts << [primary, secondary] + end + def option_to_name(name) name.sub(/\A--?/, "").tr("-", "_") end @@ -57,6 +67,7 @@ module Homebrew def parse(cmdline_args = ARGV) @parser.parse(cmdline_args) + check_constraint_violations @parsed_args end @@ -82,6 +93,57 @@ module Homebrew else name end end + + def option_passed?(name) + @parsed_args.respond_to?(name) || @parsed_args.respond_to?("#{name}?") + end + + def check_depends + @depends.each do |primary, secondary, required| + primary_passed = option_passed?(primary) + secondary_passed = option_passed?(secondary) + raise OptionDependencyError.new(primary, secondary) if required && primary_passed && + !secondary_passed + raise OptionDependencyError.new(primary, secondary, missing: true) if secondary_passed && + !primary_passed + end + end + + def check_conflicts + @conflicts.each do |primary, secondary| + primary_passed = option_passed?(primary) + secondary_passed = option_passed?(secondary) + raise OptionConflictError.new(primary, secondary) if primary_passed && secondary_passed + end + end + + def check_constraint_violations + check_conflicts + check_depends + end + end + + class OptionDependencyError < RuntimeError + def initialize(arg1, arg2, missing: false) + if !missing + message = <<~EOS + `#{arg1}` and `#{arg2}` should be passed together + EOS + else + message = <<~EOS + `#{arg2}` cannot be passed without `#{arg1}` + EOS + end + super message + end + end + + class OptionConflictError < RuntimeError + def initialize(arg1, arg2) + super <<~EOS + `#{arg1}` and `#{arg2}` should not be passed together + EOS + end end end end diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index daaf10e01a..8a3abba080 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -57,6 +57,7 @@ module Homebrew switch "--no-browse" switch :quiet switch :force + switch :verbose switch :debug flag "--url", required: true flag "--sha256", required: true @@ -65,6 +66,9 @@ module Homebrew flag "--revision", required: true flag "--version", required: true flag "--message", required: true + depends :url, :sha256 + depends :tag, :revision, mandatory: true + conflicts :url, :tag end # As this command is simplifying user run commands then let's just use a diff --git a/Library/Homebrew/test/cli_parser_spec.rb b/Library/Homebrew/test/cli_parser_spec.rb index 5f65a80c15..82ab944363 100644 --- a/Library/Homebrew/test/cli_parser_spec.rb +++ b/Library/Homebrew/test/cli_parser_spec.rb @@ -70,4 +70,41 @@ describe Homebrew::CLI::Parser do expect(args.files).to eq %w[random1.txt random2.txt] end end + + describe "test constraints" do + subject(:parser) { + described_class.new do + flag "--flag1" + flag "--flag2" + flag "--flag3" + flag "--flag4" + depends :flag1, :flag2, mandatory: true + depends :flag3, :flag4 + conflicts :flag1, :flag3 + end + } + + it "raises exception on depends mandatory constraint violation" do + expect { parser.parse(["--flag1"]) }.to raise_error(Homebrew::CLI::OptionDependencyError) + end + + it "raises exception on depends constraint violation" do + expect { parser.parse(["--flag2"]) }.to raise_error(Homebrew::CLI::OptionDependencyError) + end + + it "raises exception for conflict violation" do + expect { parser.parse(["--flag1", "--flag3"]) }.to raise_error(Homebrew::CLI::OptionConflictError) + end + + it "raises no exception" do + args = parser.parse(["--flag1", "--flag2"]) + expect(args.flag1).to be true + expect(args.flag2).to be true + end + + it "raises no exception for optional dependency" do + args = parser.parse(["--flag3"]) + expect(args.flag3).to be true + end + end end From 07ee23d711954bfada003d9edf1fa7977a00e682 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Sat, 14 Apr 2018 16:17:14 +0530 Subject: [PATCH 3/3] cli_parser: Refactor interface for depends, conflicts and add tests --- Library/Homebrew/cli_parser.rb | 77 +++++++++++++++------ Library/Homebrew/dev-cmd/bump-formula-pr.rb | 40 +++++------ Library/Homebrew/test/cli_parser_spec.rb | 31 ++++++--- 3 files changed, 97 insertions(+), 51 deletions(-) diff --git a/Library/Homebrew/cli_parser.rb b/Library/Homebrew/cli_parser.rb index d92a2a4ff4..228eab7dd9 100644 --- a/Library/Homebrew/cli_parser.rb +++ b/Library/Homebrew/cli_parser.rb @@ -1,5 +1,6 @@ require "optparse" require "ostruct" +require "set" module Homebrew module CLI @@ -13,7 +14,7 @@ module Homebrew @parsed_args = OpenStruct.new # undefine tap to allow --tap argument @parsed_args.instance_eval { undef tap } - @depends = [] + @constraints = [] @conflicts = [] instance_eval(&block) end @@ -36,7 +37,7 @@ module Homebrew end end - def flag(name, description: nil) + def flag(name, description: nil, required_for: nil, depends_on: nil) if name.end_with? "=" required = OptionParser::REQUIRED_ARGUMENT name.chomp! "=" @@ -47,18 +48,16 @@ module Homebrew @parser.on(name, description, required) do |option_value| @parsed_args[option_to_name(name)] = option_value end + + set_constraints(name, required_for: required_for, depends_on: depends_on) end - def depends(primary, secondary, mandatory: false) - @depends << [primary, secondary, mandatory] - end - - def conflicts(primary, secondary) - @conflicts << [primary, secondary] + def conflicts(*options) + @conflicts << options.map { |option| option_to_name(option) } end def option_to_name(name) - name.sub(/\A--?/, "").tr("-", "_") + name.sub(/\A--?/, "").tr("-", "_").delete("=") end def option_to_description(*names) @@ -98,32 +97,57 @@ module Homebrew @parsed_args.respond_to?(name) || @parsed_args.respond_to?("#{name}?") end - def check_depends - @depends.each do |primary, secondary, required| + def set_constraints(name, depends_on:, required_for:) + secondary = option_to_name(name) + unless required_for.nil? + primary = option_to_name(required_for) + @constraints << [primary, secondary, :mandatory] + end + + return if depends_on.nil? + primary = option_to_name(depends_on) + @constraints << [primary, secondary, :optional] + end + + def check_constraints + @constraints.each do |primary, secondary, constraint_type| primary_passed = option_passed?(primary) secondary_passed = option_passed?(secondary) - raise OptionDependencyError.new(primary, secondary) if required && primary_passed && - !secondary_passed - raise OptionDependencyError.new(primary, secondary, missing: true) if secondary_passed && - !primary_passed + if :mandatory.equal?(constraint_type) && primary_passed && !secondary_passed + raise OptionConstraintError.new(primary, secondary) + end + if secondary_passed && !primary_passed + raise OptionConstraintError.new(primary, secondary, missing: true) + end end end def check_conflicts - @conflicts.each do |primary, secondary| - primary_passed = option_passed?(primary) - secondary_passed = option_passed?(secondary) - raise OptionConflictError.new(primary, secondary) if primary_passed && secondary_passed + @conflicts.each do |mutually_exclusive_options_group| + violations = mutually_exclusive_options_group.select do |option| + option_passed? option + end + raise OptionConflictError, violations if violations.length > 1 + end + end + + def check_invalid_constraints + @conflicts.each do |mutually_exclusive_options_group| + @constraints.each do |p, s| + next unless Set[p, s].subset?(Set[*mutually_exclusive_options_group]) + raise InvalidConstraintError.new(p, s) + end end end def check_constraint_violations + check_invalid_constraints check_conflicts - check_depends + check_constraints end end - class OptionDependencyError < RuntimeError + class OptionConstraintError < RuntimeError def initialize(arg1, arg2, missing: false) if !missing message = <<~EOS @@ -139,9 +163,18 @@ module Homebrew end class OptionConflictError < RuntimeError + def initialize(args) + args_list = args.join("` and `") + super <<~EOS + `#{args_list}` are mutually exclusive + EOS + end + end + + class InvalidConstraintError < RuntimeError def initialize(arg1, arg2) super <<~EOS - `#{arg1}` and `#{arg2}` should not be passed together + `#{arg1}` and `#{arg2}` cannot be mutually exclusive and mutually dependent simultaneously EOS end end diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index 8a3abba080..66a72fe2a4 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -49,26 +49,26 @@ module Homebrew def bump_formula_pr @args = Homebrew::CLI::Parser.parse do - switch "--devel" - switch "-n", "--dry-run" - switch "--write" - switch "--audit" - switch "--strict" - switch "--no-browse" - switch :quiet - switch :force - switch :verbose - switch :debug - flag "--url", required: true - flag "--sha256", required: true - flag "--mirror", required: true - flag "--tag", required: true - flag "--revision", required: true - flag "--version", required: true - flag "--message", required: true - depends :url, :sha256 - depends :tag, :revision, mandatory: true - conflicts :url, :tag + switch "--devel" + switch "-n", "--dry-run" + switch "--write" + switch "--audit" + switch "--strict" + switch "--no-browse" + switch :quiet + switch :force + switch :verbose + switch :debug + + flag "--url=" + flag "--revision=" + flag "--tag=", required_for: "--revision=" + flag "--sha256=", depends_on: "--url=" + flag "--mirror=" + flag "--version=" + flag "--message=" + + conflicts "--url", "--tag" end # As this command is simplifying user run commands then let's just use a diff --git a/Library/Homebrew/test/cli_parser_spec.rb b/Library/Homebrew/test/cli_parser_spec.rb index 82ab944363..418d3234fa 100644 --- a/Library/Homebrew/test/cli_parser_spec.rb +++ b/Library/Homebrew/test/cli_parser_spec.rb @@ -74,22 +74,21 @@ describe Homebrew::CLI::Parser do describe "test constraints" do subject(:parser) { described_class.new do - flag "--flag1" - flag "--flag2" - flag "--flag3" - flag "--flag4" - depends :flag1, :flag2, mandatory: true - depends :flag3, :flag4 - conflicts :flag1, :flag3 + flag "--flag1" + flag "--flag3" + flag "--flag2", required_for: "--flag1" + flag "--flag4", depends_on: "--flag3" + + conflicts "--flag1", "--flag3" end } it "raises exception on depends mandatory constraint violation" do - expect { parser.parse(["--flag1"]) }.to raise_error(Homebrew::CLI::OptionDependencyError) + expect { parser.parse(["--flag1"]) }.to raise_error(Homebrew::CLI::OptionConstraintError) end it "raises exception on depends constraint violation" do - expect { parser.parse(["--flag2"]) }.to raise_error(Homebrew::CLI::OptionDependencyError) + expect { parser.parse(["--flag2"]) }.to raise_error(Homebrew::CLI::OptionConstraintError) end it "raises exception for conflict violation" do @@ -107,4 +106,18 @@ describe Homebrew::CLI::Parser do expect(args.flag3).to be true end end + + describe "test invalid constraints" do + subject(:parser) { + described_class.new do + flag "--flag1" + flag "--flag2", depends_on: "--flag1" + conflicts "--flag1", "--flag2" + end + } + + it "raises exception due to invalid constraints" do + expect { parser.parse([]) }.to raise_error(Homebrew::CLI::InvalidConstraintError) + end + end end