Merge pull request #15105 from issyl0/cask-audit-only-failures-default

audit: Make `--display-failures-only` the default for Casks

.github/workflows/tests.yml

@@ -151,7 +151,7 @@ jobs:
         run: brew readall --aliases homebrew/core
 
       - name: Run brew audit --skip-style on homebrew/core
-        run: brew audit --skip-style --except=version --display-failures-only --tap=homebrew/core
+        run: brew audit --skip-style --except=version --tap=homebrew/core
 
   cask-audit:
     name: cask audit
@@ -187,10 +187,10 @@ jobs:
 
       - name: Run brew audit --skip-style on casks
         run: |
-          brew audit --skip-style --except=version --display-failures-only --tap=homebrew/cask
+          brew audit --skip-style --except=version --tap=homebrew/cask
-          brew audit --skip-style --except=version --display-failures-only --tap=homebrew/cask-drivers
+          brew audit --skip-style --except=version --tap=homebrew/cask-drivers
-          brew audit --skip-style --except=version --display-failures-only --tap=homebrew/cask-fonts
+          brew audit --skip-style --except=version --tap=homebrew/cask-fonts
-          brew audit --skip-style --except=version --display-failures-only --tap=homebrew/cask-versions
+          brew audit --skip-style --except=version --tap=homebrew/cask-versions
 
   vendored-gems:
     name: vendored gems

Library/Homebrew/cask/audit.rb

@@ -71,53 +71,31 @@ module Cask
       @errors ||= []
     end
 
-    def warnings
-      @warnings ||= []
-    end
-
     sig { returns(T::Boolean) }
     def errors?
       errors.any?
     end
 
-    sig { returns(T::Boolean) }
-    def warnings?
-      warnings.any?
-    end
-
     sig { returns(T::Boolean) }
     def success?
-      !(errors? || warnings?)
+      !errors?
     end
 
-    sig { params(message: T.nilable(String), location: T.nilable(String)).void }
+    sig { params(message: T.nilable(String), location: T.nilable(String), strict_only: T::Boolean).void }
-    def add_error(message, location: nil)
+    def add_error(message, location: nil, strict_only: false)
+      # Only raise non-critical audits if the user specified `--strict`.
+      return if strict_only && !@strict
+
       errors << ({ message: message, location: location })
     end
 
-    sig { params(message: T.nilable(String), location: T.nilable(String)).void }
-    def add_warning(message, location: nil)
-      if strict?
-        add_error message, location: location
-      else
-        warnings << ({ message: message, location: location })
-      end
-    end
-
     def result
-      if errors?
+      Formatter.error("failed") if errors?
-        Formatter.error("failed")
-      elsif warnings?
-        Formatter.warning("warning")
-      else
-        Formatter.success("passed")
-      end
     end
 
-    sig { params(include_passed: T::Boolean, include_warnings: T::Boolean).returns(T.nilable(String)) }
+    sig { returns(T.nilable(String)) }
-    def summary(include_passed: false, include_warnings: true)
+    def summary
-      return if success? && !include_passed
+      return if success?
-      return if warnings? && !errors? && !include_warnings
 
       summary = ["audit for #{cask}: #{result}"]
 
@@ -125,12 +103,6 @@ module Cask
         summary << " #{Formatter.error("-")} #{error[:message]}"
       end
 
-      if include_warnings
-        warnings.each do |warning|
-          summary << " #{Formatter.warning("-")} #{warning[:message]}"
-        end
-      end
-
       summary.join("\n")
     end
 
@@ -220,7 +192,7 @@ module Cask
       # increases the maintenance burden.
       return if cask.tap == "homebrew/cask-fonts"
 
-      add_warning "Cask should have a description. Please add a `desc` stanza." if cask.desc.blank?
+      add_error("Cask should have a description. Please add a `desc` stanza.", strict_only: true) if cask.desc.blank?
     end
 
     sig { void }
@@ -408,8 +380,10 @@ module Cask
       return unless token_conflicts?
       return unless core_formula_names.include?(cask.token)
 
-      add_warning "possible duplicate, cask token conflicts with Homebrew core formula: " \
+      add_error(
-                  "#{Formatter.url(core_formula_url)}"
+        "possible duplicate, cask token conflicts with Homebrew core formula: #{Formatter.url(core_formula_url)}",
+        strict_only: true,
+      )
     end
 
     sig { void }
@@ -443,18 +417,19 @@ module Cask
         add_error "cask token contains version designation '#{match_data[:designation]}'"
       end
 
-      add_warning "cask token mentions launcher" if token.end_with? "launcher"
+      add_error("cask token mentions launcher", strict_only: true) if token.end_with? "launcher"
 
