From 6ada9a9665dffc72d2d7a0af913cdc13bdbd4a8f Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 12 Jan 2025 16:56:48 +0000 Subject: [PATCH 1/2] Add clarifying comments to `rubocop:disable`s - Needed for PR 18842 that adds a `DisableComment` RuboCop to ensure that all RuboCop disables have comments. --- Library/Homebrew/brew.rb | 1 + Library/Homebrew/build.rb | 2 ++ Library/Homebrew/cask/dsl/depends_on.rb | 1 + Library/Homebrew/cmd/update-report.rb | 1 + Library/Homebrew/dev-cmd/test.rb | 1 + Library/Homebrew/download_queue.rb | 2 +- Library/Homebrew/download_strategy.rbi | 3 ++- Library/Homebrew/extend/os/mac/readall.rb | 1 + Library/Homebrew/formula.rb | 1 + Library/Homebrew/formula_installer.rb | 9 ++++++++- Library/Homebrew/ignorable.rb | 1 + Library/Homebrew/migrator.rb | 2 ++ Library/Homebrew/postinstall.rb | 1 + Library/Homebrew/readall.rb | 1 + Library/Homebrew/reinstall.rb | 1 + Library/Homebrew/resource_auditor.rb | 3 ++- Library/Homebrew/tap.rb | 1 + Library/Homebrew/test.rb | 1 + Library/Homebrew/test/commands_spec.rb | 3 ++- Library/Homebrew/test/macos_version_spec.rb | 3 +++ Library/Homebrew/test/rubocops/patches_spec.rb | 2 ++ .../support/helper/spec/shared_context/homebrew_cask.rb | 1 + .../helper/spec/shared_context/integration_test.rb | 1 + Library/Homebrew/test/utils/inreplace_spec.rb | 1 + Library/Homebrew/test/utils/spdx_spec.rb | 3 +++ .../test/utils/string_inreplace_extension_spec.rb | 1 + Library/Homebrew/utils/fork.rb | 1 + 27 files changed, 44 insertions(+), 5 deletions(-) diff --git a/Library/Homebrew/brew.rb b/Library/Homebrew/brew.rb index 679abea0d245d..03b1174ddbb95 100644 --- a/Library/Homebrew/brew.rb +++ b/Library/Homebrew/brew.rb @@ -200,6 +200,7 @@ end exit 1 +# Catch any other types of exceptions. rescue Exception => e # rubocop:disable Lint/RescueException onoe e diff --git a/Library/Homebrew/build.rb b/Library/Homebrew/build.rb index 4d782c2ed5da1..ae869d5e08046 100644 --- a/Library/Homebrew/build.rb +++ b/Library/Homebrew/build.rb @@ -230,6 +230,8 @@ def fixopt(formula) options = Options.create(args.flags_only) build = Build.new(formula, options, args:) build.install +# Any exception means the build did not complete. +# The `case` for what to do per-exception class is further down. rescue Exception => e # rubocop:disable Lint/RescueException error_hash = JSON.parse e.to_json diff --git a/Library/Homebrew/cask/dsl/depends_on.rb b/Library/Homebrew/cask/dsl/depends_on.rb index 2aaad1d6b52e0..37cfb302d02a6 100644 --- a/Library/Homebrew/cask/dsl/depends_on.rb +++ b/Library/Homebrew/cask/dsl/depends_on.rb @@ -63,6 +63,7 @@ def macos=(*args) MacOSRequirement.new([T.must(md[:version]).to_sym], comparator: md[:comparator]) elsif (md = /^\s*(?<|>|[=<>]=)\s*(?\S+)\s*$/.match(first_arg)) MacOSRequirement.new([md[:version]], comparator: md[:comparator]) + # This is not duplicate of the first case: see `args.first` and a different comparator. else # rubocop:disable Lint/DuplicateBranch MacOSRequirement.new([args.first], comparator: "==") end diff --git a/Library/Homebrew/cmd/update-report.rb b/Library/Homebrew/cmd/update-report.rb index dd22f475f6ab6..8bc403d671147 100644 --- a/Library/Homebrew/cmd/update-report.rb +++ b/Library/Homebrew/cmd/update-report.rb @@ -626,6 +626,7 @@ def migrate_tap_migration unless Formulary.factory(new_full_name).keg_only? system HOMEBREW_BREW_FILE, "link", new_full_name, "--overwrite" end + # Rescue any possible exception types. rescue Exception => e # rubocop:disable Lint/RescueException if Homebrew::EnvConfig.developer? require "utils/backtrace" diff --git a/Library/Homebrew/dev-cmd/test.rb b/Library/Homebrew/dev-cmd/test.rb index 98556f4d5b716..5d133ce882b48 100644 --- a/Library/Homebrew/dev-cmd/test.rb +++ b/Library/Homebrew/dev-cmd/test.rb @@ -99,6 +99,7 @@ def run exec(*exec_args) end end + # Rescue any possible exception types. rescue Exception => e # rubocop:disable Lint/RescueException retry if retry_test?(f) diff --git a/Library/Homebrew/download_queue.rb b/Library/Homebrew/download_queue.rb index e591c3f2ae5e5..c77b5c1d678e3 100644 --- a/Library/Homebrew/download_queue.rb +++ b/Library/Homebrew/download_queue.rb @@ -19,7 +19,7 @@ def initialize(size = 1) sig { params(downloadable: Downloadable, force: T::Boolean).returns(Concurrent::Promises::Future) } def enqueue(downloadable, force: false) quiet = pool.max_length > 1 - # Passing in arguments from outside into the future is a common `concurrent-ruby` pattern. + # Passing in arguments from outside into the future is a common `concurrent-ruby` pattern. # rubocop:disable Lint/ShadowingOuterLocalVariable Concurrent::Promises.future_on(pool, downloadable, force, quiet) do |downloadable, force, quiet| downloadable.clear_cache if force diff --git a/Library/Homebrew/download_strategy.rbi b/Library/Homebrew/download_strategy.rbi index b46c90106143f..656649cb3438c 100644 --- a/Library/Homebrew/download_strategy.rbi +++ b/Library/Homebrew/download_strategy.rbi @@ -1,6 +1,6 @@ # typed: strict -# This is a third-party implementation +# This is a third-party implementation. # rubocop:disable Lint/StructNewOverride class Mechanize::HTTP ContentDisposition = Struct.new :type, :filename, :creation_date, @@ -8,6 +8,7 @@ class Mechanize::HTTP end # rubocop:enable Lint/StructNewOverride +# This is a third-party implementation. # rubocop:disable Style/OptionalBooleanParameter class Mechanize::HTTP::ContentDispositionParser sig { diff --git a/Library/Homebrew/extend/os/mac/readall.rb b/Library/Homebrew/extend/os/mac/readall.rb index 69a4bd2727e0a..853654a24e6c9 100644 --- a/Library/Homebrew/extend/os/mac/readall.rb +++ b/Library/Homebrew/extend/os/mac/readall.rb @@ -31,6 +31,7 @@ def valid_casks?(tap, os_name: nil, arch: ::Hardware::CPU.type) raise "Missing URL" if cask.url.nil? rescue Interrupt raise + # Handle all possible exceptions reading Casks. rescue Exception => e # rubocop:disable Lint/RescueException os_and_arch = "macOS #{current_macos_version} on #{arch}" onoe "Invalid cask (#{os_and_arch}): #{file}" diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index cddc9f57d7429..0071393919c87 100644 --- a/Library/Homebrew/formula.rb +++ b/Library/Homebrew/formula.rb @@ -2903,6 +2903,7 @@ def run_test(keep_tmp: false) test end end + # Handle all possible exceptions running formula tests. rescue Exception # rubocop:disable Lint/RescueException staging.retain! if debug? raise diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index 57c9233382093..5b0e156ebc356 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -484,8 +484,8 @@ def install if pour_bottle? begin pour + # Catch any other types of exceptions as they leave us with nothing installed. rescue Exception # rubocop:disable Lint/RescueException - # any exceptions must leave us with nothing installed ignore_interrupts do begin FileUtils.rm_r(formula.prefix) if formula.prefix.directory? @@ -819,6 +819,7 @@ def install_dependency(dep, inherited_options) oh1 "Installing #{formula.full_name} dependency: #{Formatter.identifier(dep.name)}" fi.install fi.finish + # Handle all possible exceptions installing deps. rescue Exception => e # rubocop:disable Lint/RescueException ignore_interrupts do tmp_keg.rename(installed_keg.to_path) if tmp_keg && !installed_keg.directory? @@ -1016,6 +1017,7 @@ def build formula.update_head_version raise "Empty installation" if !formula.prefix.directory? || Keg.new(formula.prefix).empty_installation? + # Handle all possible exceptions when building. rescue Exception => e # rubocop:disable Lint/RescueException if e.is_a? BuildError e.formula = formula @@ -1093,6 +1095,7 @@ def link(keg) puts "You can try again using:" puts " brew link #{formula.name}" @show_summary_heading = true + # Handle all other possible exceptions when linking. rescue Exception => e # rubocop:disable Lint/RescueException ofail "An unexpected error occurred during the `brew link` step" puts "The formula built, but is not symlinked into #{HOMEBREW_PREFIX}" @@ -1145,6 +1148,7 @@ def install_service launchd_service_path.chmod 0644 log = formula.var/"log" log.mkpath if service.include? log.to_s + # Handle all possible exceptions when installing service files. rescue Exception => e # rubocop:disable Lint/RescueException puts e ofail "Failed to install service files" @@ -1156,6 +1160,7 @@ def install_service sig { params(keg: Keg).void } def fix_dynamic_linkage(keg) keg.fix_dynamic_linkage + # Rescue all possible exceptions when fixing linkage. rescue Exception => e # rubocop:disable Lint/RescueException ofail "Failed to fix install linkage" puts "The formula built, but you may encounter issues using it or linking other" @@ -1171,6 +1176,7 @@ def fix_dynamic_linkage(keg) def clean ohai "Cleaning" if verbose? Cleaner.new(formula).clean + # Handle all possible exceptions when cleaning does not complete. rescue Exception => e # rubocop:disable Lint/RescueException opoo "The cleaning step did not complete successfully" puts "Still, the installation was successful, so we will link it into your prefix." @@ -1243,6 +1249,7 @@ def post_install exec(*args) end end + # Handle all possible exceptions when postinstall does not complete. rescue Exception => e # rubocop:disable Lint/RescueException opoo "The post-install step did not complete successfully" puts "You can try again using:" diff --git a/Library/Homebrew/ignorable.rb b/Library/Homebrew/ignorable.rb index 624dd2269c177..038bebd380fad 100644 --- a/Library/Homebrew/ignorable.rb +++ b/Library/Homebrew/ignorable.rb @@ -25,6 +25,7 @@ def self.hook_raise def raise(*) callcc do |continuation| super + # Handle all possible exceptions. rescue Exception => e # rubocop:disable Lint/RescueException unless e.is_a?(ScriptError) e.extend(ExceptionMixin) diff --git a/Library/Homebrew/migrator.rb b/Library/Homebrew/migrator.rb index f44517d4572ad..ce3b16fb27a34 100644 --- a/Library/Homebrew/migrator.rb +++ b/Library/Homebrew/migrator.rb @@ -223,6 +223,7 @@ def migrate EOS rescue Interrupt ignore_interrupts { backup_oldname } + # Any exception means the migration did not complete. rescue Exception => e # rubocop:disable Lint/RescueException onoe "The migration did not complete successfully." puts e @@ -357,6 +358,7 @@ def link_newname puts puts "You can try again using:" puts " brew link #{formula.name}" + # Any exception means the `brew link` step did not complete. rescue Exception => e # rubocop:disable Lint/RescueException onoe "An unexpected error occurred during linking" puts e diff --git a/Library/Homebrew/postinstall.rb b/Library/Homebrew/postinstall.rb index 5b4db3f545267..cde5b8b6762da 100644 --- a/Library/Homebrew/postinstall.rb +++ b/Library/Homebrew/postinstall.rb @@ -27,6 +27,7 @@ formula.extend(Debrew::Formula) end formula.run_post_install +# Handle all possible exceptions. rescue Exception => e # rubocop:disable Lint/RescueException error_pipe.puts e.to_json error_pipe.close diff --git a/Library/Homebrew/readall.rb b/Library/Homebrew/readall.rb index 8966214a5cf0c..363fad0d29380 100644 --- a/Library/Homebrew/readall.rb +++ b/Library/Homebrew/readall.rb @@ -74,6 +74,7 @@ def self.valid_formulae?(tap, bottle_tag: nil) end rescue Interrupt raise + # Handle all possible exceptions reading formulae. rescue Exception => e # rubocop:disable Lint/RescueException onoe "Invalid formula (#{bottle_tag}): #{file}" $stderr.puts e diff --git a/Library/Homebrew/reinstall.rb b/Library/Homebrew/reinstall.rb index 25e27bcc141a8..b61f0f9145a1a 100644 --- a/Library/Homebrew/reinstall.rb +++ b/Library/Homebrew/reinstall.rb @@ -70,6 +70,7 @@ def self.reinstall_formula( fi.finish rescue FormulaInstallationAlreadyAttemptedError nil + # Any other exceptions we want to restore the previous keg and report the error. rescue Exception # rubocop:disable Lint/RescueException ignore_interrupts { restore_backup(keg, link_keg, verbose:) } raise diff --git a/Library/Homebrew/resource_auditor.rb b/Library/Homebrew/resource_auditor.rb index ce92f9d9b39e2..8e8a856d72718 100644 --- a/Library/Homebrew/resource_auditor.rb +++ b/Library/Homebrew/resource_auditor.rb @@ -91,7 +91,8 @@ def audit_download_strategy def audit_checksum return if spec_name == :head - # rubocop:disable Style/InvertibleUnlessCondition (non-invertible) + # This condition is non-invertible. + # rubocop:disable Style/InvertibleUnlessCondition return unless DownloadStrategyDetector.detect(url, using) <= CurlDownloadStrategy # rubocop:enable Style/InvertibleUnlessCondition diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb index d8605c280a03e..52794cb99b57c 100644 --- a/Library/Homebrew/tap.rb +++ b/Library/Homebrew/tap.rb @@ -1061,6 +1061,7 @@ def self.all cache[:all] ||= begin core_taps = [ CoreTap.instance, + # The conditional is valid here because we only want the cask tap on macOS. (CoreCaskTap.instance if OS.mac?), # rubocop:disable Homebrew/MoveToExtendOS ].compact diff --git a/Library/Homebrew/test.rb b/Library/Homebrew/test.rb index 1ba9396a7923a..338e4d7d9b945 100644 --- a/Library/Homebrew/test.rb +++ b/Library/Homebrew/test.rb @@ -53,6 +53,7 @@ timeout = ENV["HOMEBREW_TEST_TIMEOUT_SECS"]&.to_i || DEFAULT_TEST_TIMEOUT_SECONDS Timeout.timeout(timeout, &run_test) end +# Any exceptions during the test run are reported. rescue Exception => e # rubocop:disable Lint/RescueException error_pipe.puts e.to_json error_pipe.close diff --git a/Library/Homebrew/test/commands_spec.rb b/Library/Homebrew/test/commands_spec.rb index 32c2293e5bfdb..5b2cd0a2a9c5b 100644 --- a/Library/Homebrew/test/commands_spec.rb +++ b/Library/Homebrew/test/commands_spec.rb @@ -2,7 +2,8 @@ require "commands" -RSpec.shared_context "custom internal commands" do # rubocop:disable RSpec/ContextWording +# These shared contexts starting with `when` don't make sense. +RSpec.shared_context "in custom internal commands" do # rubocop:disable RSpec/ContextWording let(:cmds) do [ # internal commands diff --git a/Library/Homebrew/test/macos_version_spec.rb b/Library/Homebrew/test/macos_version_spec.rb index 6f74bc1fa3ee3..b6917f7797bdc 100644 --- a/Library/Homebrew/test/macos_version_spec.rb +++ b/Library/Homebrew/test/macos_version_spec.rb @@ -22,6 +22,7 @@ specify "comparison with Symbol" do expect(version).to be > :high_sierra expect(version).to eq :mojave + # We're explicitly testing the `===` operator results here. expect(version).to be === :mojave # rubocop:disable Style/CaseEquality expect(version).to be < :catalina end @@ -34,6 +35,7 @@ specify "comparison with String" do expect(version).to be > "10.3" expect(version).to eq "10.14" + # We're explicitly testing the `===` operator results here. expect(version).to be === "10.14" # rubocop:disable Style/CaseEquality expect(version).to be < "10.15" end @@ -41,6 +43,7 @@ specify "comparison with Version" do expect(version).to be > Version.new("10.3") expect(version).to eq Version.new("10.14") + # We're explicitly testing the `===` operator results here. expect(version).to be === Version.new("10.14") # rubocop:disable Style/CaseEquality expect(version).to be < Version.new("10.15") end diff --git a/Library/Homebrew/test/rubocops/patches_spec.rb b/Library/Homebrew/test/rubocops/patches_spec.rb index 0c56e3f6f195e..45a2acbb91436 100644 --- a/Library/Homebrew/test/rubocops/patches_spec.rb +++ b/Library/Homebrew/test/rubocops/patches_spec.rb @@ -67,6 +67,7 @@ def patches expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 4, source:) FormulaAudit/Patches: Patches from Debian should be https://, not http: #{patch_url} EOS + # GitHub patch diff regexps can't be any shorter. # rubocop:disable Layout/LineLength elsif patch_url.match?(%r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}) # rubocop:enable Layout/LineLength @@ -232,6 +233,7 @@ class Foo < Formula expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 8, source:) FormulaAudit/Patches: GitLab patches should end with .diff, not .patch: #{patch_url} EOS + # GitHub patch diff regexps can't be any shorter. # rubocop:disable Layout/LineLength elsif patch_url.match?(%r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}) # rubocop:enable Layout/LineLength diff --git a/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb b/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb index b62d783305eef..0eb124b1be484 100644 --- a/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb +++ b/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb @@ -31,6 +31,7 @@ class Config end end +# These shared contexts starting with `when` don't make sense. RSpec.shared_context "Homebrew Cask", :needs_macos do # rubocop:disable RSpec/ContextWording around do |example| third_party_tap = Tap.fetch("third-party", "tap") diff --git a/Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb b/Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb index 9981aabadad59..1d7edccdea0e1 100644 --- a/Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb +++ b/Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb @@ -6,6 +6,7 @@ RSpec::Matchers.define_negated_matcher :be_a_failure, :be_a_success +# These shared contexts starting with `when` don't make sense. RSpec.shared_context "integration test" do # rubocop:disable RSpec/ContextWording extend RSpec::Matchers::DSL diff --git a/Library/Homebrew/test/utils/inreplace_spec.rb b/Library/Homebrew/test/utils/inreplace_spec.rb index bc2fa549cabf1..3014f1b789fa6 100644 --- a/Library/Homebrew/test/utils/inreplace_spec.rb +++ b/Library/Homebrew/test/utils/inreplace_spec.rb @@ -33,6 +33,7 @@ it "raises error if there is nothing to replace in block form" do expect do described_class.inreplace(file.path) do |s| + # Using `gsub!` here is what we want, and it's only a test. s.gsub!("d", "f") # rubocop:disable Performance/StringReplacement end end.to raise_error(Utils::Inreplace::Error) diff --git a/Library/Homebrew/test/utils/spdx_spec.rb b/Library/Homebrew/test/utils/spdx_spec.rb index 495108f058396..6b3c4c8e2970d 100644 --- a/Library/Homebrew/test/utils/spdx_spec.rb +++ b/Library/Homebrew/test/utils/spdx_spec.rb @@ -75,6 +75,7 @@ license_expression = { any_of: [ "MIT", :public_domain, + # The final array item is legitimately a hash in the case of license expressions. all_of: ["0BSD", "Zlib"], # rubocop:disable Style/HashAsLastArrayItem "curl" => { with: "LLVM-exception" }, ] } @@ -195,6 +196,7 @@ license_expression = { any_of: [ "MIT", :public_domain, + # The final array item is legitimately a hash in the case of license expressions. all_of: ["0BSD", "Zlib"], # rubocop:disable Style/HashAsLastArrayItem "curl" => { with: "LLVM-exception" }, ] } @@ -235,6 +237,7 @@ }) end + # The final array item is legitimately a hash in the case of license expressions. # rubocop:disable Style/HashAsLastArrayItem it "handles nested brackets" do expect(described_class.string_to_license_expression("A AND (B OR (C AND D))")).to eq({ diff --git a/Library/Homebrew/test/utils/string_inreplace_extension_spec.rb b/Library/Homebrew/test/utils/string_inreplace_extension_spec.rb index c59c4a342a4b6..4702ef7d2957b 100644 --- a/Library/Homebrew/test/utils/string_inreplace_extension_spec.rb +++ b/Library/Homebrew/test/utils/string_inreplace_extension_spec.rb @@ -269,6 +269,7 @@ let(:string) { "foo" } it "replaces all occurrences" do + # Using `gsub!` here is what we want, and it's only a test. string_extension.gsub!("o", "e") # rubocop:disable Performance/StringReplacement expect(string_extension.inreplace_string).to eq("fee") end diff --git a/Library/Homebrew/utils/fork.rb b/Library/Homebrew/utils/fork.rb index 8a095941d4010..8c19ac282ff71 100644 --- a/Library/Homebrew/utils/fork.rb +++ b/Library/Homebrew/utils/fork.rb @@ -52,6 +52,7 @@ def self.safe_fork(directory: nil, yield_parent: false) Process::UID.change_privilege(Process.euid) if Process.euid != Process.uid yield(error_pipe) + # This could be any type of exception, so rescue them all. rescue Exception => e # rubocop:disable Lint/RescueException error_hash = JSON.parse e.to_json From 9c8ff4c7d64f7e667c8d14cc8009cf46f9adcf7e Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 13 Jan 2025 11:18:05 +0000 Subject: [PATCH 2/2] Reset "custom internal commands" `RSpec.shared_context` wording - I tried this as a previous approach to see if we could enable the rule, but it didn't work, then I forgot about it. --- Library/Homebrew/test/commands_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/test/commands_spec.rb b/Library/Homebrew/test/commands_spec.rb index 5b2cd0a2a9c5b..56554f74e85f1 100644 --- a/Library/Homebrew/test/commands_spec.rb +++ b/Library/Homebrew/test/commands_spec.rb @@ -3,7 +3,7 @@ require "commands" # These shared contexts starting with `when` don't make sense. -RSpec.shared_context "in custom internal commands" do # rubocop:disable RSpec/ContextWording +RSpec.shared_context "custom internal commands" do # rubocop:disable RSpec/ContextWording let(:cmds) do [ # internal commands