diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index f164a0fc60..9c978357ce 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,29 @@ module Homebrew end end - return if problem_count.zero? + created_pr_comment = false + if new_formula && !new_formula_problem_lines.empty? + begin + if GitHub.create_issue_comment(new_formula_problem_lines.join("\n")) + created_pr_comment = true + end + rescue *GitHub.api_errors + nil + end + end - ofail "#{Formatter.pluralize(problem_count, "problem")} in #{Formatter.pluralize(formula_count, "formula")}" + unless created_pr_comment + 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 +206,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 +221,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 +229,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 @@ -358,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| @@ -473,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 @@ -508,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 "Formulae should not require patches to build. Patches should be submitted and accepted upstream first." end %w[Stable Devel].each do |name| @@ -532,7 +558,7 @@ module Homebrew end if @new_formula && formula.head - problem "New formulae should not have a HEAD spec" + new_formula_problem "Formulae should not have a HEAD spec" end unstable_whitelist = %w[ @@ -813,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 f68126b773..aaaa158ab3 100644 --- a/Library/Homebrew/dev-cmd/bump-formula-pr.rb +++ b/Library/Homebrew/dev-cmd/bump-formula-pr.rb @@ -86,9 +86,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 @@ -291,7 +288,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 @bump_args.dry_run? odie "Unable to fork: #{e.message}!" end @@ -327,7 +324,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..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) @@ -291,4 +293,38 @@ module GitHub uri.query = query_string(*queries, **qualifiers) open_api(uri) { |json| json.fetch("items", []) } end + + def create_issue_comment(body) + 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 } + 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 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) } + end + + def api_errors + [GitHub::AuthenticationFailedError, GitHub::HTTPNotFoundError, + GitHub::RateLimitExceededError, GitHub::Error, JSON::ParserError].freeze + end end