-      add_warning "cask token mentions desktop" if token.end_with? "desktop"
+      add_error("cask token mentions desktop", strict_only: true) if token.end_with? "desktop"
 
-      add_warning "cask token mentions platform" if token.end_with? "mac", "osx", "macos"
+      add_error("cask token mentions platform", strict_only: true) if token.end_with? "mac", "osx", "macos"
 
-      add_warning "cask token mentions architecture" if token.end_with? "x86", "32_bit", "x86_64", "64_bit"
+      add_error("cask token mentions architecture", strict_only: true) if token.end_with? "x86", "32_bit", "x86_64",
+                                                                                          "64_bit"
 
       frameworks = %w[cocoa qt gtk wx java]
       return if frameworks.include?(token) || !token.end_with?(*frameworks)
 
-      add_warning "cask token mentions framework"
+      add_error("cask token mentions framework", strict_only: true)
     end
 
     sig { void }
@@ -474,7 +449,10 @@ module Cask
       return if cask.url.to_s.include? cask.version.csv.second
       return if cask.version.csv.third.present? && cask.url.to_s.include?(cask.version.csv.third)
 
-      add_warning "Download does not require additional version components. Use `&:short_version` in the livecheck"
+      add_error(
+        "Download does not require additional version components. Use `&:short_version` in the livecheck",
+        strict_only: true,
+      )
     end
 
     sig { void }
@@ -518,7 +496,7 @@ module Cask
               "#{message} fix the signature of their app."
             end
 
-            add_warning message
+            add_error(message, strict_only: true)
           when Artifact::Pkg
             path = downloaded_path
             next unless path.exist?
@@ -526,7 +504,7 @@ module Cask
             result = system_command("pkgutil", args: ["--check-signature", path], print_stderr: false)
 
             unless result.success?
-              add_warning <<~EOS
+              add_error(<<~EOS, strict_only: true)
                 Signature verification failed:
                 #{result.merged_output}
                 macOS on ARM requires applications to be signed.
@@ -538,7 +516,7 @@ module Cask
             result = system_command("stapler", args: ["validate", path], print_stderr: false)
             next if result.success?
 
-            add_warning <<~EOS
+            add_error(<<~EOS, strict_only: true)
               Signature verification failed:
               #{result.merged_output}
               macOS on ARM requires applications to be signed.
@@ -663,16 +641,10 @@ module Cask
 
       metadata = SharedAudits.github_repo_data(user, repo)
       return if metadata.nil?
-
       return unless metadata["archived"]
 
-      message = "GitHub repo is archived"
+      # Discontinued casks shouldn't show up as errors.
-
+      add_error("GitHub repo is archived", strict_only: !cask.discontinued?)
-      if cask.discontinued?
-        add_warning message
-      else
-        add_error message
-      end
     end
 
     sig { void }
@@ -684,16 +656,10 @@ module Cask
 
       metadata = SharedAudits.gitlab_repo_data(user, repo)
       return if metadata.nil?
-
       return unless metadata["archived"]
 
-      message = "GitLab repo is archived"
+      # Discontinued casks shouldn't show up as errors.
-
+      add_error("GitLab repo is archived") unless cask.discontinued?
-      if cask.discontinued?
-        add_warning message
-      else
-        add_error message
-      end
     end
 
     sig { void }

Library/Homebrew/cask/auditor.rb

@@ -25,8 +25,6 @@ module Cask
       quarantine: nil,
       any_named_args: nil,
       language: nil,
-      display_passes: nil,
-      display_failures_only: nil,
       only: [],
       except: []
     )
@@ -40,8 +38,6 @@ module Cask
       @audit_token_conflicts = audit_token_conflicts
       @any_named_args = any_named_args
       @language = language
-      @display_passes = display_passes
-      @display_failures_only = display_failures_only
       @only = only
       @except = except
     end
@@ -49,7 +45,6 @@ module Cask
     LANGUAGE_BLOCK_LIMIT = 10
 
     def audit
-      warnings = Set.new
       errors = Set.new
 
       if !language && language_blocks
@@ -63,23 +58,19 @@ module Cask
 
         sample_languages.each_key do |l|
           audit = audit_languages(l)
-          summary = audit.summary(include_passed: output_passed?, include_warnings: output_warnings?)
+          if audit.summary.present? && output_summary?(audit)
-          if summary.present? && output_summary?(audit)
             ohai "Auditing language: #{l.map { |lang| "'#{lang}'" }.to_sentence}" if output_summary?
-            puts summary
+            puts audit.summary
           end
