From a89457fcb9a976f23a03e958b2d0c74b08a25081 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Sun, 24 Nov 2024 10:01:48 -0500 Subject: [PATCH 1/3] bump: skip PR checking when up to date `brew bump` will check for PRs related to a package even if the package version and livecheck version are the same. We're presumably only interested in related PRs when the livecheck version differs, so we can reduce GitHub API requests by skipping the check(s) when the versions are equal. We use `bump` in `autobump` workflows, so this should help with recent rate limiting issues (e.g., 3203 out of 3426 autobumped formulae were up to date in a recent run). This also reworks the output for duplicate PRs, making it clear when `bump` skipped checking PRs (as printing "none" would suggest that PRs were checked) and only printing the "Maybe duplicate" information when checked. This makes it a little easier to understand what `bump` has done internally from the output. --- Library/Homebrew/bump_version_parser.rb | 7 +++ Library/Homebrew/dev-cmd/bump.rb | 47 ++++++++++++------- .../Homebrew/test/bump_version_parser_spec.rb | 35 ++++++++++++++ 3 files changed, 73 insertions(+), 16 deletions(-) diff --git a/Library/Homebrew/bump_version_parser.rb b/Library/Homebrew/bump_version_parser.rb index cc47324e32f35..40c23e4b0de20 100644 --- a/Library/Homebrew/bump_version_parser.rb +++ b/Library/Homebrew/bump_version_parser.rb @@ -48,5 +48,12 @@ def parse_cask_version(version) def blank? @general.blank? && @arm.blank? && @intel.blank? end + + sig { params(other: T.untyped).returns(T::Boolean) } + def ==(other) + return false unless other.is_a?(BumpVersionParser) + + (general == other.general) && (arm == other.arm) && (intel == other.intel) + end end end diff --git a/Library/Homebrew/dev-cmd/bump.rb b/Library/Homebrew/dev-cmd/bump.rb index 771d4c5644cbe..1491e4dc7ea7e 100644 --- a/Library/Homebrew/dev-cmd/bump.rb +++ b/Library/Homebrew/dev-cmd/bump.rb @@ -277,8 +277,9 @@ def retrieve_pull_requests(formula_or_cask, name, version: nil) odebug "Error fetching pull requests for #{formula_or_cask} #{name}: #{e}" nil end + return if pull_requests.blank? - pull_requests&.map { |pr| "#{pr["title"]} (#{Formatter.url(pr["html_url"])})" }&.join(", ") + pull_requests.map { |pr| "#{pr["title"]} (#{Formatter.url(pr["html_url"])})" }.join(", ") end sig { @@ -373,13 +374,17 @@ def retrieve_versions_by_arch(formula_or_cask:, repositories:, name:) new_version.general.to_s end - duplicate_pull_requests = unless args.no_pull_requests? - retrieve_pull_requests(formula_or_cask, name, version: pull_request_version) - end.presence + if !args.no_pull_requests? && (new_version != current_version) + duplicate_pull_requests = retrieve_pull_requests( + formula_or_cask, + name, + version: pull_request_version, + ) - maybe_duplicate_pull_requests = if !args.no_pull_requests? && duplicate_pull_requests.blank? - retrieve_pull_requests(formula_or_cask, name) - end.presence + maybe_duplicate_pull_requests = if duplicate_pull_requests.nil? + retrieve_pull_requests(formula_or_cask, name) + end + end VersionBumpInfo.new( type:, @@ -411,9 +416,7 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a repology_latest = version_info.repology_latest # Check if all versions are equal - versions_equal = [:arm, :intel, :general].all? do |key| - current_version.send(key) == new_version.send(key) - end + versions_equal = (new_version == current_version) title_name = ambiguous_cask ? "#{name} (cask)" : name title = if (repology_latest == current_version.general || !repology_latest.is_a?(Version)) && versions_equal @@ -439,8 +442,8 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a end version_label = version_info.version_name - duplicate_pull_requests = version_info.duplicate_pull_requests.presence - maybe_duplicate_pull_requests = version_info.maybe_duplicate_pull_requests.presence + duplicate_pull_requests = version_info.duplicate_pull_requests + maybe_duplicate_pull_requests = version_info.maybe_duplicate_pull_requests ohai title puts <<~EOS @@ -457,10 +460,22 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a #{outdated_synced_formulae.join(", ")}. EOS end - puts <<~EOS unless args.no_pull_requests? - Duplicate pull requests: #{duplicate_pull_requests || "none"} - Maybe duplicate pull requests: #{maybe_duplicate_pull_requests || "none"} - EOS + if !args.no_pull_requests? && !versions_equal + if duplicate_pull_requests + duplicate_pull_requests_text = duplicate_pull_requests + elsif maybe_duplicate_pull_requests + duplicate_pull_requests_text = "none" + maybe_duplicate_pull_requests_text = maybe_duplicate_pull_requests + else + duplicate_pull_requests_text = "none" + maybe_duplicate_pull_requests_text = "none" + end + + puts "Duplicate pull requests: #{duplicate_pull_requests_text}" + if maybe_duplicate_pull_requests_text + puts "Maybe duplicate pull requests: #{maybe_duplicate_pull_requests_text}" + end + end return unless args.open_pr? diff --git a/Library/Homebrew/test/bump_version_parser_spec.rb b/Library/Homebrew/test/bump_version_parser_spec.rb index 2e47b14868f03..578d723e864cf 100644 --- a/Library/Homebrew/test/bump_version_parser_spec.rb +++ b/Library/Homebrew/test/bump_version_parser_spec.rb @@ -69,4 +69,39 @@ end end end + + describe "#==" do + let(:new_version) { described_class.new(general: general_version, arm: arm_version, intel: intel_version) } + + context "when other object is not a `BumpVersionParser`" do + it "returns false" do + expect(new_version == Version.new("1.2.3")).to be(false) + end + end + + context "when comparing objects with equal versions" do + it "returns true" do + same_version = described_class.new(general: general_version, arm: arm_version, intel: intel_version) + expect(new_version == same_version).to be(true) + end + end + + context "when comparing objects with different versions" do + it "returns false" do + different_general_version = described_class.new(general: "3.2.1", arm: arm_version, intel: intel_version) + different_arm_version = described_class.new(general: general_version, arm: "4.3.2", intel: intel_version) + different_intel_version = described_class.new(general: general_version, arm: arm_version, intel: "5.4.3") + different_general_arm_versions = described_class.new(general: "3.2.1", arm: "4.3.2", intel: intel_version) + different_arm_intel_versions = described_class.new(general: general_version, arm: "4.3.2", intel: "5.4.3") + different_general_intel_versions = described_class.new(general: "3.2.1", arm: arm_version, intel: "5.4.3") + + expect(new_version == different_general_version).to be(false) + expect(new_version == different_arm_version).to be(false) + expect(new_version == different_intel_version).to be(false) + expect(new_version == different_general_arm_versions).to be(false) + expect(new_version == different_arm_intel_versions).to be(false) + expect(new_version == different_general_intel_versions).to be(false) + end + end + end end From 918206e1fd327cc5d6cd60b1e6ffe2c32795211f Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Sun, 24 Nov 2024 10:10:43 -0500 Subject: [PATCH 2/3] BumpVersionParser: expand tests This expands test coverage for `BumpVersionParser`, bringing line coverage to 100% and branch coverage to 95.45%. This is the best we can do for branch coverage, as Sorbet will catch an invalid argument type before the method is executed, so we can't exercise the method in a test to get 100% coverage. --- .../Homebrew/test/bump_version_parser_spec.rb | 60 ++++++++++++------- 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/Library/Homebrew/test/bump_version_parser_spec.rb b/Library/Homebrew/test/bump_version_parser_spec.rb index 578d723e864cf..b6ab559d57a09 100644 --- a/Library/Homebrew/test/bump_version_parser_spec.rb +++ b/Library/Homebrew/test/bump_version_parser_spec.rb @@ -11,41 +11,61 @@ it "raises a usage error" do expect do described_class.new - end.to raise_error(UsageError, "Invalid usage: `--version` must not be empty.") + end.to raise_error(UsageError, + "Invalid usage: `--version` must not be empty.") end end - context "when initializing with valid versions" do + context "when initializing with only an intel version" do + it "raises a UsageError" do + expect do + described_class.new(intel: intel_version) + end.to raise_error(UsageError, + "Invalid usage: `--version-arm` must not be empty.") + end + end + + context "when initializing with only an arm version" do + it "raises a UsageError" do + expect do + described_class.new(arm: arm_version) + end.to raise_error(UsageError, + "Invalid usage: `--version-intel` must not be empty.") + end + end + + context "when initializing with arm and intel versions" do + let(:new_version_arm_intel) { described_class.new(arm: arm_version, intel: intel_version) } + + it "correctly parses arm and intel versions" do + expect(new_version_arm_intel.arm).to eq(Cask::DSL::Version.new(arm_version.to_s)) + expect(new_version_arm_intel.intel).to eq(Cask::DSL::Version.new(intel_version.to_s)) + end + end + + context "when initializing with all versions" do let(:new_version) { described_class.new(general: general_version, arm: arm_version, intel: intel_version) } + let(:new_version_version) do + described_class.new( + general: Version.new(general_version), + arm: Version.new(arm_version), + intel: Version.new(intel_version), + ) + end it "correctly parses general version" do expect(new_version.general).to eq(Cask::DSL::Version.new(general_version.to_s)) + expect(new_version_version.general).to eq(Cask::DSL::Version.new(general_version.to_s)) end it "correctly parses arm version" do expect(new_version.arm).to eq(Cask::DSL::Version.new(arm_version.to_s)) + expect(new_version_version.arm).to eq(Cask::DSL::Version.new(arm_version.to_s)) end it "correctly parses intel version" do expect(new_version.intel).to eq(Cask::DSL::Version.new(intel_version.to_s)) - end - - context "when only the intel version is provided" do - it "raises a UsageError" do - expect do - described_class.new(intel: intel_version) - end.to raise_error(UsageError, - "Invalid usage: `--version-arm` must not be empty.") - end - end - - context "when only the arm version is provided" do - it "raises a UsageError" do - expect do - described_class.new(arm: arm_version) - end.to raise_error(UsageError, - "Invalid usage: `--version-intel` must not be empty.") - end + expect(new_version_version.intel).to eq(Cask::DSL::Version.new(intel_version.to_s)) end context "when the version is latest" do From 1cbcc44cb4a322f9846e685b9482bbf928458bf9 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Sun, 24 Nov 2024 20:37:18 -0500 Subject: [PATCH 3/3] bump: skip PR checking when livecheck fails `brew bump` will check for PRs related to a package even if livecheck fails to identify new versions. This reworks related conditions to ensure that related PR checks are only run when livecheck returns version information. --- Library/Homebrew/dev-cmd/bump.rb | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/Library/Homebrew/dev-cmd/bump.rb b/Library/Homebrew/dev-cmd/bump.rb index 1491e4dc7ea7e..a275eb7551d31 100644 --- a/Library/Homebrew/dev-cmd/bump.rb +++ b/Library/Homebrew/dev-cmd/bump.rb @@ -366,15 +366,17 @@ def retrieve_versions_by_arch(formula_or_cask:, repositories:, name:) new_version = BumpVersionParser.new(general: "unable to get versions") end - # We use the arm version for the pull request version. This is consistent - # with the behavior of bump-cask-pr. - pull_request_version = if multiple_versions && new_version.general != "unable to get versions" - new_version.arm.to_s - else - new_version.general.to_s - end + if !args.no_pull_requests? && + (new_version.general != "unable to get versions") && + (new_version != current_version) + # We use the ARM version for the pull request version. This is + # consistent with the behavior of bump-cask-pr. + pull_request_version = if multiple_versions + new_version.arm.to_s + else + new_version.general.to_s + end - if !args.no_pull_requests? && (new_version != current_version) duplicate_pull_requests = retrieve_pull_requests( formula_or_cask, name, @@ -460,7 +462,9 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a #{outdated_synced_formulae.join(", ")}. EOS end - if !args.no_pull_requests? && !versions_equal + if !args.no_pull_requests? && + (new_version.general != "unable to get versions") && + !versions_equal if duplicate_pull_requests duplicate_pull_requests_text = duplicate_pull_requests elsif maybe_duplicate_pull_requests