From ccbde5952da4cfa1834d3e853b06d9fb9d904ce9 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Wed, 13 Dec 2023 14:53:35 -0500 Subject: [PATCH] FormulaAuditor: Separate stable version audit The "stable version should not decrease" formula audit currently prevents us from being able to create bottles when downgrading a formula version. We previously worked around this by bumping `version_scheme` but this wasn't an intended use case and we now avoid using it for this purpose. We can handle simple formula downgrades by reverting changes in a syntax-only PR but that isn't sufficient when we need new bottles (i.e., if additional changes have been made to the formula in the interim time). In the latter case, the only available solution may be to revert all changes made after the previous version using a syntax-only PR and then create another PR to reintroduce the other changes and create new bottles. To avoid the aforementioned approach, this splits the stable version audit into a separate method, so we can use `brew audit --except=stable_version` to selectively skip it. --- Library/Homebrew/formula_auditor.rb | 53 +++++-- Library/Homebrew/test/dev-cmd/audit_spec.rb | 147 ++++++++++++-------- 2 files changed, 134 insertions(+), 66 deletions(-) diff --git a/Library/Homebrew/formula_auditor.rb b/Library/Homebrew/formula_auditor.rb index f01d6986a3..a4bc8d41c6 100644 --- a/Library/Homebrew/formula_auditor.rb +++ b/Library/Homebrew/formula_auditor.rb @@ -777,6 +777,50 @@ module Homebrew end end + def audit_stable_version + return unless @git + return unless formula.tap # skip formula not from core or any taps + return unless formula.tap.git? # git log is required + return if formula.stable.blank? + + current_version = formula.stable.version + current_version_scheme = formula.version_scheme + current_revision = formula.revision + + previous_version = T.let(nil, T.nilable(Version)) + previous_version_scheme = T.let(nil, T.nilable(Integer)) + previous_revision = T.let(nil, T.nilable(Integer)) + + newest_committed_version = T.let(nil, T.nilable(Version)) + + fv = FormulaVersions.new(formula) + fv.rev_list("origin/HEAD") do |revision, path| + begin + fv.formula_at_revision(revision, path) do |f| + stable = f.stable + next if stable.blank? + + previous_version = stable.version + previous_version_scheme = f.version_scheme + previous_revision = f.revision + + newest_committed_version ||= previous_version + end + rescue MacOSVersion::Error + break + end + + break if previous_version && current_version != previous_version + break if previous_revision && current_revision != previous_revision + end + + if !newest_committed_version.nil? && + current_version < newest_committed_version && + current_version_scheme == previous_version_scheme + problem "stable version should not decrease (from #{newest_committed_version} to #{current_version})" + end + end + def audit_revision_and_version_scheme new_formula_problem("New formulae should not define a revision.") if @new_formula && !formula.revision.zero? @@ -785,8 +829,6 @@ module Homebrew return unless formula.tap.git? # git log is required return if formula.stable.blank? - fv = FormulaVersions.new(formula) - current_version = formula.stable.version current_checksum = formula.stable.checksum current_version_scheme = formula.version_scheme @@ -802,6 +844,7 @@ module Homebrew newest_committed_revision = T.let(nil, T.nilable(Integer)) newest_committed_url = T.let(nil, T.nilable(String)) + fv = FormulaVersions.new(formula) fv.rev_list("origin/HEAD") do |revision, path| begin fv.formula_at_revision(revision, path) do |f| @@ -837,12 +880,6 @@ module Homebrew ) end - if !newest_committed_version.nil? && - current_version < newest_committed_version && - current_version_scheme == previous_version_scheme - problem "stable version should not decrease (from #{newest_committed_version} to #{current_version})" - end - unless previous_version_scheme.nil? if current_version_scheme < previous_version_scheme problem "version_scheme should not decrease (from #{previous_version_scheme} " \ diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index b5b3a93701..b7d5ae2605 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -57,6 +57,14 @@ module Homebrew end describe FormulaAuditor do + let(:dir) { mktmpdir } + let(:foo_version) { Count.increment } + let(:formula_subpath) { "Formula/foo#{foo_version}.rb" } + let(:origin_tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" } + let(:origin_formula_path) { origin_tap_path/formula_subpath } + let(:tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-bar" } + let(:formula_path) { tap_path/formula_subpath } + def formula_auditor(name, text, options = {}) path = Pathname.new "#{dir}/#{name}.rb" path.open("w") do |f| @@ -75,7 +83,28 @@ module Homebrew described_class.new(formula, options) end - let(:dir) { mktmpdir } + def formula_gsub(before, after = "") + text = formula_path.read + text.gsub! before, after + formula_path.unlink + formula_path.write text + end + + def formula_gsub_origin_commit(before, after = "") + text = origin_formula_path.read + text.gsub!(before, after) + origin_formula_path.unlink + origin_formula_path.write text + + origin_tap_path.cd do + system "git", "commit", "-am", "commit" + end + + tap_path.cd do + system "git", "fetch" + system "git", "reset", "--hard", "origin/HEAD" + end + end describe "#problems" do it "is empty by default" do @@ -873,6 +902,65 @@ module Homebrew end end + describe "#audit_stable_version" do + subject do + fa = described_class.new(Formulary.factory(formula_path), git: true) + fa.audit_stable_version + fa.problems.first&.fetch(:message) + end + + before do + origin_formula_path.dirname.mkpath + origin_formula_path.write <<~RUBY + class Foo#{foo_version} < Formula + url "https://brew.sh/foo-1.0.tar.gz" + sha256 "31cccfc6630528db1c8e3a06f6decf2a370060b982841cfab2b8677400a5092e" + revision 2 + version_scheme 1 + end + RUBY + + origin_tap_path.mkpath + origin_tap_path.cd do + system "git", "init" + system "git", "add", "--all" + system "git", "commit", "-m", "init" + end + + tap_path.mkpath + tap_path.cd do + system "git", "clone", origin_tap_path, "." + end + end + + describe "versions" do + context "when uncommitted should not decrease" do + before { formula_gsub "foo-1.0.tar.gz", "foo-0.9.tar.gz" } + + it { is_expected.to match("stable version should not decrease (from 1.0 to 0.9)") } + end + + context "when committed can decrease" do + before do + formula_gsub_origin_commit "revision 2" + formula_gsub_origin_commit "foo-1.0.tar.gz", "foo-0.9.tar.gz" + end + + it { is_expected.to be_nil } + end + + describe "can decrease with version_scheme increased" do + before do + formula_gsub "revision 2" + formula_gsub "foo-1.0.tar.gz", "foo-0.9.tar.gz" + formula_gsub "version_scheme 1", "version_scheme 2" + end + + it { is_expected.to be_nil } + end + end + end + describe "#audit_revision_and_version_scheme" do subject do fa = described_class.new(Formulary.factory(formula_path), git: true) @@ -880,13 +968,6 @@ module Homebrew fa.problems.first&.fetch(:message) end - let(:origin_tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" } - let(:foo_version) { Count.increment } - let(:formula_subpath) { "Formula/foo#{foo_version}.rb" } - let(:origin_formula_path) { origin_tap_path/formula_subpath } - let(:tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-bar" } - let(:formula_path) { tap_path/formula_subpath } - before do origin_formula_path.dirname.mkpath origin_formula_path.write <<~RUBY @@ -928,29 +1009,6 @@ module Homebrew end end - def formula_gsub(before, after = "") - text = formula_path.read - text.gsub! before, after - formula_path.unlink - formula_path.write text - end - - def formula_gsub_origin_commit(before, after = "") - text = origin_formula_path.read - text.gsub!(before, after) - origin_formula_path.unlink - origin_formula_path.write text - - origin_tap_path.cd do - system "git", "commit", "-am", "commit" - end - - tap_path.cd do - system "git", "fetch" - system "git", "reset", "--hard", "origin/HEAD" - end - end - describe "checksums" do describe "should not change with the same version" do before do @@ -1113,33 +1171,6 @@ module Homebrew it { is_expected.to match("version_schemes should only increment by 1") } end end - - describe "versions" do - context "when uncommitted should not decrease" do - before { formula_gsub "foo-1.0.tar.gz", "foo-0.9.tar.gz" } - - it { is_expected.to match("stable version should not decrease (from 1.0 to 0.9)") } - end - - context "when committed can decrease" do - before do - formula_gsub_origin_commit "revision 2" - formula_gsub_origin_commit "foo-1.0.tar.gz", "foo-0.9.tar.gz" - end - - it { is_expected.to be_nil } - end - - describe "can decrease with version_scheme increased" do - before do - formula_gsub "revision 2" - formula_gsub "foo-1.0.tar.gz", "foo-0.9.tar.gz" - formula_gsub "version_scheme 1", "version_scheme 2" - end - - it { is_expected.to be_nil } - end - end end describe "#audit_versioned_keg_only" do