From 4c14675021ce399dc64459f94763e2016bc58b41 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Mon, 14 Jun 2021 14:14:36 +0100 Subject: [PATCH 1/6] audit: add more checks for conflics_with audit --- Library/Homebrew/formula_auditor.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/formula_auditor.rb b/Library/Homebrew/formula_auditor.rb index c942c0c97d75c..a45d7166f4b56 100644 --- a/Library/Homebrew/formula_auditor.rb +++ b/Library/Homebrew/formula_auditor.rb @@ -315,7 +315,14 @@ def audit_deps def audit_conflicts formula.conflicts.each do |c| - Formulary.factory(c.name) + conflicting_formula = Formulary.factory(c.name) + problem "Formula should not conflict with itself" if formula == conflicting_formula + + # Use Formula instead of FormulaConflict to be able correctly handle renamed formulae and aliases + reverse_conflicts = conflicting_formula.conflicts.map { |rc| Formulary.factory(rc.name) } + if reverse_conflicts.exclude? formula + problem "Formula #{conflicting_formula.name} should also have a conflict declared with #{formula.name}" + end rescue TapFormulaUnavailableError # Don't complain about missing cross-tap conflicts. next From 84e3e0a6b87229067041da4e947ee4f352892b62 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Tue, 15 Jun 2021 13:25:26 +0100 Subject: [PATCH 2/6] audit_spec: add tests for audit_conflicts --- Library/Homebrew/test/dev-cmd/audit_spec.rb | 53 +++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index 92dcb75377e53..259e6bd2c184a 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -1139,5 +1139,58 @@ class FooAT11 < Formula expect(fa.problems).to be_empty end end + + describe "#audit_conflicts" do + specify "it warns when conflicting with non-existing formula" do + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + conflicts_with "bar" + end + RUBY + + fa.audit_conflicts + + expect(fa.problems.first[:message]) + .to match("Can't find conflicting formula \"bar\"") + end + + specify "it warns when conflicting with itself" do + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + conflicts_with "#{dir}/foo.rb" + end + RUBY + + fa.audit_conflicts + + expect(fa.problems.first[:message]) + .to match("Formula should not conflict with itself") + end + + specify "it warns when another formula does not have a symmetric conflict" do + formula_auditor "bar", <<~RUBY + class Bar < Formula + url "https://brew.sh/foo-1.0.tgz" + end + RUBY + + fa = formula_auditor "foo", <<~RUBY + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + conflicts_with "#{dir}/bar.rb" + end + RUBY + + fa.audit_conflicts + + expect(fa.problems.first[:message]) + .to match("Formula bar should also have a conflict declared with foo") + end + end end end From 1ddb6ef584ceda0e5724f8c48ae6f2ba8503ec56 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Wed, 16 Jun 2021 22:52:09 +0100 Subject: [PATCH 3/6] audit: do not allow aliases and renames in formula conflicts --- Library/Homebrew/formula_auditor.rb | 21 ++++++++++++++++++--- Library/Homebrew/test/dev-cmd/audit_spec.rb | 8 ++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/formula_auditor.rb b/Library/Homebrew/formula_auditor.rb index a45d7166f4b56..55c6d0976f89e 100644 --- a/Library/Homebrew/formula_auditor.rb +++ b/Library/Homebrew/formula_auditor.rb @@ -318,9 +318,24 @@ def audit_conflicts conflicting_formula = Formulary.factory(c.name) problem "Formula should not conflict with itself" if formula == conflicting_formula - # Use Formula instead of FormulaConflict to be able correctly handle renamed formulae and aliases - reverse_conflicts = conflicting_formula.conflicts.map { |rc| Formulary.factory(rc.name) } - if reverse_conflicts.exclude? formula + next unless @core_tap + + if CoreTap.instance.formula_renames.key?(c.name) || Formula.aliases.include?(c.name) + problem "Formula conflict should be declared using " \ + "canonical name (#{conflicting_formula.name}) instead of #{c.name}" + end + + rev_conflict_found = false + conflicting_formula.conflicts.each do |rc| + rc_formula = Formulary.factory(rc.name) + if CoreTap.instance.formula_renames.key?(rc.name) || Formula.aliases.include?(rc.name) + problem "Formula #{conflicting_formula.name} conflict should be declared using " \ + "canonical name (#{rc_formula.name}) instead of #{rc.name}" + end + + rev_conflict_found ||= rc_formula == formula + end + unless rev_conflict_found problem "Formula #{conflicting_formula.name} should also have a conflict declared with #{formula.name}" end rescue TapFormulaUnavailableError diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index 259e6bd2c184a..d9040f4055e01 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -1142,7 +1142,7 @@ class FooAT11 < Formula describe "#audit_conflicts" do specify "it warns when conflicting with non-existing formula" do - fa = formula_auditor "foo", <<~RUBY + fa = formula_auditor "foo", <<~RUBY, core_tap: true class Foo < Formula url "https://brew.sh/foo-1.0.tgz" @@ -1157,7 +1157,7 @@ class Foo < Formula end specify "it warns when conflicting with itself" do - fa = formula_auditor "foo", <<~RUBY + fa = formula_auditor "foo", <<~RUBY, core_tap: true class Foo < Formula url "https://brew.sh/foo-1.0.tgz" @@ -1172,13 +1172,13 @@ class Foo < Formula end specify "it warns when another formula does not have a symmetric conflict" do - formula_auditor "bar", <<~RUBY + formula_auditor "bar", <<~RUBY, core_tap: true class Bar < Formula url "https://brew.sh/foo-1.0.tgz" end RUBY - fa = formula_auditor "foo", <<~RUBY + fa = formula_auditor "foo", <<~RUBY, core_tap: true class Foo < Formula url "https://brew.sh/foo-1.0.tgz" From 43d67816ea48ac9ef1356015bef2d515b1de7b0f Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Fri, 18 Jun 2021 16:40:12 +0100 Subject: [PATCH 4/6] audit_conflicts: enable for third-party taps --- Library/Homebrew/formula_auditor.rb | 9 +-- Library/Homebrew/test/dev-cmd/audit_spec.rb | 66 ++++++++++++--------- 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/Library/Homebrew/formula_auditor.rb b/Library/Homebrew/formula_auditor.rb index 55c6d0976f89e..1bb9c553392d6 100644 --- a/Library/Homebrew/formula_auditor.rb +++ b/Library/Homebrew/formula_auditor.rb @@ -314,13 +314,14 @@ def audit_deps end def audit_conflicts + tap = formula.tap formula.conflicts.each do |c| conflicting_formula = Formulary.factory(c.name) - problem "Formula should not conflict with itself" if formula == conflicting_formula + next if tap != conflicting_formula.tap - next unless @core_tap + problem "Formula should not conflict with itself" if formula == conflicting_formula - if CoreTap.instance.formula_renames.key?(c.name) || Formula.aliases.include?(c.name) + if tap.formula_renames.key?(c.name) || tap.aliases.include?(c.name) problem "Formula conflict should be declared using " \ "canonical name (#{conflicting_formula.name}) instead of #{c.name}" end @@ -328,7 +329,7 @@ def audit_conflicts rev_conflict_found = false conflicting_formula.conflicts.each do |rc| rc_formula = Formulary.factory(rc.name) - if CoreTap.instance.formula_renames.key?(rc.name) || Formula.aliases.include?(rc.name) + if tap.formula_renames.key?(rc.name) || tap.aliases.include?(rc.name) problem "Formula #{conflicting_formula.name} conflict should be declared using " \ "canonical name (#{rc_formula.name}) instead of #{rc.name}" end diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index d9040f4055e01..cd33d61b27e60 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -58,13 +58,20 @@ class #{Formulary.class_s(name)} < Formula end describe FormulaAuditor do - def formula_auditor(name, text, options = {}) - path = Pathname.new "#{dir}/#{name}.rb" - path.open("w") do |f| - f.write text + def formula_auditor(name_or_formula, text = nil, options = {}) + formula = case name_or_formula + when String + path = Pathname.new "#{dir}/#{name_or_formula}.rb" + path.open("w") do |f| + f.write text + end + + Formulary.factory(path) + when Formula + name_or_formula end - described_class.new(Formulary.factory(path), options) + described_class.new(formula, options) end let(:dir) { mktmpdir } @@ -1141,15 +1148,18 @@ class FooAT11 < Formula end describe "#audit_conflicts" do + before do + allow(File).to receive(:open).and_return("") + end + specify "it warns when conflicting with non-existing formula" do - fa = formula_auditor "foo", <<~RUBY, core_tap: true - class Foo < Formula - url "https://brew.sh/foo-1.0.tgz" + foo = formula("foo") do + url "https://brew.sh/bar-1.0.tgz" - conflicts_with "bar" - end - RUBY + conflicts_with "bar" + end + fa = formula_auditor foo fa.audit_conflicts expect(fa.problems.first[:message]) @@ -1157,14 +1167,14 @@ class Foo < Formula end specify "it warns when conflicting with itself" do - fa = formula_auditor "foo", <<~RUBY, core_tap: true - class Foo < Formula - url "https://brew.sh/foo-1.0.tgz" + foo = formula("foo") do + url "https://brew.sh/bar-1.0.tgz" - conflicts_with "#{dir}/foo.rb" - end - RUBY + conflicts_with "foo" + end + stub_formula_loader foo + fa = formula_auditor foo fa.audit_conflicts expect(fa.problems.first[:message]) @@ -1172,24 +1182,22 @@ class Foo < Formula end specify "it warns when another formula does not have a symmetric conflict" do - formula_auditor "bar", <<~RUBY, core_tap: true - class Bar < Formula - url "https://brew.sh/foo-1.0.tgz" - end - RUBY + foo = formula("foo") do + url "https://brew.sh/foo-1.0.tgz" + end + stub_formula_loader foo - fa = formula_auditor "foo", <<~RUBY, core_tap: true - class Foo < Formula - url "https://brew.sh/foo-1.0.tgz" + bar = formula("bar") do + url "https://brew.sh/bar-1.0.tgz" - conflicts_with "#{dir}/bar.rb" - end - RUBY + conflicts_with "foo" + end + fa = formula_auditor bar fa.audit_conflicts expect(fa.problems.first[:message]) - .to match("Formula bar should also have a conflict declared with foo") + .to match("Formula foo should also have a conflict declared with bar") end end end From c95c2e325807890c3eeb58baba76c14f52634e34 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Fri, 18 Jun 2021 16:57:53 +0100 Subject: [PATCH 5/6] audit_conflicts: verbose variables name --- Library/Homebrew/formula_auditor.rb | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/Library/Homebrew/formula_auditor.rb b/Library/Homebrew/formula_auditor.rb index 1bb9c553392d6..535c61cd8187f 100644 --- a/Library/Homebrew/formula_auditor.rb +++ b/Library/Homebrew/formula_auditor.rb @@ -315,37 +315,37 @@ def audit_deps def audit_conflicts tap = formula.tap - formula.conflicts.each do |c| - conflicting_formula = Formulary.factory(c.name) + formula.conflicts.each do |conflict| + conflicting_formula = Formulary.factory(conflict.name) next if tap != conflicting_formula.tap problem "Formula should not conflict with itself" if formula == conflicting_formula - if tap.formula_renames.key?(c.name) || tap.aliases.include?(c.name) + if tap.formula_renames.key?(conflict.name) || tap.aliases.include?(conflict.name) problem "Formula conflict should be declared using " \ - "canonical name (#{conflicting_formula.name}) instead of #{c.name}" + "canonical name (#{conflicting_formula.name}) instead of #{conflict.name}" end - rev_conflict_found = false - conflicting_formula.conflicts.each do |rc| - rc_formula = Formulary.factory(rc.name) - if tap.formula_renames.key?(rc.name) || tap.aliases.include?(rc.name) + reverse_conflict_found = false + conflicting_formula.conflicts.each do |reverse_conflict| + reverse_conflict_formula = Formulary.factory(reverse_conflict.name) + if tap.formula_renames.key?(reverse_conflict.name) || tap.aliases.include?(reverse_conflict.name) problem "Formula #{conflicting_formula.name} conflict should be declared using " \ - "canonical name (#{rc_formula.name}) instead of #{rc.name}" + "canonical name (#{reverse_conflict_formula.name}) instead of #{reverse_conflict.name}" end - rev_conflict_found ||= rc_formula == formula + reverse_conflict_found ||= reverse_conflict_formula == formula end - unless rev_conflict_found + unless reverse_conflict_found problem "Formula #{conflicting_formula.name} should also have a conflict declared with #{formula.name}" end rescue TapFormulaUnavailableError # Don't complain about missing cross-tap conflicts. next rescue FormulaUnavailableError - problem "Can't find conflicting formula #{c.name.inspect}." + problem "Can't find conflicting formula #{conflict.name.inspect}." rescue TapFormulaAmbiguityError, TapFormulaWithOldnameAmbiguityError - problem "Ambiguous conflicting formula #{c.name.inspect}." + problem "Ambiguous conflicting formula #{conflict.name.inspect}." end end From 7c962c0aa8b20d3b3b3f09975d842b01fcae29f2 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Fri, 18 Jun 2021 17:26:57 +0100 Subject: [PATCH 6/6] audit_spec: simplify tests --- Library/Homebrew/test/dev-cmd/audit_spec.rb | 24 ++++++++------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index cd33d61b27e60..7ed678d25404e 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -58,20 +58,13 @@ class #{Formulary.class_s(name)} < Formula end describe FormulaAuditor do - def formula_auditor(name_or_formula, text = nil, options = {}) - formula = case name_or_formula - when String - path = Pathname.new "#{dir}/#{name_or_formula}.rb" - path.open("w") do |f| - f.write text - end - - Formulary.factory(path) - when Formula - name_or_formula + def formula_auditor(name, text, options = {}) + path = Pathname.new "#{dir}/#{name}.rb" + path.open("w") do |f| + f.write text end - described_class.new(formula, options) + described_class.new(Formulary.factory(path), options) end let(:dir) { mktmpdir } @@ -1149,6 +1142,7 @@ class FooAT11 < Formula describe "#audit_conflicts" do before do + # We don't really test FormulaTextAuditor here allow(File).to receive(:open).and_return("") end @@ -1159,7 +1153,7 @@ class FooAT11 < Formula conflicts_with "bar" end - fa = formula_auditor foo + fa = described_class.new foo fa.audit_conflicts expect(fa.problems.first[:message]) @@ -1174,7 +1168,7 @@ class FooAT11 < Formula end stub_formula_loader foo - fa = formula_auditor foo + fa = described_class.new foo fa.audit_conflicts expect(fa.problems.first[:message]) @@ -1193,7 +1187,7 @@ class FooAT11 < Formula conflicts_with "foo" end - fa = formula_auditor bar + fa = described_class.new bar fa.audit_conflicts expect(fa.problems.first[:message])