diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index e84225cfcd..1c7476c4ba 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -817,16 +817,6 @@ class FormulaAuditor end end - def audit_legacy_patches - return unless formula.respond_to?(:patches) - legacy_patches = Patch.normalize_legacy_patches(formula.patches).grep(LegacyPatch) - - return if legacy_patches.empty? - - problem "Use the patch DSL instead of defining a 'patches' method" - legacy_patches.each { |p| patch_problems(p) } - end - def patch_problems(patch) case patch.url when /raw\.github\.com/, %r{gist\.github\.com/raw}, %r{gist\.github\.com/.+/raw}, diff --git a/Library/Homebrew/rubocops/legacy_patches_cop.rb b/Library/Homebrew/rubocops/legacy_patches_cop.rb new file mode 100644 index 0000000000..e569f650e1 --- /dev/null +++ b/Library/Homebrew/rubocops/legacy_patches_cop.rb @@ -0,0 +1,53 @@ +require_relative "./extend/formula_cop" +require_relative "../extend/string" + +module RuboCop + module Cop + module FormulaAudit + # This cop checks for and audits legacy patches in Formulae + class LegacyPatches < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body) + patches_node = find_method_def(body, :patches) + return if patches_node.nil? + legacy_patches = find_strings(patches_node) + problem "Use the patch DSL instead of defining a 'patches' method" + legacy_patches.each { |p| patch_problems(p) } + end + + def patch_problems(patch) + patch_url = string_content(patch) + gh_patch_patterns = Regexp.union([%r{/raw\.github\.com/}, + %r{gist\.github\.com/raw}, + %r{gist\.github\.com/.+/raw}, + %r{gist\.githubusercontent\.com/.+/raw}]) + if regex_match_group(patch, gh_patch_patterns) + unless patch_url =~ /[a-fA-F0-9]{40}/ + problem "GitHub/Gist patches should specify a revision:\n#{patch_url}" + end + end + + gh_patch_diff_pattern = %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)} + if match_obj = regex_match_group(patch, gh_patch_diff_pattern) + problem <<-EOS.undent + use GitHub pull request URLs: + https://github.com/#{match_obj[1]}/#{match_obj[2]}/pull/#{match_ojb[3]}.patch + Rather than patch-diff: + #{patch_url} + EOS + end + + if regex_match_group(patch, %r{macports/trunk}) + problem "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}" + end + + if regex_match_group(patch, %r{^http://trac\.macports\.org}) + problem "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}" + end + + return unless regex_match_group(patch, %r{^http://bugs\.debian\.org}) + problem "Patches from Debian should be https://, not http:\n#{patch_url}" + end + end + end + end +end diff --git a/Library/Homebrew/test/rubocops/legacy_patches_cop_spec.rb b/Library/Homebrew/test/rubocops/legacy_patches_cop_spec.rb new file mode 100644 index 0000000000..a08fa614db --- /dev/null +++ b/Library/Homebrew/test/rubocops/legacy_patches_cop_spec.rb @@ -0,0 +1,128 @@ +require "rubocop" +require "rubocop/rspec/support" +require_relative "../../extend/string" +require_relative "../../rubocops/legacy_patches_cop" + +describe RuboCop::Cop::FormulaAudit::LegacyPatches do + subject(:cop) { described_class.new } + + context "When auditing legacy patches" do + it "When there is no legacy patch" do + source = <<-EOS.undent + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + end + EOS + inspect_source(cop, source) + expect(cop.offenses).to eq([]) + end + + it "Formula with `def patches`" do + source = <<-EOS.undent + class Foo < Formula + homepage "ftp://example.com/foo" + url "http://example.com/foo-1.0.tgz" + def patches + DATA + end + end + EOS + + expected_offenses = [{ message: "Use the patch DSL instead of defining a 'patches' method", + severity: :convention, + line: 4, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "Patch URLs" do + patch_urls = [ + "https://raw.github.com/mogaal/sendemail", + "https://mirrors.ustc.edu.cn/macports/trunk/", + "http://trac.macports.org/export/102865/trunk/dports/mail/uudeview/files/inews.c.patch", + "http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=patch-libunac1.txt;att=1;bug=623340", + ] + patch_urls.each do |patch_url| + source = <<-EOS.undent + class Foo < Formula + homepage "ftp://example.com/foo" + url "http://example.com/foo-1.0.tgz" + def patches + "#{patch_url}" + end + end + EOS + + inspect_source(cop, source) + if patch_url =~ %r{/raw\.github\.com/} + expected_offenses = [{ message: "GitHub/Gist patches should specify a revision:\n#{patch_url}", + severity: :convention, + line: 5, + column: 12, + source: source }] + elsif patch_url =~ %r{macports/trunk} + expected_offenses = [{ message: "MacPorts patches should specify a revision instead of trunk:\n#{patch_url}", + severity: :convention, + line: 5, + column: 33, + source: source }] + elsif patch_url =~ %r{^http://trac\.macports\.org} + expected_offenses = [{ message: "Patches from MacPorts Trac should be https://, not http:\n#{patch_url}", + severity: :convention, + line: 5, + column: 5, + source: source }] + elsif patch_url =~ %r{^http://bugs\.debian\.org} + expected_offenses = [{ message: "Patches from Debian should be https://, not http:\n#{patch_url}", + severity: :convention, + line: 5, + column: 5, + source: source }] + end + expected_offenses.zip([cop.offenses.last]).each do |expected, actual| + expect_offense(expected, actual) + end + end + end + + it "Formula with nested `def patches`" do + source = <<-EOS.undent + class Foo < Formula + homepage "ftp://example.com/foo" + url "http://example.com/foo-1.0.tgz" + def patches + files = %w[patch-domain_resolver.c patch-colormask.c patch-trafshow.c patch-trafshow.1 patch-configure] + { + :p0 => + files.collect{|p| "http://trac.macports.org/export/68507/trunk/dports/net/trafshow/files/\#{p}"} + } + end + end + EOS + + expected_offenses = [{ message: "Use the patch DSL instead of defining a 'patches' method", + severity: :convention, + line: 4, + column: 2, + source: source }, + { message: "Patches from MacPorts Trac should be https://, not http:\n"\ + "http://trac.macports.org/export/68507/trunk/dports/net/trafshow/files/", + severity: :convention, + line: 8, + column: 26, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + end +end