-          warnings += audit.warnings
           errors += audit.errors
         end
       else
         audit = audit_cask_instance(cask)
-        summary = audit.summary(include_passed: output_passed?, include_warnings: output_warnings?)
+        puts audit.summary if audit.summary.present? && output_summary?(audit)
-        puts summary if summary.present? && output_summary?(audit)
-        warnings += audit.warnings
         errors += audit.errors
       end
 
-      { warnings: warnings, errors: errors }
+      errors
     end
 
     private
@@ -92,19 +83,6 @@ module Cask
       audit.errors?
     end
 
-    def output_passed?
-      return false if @display_failures_only.present?
-      return true if @display_passes.present?
-
-      false
-    end
-
-    def output_warnings?
-      return false if @display_failures_only.present?
-
-      true
-    end
-
     def audit_languages(languages)
       original_config = cask.config
       localized_config = original_config.merge(Config.new(explicit: { languages: languages }))

Library/Homebrew/cask/cmd/audit.rb

@@ -30,8 +30,6 @@ module Cask
                  description: "Run various additional style checks to determine if a new cask is eligible " \
                               "for Homebrew. This should be used when creating new casks and implies " \
                               "`--strict` and `--online`"
-          switch "--display-failures-only",
-                 description: "Only display casks that fail the audit. This is the default for formulae."
         end
       end
 
@@ -49,19 +47,17 @@ module Cask
 
         results = self.class.audit_casks(
           *casks,
-          download:              args.download?,
+          download:        args.download?,
-          online:                args.online?,
+          online:          args.online?,
-          strict:                args.strict?,
+          strict:          args.strict?,
-          signing:               args.signing?,
+          signing:         args.signing?,
-          new_cask:              args.new_cask?,
+          new_cask:        args.new_cask?,
-          token_conflicts:       args.token_conflicts?,
+          token_conflicts: args.token_conflicts?,
-          quarantine:            args.quarantine?,
+          quarantine:      args.quarantine?,
-          any_named_args:        any_named_args,
+          any_named_args:  any_named_args,
-          language:              args.language,
+          language:        args.language,
-          display_passes:        args.verbose? || args.named.count == 1,
+          only:            [],
-          display_failures_only: args.display_failures_only?,
+          except:          [],
-          only:                  [],
-          except:                [],
         )
 
         failed_casks = results.reject { |_, result| result[:errors].empty? }.map(&:first)
@@ -81,8 +77,6 @@ module Cask
         quarantine:,
         any_named_args:,
         language:,
-        display_passes:,
-        display_failures_only:,
         only:,
         except:
       )
@@ -96,8 +90,6 @@ module Cask
           quarantine:            quarantine,
           language:              language,
           any_named_args:        any_named_args,
-          display_passes:        display_passes,
-          display_failures_only: display_failures_only,
           only:                  only,
           except:                except,
         }.compact
@@ -110,7 +102,7 @@ module Cask
 
         casks.to_h do |cask|
           odebug "Auditing Cask #{cask}"
-          [cask.sourcefile_path, Auditor.audit(cask, **options)]
+          [cask.sourcefile_path, { errors: Auditor.audit(cask, **options), warnings: [] }]
         end
       end
     end

Library/Homebrew/dev-cmd/audit.rb

@@ -65,7 +65,8 @@ module Homebrew
              description: "Prefix every line of output with the file or formula name being audited, to " \
                           "make output easy to grep."
       switch "--display-failures-only",
-             description: "Only display casks that fail the audit. This is the default for formulae."
+             description: "Only display casks that fail the audit. This is the default for formulae and casks.",
+             hidden:      true
       switch "--skip-style",
              description: "Skip running non-RuboCop style checks. Useful if you plan on running " \
                           "`brew style` separately. Enabled by default unless a formula is specified by name."
@@ -242,24 +243,26 @@ module Homebrew
       require "cask/cmd/abstract_command"
       require "cask/cmd/audit"
 
