diff options
| -rw-r--r-- | Library/Homebrew/dev-cmd/audit.rb | 80 | ||||
| -rw-r--r-- | Library/Homebrew/formula_versions.rb | 47 | ||||
| -rw-r--r-- | Library/Homebrew/test/dev-cmd/audit_spec.rb | 190 | ||||
| -rw-r--r-- | Library/Homebrew/test/spec_helper.rb | 1 | ||||
| -rw-r--r-- | Library/Homebrew/test/utils_spec.rb | 8 | ||||
| -rw-r--r-- | Library/Homebrew/utils.rb | 16 |
6 files changed, 297 insertions, 45 deletions
diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 95d54caee..8d76c2075 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -827,51 +827,71 @@ class FormulaAuditor return unless formula.tap.git? # git log is required return if @new_formula - fv = FormulaVersions.new(formula, max_depth: 1) + fv = FormulaVersions.new(formula) attributes = [:revision, :version_scheme] - attributes_map = fv.version_attributes_map(attributes, "origin/master") - attributes.each do |attribute| - stable_attribute_map = attributes_map[attribute][:stable] - next if stable_attribute_map.nil? || stable_attribute_map.empty? - - attributes_for_version = stable_attribute_map[formula.version] - next if attributes_for_version.nil? || attributes_for_version.empty? - - old_attribute = formula.send(attribute) - max_attribute = attributes_for_version.max - if max_attribute && old_attribute < max_attribute - problem "#{attribute} should not decrease (from #{max_attribute} to #{old_attribute})" - end - end - + current_version_scheme = formula.version_scheme [:stable, :devel].each do |spec| spec_version_scheme_map = attributes_map[:version_scheme][spec] next if spec_version_scheme_map.nil? || spec_version_scheme_map.empty? - max_version_scheme = spec_version_scheme_map.values.flatten.max + version_schemes = spec_version_scheme_map.values.flatten + max_version_scheme = version_schemes.max max_version = spec_version_scheme_map.select do |_, version_scheme| version_scheme.first == max_version_scheme end.keys.max - formula_spec = formula.send(spec) - next if formula_spec.nil? + if max_version_scheme && current_version_scheme < max_version_scheme + problem "version_scheme should not decrease (from #{max_version_scheme} to #{current_version_scheme})" + end - if max_version && formula_spec.version < max_version - problem "#{spec} version should not decrease (from #{max_version} to #{formula_spec.version})" + if max_version_scheme && current_version_scheme >= max_version_scheme && + current_version_scheme > 1 && + !version_schemes.include?(current_version_scheme - 1) + problem "version_schemes should only increment by 1" end + + formula_spec = formula.send(spec) + next unless formula_spec + + spec_version = formula_spec.version + next unless max_version + next if spec_version >= max_version + + above_max_version_scheme = current_version_scheme > max_version_scheme + map_includes_version = spec_version_scheme_map.keys.include?(spec_version) + next if !current_version_scheme.zero? && + (above_max_version_scheme || map_includes_version) + problem "#{spec} version should not decrease (from #{max_version} to #{spec_version})" end - return if formula.revision.zero? + current_revision = formula.revision if formula.stable - revision_map = attributes_map[:revision][:stable] - stable_revisions = revision_map[formula.stable.version] if revision_map - if !stable_revisions || stable_revisions.empty? - problem "'revision #{formula.revision}' should be removed" + if revision_map = attributes_map[:revision][:stable] + if !revision_map.nil? && !revision_map.empty? + stable_revisions = revision_map[formula.stable.version] + stable_revisions ||= [] + current_revision = formula.revision + max_revision = stable_revisions.max || 0 + + if current_revision < max_revision + problem "revision should not decrease (from #{max_revision} to #{current_revision})" + end + + stable_revisions -= [formula.revision] + if !current_revision.zero? && stable_revisions.empty? && + revision_map.keys.length > 1 + problem "'revision #{formula.revision}' should be removed" + elsif current_revision > 1 && + current_revision != max_revision && + !stable_revisions.include?(current_revision - 1) + problem "revisions should only increment by 1" + end + end end - else # head/devel-only formula - problem "'revision #{formula.revision}' should be removed" + elsif !current_revision.zero? # head/devel-only formula + problem "'revision #{current_revision}' should be removed" end end @@ -1211,6 +1231,10 @@ class FormulaAuditor end end + if line =~ /((revision|version_scheme)\s+0)/ + problem "'#{$1}' should be removed" + end + return unless @strict problem "`#{$1}` in formulae is deprecated" if line =~ /(env :(std|userpaths))/ diff --git a/Library/Homebrew/formula_versions.rb b/Library/Homebrew/formula_versions.rb index 28c2a3be8..f0fbb3aaf 100644 --- a/Library/Homebrew/formula_versions.rb +++ b/Library/Homebrew/formula_versions.rb @@ -7,21 +7,22 @@ class FormulaVersions ErrorDuringExecution, LoadError, MethodDeprecatedError ].freeze + MAX_VERSIONS_DEPTH = 2 + attr_reader :name, :path, :repository, :entry_name - def initialize(formula, options = {}) + def initialize(formula) @name = formula.name @path = formula.path @repository = formula.tap.path @entry_name = @path.relative_path_from(repository).to_s - @max_depth = options[:max_depth] + @current_formula = formula end def rev_list(branch) repository.cd do - depth = 0 Utils.popen_read("git", "rev-list", "--abbrev-commit", "--remove-empty", branch, "--", entry_name) do |io| - yield io.readline.chomp until io.eof? || (@max_depth && (depth += 1) > @max_depth) + yield io.readline.chomp until io.eof? end end end @@ -49,11 +50,15 @@ class FormulaVersions def bottle_version_map(branch) map = Hash.new { |h, k| h[k] = [] } + + versions_seen = 0 rev_list(branch) do |rev| formula_at_revision(rev) do |f| bottle = f.bottle_specification map[f.pkg_version] << bottle.rebuild unless bottle.checksums.empty? + versions_seen = (map.keys + [f.pkg_version]).uniq.length end + return map if versions_seen > MAX_VERSIONS_DEPTH end map end @@ -62,27 +67,35 @@ class FormulaVersions attributes_map = {} return attributes_map if attributes.empty? - attributes.each do |attribute| - attributes_map[attribute] ||= {} - end - + stable_versions_seen = 0 rev_list(branch) do |rev| formula_at_revision(rev) do |f| attributes.each do |attribute| + attributes_map[attribute] ||= {} map = attributes_map[attribute] - if f.stable - map[:stable] ||= {} - map[:stable][f.stable.version] ||= [] - map[:stable][f.stable.version] << f.send(attribute) - end - next unless f.devel - map[:devel] ||= {} - map[:devel][f.devel.version] ||= [] - map[:devel][f.devel.version] << f.send(attribute) + set_attribute_map(map, f, attribute) + + stable_keys_length = (map[:stable].keys + [f.version]).uniq.length + stable_versions_seen = [stable_versions_seen, stable_keys_length].max end end + break if stable_versions_seen > MAX_VERSIONS_DEPTH end attributes_map end + + private + + def set_attribute_map(map, f, attribute) + if f.stable + map[:stable] ||= {} + map[:stable][f.stable.version] ||= [] + map[:stable][f.stable.version] << f.send(attribute) + end + return unless f.devel + map[:devel] ||= {} + map[:devel][f.devel.version] ||= [] + map[:devel][f.devel.version] << f.send(attribute) + end end diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index 8a8096849..d07d9d72b 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -5,6 +5,13 @@ RSpec::Matchers.alias_matcher :have_data, :be_data RSpec::Matchers.alias_matcher :have_end, :be_end RSpec::Matchers.alias_matcher :have_trailing_newline, :be_trailing_newline +module Count + def self.increment + @count ||= 0 + @count += 1 + end +end + describe FormulaText do let(:dir) { mktmpdir } @@ -518,4 +525,187 @@ describe FormulaAuditor do .to match('xcodebuild should be passed an explicit "SYMROOT"') end end + + describe "#audit_revision_and_version_scheme" do + let(:origin_tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" } + 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(:each) do + @foo_version = Count.increment + + origin_formula_path.write <<-EOS.undent + class Foo#{@foo_version} < Formula + url "https://example.com/foo-1.0.tar.gz" + revision 2 + version_scheme 1 + end + EOS + + origin_tap_path.mkpath + origin_tap_path.cd do + shutup do + system "git", "init" + system "git", "add", "--all" + system "git", "commit", "-m", "init" + end + end + + tap_path.mkpath + tap_path.cd do + shutup do + system "git", "clone", origin_tap_path, "." + end + end + end + + subject do + fa = described_class.new(Formulary.factory(formula_path)) + fa.audit_revision_and_version_scheme + fa.problems.first + 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_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 + shutup do + system "git", "commit", "-am", "commit" + end + end + + tap_path.cd do + shutup do + system "git", "fetch" + system "git", "reset", "--hard", "origin/master" + end + end + end + + context "revisions" do + context "should not be removed when first committed above 0" do + it { is_expected.to be_nil } + end + + context "should not decrease with the same version" do + before { formula_gsub_commit "revision 2", "revision 1" } + + it { is_expected.to match("revision should not decrease (from 2 to 1)") } + end + + context "should not be removed with the same version" do + before { formula_gsub_commit "revision 2" } + + it { is_expected.to match("revision should not decrease (from 2 to 0)") } + end + + context "should not decrease with the same, uncommitted version" do + before { formula_gsub "revision 2", "revision 1" } + + it { is_expected.to match("revision should not decrease (from 2 to 1)") } + end + + context "should be removed with a newer version" do + before { formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz" } + + it { is_expected.to match("'revision 2' should be removed") } + end + + context "should not warn on an newer version revision removal" do + before do + formula_gsub_commit "revision 2", "" + formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz" + end + + it { is_expected.to be_nil } + end + + context "should only increment by 1 with an uncommitted version" do + before do + formula_gsub "foo-1.0.tar.gz", "foo-1.1.tar.gz" + formula_gsub "revision 2", "revision 4" + end + + it { is_expected.to match("revisions should only increment by 1") } + end + + context "should not warn on past increment by more than 1" do + before do + formula_gsub_commit "revision 2", "# no revision" + formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz" + formula_gsub_commit "# no revision", "revision 3" + end + + it { is_expected.to be_nil } + end + end + + context "version_schemes" do + context "should not decrease with the same version" do + before { formula_gsub_commit "version_scheme 1" } + + it { is_expected.to match("version_scheme should not decrease (from 1 to 0)") } + end + + context "should not decrease with a new version" do + before do + formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz" + formula_gsub_commit "version_scheme 1", "" + formula_gsub_commit "revision 2", "" + end + + it { is_expected.to match("version_scheme should not decrease (from 1 to 0)") } + end + + context "should only increment by 1" do + before do + formula_gsub_commit "version_scheme 1", "# no version_scheme" + formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz" + formula_gsub_commit "revision 2", "" + formula_gsub_commit "# no version_scheme", "version_scheme 3" + end + + it { is_expected.to match("version_schemes should only increment by 1") } + end + end + + context "versions" do + context "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 "committed can decrease" do + before do + formula_gsub_commit "revision 2" + formula_gsub_commit "foo-1.0.tar.gz", "foo-0.9.tar.gz" + end + + it { is_expected.to be_nil } + end + + context "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 end diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index 005c6d2fb..e193b91a0 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -93,6 +93,7 @@ RSpec.configure do |config| HOMEBREW_PREFIX/"opt", HOMEBREW_PREFIX/"Caskroom", HOMEBREW_LIBRARY/"Taps/caskroom", + HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bar", HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bundle", HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-foo", HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-services", diff --git a/Library/Homebrew/test/utils_spec.rb b/Library/Homebrew/test/utils_spec.rb index dd7ea20de..5f370a040 100644 --- a/Library/Homebrew/test/utils_spec.rb +++ b/Library/Homebrew/test/utils_spec.rb @@ -270,4 +270,12 @@ describe "globally-scoped helper methods" do }.to raise_error(MethodDeprecatedError, %r{method.*replacement.*homebrew/homebrew-core.*homebrew/core}m) end end + + describe "#puts_hash" do + it "outputs a hash" do + expect { + puts_hash(a: 1, b: 2, c: [3, { "d"=>4 }]) + }.to output("a: 1\nb: 2\nc: [3, {\"d\"=>4}]\n").to_stdout + end + end end diff --git a/Library/Homebrew/utils.rb b/Library/Homebrew/utils.rb index 0ecc06d2a..82bc38895 100644 --- a/Library/Homebrew/utils.rb +++ b/Library/Homebrew/utils.rb @@ -517,3 +517,19 @@ def migrate_legacy_keg_symlinks_if_necessary end FileUtils.rm_rf legacy_pinned_kegs end + +def puts_hash(hash, indent: 0) + return hash unless hash.is_a? Hash + hash.each do |key, value| + indent_spaces = " " * (indent * 2) + printf "#{indent_spaces}#{key}:" + if value.is_a? Hash + puts + puts_hash(value, indent: indent+1) + else + puts " #{value}" + end + end + hash +end +alias ph puts_hash |