Skip to content

Commit

Permalink
Merge pull request #16335 from samford/separate-stable-version-audit
Browse files Browse the repository at this point in the history 
FormulaAuditor: Separate stable version audit
  • Loading branch information
@MikeMcQuaid
MikeMcQuaid authored Dec 15, 2023
2 parents af7f12b + 4a4f8cb commit 29d1be9
Show file tree
Hide file tree
Showing 2 changed files with 169 additions and 116 deletions.
138 changes: 80 additions & 58 deletions Library/Homebrew/formula_auditor.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ def initialize(formula, options = {})
@spdx_license_data = options[:spdx_license_data]
@spdx_exception_data = options[:spdx_exception_data]
@tap_audit = options[:tap_audit]
@previous_committed = {}
@newest_committed = {}
end

def audit_style
Expand Down Expand Up @@ -777,6 +779,24 @@ def audit_specs
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

previous_committed, newest_committed = committed_version_info

if !newest_committed[:version].nil? &&
current_version < newest_committed[:version] &&
current_version_scheme == previous_committed[: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?

Expand All @@ -785,85 +805,46 @@ def audit_revision_and_version_scheme
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
current_revision = formula.revision
current_url = formula.stable.url

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))
newest_committed_checksum = T.let(nil, T.nilable(String))
newest_committed_revision = T.let(nil, T.nilable(Integer))
newest_committed_url = T.let(nil, T.nilable(String))

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_checksum = stable.checksum
previous_version_scheme = f.version_scheme
previous_revision = f.revision

newest_committed_version ||= previous_version
newest_committed_checksum ||= previous_checksum
newest_committed_revision ||= previous_revision
newest_committed_url ||= stable.url
end
rescue MacOSVersion::Error
break
end
previous_committed, newest_committed = committed_version_info

break if previous_version && current_version != previous_version
break if previous_revision && current_revision != previous_revision
end

if current_version == newest_committed_version &&
current_url == newest_committed_url &&
current_checksum != newest_committed_checksum &&
current_checksum.present? && newest_committed_checksum.present?
if current_version == newest_committed[:version] &&
current_url == newest_committed[:url] &&
current_checksum != newest_committed[:checksum] &&
current_checksum.present? && newest_committed[:checksum].present?
problem(
"stable sha256 changed without the url/version also changing; " \
"please create an issue upstream to rule out malicious " \
"circumstances and to find out why the file changed.",
)
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} " \
unless previous_committed[:version_scheme].nil?
if current_version_scheme < previous_committed[:version_scheme]
problem "version_scheme should not decrease (from #{previous_committed[:version_scheme]} " \
"to #{current_version_scheme})"
elsif current_version_scheme > (previous_version_scheme + 1)
elsif current_version_scheme > (previous_committed[:version_scheme] + 1)
problem "version_schemes should only increment by 1"
end
end

if (previous_version != newest_committed_version ||
current_version != newest_committed_version) &&
if (previous_committed[:version] != newest_committed[:version] ||
current_version != newest_committed[:version]) &&
!current_revision.zero? &&
current_revision == newest_committed_revision &&
current_revision == previous_revision
current_revision == newest_committed[:revision] &&
current_revision == previous_committed[:revision]
problem "'revision #{current_revision}' should be removed"
elsif current_version == previous_version &&
!previous_revision.nil? &&
current_revision < previous_revision
problem "revision should not decrease (from #{previous_revision} to #{current_revision})"
elsif newest_committed_revision &&
current_revision > (newest_committed_revision + 1)
elsif current_version == previous_committed[:version] &&
!previous_committed[:revision].nil? &&
current_revision < previous_committed[:revision]
problem "revision should not decrease (from #{previous_committed[:revision]} to #{current_revision})"
elsif newest_committed[:revision] &&
current_revision > (newest_committed[:revision] + 1)
problem "revisions should only increment by 1"
end
end
Expand Down Expand Up @@ -971,5 +952,46 @@ def linux_only_gcc_dep?(formula)

true
end

def committed_version_info
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?
return [@previous_committed, @newest_committed] if @previous_committed.present? || @newest_committed.present?

current_version = formula.stable.version
current_revision = formula.revision

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_committed[:version] = stable.version
@previous_committed[:checksum] = stable.checksum
@previous_committed[:version_scheme] = f.version_scheme
@previous_committed[:revision] = f.revision

@newest_committed[:version] ||= @previous_committed[:version]
@newest_committed[:checksum] ||= @previous_committed[:checksum]
@newest_committed[:revision] ||= @previous_committed[:revision]
@newest_committed[:url] ||= stable.url
end
rescue MacOSVersion::Error
break
end

break if @previous_committed[:version] && current_version != @previous_committed[:version]
break if @previous_committed[:revision] && current_revision != @previous_committed[:revision]
end

@previous_committed.compact!
@newest_committed.compact!

[@previous_committed, @newest_committed]
end
end
end
147 changes: 89 additions & 58 deletions Library/Homebrew/test/dev-cmd/audit_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ class #{Formulary.class_s(name)} < Formula
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|
Expand All @@ -75,7 +83,28 @@ def formula_auditor(name, text, options = {})
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
Expand Down Expand Up @@ -873,20 +902,72 @@ class Foo < Formula
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)
fa.audit_revision_and_version_scheme
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
Expand Down Expand Up @@ -928,29 +1009,6 @@ class Foo < Formula
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
Expand Down Expand Up @@ -1113,33 +1171,6 @@ def formula_gsub_origin_commit(before, after = "")
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
Expand Down

0 comments on commit 29d1be9

Please sign in to comment.