diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 03839485127f9..4e2bde1be00e7 100644 --- a/.github/workflows/tests.yml +++ b/.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 --display-failures-only --tap=homebrew/cask-drivers - brew audit --skip-style --except=version --display-failures-only --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 + brew audit --skip-style --except=version --tap=homebrew/cask-drivers + brew audit --skip-style --except=version --tap=homebrew/cask-fonts + brew audit --skip-style --except=version --tap=homebrew/cask-versions vendored-gems: name: vendored gems diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index 8b65aca250f4b..34f0ffb9c96bc 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -71,53 +71,31 @@ def errors @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 } - def add_error(message, location: nil) - errors << ({ message: message, location: location }) - end + sig { params(message: T.nilable(String), location: T.nilable(String), strict_only: T::Boolean).void } + def add_error(message, location: nil, strict_only: false) + # Only raise non-critical audits if the user specified `--strict`. + return if strict_only && !@strict - 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 + errors << ({ message: message, location: location }) end def result - if errors? - Formatter.error("failed") - elsif warnings? - Formatter.warning("warning") - else - Formatter.success("passed") - end + Formatter.error("failed") if errors? end - sig { params(include_passed: T::Boolean, include_warnings: T::Boolean).returns(T.nilable(String)) } - def summary(include_passed: false, include_warnings: true) - return if success? && !include_passed - return if warnings? && !errors? && !include_warnings + sig { returns(T.nilable(String)) } + def summary + return if success? summary = ["audit for #{cask}: #{result}"] @@ -125,12 +103,6 @@ def summary(include_passed: false, include_warnings: true) 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 @@ def audit_description # 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 @@ def audit_token_conflicts return unless token_conflicts? return unless core_formula_names.include?(cask.token) - add_warning "possible duplicate, cask token conflicts with Homebrew core formula: " \ - "#{Formatter.url(core_formula_url)}" + add_error( + "possible duplicate, cask token conflicts with Homebrew core formula: #{Formatter.url(core_formula_url)}", + strict_only: true, + ) end sig { void } @@ -443,18 +417,19 @@ def audit_token_bad_words 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 @@ def audit_livecheck_unneeded_long_version 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 @@ def audit_signing "#{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 @@ def audit_signing 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 @@ def audit_signing 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 @@ def audit_github_repository_archived metadata = SharedAudits.github_repo_data(user, repo) return if metadata.nil? - return unless metadata["archived"] - message = "GitHub repo is archived" - - if cask.discontinued? - add_warning message - else - add_error message - end + # Discontinued casks shouldn't show up as errors. + add_error("GitHub repo is archived", strict_only: !cask.discontinued?) end sig { void } @@ -684,16 +656,10 @@ def audit_gitlab_repository_archived metadata = SharedAudits.gitlab_repo_data(user, repo) return if metadata.nil? - return unless metadata["archived"] - message = "GitLab repo is archived" - - if cask.discontinued? - add_warning message - else - add_error message - end + # Discontinued casks shouldn't show up as errors. + add_error("GitLab repo is archived") unless cask.discontinued? end sig { void } diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index c4f93896dc710..c631f1e5b7a14 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/Library/Homebrew/cask/auditor.rb @@ -25,8 +25,6 @@ def initialize( quarantine: nil, any_named_args: nil, language: nil, - display_passes: nil, - display_failures_only: nil, only: [], except: [] ) @@ -40,8 +38,6 @@ def initialize( @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 @@ def initialize( LANGUAGE_BLOCK_LIMIT = 10 def audit - warnings = Set.new errors = Set.new if !language && language_blocks @@ -63,23 +58,19 @@ def audit sample_languages.each_key do |l| audit = audit_languages(l) - summary = audit.summary(include_passed: output_passed?, include_warnings: output_warnings?) - if summary.present? && output_summary?(audit) + if audit.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 summary if summary.present? && output_summary?(audit) - warnings += audit.warnings + puts audit.summary if audit.summary.present? && output_summary?(audit) errors += audit.errors end - { warnings: warnings, errors: errors } + errors end private @@ -92,19 +83,6 @@ def output_summary?(audit = nil) 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 })) diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 35179c3455e71..363c779ec175e 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/Library/Homebrew/cask/cmd/audit.rb @@ -30,8 +30,6 @@ def self.parser 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 @@ def run results = self.class.audit_casks( *casks, - download: args.download?, - online: args.online?, - strict: args.strict?, - signing: args.signing?, - new_cask: args.new_cask?, - token_conflicts: args.token_conflicts?, - quarantine: args.quarantine?, - any_named_args: any_named_args, - language: args.language, - display_passes: args.verbose? || args.named.count == 1, - display_failures_only: args.display_failures_only?, - only: [], - except: [], + download: args.download?, + online: args.online?, + strict: args.strict?, + signing: args.signing?, + new_cask: args.new_cask?, + token_conflicts: args.token_conflicts?, + quarantine: args.quarantine?, + any_named_args: any_named_args, + language: args.language, + only: [], + except: [], ) failed_casks = results.reject { |_, result| result[:errors].empty? }.map(&:first) @@ -81,8 +77,6 @@ def self.audit_casks( quarantine:, any_named_args:, language:, - display_passes:, - display_failures_only:, only:, except: ) @@ -96,8 +90,6 @@ def self.audit_casks( 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 @@ def self.audit_casks( 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 diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index a481432293917..db4a084d25938 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -65,7 +65,8 @@ def self.audit_args 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 @@ def self.audit require "cask/cmd/abstract_command" require "cask/cmd/audit" + if args.display_failures_only? + odeprecated "`brew audit --display-failures-only`", "`brew audit ` 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?, - online: args.online? || nil, - strict: args.strict? || nil, - new_cask: args.new_cask? || nil, - token_conflicts: args.token_conflicts? || nil, - quarantine: nil, - any_named_args: !no_named_args, - language: nil, - display_passes: args.verbose? || args.named.count == 1, - display_failures_only: args.display_failures_only?, - only: args.only, - except: args.except, + signing: args.signing?, + online: args.online? || nil, + strict: args.strict? || nil, + new_cask: args.new_cask? || nil, + token_conflicts: args.token_conflicts? || nil, + quarantine: nil, + any_named_args: !no_named_args, + language: nil, + only: args.only, + except: args.except, ) end @@ -267,7 +270,7 @@ def self.audit 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 diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb index 251aa00217071..e71514440cd90 100644 --- a/Library/Homebrew/test/cask/audit_spec.rb +++ b/Library/Homebrew/test/cask/audit_spec.rb @@ -13,23 +13,14 @@ def include_msg?(problems, msg) end def passed?(audit) - !audit.errors? && !audit.warnings? + !audit.errors? end def outcome(audit) if passed?(audit) "passed" else - message = "" - - 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 + "errored with #{audit.errors.map { |e| e.fetch(:message).inspect }.join(",")}" end end @@ -53,16 +44,6 @@ def outcome(audit) 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 @@ def outcome(audit) 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 @@ def outcome(audit) it { is_expected.to match(/failed/) } end - context "when there are warnings" do + context "when there are errors and warnings" do before do - audit.add_warning "eh" + audit.add_error "bad" + audit.add_error("eh", strict_only: true) end - it { is_expected.to match(/warning/) } + it { is_expected.to match(/failed/) } end - context "when there are errors and warnings" do + context "when there are errors and warnings and `--strict` is passed" do + let(:strict) { true } + before do - audit.add_error "bad" - audit.add_warning "eh" + 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 no errors or warnings" do - it { is_expected.to match(/passed/) } + 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 @@ def tmp_cask(name, text) 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 @@ def tmp_cask(name, text) 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 @@ def tmp_cask(name, text) context "when cask token conflicts with a core formula" do let(:formula_names) { %w[with-binary other-formula] } - it "warns about duplicates" do - expect(audit).to receive(:core_formula_names).and_return(formula_names) - expect(run).to warn_with(/possible duplicate/) + context "when `--strict` is passed" do + let(:strict) { true } + + 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 @@ def tmp_cask(name, text) context "when `new_cask` is false" do let(:new_cask) { false } - it "warns" do - expect(run).to warn_with(/should have a description/) + it "does not warn" do + expect(run).not_to error_with(/should have a description/) end end diff --git a/Library/Homebrew/test/cask/cmd/audit_spec.rb b/Library/Homebrew/test/cask/cmd/audit_spec.rb index cf15e3939afbf..604f281f02f9b 100644 --- a/Library/Homebrew/test/cask/cmd/audit_spec.rb +++ b/Library/Homebrew/test/cask/cmd/audit_spec.rb @@ -6,7 +6,7 @@ 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 @@ .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 @@ .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 @@ .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 @@ .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 @@ .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 @@ .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 @@ .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 @@ .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 @@ .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 @@ .with( cask, audit_new_cask: false, quarantine: false, any_named_args: true, - display_failures_only: false, display_passes: true, only: [], except: [] ) .and_return(result) diff --git a/Library/Homebrew/test/cask/quarantine_spec.rb b/Library/Homebrew/test/cask/quarantine_spec.rb index f14d4bd70b6ca..b5c49772dad39 100644 --- a/Library/Homebrew/test/cask/quarantine_spec.rb +++ b/Library/Homebrew/test/cask/quarantine_spec.rb @@ -39,9 +39,7 @@ it "quarantines Cask audits" do expect do Cask::Cmd::Audit.run("local-transmission", "--download") - end.to not_raise_error - .and output(/audit for local-transmission: passed/).to_stdout - .and not_to_output.to_stderr + end.to not_raise_error.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 @@ it "does not quarantine Cask audits" do expect do Cask::Cmd::Audit.run("local-transmission", "--download", "--no-quarantine") - end.to not_raise_error - .and output(/audit for local-transmission: passed/).to_stdout - .and not_to_output.to_stderr + end.to not_raise_error.and not_to_output.to_stderr local_transmission = Cask::CaskLoader.load(cask_path("local-transmission")) cached_location = Cask::Download.new(local_transmission).fetch