+      if args.display_failures_only?
+        odeprecated "`brew audit <cask> --display-failures-only`", "`brew audit <cask>` without the argument"
+      end
+
       # For switches, we add `|| nil` so that `nil` will be passed instead of `false` if they aren't set.
       # This way, we can distinguish between "not set" and "set to false".
       Cask::Cmd::Audit.audit_casks(
         *audit_casks,
-        download:              nil,
+        download:        nil,
         # No need for `|| nil` for `--[no-]signing` because boolean switches are already `nil` if not passed
-        signing:               args.signing?,
+        signing:         args.signing?,
-        online:                args.online? || nil,
+        online:          args.online? || nil,
-        strict:                args.strict? || nil,
+        strict:          args.strict? || nil,
-        new_cask:              args.new_cask? || nil,
+        new_cask:        args.new_cask? || nil,
-        token_conflicts:       args.token_conflicts? || nil,
+        token_conflicts: args.token_conflicts? || nil,
-        quarantine:            nil,
+        quarantine:      nil,
-        any_named_args:        !no_named_args,
+        any_named_args:  !no_named_args,
-        language:              nil,
+        language:        nil,
-        display_passes:        args.verbose? || args.named.count == 1,
+        only:            args.only,
-        display_failures_only: args.display_failures_only?,
+        except:          args.except,
-        only:                  args.only,
-        except:                args.except,
       )
     end
 
@@ -267,7 +270,7 @@ module Homebrew
 
     cask_count = failed_casks.count
 
-    cask_problem_count = failed_casks.sum { |_, result| result[:warnings].count + result[:errors].count }
+    cask_problem_count = failed_casks.sum { |_, result| result.count }
     new_formula_problem_count += new_formula_problem_lines.count
     total_problems_count = problem_count + new_formula_problem_count + cask_problem_count + tap_problem_count
 

Library/Homebrew/test/cask/audit_spec.rb

@@ -13,23 +13,14 @@ describe Cask::Audit, :cask do
   end
 
   def passed?(audit)
-    !audit.errors? && !audit.warnings?
+    !audit.errors?
   end
 
   def outcome(audit)
     if passed?(audit)
       "passed"
     else
-      message = ""
+      "errored with #{audit.errors.map { |e| e.fetch(:message).inspect }.join(",")}"
-
-      message += "warned with #{audit.warnings.map { |e| e.fetch(:message).inspect }.join(",")}" if audit.warnings?
-
-      if audit.errors?
-        message += " and " if audit.warnings?
-        message += "errored with #{audit.errors.map { |e| e.fetch(:message).inspect }.join(",")}"
-      end
-
-      message
     end
   end
 
@@ -53,16 +44,6 @@ describe Cask::Audit, :cask do
     end
   end
 
-  matcher :warn_with do |message|
-    match do |audit|
-      include_msg?(audit.warnings, message)
-    end
-
-    failure_message do |audit|
-      "expected to warn with message #{message.inspect} but #{outcome(audit)}"
-    end
-  end
-
   let(:cask) { instance_double(Cask::Cask) }
   let(:new_cask) { nil }
   let(:online) { nil }
@@ -118,6 +99,14 @@ describe Cask::Audit, :cask do
   describe "#result" do
     subject { audit.result }
 
+    context "when there are no errors and `--strict` is not passed so we should not show anything" do
+      before do
+        audit.add_error("eh", strict_only: true)
+      end
+
+      it { is_expected.not_to match(/failed/) }
+    end
+
     context "when there are errors" do
       before do
         audit.add_error "bad"
@@ -126,25 +115,42 @@ describe Cask::Audit, :cask do
       it { is_expected.to match(/failed/) }
     end
 
-    context "when there are warnings" do
-      before do
-        audit.add_warning "eh"
-      end
-
-      it { is_expected.to match(/warning/) }
-    end
-
     context "when there are errors and warnings" do
       before do
         audit.add_error "bad"
-        audit.add_warning "eh"
+        audit.add_error("eh", strict_only: true)
       end
 
       it { is_expected.to match(/failed/) }
     end
 
-    context "when there are no errors or warnings" do
+    context "when there are errors and warnings and `--strict` is passed" do
-      it { is_expected.to match(/passed/) }
+      let(:strict) { true }
+
+      before do
+        audit.add_error "very bad"
+        audit.add_error("a little bit bad", strict_only: true)
+      end
+
+      it { is_expected.to match(/failed/) }
+    end
+
+    context "when there are warnings and `--strict` is not passed" do
+      before do
+        audit.add_error("a little bit bad", strict_only: true)
+      end
+
+      it { is_expected.not_to match(/failed/) }
+    end
+
+    context "when there are warnings and `--strict` is passed" do
+      let(:strict) { true }
+
+      before do
+        audit.add_error("a little bit bad", strict_only: true)
+      end
+
+      it { is_expected.to match(/failed/) }
     end
   end
 
@@ -485,7 +491,7 @@ describe Cask::Audit, :cask do
         it "does not fail" do
           expect(download_double).not_to receive(:fetch)
           expect(UnpackStrategy).not_to receive(:detect)
-          expect(run).not_to warn_with(/Audit\.app/)
+          expect(run).not_to error_with(/Audit\.app/)
         end
       end
 
