Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

audit: Port patches audit code to a rubocop and add tests #2976

Merged
merged 2 commits into from
Aug 5, 2017
Merged

audit: Port patches audit code to a rubocop and add tests #2976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43cec10
audit: Port patches audit code to a rubocop
GauthamGoli Jul 30, 2017
66ffdb5
Refactor legacy_patches_cop to merge external patch audit rules in a …
GauthamGoli Aug 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 0 additions & 31 deletions Library/Homebrew/dev-cmd/audit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,6 @@ def audit_specs
end

next if spec.patches.empty?
spec.patches.each { |p| patch_problems(p) if p.external? }
next unless @new_formula
problem "New formulae should not require patches to build. Patches should be submitted and accepted upstream first."
end
Expand Down Expand Up @@ -786,36 +785,6 @@ def audit_revision_and_version_scheme
end
end

def patch_problems(patch)
case patch.url
when %r{https?://github\.com/.+/.+/(?:commit|pull)/[a-fA-F0-9]*.(?:patch|diff)}
unless patch.url =~ /\?full_index=\w+$/
problem <<-EOS.undent
GitHub patches should use the full_index parameter:
#{patch.url}?full_index=1
EOS
end
when /raw\.github\.com/, %r{gist\.github\.com/raw}, %r{gist\.github\.com/.+/raw},
%r{gist\.githubusercontent\.com/.+/raw}
unless patch.url =~ /[a-fA-F0-9]{40}/
problem "GitHub/Gist patches should specify a revision:\n#{patch.url}"
end
when %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}
problem <<-EOS.undent
use GitHub pull request URLs:
https://github.com/#{Regexp.last_match(1)}/#{Regexp.last_match(2)}/pull/#{Regexp.last_match(3)}.patch?full_index=1
Rather than patch-diff:
#{patch.url}
EOS
when %r{macports/trunk}
problem "MacPorts patches should specify a revision instead of trunk:\n#{patch.url}"
when %r{^http://trac\.macports\.org}
problem "Patches from MacPorts Trac should be https://, not http:\n#{patch.url}"
when %r{^http://bugs\.debian\.org}
problem "Patches from Debian should be https://, not http:\n#{patch.url}"
end
end

def audit_text
bin_names = Set.new
bin_names << formula.name
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/rubocops.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
require_relative "./rubocops/text_cop"
require_relative "./rubocops/caveats_cop"
require_relative "./rubocops/checksum_cop"
require_relative "./rubocops/legacy_patches_cop"
require_relative "./rubocops/patches_cop"
require_relative "./rubocops/conflicts_cop"
require_relative "./rubocops/options_cop"
require_relative "./rubocops/urls_cop"
Expand Down
15 changes: 12 additions & 3 deletions ...y/Homebrew/rubocops/legacy_patches_cop.rb → Library/Homebrew/rubocops/patches_cop.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,25 @@
module RuboCop
module Cop
module FormulaAudit
# This cop checks for and audits legacy patches in Formulae
class LegacyPatches < FormulaCop
# This cop audits patches in Formulae
class Patches < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, body)
external_patches = find_all_blocks(body, :patch)
external_patches.each do |patch_block|
url_node = find_every_method_call_by_name(patch_block, :url).first
url_string = parameters(url_node).first
patch_problems(url_string)
end

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

private

def patch_problems(patch)
patch_url = string_content(patch)
gh_patch_patterns = Regexp.union([%r{/raw\.github\.com/},
Expand All @@ -30,7 +39,7 @@ def patch_problems(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
https://github.com/#{match_obj[1]}/#{match_obj[2]}/pull/#{match_obj[3]}.patch
Rather than patch-diff:
#{patch_url}
EOS
Expand Down
77 changes: 75 additions & 2 deletions .../test/rubocops/legacy_patches_cop_spec.rb → ...omebrew/test/rubocops/patches_cop_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
require "rubocop"
require "rubocop/rspec/support"
require_relative "../../extend/string"
require_relative "../../rubocops/legacy_patches_cop"
require_relative "../../rubocops/patches_cop"

describe RuboCop::Cop::FormulaAudit::LegacyPatches do
describe RuboCop::Cop::FormulaAudit::Patches do
subject(:cop) { described_class.new }

context "When auditing legacy patches" do
Expand Down Expand Up @@ -47,6 +47,7 @@ def patches
"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",
"https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch",
]
patch_urls.each do |patch_url|
source = <<-EOS.undent
Expand Down Expand Up @@ -84,6 +85,15 @@ def patches
line: 5,
column: 5,
source: source }]
elsif patch_url =~ %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}
expected_offenses = [{ message: "use GitHub pull request URLs:\n"\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Favour using EOS.indent over embedding \n in multiline strings. If you've done this in other tests: make a new PR (or add to this one) that global style change. Alternatively, these could all be addressed in another PR after this is merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah will change these in another PR.

" https://github.com/foo/foo-bar/pull/100.patch\n"\
"Rather than patch-diff:\n"\
" https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch\n",
severity: :convention,
line: 5,
column: 5,
source: source }]
end
expected_offenses.zip([cop.offenses.last]).each do |expected, actual|
expect_offense(expected, actual)
Expand Down Expand Up @@ -125,4 +135,67 @@ def patches
end
end
end

context "When auditing external patches" do
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",
"https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch",
]
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"
patch do
url "#{patch_url}"
sha256 "63376b8fdd6613a91976106d9376069274191860cd58f039b29ff16de1925621"
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: 16,
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: 37,
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: 9,
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: 9,
source: source }]
elsif patch_url =~ %r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)}
expected_offenses = [{ message: "use GitHub pull request URLs:\n"\
" https://github.com/foo/foo-bar/pull/100.patch\n"\
"Rather than patch-diff:\n"\
" https://patch-diff.githubusercontent.com/raw/foo/foo-bar/pull/100.patch\n",
severity: :convention,
line: 5,
column: 9,
source: source }]
end
expected_offenses.zip([cop.offenses.last]).each do |expected, actual|
expect_offense(expected, actual)
end
end
end
end
end