Merge pull request #3835 from GauthamGoli/bot-comment

audit:  `--new-formula`, Don't fail CI instead comment on PR about audit violations

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
+        new_formula_problem "Formulae should not require patches to build. Patches should be submitted and accepted upstream first."
-        problem "New 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

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

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

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