@@ -503,7 +509,7 @@ describe Cask::Audit, :cask do
         it "does not fail since no extract" do
           allow(download_double).to receive(:fetch).and_return(Pathname.new("/tmp/test.zip"))
           allow(UnpackStrategy).to receive(:detect).and_return(nil)
-          expect(run).not_to warn_with(/Audit\.app/)
+          expect(run).not_to error_with(/Audit\.app/)
         end
       end
     end
@@ -984,9 +990,20 @@ describe Cask::Audit, :cask do
       context "when cask token conflicts with a core formula" do
         let(:formula_names) { %w[with-binary other-formula] }
 
-        it "warns about duplicates" do
+        context "when `--strict` is passed" do
-          expect(audit).to receive(:core_formula_names).and_return(formula_names)
+          let(:strict) { true }
-          expect(run).to warn_with(/possible duplicate/)
+
+          it "warns about duplicates" do
+            expect(audit).to receive(:core_formula_names).and_return(formula_names)
+            expect(run).to error_with(/possible duplicate/)
+          end
+        end
+
+        context "when `--strict` is not passed" do
+          it "does not warn about duplicates" do
+            expect(audit).to receive(:core_formula_names).and_return(formula_names)
+            expect(run).not_to error_with(/possible duplicate/)
+          end
         end
       end
 
@@ -1058,8 +1075,8 @@ describe Cask::Audit, :cask do
       context "when `new_cask` is false" do
         let(:new_cask) { false }
 
-        it "warns" do
+        it "does not warn" do
-          expect(run).to warn_with(/should have a description/)
+          expect(run).not_to error_with(/should have a description/)
         end
       end
 

Library/Homebrew/test/cask/cmd/audit_spec.rb

@@ -6,7 +6,7 @@ require "cask/auditor"
 describe Cask::Cmd::Audit, :cask do
   let(:cask) { Cask::Cask.new("cask") }
   let(:cask_with_many_languages) { Cask::CaskLoader.load(cask_path("with-many-languages")) }
-  let(:result) { { warnings: Set.new, errors: Set.new } }
+  let(:result) { Set.new }
 
   describe "selection of Casks to audit" do
     it "audits all Casks if no tokens are given" do
@@ -25,7 +25,6 @@ describe Cask::Cmd::Audit, :cask do
         .with(
           cask,
           audit_new_cask: false, quarantine: true, any_named_args: true,
-          display_failures_only: false, display_passes: true,
           only: [], except: []
         )
         .and_return(result)
@@ -40,7 +39,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_new_cask: false, quarantine: true, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -54,7 +52,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_download: true, audit_new_cask: false, quarantine: true, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -68,7 +65,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_token_conflicts: true, audit_new_cask: false, quarantine: true, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -82,7 +78,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_strict: true, audit_new_cask: false, quarantine: true, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -96,7 +91,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_online: true, audit_new_cask: false, quarantine: true, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -110,7 +104,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_new_cask: true, quarantine: true, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -124,7 +117,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_new_cask: false, quarantine: true, language: ["de-AT"], any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -138,7 +130,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_new_cask: false, quarantine: false, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)
@@ -154,7 +145,6 @@ describe Cask::Cmd::Audit, :cask do
       .with(
         cask,
         audit_new_cask: false, quarantine: false, any_named_args: true,
-        display_failures_only: false, display_passes: true,
         only: [], except: []
       )
       .and_return(result)

Library/Homebrew/test/cask/quarantine_spec.rb

@@ -39,9 +39,7 @@ describe Cask::Quarantine, :cask do
     it "quarantines Cask audits" do
       expect do
         Cask::Cmd::Audit.run("local-transmission", "--download")
-      end.to not_raise_error
+      end.to not_raise_error.and not_to_output.to_stderr
-        .and output(/audit for local-transmission: passed/).to_stdout
-                                                           .and not_to_output.to_stderr
 
       local_transmission = Cask::CaskLoader.load(cask_path("local-transmission"))
       cached_location = Cask::Download.new(local_transmission).fetch
@@ -156,9 +154,7 @@ describe Cask::Quarantine, :cask do
     it "does not quarantine Cask audits" do
       expect do
         Cask::Cmd::Audit.run("local-transmission", "--download", "--no-quarantine")
-      end.to not_raise_error
+      end.to not_raise_error.and not_to_output.to_stderr
-        .and output(/audit for local-transmission: passed/).to_stdout
-                                                           .and not_to_output.to_stderr
 
       local_transmission = Cask::CaskLoader.load(cask_path("local-transmission"))
       cached_location = Cask::Download.new(local_transmission).fetch