From a4e8f9e22b78c740cb0df0d3d8e70ff6c0bd6036 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Wed, 29 Mar 2023 00:40:46 +0100 Subject: [PATCH 01/10] audit: Make `--display-failures-only` the default for Casks - Cask warnings are really noisy and numerous. Let's only show them if the user passes `--strict` or something implying `--strict`, like `--new-cask`. - Additionally remove `display_passes` since we would like silence if nothing is wrong with the cask, the same as with formula audits. --- Library/Homebrew/cask/audit.rb | 8 ++---- Library/Homebrew/cask/auditor.rb | 19 ++----------- Library/Homebrew/cask/cmd/audit.rb | 29 +++++++------------ Library/Homebrew/dev-cmd/audit.rb | 30 +++++++++++--------- Library/Homebrew/test/cask/cmd/audit_spec.rb | 10 ------- 5 files changed, 33 insertions(+), 63 deletions(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index 8b65aca250..c9d299e3e9 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -109,14 +109,12 @@ module Cask 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)) } - def summary(include_passed: false, include_warnings: true) - return if success? && !include_passed + sig { params(include_warnings: T::Boolean).returns(T.nilable(String)) } + def summary(include_warnings: true) + return if success? return if warnings? && !errors? && !include_warnings summary = ["audit for #{cask}: #{result}"] diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index c4f93896dc..cae71e5d13 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/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 @@ -63,7 +59,7 @@ module Cask sample_languages.each_key do |l| audit = audit_languages(l) - summary = audit.summary(include_passed: output_passed?, include_warnings: output_warnings?) + summary = audit.summary(include_warnings: output_warnings?) if summary.present? && output_summary?(audit) ohai "Auditing language: #{l.map { |lang| "'#{lang}'" }.to_sentence}" if output_summary? puts summary @@ -73,7 +69,7 @@ module Cask end else audit = audit_cask_instance(cask) - summary = audit.summary(include_passed: output_passed?, include_warnings: output_warnings?) + summary = audit.summary(include_warnings: output_warnings?) puts summary if summary.present? && output_summary?(audit) warnings += audit.warnings errors += audit.errors @@ -92,17 +88,8 @@ 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 + @new_cask.present? || @audit_strict.present? end def audit_languages(languages) diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 35179c3455..0c65d24b22 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/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?, - 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 @@ module Cask quarantine:, any_named_args:, language:, - display_passes:, - display_failures_only:, only:, except: ) @@ -96,7 +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, diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index a481432293..7cb204f9cb 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -65,7 +65,7 @@ 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." 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 +242,26 @@ module Homebrew 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 diff --git a/Library/Homebrew/test/cask/cmd/audit_spec.rb b/Library/Homebrew/test/cask/cmd/audit_spec.rb index cf15e3939a..12728b4777 100644 --- a/Library/Homebrew/test/cask/cmd/audit_spec.rb +++ b/Library/Homebrew/test/cask/cmd/audit_spec.rb @@ -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) From 2b8127d5181b4f8a7e56b1afc210ec454162829a Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 30 Mar 2023 23:52:24 +0100 Subject: [PATCH 02/10] Turn cask warnings into errors when `--strict` is passed (or implied) - Ignore them and don't show them otherwise. - Part three of issue 15074: > As a result, I propose that all current cask audit warnings are never > displayed as warnings but the underlying audit checks turned into > errors displayed only with --strict (or one of the other relevant > flags). --- Library/Homebrew/cask/audit.rb | 35 ++------ Library/Homebrew/cask/auditor.rb | 13 +-- Library/Homebrew/cask/cmd/audit.rb | 1 - Library/Homebrew/test/cask/audit_spec.rb | 81 ++++++++++++------- Library/Homebrew/test/cask/quarantine_spec.rb | 8 +- 5 files changed, 66 insertions(+), 72 deletions(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index c9d299e3e9..aab2793c12 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -71,23 +71,14 @@ 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 } @@ -97,25 +88,17 @@ module Cask 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 + # Warnings are ignored unless `--strict` is passed in which case they're turned into errors. + add_error(message, location: location) if strict? end def result - if errors? - Formatter.error("failed") - elsif warnings? - Formatter.warning("warning") - end + Formatter.error("failed") if errors? end - sig { params(include_warnings: T::Boolean).returns(T.nilable(String)) } - def summary(include_warnings: true) + sig { returns(T.nilable(String)) } + def summary return if success? - return if warnings? && !errors? && !include_warnings summary = ["audit for #{cask}: #{result}"] @@ -123,12 +106,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 diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index cae71e5d13..7a744f2172 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/Library/Homebrew/cask/auditor.rb @@ -45,7 +45,6 @@ module Cask LANGUAGE_BLOCK_LIMIT = 10 def audit - warnings = Set.new errors = Set.new if !language && language_blocks @@ -59,23 +58,19 @@ module Cask sample_languages.each_key do |l| audit = audit_languages(l) - summary = audit.summary(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_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: errors } end private diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 0c65d24b22..59d759f8d2 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/Library/Homebrew/cask/cmd/audit.rb @@ -90,7 +90,6 @@ module Cask quarantine: quarantine, language: language, any_named_args: any_named_args, - display_failures_only: display_failures_only, only: only, except: except, }.compact diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb index 251aa00217..873fdb954a 100644 --- a/Library/Homebrew/test/cask/audit_spec.rb +++ b/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 = "" - - 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 @@ -55,7 +46,7 @@ describe Cask::Audit, :cask do matcher :warn_with do |message| match do |audit| - include_msg?(audit.warnings, message) + include_msg?(audit.errors, message) end failure_message do |audit| @@ -118,6 +109,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_warning "eh" + end + + it { is_expected.not_to match(/failed/) } + end + context "when there are errors" do before do audit.add_error "bad" @@ -126,14 +125,6 @@ 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" @@ -143,8 +134,33 @@ describe Cask::Audit, :cask do 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 errors and warnings and `--strict` is passed" do + let(:strict) { true } + + before do + audit.add_error "very bad" + audit.add_warning "a little bit bad" + end + + it { is_expected.to match(/failed/) } + end + + context "when there are warnings and `--strict` is not passed" do + before do + audit.add_warning "a little bit bad" + 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_warning "a little bit bad" + end + + it { is_expected.to match(/failed/) } end end @@ -984,9 +1000,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 - 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 warn_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 warn_with(/possible duplicate/) + end end end @@ -1058,8 +1085,8 @@ describe Cask::Audit, :cask do 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 warn_with(/should have a description/) end end diff --git a/Library/Homebrew/test/cask/quarantine_spec.rb b/Library/Homebrew/test/cask/quarantine_spec.rb index f14d4bd70b..b5c49772da 100644 --- a/Library/Homebrew/test/cask/quarantine_spec.rb +++ b/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 - .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 @@ 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 - .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 From df8e97fef60ac71337867d87431e304b78ae32c0 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Fri, 31 Mar 2023 01:25:36 +0100 Subject: [PATCH 03/10] Consolidate `add_{warning,error}` methods into one - Specify `strictish: true` in `add_error` to specify that it's not a super big critical error. - These will be shown only if `brew audit --strict` is requested. --- Library/Homebrew/cask/audit.rb | 61 ++++++++++-------------- Library/Homebrew/test/cask/audit_spec.rb | 30 ++++-------- 2 files changed, 35 insertions(+), 56 deletions(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index aab2793c12..b7bc785c3c 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -81,15 +81,12 @@ module Cask !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), strictish: T::Boolean).void } + def add_error(message, location: nil, strictish: false) + # Only raise non-critical audits if the user specified `--strict`. + return if strictish && !@strict - sig { params(message: T.nilable(String), location: T.nilable(String)).void } - def add_warning(message, location: nil) - # Warnings are ignored unless `--strict` is passed in which case they're turned into errors. - add_error(message, location: location) if strict? + errors << ({ message: message, location: location }) end def result @@ -195,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.", strictish: true) if cask.desc.blank? end sig { void } @@ -383,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: " \ - "#{Formatter.url(core_formula_url)}" + add_error( + "possible duplicate, cask token conflicts with Homebrew core formula: #{Formatter.url(core_formula_url)}", + strictish: true, + ) end sig { void } @@ -418,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", strictish: true) if token.end_with? "launcher" - add_warning "cask token mentions desktop" if token.end_with? "desktop" + add_error("cask token mentions desktop", strictish: 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", strictish: 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", strictish: 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", strictish: true) end sig { void } @@ -449,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", + strictish: true, + ) end sig { void } @@ -493,7 +496,7 @@ module Cask "#{message} fix the signature of their app." end - add_warning message + add_error(message, strictish: true) when Artifact::Pkg path = downloaded_path next unless path.exist? @@ -501,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, strictish: true) Signature verification failed: #{result.merged_output} macOS on ARM requires applications to be signed. @@ -513,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, strictish: true) Signature verification failed: #{result.merged_output} macOS on ARM requires applications to be signed. @@ -638,16 +641,9 @@ module Cask 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 + add_error("GitHub repo is archived", strictish: cask.discontinued?) end sig { void } @@ -659,16 +655,9 @@ module Cask 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 + add_error("GitLab repo is archived", strictish: cask.discontinued?) end sig { void } diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb index 873fdb954a..6a32f27030 100644 --- a/Library/Homebrew/test/cask/audit_spec.rb +++ b/Library/Homebrew/test/cask/audit_spec.rb @@ -44,16 +44,6 @@ describe Cask::Audit, :cask do end end - matcher :warn_with do |message| - match do |audit| - include_msg?(audit.errors, 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 } @@ -111,7 +101,7 @@ describe Cask::Audit, :cask do context "when there are no errors and `--strict` is not passed so we should not show anything" do before do - audit.add_warning "eh" + audit.add_error("eh", strictish: true) end it { is_expected.not_to match(/failed/) } @@ -128,7 +118,7 @@ describe Cask::Audit, :cask do context "when there are errors and warnings" do before do audit.add_error "bad" - audit.add_warning "eh" + audit.add_error("eh", strictish: true) end it { is_expected.to match(/failed/) } @@ -139,7 +129,7 @@ describe Cask::Audit, :cask do before do audit.add_error "very bad" - audit.add_warning "a little bit bad" + audit.add_error("a little bit bad", strictish: true) end it { is_expected.to match(/failed/) } @@ -147,7 +137,7 @@ describe Cask::Audit, :cask do context "when there are warnings and `--strict` is not passed" do before do - audit.add_warning "a little bit bad" + audit.add_error("a little bit bad", strictish: true) end it { is_expected.not_to match(/failed/) } @@ -157,7 +147,7 @@ describe Cask::Audit, :cask do let(:strict) { true } before do - audit.add_warning "a little bit bad" + audit.add_error("a little bit bad", strictish: true) end it { is_expected.to match(/failed/) } @@ -501,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 @@ -519,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 @@ -1005,14 +995,14 @@ describe Cask::Audit, :cask do it "warns about duplicates" do expect(audit).to receive(:core_formula_names).and_return(formula_names) - expect(run).to warn_with(/possible duplicate/) + 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 warn_with(/possible duplicate/) + expect(run).not_to error_with(/possible duplicate/) end end end @@ -1086,7 +1076,7 @@ describe Cask::Audit, :cask do let(:new_cask) { false } it "does not warn" do - expect(run).not_to warn_with(/should have a description/) + expect(run).not_to error_with(/should have a description/) end end From e9f233e33398835211237350b881900f1023bdff Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sat, 1 Apr 2023 02:02:23 +0100 Subject: [PATCH 04/10] Remove unused `output_warnings?` Cask audit method --- Library/Homebrew/cask/auditor.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index 7a744f2172..85da6e23be 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/Library/Homebrew/cask/auditor.rb @@ -83,10 +83,6 @@ module Cask audit.errors? end - def output_warnings? - @new_cask.present? || @audit_strict.present? - end - def audit_languages(languages) original_config = cask.config localized_config = original_config.merge(Config.new(explicit: { languages: languages })) From c240cf81a48d5a702016f5543ced1d7ccd0f3be9 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sat, 1 Apr 2023 13:44:26 +0100 Subject: [PATCH 05/10] tests: Remove `--display-failures-only` from `brew audit` cmd --- .github/workflows/tests.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0383948512..4e2bde1be0 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 From 53a17b921f714fa2ec38570caa15b168d137fea7 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 3 Apr 2023 17:23:02 +0100 Subject: [PATCH 06/10] Make `--display-failures-only` help hidden as it's deprecated Co-authored-by: Mike McQuaid --- Library/Homebrew/dev-cmd/audit.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 7cb204f9cb..50d32e7c34 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/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 and casks." + 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." From d636d2de372749306ec13efc0c93d54dddb9236f Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 4 Apr 2023 17:22:00 +0100 Subject: [PATCH 07/10] Apply suggestions from review comments - Rename `strictish` to `strict_only` in `add_error` method. - Return just `errors`, a Set, not `{ errors: errors }`, a Hash, from `Auditor.audit`. --- Library/Homebrew/cask/audit.rb | 34 ++++++++++---------- Library/Homebrew/cask/auditor.rb | 2 +- Library/Homebrew/cask/cmd/audit.rb | 2 +- Library/Homebrew/dev-cmd/audit.rb | 4 +-- Library/Homebrew/test/cask/audit_spec.rb | 10 +++--- Library/Homebrew/test/cask/cmd/audit_spec.rb | 2 +- 6 files changed, 27 insertions(+), 27 deletions(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index b7bc785c3c..b6597bf36b 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -81,10 +81,10 @@ module Cask !errors? end - sig { params(message: T.nilable(String), location: T.nilable(String), strictish: T::Boolean).void } - def add_error(message, location: nil, strictish: false) + 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 strictish && !@strict + return if strict_only && !@strict errors << ({ message: message, location: location }) end @@ -192,7 +192,7 @@ module Cask # increases the maintenance burden. return if cask.tap == "homebrew/cask-fonts" - add_error("Cask should have a description. Please add a `desc` stanza.", strictish: true) 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 } @@ -382,7 +382,7 @@ module Cask add_error( "possible duplicate, cask token conflicts with Homebrew core formula: #{Formatter.url(core_formula_url)}", - strictish: true, + strict_only: true, ) end @@ -417,19 +417,19 @@ module Cask add_error "cask token contains version designation '#{match_data[:designation]}'" end - add_error("cask token mentions launcher", strictish: true) if token.end_with? "launcher" + add_error("cask token mentions launcher", strict_only: true) if token.end_with? "launcher" - add_error("cask token mentions desktop", strictish: true) if token.end_with? "desktop" + add_error("cask token mentions desktop", strict_only: true) if token.end_with? "desktop" - add_error("cask token mentions platform", strictish: true) if token.end_with? "mac", "osx", "macos" + add_error("cask token mentions platform", strict_only: true) if token.end_with? "mac", "osx", "macos" - add_error("cask token mentions architecture", strictish: true) 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_error("cask token mentions framework", strictish: true) + add_error("cask token mentions framework", strict_only: true) end sig { void } @@ -451,7 +451,7 @@ module Cask add_error( "Download does not require additional version components. Use `&:short_version` in the livecheck", - strictish: true, + strict_only: true, ) end @@ -496,7 +496,7 @@ module Cask "#{message} fix the signature of their app." end - add_error(message, strictish: true) + add_error(message, strict_only: true) when Artifact::Pkg path = downloaded_path next unless path.exist? @@ -504,7 +504,7 @@ module Cask result = system_command("pkgutil", args: ["--check-signature", path], print_stderr: false) unless result.success? - add_error(<<~EOS, strictish: true) + add_error(<<~EOS, strict_only: true) Signature verification failed: #{result.merged_output} macOS on ARM requires applications to be signed. @@ -516,7 +516,7 @@ module Cask result = system_command("stapler", args: ["validate", path], print_stderr: false) next if result.success? - add_error(<<~EOS, strictish: true) + add_error(<<~EOS, strict_only: true) Signature verification failed: #{result.merged_output} macOS on ARM requires applications to be signed. @@ -643,7 +643,7 @@ module Cask return if metadata.nil? return unless metadata["archived"] - add_error("GitHub repo is archived", strictish: cask.discontinued?) + add_error("GitHub repo is archived", strict_only: cask.discontinued?) end sig { void } @@ -657,7 +657,7 @@ module Cask return if metadata.nil? return unless metadata["archived"] - add_error("GitLab repo is archived", strictish: cask.discontinued?) + add_error("GitLab repo is archived", strict_only: cask.discontinued?) end sig { void } diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index 85da6e23be..c631f1e5b7 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/Library/Homebrew/cask/auditor.rb @@ -70,7 +70,7 @@ module Cask errors += audit.errors end - { errors: errors } + errors end private diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 59d759f8d2..57ba90e369 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/Library/Homebrew/cask/cmd/audit.rb @@ -60,7 +60,7 @@ module Cask except: [], ) - failed_casks = results.reject { |_, result| result[:errors].empty? }.map(&:first) + failed_casks = results.reject { |_, result| result.empty? }.map(&:first) return if failed_casks.empty? raise CaskError, "audit failed for casks: #{failed_casks.join(" ")}" diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 50d32e7c34..bd919b84bb 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -266,11 +266,11 @@ module Homebrew ) end - failed_casks = cask_results.reject { |_, result| result[:errors].empty? } + failed_casks = cask_results.reject { |_, result| result.empty? } 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 6a32f27030..e71514440c 100644 --- a/Library/Homebrew/test/cask/audit_spec.rb +++ b/Library/Homebrew/test/cask/audit_spec.rb @@ -101,7 +101,7 @@ describe Cask::Audit, :cask do context "when there are no errors and `--strict` is not passed so we should not show anything" do before do - audit.add_error("eh", strictish: true) + audit.add_error("eh", strict_only: true) end it { is_expected.not_to match(/failed/) } @@ -118,7 +118,7 @@ describe Cask::Audit, :cask do context "when there are errors and warnings" do before do audit.add_error "bad" - audit.add_error("eh", strictish: true) + audit.add_error("eh", strict_only: true) end it { is_expected.to match(/failed/) } @@ -129,7 +129,7 @@ describe Cask::Audit, :cask do before do audit.add_error "very bad" - audit.add_error("a little bit bad", strictish: true) + audit.add_error("a little bit bad", strict_only: true) end it { is_expected.to match(/failed/) } @@ -137,7 +137,7 @@ describe Cask::Audit, :cask do context "when there are warnings and `--strict` is not passed" do before do - audit.add_error("a little bit bad", strictish: true) + audit.add_error("a little bit bad", strict_only: true) end it { is_expected.not_to match(/failed/) } @@ -147,7 +147,7 @@ describe Cask::Audit, :cask do let(:strict) { true } before do - audit.add_error("a little bit bad", strictish: true) + audit.add_error("a little bit bad", strict_only: true) end it { is_expected.to match(/failed/) } diff --git a/Library/Homebrew/test/cask/cmd/audit_spec.rb b/Library/Homebrew/test/cask/cmd/audit_spec.rb index 12728b4777..604f281f02 100644 --- a/Library/Homebrew/test/cask/cmd/audit_spec.rb +++ b/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 From 93de196a345c88acc67b2ab4c1243ade9774b45b Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 6 Apr 2023 09:49:20 +0100 Subject: [PATCH 08/10] Update Library/Homebrew/cask/audit.rb --- Library/Homebrew/cask/audit.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index b6597bf36b..0b9c12188a 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -643,7 +643,8 @@ module Cask return if metadata.nil? return unless metadata["archived"] - add_error("GitHub repo is archived", strict_only: cask.discontinued?) + # Discontinued casks shouldn't show up as errors. + add_error("GitHub repo is archived", strict_only: !cask.discontinued?) end sig { void } From a1d4a46f064bbf0957ed797a094d3ab774406013 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 6 Apr 2023 10:13:02 +0100 Subject: [PATCH 09/10] Update Library/Homebrew/cask/audit.rb Co-authored-by: Markus Reiter --- Library/Homebrew/cask/audit.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index 0b9c12188a..34f0ffb9c9 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -658,7 +658,8 @@ module Cask return if metadata.nil? return unless metadata["archived"] - add_error("GitLab repo is archived", strict_only: cask.discontinued?) + # Discontinued casks shouldn't show up as errors. + add_error("GitLab repo is archived") unless cask.discontinued? end sig { void } From 8319c8f9b936cffa43571571e6eaf2f266f8e3df Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 6 Apr 2023 21:24:27 +0100 Subject: [PATCH 10/10] Make `audit_casks` return `[path, { errors:, warnings:}]` for CI - This was failing but only in CI because the annotations require "errors" and "warnings" hash elements. Since formulae have this despite formulae not having warnings (probably for compatibility with casks), I decided to reinstate `errors` and `warnings` hash for Casks _right at the end_ and we can clean this up another time (famous last words). --- Library/Homebrew/cask/cmd/audit.rb | 4 ++-- Library/Homebrew/dev-cmd/audit.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 57ba90e369..363c779ec1 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/Library/Homebrew/cask/cmd/audit.rb @@ -60,7 +60,7 @@ module Cask except: [], ) - failed_casks = results.reject { |_, result| result.empty? }.map(&:first) + failed_casks = results.reject { |_, result| result[:errors].empty? }.map(&:first) return if failed_casks.empty? raise CaskError, "audit failed for casks: #{failed_casks.join(" ")}" @@ -102,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 diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index bd919b84bb..db4a084d25 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -266,7 +266,7 @@ module Homebrew ) end - failed_casks = cask_results.reject { |_, result| result.empty? } + failed_casks = cask_results.reject { |_, result| result[:errors].empty? } cask_count = failed_casks.count