From 26e77dd75c11ed94a4eb420b8c0f63ad1e1ca969 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Mon, 30 Apr 2018 01:22:04 +0530 Subject: [PATCH 1/3] new-formula: Don't fail CI instead comment on PR about audit violations --- Library/Homebrew/dev-cmd/audit.rb | 50 ++++++++++++++++----- Library/Homebrew/dev-cmd/bump-formula-pr.rb | 7 +-- Library/Homebrew/test/dev-cmd/audit_spec.rb | 2 +- Library/Homebrew/utils/github.rb | 31 +++++++++++++ 4 files changed, 73 insertions(+), 17 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 45c9561c62..d7d54a9134 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -117,6 +117,7 @@ module Homebrew # Check style in a single batch run up front for performance style_results = check_style_json(files, options) + new_formula_problem_lines = [] ff.sort.each do |f| options = { new_formula: new_formula, strict: strict, online: online, only: args.only, except: args.except } options[:style_offenses] = style_results.file_offenses(f.path) @@ -127,7 +128,8 @@ module Homebrew fa.problems formula_count += 1 problem_count += fa.problems.size - problem_lines = fa.problems.map { |p| "* #{p.chomp.gsub("\n", "\n ")}" } + problem_lines = format_problem_lines(fa.problems) + new_formula_problem_lines = format_problem_lines(fa.new_formula_problems) if args.display_filename? puts problem_lines.map { |s| "#{f.path}: #{s}" } else @@ -135,9 +137,27 @@ module Homebrew end end - return if problem_count.zero? + create_issue_failed = false + if new_formula && !new_formula_problem_lines.size.zero? && !GitHub.create_issue_no_op? + begin + GitHub.create_issue_comment(new_formula_problem_lines.join("\n")) + rescue *GitHub.api_errors + create_issue_failed = true + end + end - ofail "#{Formatter.pluralize(problem_count, "problem")} in #{Formatter.pluralize(formula_count, "formula")}" + if GitHub.create_issue_no_op? || create_issue_failed + problem_count += new_formula_problem_lines.size + puts new_formula_problem_lines.map { |s| " #{s}" } + end + + errors_summary = "#{Formatter.pluralize(problem_count, "problem")} in #{Formatter.pluralize(formula_count, "formula")}" + return if problem_count.zero? + ofail errors_summary unless strict + end + + def format_problem_lines(problems) + problems.map { |p| "* #{p.chomp.gsub("\n", "\n ")}" } end class FormulaText @@ -184,7 +204,7 @@ module Homebrew class FormulaAuditor include FormulaCellarChecks - attr_reader :formula, :text, :problems + attr_reader :formula, :text, :problems, :new_formula_problems def initialize(formula, options = {}) @formula = formula @@ -199,6 +219,7 @@ module Homebrew # Allow the actual official-ness of a formula to be overridden, for testing purposes @official_tap = formula.tap&.official? || options[:official_tap] @problems = [] + @new_formula_problems = [] @text = FormulaText.new(formula.path) @specs = %w[stable devel head].map { |s| formula.send(s) }.compact end @@ -206,6 +227,10 @@ module Homebrew def audit_style return unless @style_offenses @style_offenses.each do |offense| + if offense.cop_name.start_with?("NewFormulaAudit") + new_formula_problem offense.to_s(display_cop_name: @display_cop_names) + next + end problem offense.to_s(display_cop_name: @display_cop_names) end end @@ -360,7 +385,7 @@ module Homebrew if @new_formula && dep_f.keg_only_reason && !["openssl", "apr", "apr-util"].include?(dep.name) && [:provided_by_macos, :provided_by_osx].include?(dep_f.keg_only_reason.reason) - problem "Dependency '#{dep.name}' may be unnecessary as it is provided by macOS; try to build this formula without it." + new_formula_problem "Dependency '#{dep.name}' may be unnecessary as it is provided by macOS; try to build this formula without it." end dep.options.each do |opt| @@ -475,15 +500,15 @@ module Homebrew return if metadata.nil? - problem "GitHub fork (not canonical repository)" if metadata["fork"] + new_formula_problem "GitHub fork (not canonical repository)" if metadata["fork"] if formula&.tap&.core_tap? && (metadata["forks_count"] < 20) && (metadata["subscribers_count"] < 20) && (metadata["stargazers_count"] < 50) - problem "GitHub repository not notable enough (<20 forks, <20 watchers and <50 stars)" + new_formula_problem "GitHub repository not notable enough (<20 forks, <20 watchers and <50 stars)" end return if Date.parse(metadata["created_at"]) <= (Date.today - 30) - problem "GitHub repository too new (<30 days old)" + new_formula_problem "GitHub repository too new (<30 days old)" end def audit_specs @@ -510,8 +535,7 @@ module Homebrew end next if spec.patches.empty? - next unless @new_formula - problem "New formulae should not require patches to build. Patches should be submitted and accepted upstream first." + new_formula_problem "New formulae should not require patches to build. Patches should be submitted and accepted upstream first." end %w[Stable Devel].each do |name| @@ -534,7 +558,7 @@ module Homebrew end if @new_formula && formula.head - problem "New formulae should not have a HEAD spec" + new_formula_problem "New formulae should not have a HEAD spec" end unstable_whitelist = %w[ @@ -815,6 +839,10 @@ module Homebrew @problems << p end + def new_formula_problem(p) + @new_formula_problems << p + end + def head_only?(formula) formula.head && formula.devel.nil? && formula.stable.nil? end diff --git a/Library/Homebrew/dev-cmd/bump-formula-pr.rb b/Library/Homebrew/dev-cmd/bump-formula-pr.rb index b5c1dccdd7..7ae7c461b2 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -131,9 +131,6 @@ module Homebrew 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 @@ -336,7 +333,7 @@ module Homebrew 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 + rescue *GitHub.api_errors => e formula.path.atomic_write(backup_file) unless ARGV.dry_run? odie "Unable to fork: #{e.message}!" end @@ -372,7 +369,7 @@ module Homebrew else exec_browser url end - rescue *gh_api_errors => e + rescue *GitHub.api_errors => e odie "Unable to open pull request: #{e.message}!" end end diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index 83b3ad215a..7d898f987f 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -281,7 +281,7 @@ module Homebrew fa.audit_deps end - its(:problems) { are_expected.to match([/unnecessary/]) } + its(:new_formula_problems) { are_expected.to match([/unnecessary/]) } end end end diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index e69e37cc5e..83496a45d9 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -291,4 +291,35 @@ module GitHub uri.query = query_string(*queries, **qualifiers) open_api(uri) { |json| json.fetch("items", []) } end + + def create_issue_no_op? + !ENV.key?("HOMEBREW_GH_REPO") || !ENV.key?("HOMEBREW_NEW_FORMULA_PULL_REQUEST_URL") + end + + def create_issue_comment(body) + repo = ENV["HOMEBREW_GH_REPO"] + pull_request = extract_pull_request_number + url = "https://api.github.com/repos/Homebrew/#{repo}/issues/#{pull_request}/comments" + data = { "body" => body } + return if issue_comment_exists?(repo, pr_number, body) + scopes = CREATE_ISSUE_SCOPES + open_api(url, data: data, scopes: scopes) + end + + def extract_pull_request_number + pattern = /#(\d+)$/ + ENV["HOMEBREW_NEW_FORMULA_PULL_REQUEST_URL"].match(pattern)[1] + end + + def issue_comment_exists?(repo, pull_reqest, body) + url = "https://api.github.com/repos/Homebrew/#{repo}/issues/#{pull_reqest}/comments" + comments = open_api(url) + return unless comments + comments.any? { |comment| comment["body"].eql?(body) } + end + + def api_errors + [GitHub::AuthenticationFailedError, GitHub::HTTPNotFoundError, + GitHub::RateLimitExceededError, GitHub::Error, JSON::ParserError].freeze + end end From e2968c788971e5a0fa5f7b31f4ad186477779cc4 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Mon, 21 May 2018 15:36:26 +0100 Subject: [PATCH 2/3] Various cleanup and fixes. Rename some variables and use existing regexes and a single variable. --- Library/Homebrew/dev-cmd/audit.rb | 12 +++++---- Library/Homebrew/utils/github.rb | 41 +++++++++++++++++-------------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index d7d54a9134..e915b62e63 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -137,16 +137,18 @@ module Homebrew end end - create_issue_failed = false - if new_formula && !new_formula_problem_lines.size.zero? && !GitHub.create_issue_no_op? + created_pr_comment = false + if new_formula && !new_formula_problem_lines.empty? begin - GitHub.create_issue_comment(new_formula_problem_lines.join("\n")) + if GitHub.create_issue_comment(new_formula_problem_lines.join("\n")) + created_pr_comment = true + end rescue *GitHub.api_errors - create_issue_failed = true + nil end end - if GitHub.create_issue_no_op? || create_issue_failed + unless created_pr_comment problem_count += new_formula_problem_lines.size puts new_formula_problem_lines.map { |s| " #{s}" } end diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 83496a45d9..75160c1edb 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -10,6 +10,8 @@ module GitHub CREATE_ISSUE_FORK_OR_PR_SCOPES = ["public_repo"].freeze ALL_SCOPES = (CREATE_GIST_SCOPES + CREATE_ISSUE_FORK_OR_PR_SCOPES).freeze ALL_SCOPES_URL = Formatter.url("https://github.com/settings/tokens/new?scopes=#{ALL_SCOPES.join(",")}&description=Homebrew").freeze + PR_ENV_KEY = "HOMEBREW_NEW_FORMULA_PULL_REQUEST_URL".freeze + PR_ENV = ENV[PR_ENV_KEY] Error = Class.new(RuntimeError) HTTPNotFoundError = Class.new(Error) @@ -254,14 +256,14 @@ module GitHub end def create_fork(repo) - url = "https://api.github.com/repos/#{repo}/forks" + url = "#{API_URL}/repos/#{repo}/forks" data = {} scopes = CREATE_ISSUE_FORK_OR_PR_SCOPES open_api(url, data: data, scopes: scopes) end def create_pull_request(repo, title, head, base, body) - url = "https://api.github.com/repos/#{repo}/pulls" + url = "#{API_URL}/repos/#{repo}/pulls" data = { title: title, head: head, base: base, body: body } scopes = CREATE_ISSUE_FORK_OR_PR_SCOPES open_api(url, data: data, scopes: scopes) @@ -292,27 +294,30 @@ module GitHub open_api(uri) { |json| json.fetch("items", []) } end - def create_issue_no_op? - !ENV.key?("HOMEBREW_GH_REPO") || !ENV.key?("HOMEBREW_NEW_FORMULA_PULL_REQUEST_URL") - end - def create_issue_comment(body) - repo = ENV["HOMEBREW_GH_REPO"] - pull_request = extract_pull_request_number - url = "https://api.github.com/repos/Homebrew/#{repo}/issues/#{pull_request}/comments" + return false unless PR_ENV + _, user, repo, pr = *PR_ENV.match(HOMEBREW_PULL_OR_COMMIT_URL_REGEX) + if !user || !repo || !pr + opoo <<-EOS.undent + #{PR_ENV_KEY} set but regex matched: + user: #{user.inspect}, repo: #{repo.inspect}, pr: #{pr.inspect} + EOS + return false + end + + url = "#{API_URL}/repos/#{user}/#{repo}/issues/#{pr}/comments" data = { "body" => body } - return if issue_comment_exists?(repo, pr_number, body) - scopes = CREATE_ISSUE_SCOPES + if issue_comment_exists?(user, repo, pr, body) + ohai "Skipping: identical comment exists on #{PR_ENV}." + return true + end + + scopes = CREATE_ISSUE_FORK_OR_PR_SCOPES open_api(url, data: data, scopes: scopes) end - def extract_pull_request_number - pattern = /#(\d+)$/ - ENV["HOMEBREW_NEW_FORMULA_PULL_REQUEST_URL"].match(pattern)[1] - end - - def issue_comment_exists?(repo, pull_reqest, body) - url = "https://api.github.com/repos/Homebrew/#{repo}/issues/#{pull_reqest}/comments" + def issue_comment_exists?(user, repo, pr, body) + url = "#{API_URL}/repos/#{user}/#{repo}/issues/#{pr}/comments" comments = open_api(url) return unless comments comments.any? { |comment| comment["body"].eql?(body) } From 8401cffe416e32b40301e4f9aabfbe9d07a2f5db Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 22 May 2018 09:55:41 +0100 Subject: [PATCH 3/3] audit: remove "new formula" from messaging. --- Library/Homebrew/dev-cmd/audit.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index d214b9c15f..9c978357ce 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -535,7 +535,7 @@ module Homebrew end next if spec.patches.empty? - new_formula_problem "New formulae should not require patches to build. Patches should be submitted and accepted upstream first." + new_formula_problem "Formulae should not require patches to build. Patches should be submitted and accepted upstream first." end %w[Stable Devel].each do |name| @@ -558,7 +558,7 @@ module Homebrew end if @new_formula && formula.head - new_formula_problem "New formulae should not have a HEAD spec" + new_formula_problem "Formulae should not have a HEAD spec" end unstable_whitelist = %w[