admin/brew
1
0
Fork 0
Code Issues Packages Projects Releases 6 Wiki Activity

Merge pull request #2662 from GauthamGoli/audit_text_rubocop

Browse Source 
audit: Port audit_text method to rubocop and add tests
This commit is contained in:
Mike McQuaid 2017-05-30 19:10:39 +01:00 committed by GitHub
commit 344c492299
7 changed files with 455 additions and 96 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
Library/.rubocop.yml
View File

@ -8,6 +8,9 @@ AllCops:
require: ./Homebrew/rubocops.rb
FormulaAudit/Text:
Enabled: true
FormulaAuditStrict/BottleBlock:
Enabled: true

36
Library/Homebrew/dev-cmd/audit.rb
View File

@ -850,14 +850,6 @@ class FormulaAuditor
end
def audit_text
if text =~ /system\s+['"]scons/
problem "use \"scons *args\" instead of \"system 'scons', *args\""
end
if text =~ /system\s+['"]xcodebuild/
problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args")
end
bin_names = Set.new
bin_names << formula.name
bin_names += formula.aliases
@ -872,34 +864,6 @@ class FormulaAuditor
end
end
end
if text =~ /xcodebuild[ (]*["'*]*/ && !text.include?("SYMROOT=")
problem 'xcodebuild should be passed an explicit "SYMROOT"'
end
if text.include? "Formula.factory("
problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\""
end
if text.include?("def plist") && !text.include?("plist_options")
problem "Please set plist_options when using a formula-defined plist."
end
if text =~ /depends_on\s+['"]openssl['"]/ && text =~ /depends_on\s+['"]libressl['"]/
problem "Formulae should not depend on both OpenSSL and LibreSSL (even optionally)."
end
if text =~ /virtualenv_(create|install_with_resources)/ &&
text =~ /resource\s+['"]setuptools['"]\s+do/
problem "Formulae using virtualenvs do not need a `setuptools` resource."
end
if text =~ /system\s+['"]go['"],\s+['"]get['"]/
problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s."
end
return unless text.include?('require "language/go"') && !text.include?("go_resource")
problem "require \"language/go\" is unnecessary unless using `go_resource`s"
end
def audit_lines

1
Library/Homebrew/rubocops.rb
View File

@ -3,3 +3,4 @@ require_relative "./rubocops/formula_desc_cop"
require_relative "./rubocops/components_order_cop"
require_relative "./rubocops/components_redundancy_cop"
require_relative "./rubocops/homepage_cop"
require_relative "./rubocops/text_cop"

144
Library/Homebrew/rubocops/extend/formula_cop.rb
View File

@ -1,3 +1,5 @@
require "parser/current"
module RuboCop
module Cop
class FormulaCop < Cop
@ -7,11 +9,11 @@ module RuboCop
def on_class(node)
file_path = processed_source.buffer.name
return unless file_path_allowed?(file_path)
class_node, parent_class_node, body = *node
return unless formula_class?(parent_class_node)
return unless formula_class?(node)
return unless respond_to?(:audit_formula)
class_node, parent_class_node, @body = *node
@formula_name = class_name(class_node)
audit_formula(node, class_node, parent_class_node, body)
audit_formula(node, class_node, parent_class_node, @body)
end
# Checks for regex match of pattern in the node and
@ -46,12 +48,100 @@ module RuboCop
nil
end
# Returns an array of method call nodes matching method_name inside node
# Set the given node as the offending node when required in custom cops
def offending_node(node)
@offensive_node = node
@offense_source_range = node.source_range
end
# Returns an array of method call nodes matching method_name inside node with depth first order (Children nodes)
def find_method_calls_by_name(node, method_name)
return if node.nil?
node.each_child_node(:send).select { |method_node| method_name == method_node.method_name }
end
# Returns an array of method call nodes matching method_name in every descendant of node
def find_every_method_call_by_name(node, method_name)
return if node.nil?
node.each_descendant(:send).select { |method_node| method_name == method_node.method_name }
end
# Given a method_name and arguments, yields to a block with
# matching method passed as a parameter to the block
def find_method_with_args(node, method_name, *args)
methods = find_every_method_call_by_name(node, method_name)
methods.each do |method|
next unless parameters_passed?(method, *args)
yield method
end
end
# Matches a method with a receiver,
# EX: to match `Formula.factory(name)`
# call `find_instance_method_call(node, "Formula", :factory)`
# yields to a block with matching method node
def find_instance_method_call(node, instance, method_name)
methods = find_every_method_call_by_name(node, method_name)
methods.each do |method|
next unless method.receiver.const_name == instance
@offense_source_range = method.source_range
@offensive_node = method
yield method
end
end
# Returns nil if does not depend on dependency_name
# args: node - dependency_name - dependency's name
def depends_on?(dependency_name)
dependency_nodes = find_every_method_call_by_name(@body, :depends_on)
idx = dependency_nodes.index do |n|
depends_on_name_type?(n, dependency_name, :required) ||
depends_on_name_type?(n, dependency_name, :build) ||
depends_on_name_type?(n, dependency_name, :optional) ||
depends_on_name_type?(n, dependency_name, :recommended) ||
depends_on_name_type?(n, dependency_name, :run)
end
return if idx.nil?
@offense_source_range = dependency_nodes[idx].source_range
@offensive_node = dependency_nodes[idx]
end
# Returns true if given dependency name and dependency type exist in given dependency method call node
# TODO: Add case where key of hash is an array
def depends_on_name_type?(node, name = nil, type = :required)
if name
name_match = false
else
name_match = true # Match only by type when name is nil
end
case type
when :required
type_match = !node.method_args.nil? && node.method_args.first.str_type?
if type_match && !name_match
name_match = node_equals?(node.method_args.first, name)
end
when :build, :optional, :recommended, :run
type_match = !node.method_args.nil? &&
node.method_args.first.hash_type? &&
node.method_args.first.values.first.children.first == type
if type_match && !name_match
name_match = node_equals?(node.method_args.first.keys.first.children.first, name)
end
end
if type_match || name_match
@offensive_node = node
@offense_source_range = node.source_range
end
type_match && name_match
end
# To compare node with appropriate Ruby variable
def node_equals?(node, var)
node == Parser::CurrentRuby.parse(var.inspect)
end
# Returns a block named block_name inside node
def find_block(node, block_name)
return if node.nil?
@ -112,6 +202,17 @@ module RuboCop
false
end
# Check if method_name is called among every descendant node of given node
def method_called_ever?(node, method_name)
node.each_descendant(:send) do |call_node|
next unless call_node.method_name == method_name
@offensive_node = call_node
@offense_source_range = call_node.source_range
return true
end
false
end
# Checks for precedence, returns the first pair of precedence violating nodes
def check_precedence(first_nodes, next_nodes)
next_nodes.each do |each_next_node|
@ -138,6 +239,24 @@ module RuboCop
method_node.method_args
end
# Returns true if the given parameters are present in method call
# and sets the method call as the offending node
# params can be string, symbol, array, hash, matching regex
def parameters_passed?(method_node, *params)
method_params = parameters(method_node)
@offensive_node = method_node
@offense_source_range = method_node.source_range
params.all? do |given_param|
method_params.any? do |method_param|
if given_param.class == Regexp
regex_match_group(method_param, given_param)
else
node_equals?(method_param, given_param)
end
end
end
end
# Returns the begin position of the node's line in source code
def line_start_column(node)
node.source_range.source_buffer.line_range(node.loc.line).begin_pos
@ -180,10 +299,16 @@ module RuboCop
node.source_range.source_buffer
end
# Returns the string representation if node is of type str(plain) or dstr(interpolated)
# Returns the string representation if node is of type str(plain) or dstr(interpolated) or const
def string_content(node)
return node.str_content if node.type == :str
node.each_child_node(:str).map(&:str_content).join("") if node.type == :dstr
case node.type
when :str
return node.str_content if node.type == :str
when :dstr
return node.each_child_node(:str).map(&:str_content).join("") if node.type == :dstr
when :const
return node.const_name if node.type == :const
end
end
# Returns printable component name
@ -198,8 +323,9 @@ module RuboCop
private
def formula_class?(parent_class_node)
parent_class_node && parent_class_node.const_name == "Formula"
def formula_class?(node)
_, class_node, = *node
class_node && string_content(class_node) == "Formula"
end
def file_path_allowed?(file_path)

55
Library/Homebrew/rubocops/text_cop.rb Normal file
View File

@ -0,0 +1,55 @@
require_relative "./extend/formula_cop"
module RuboCop
module Cop
module FormulaAudit
class Text < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, body_node)
if !find_node_method_by_name(body_node, :plist_options) &&
find_method_def(body_node, :plist)
problem "Please set plist_options when using a formula-defined plist."
end
if depends_on?("openssl") && depends_on?("libressl")
problem "Formulae should not depend on both OpenSSL and LibreSSL (even optionally)."
end
if method_called_ever?(body_node, :virtualenv_create) ||
method_called_ever?(body_node, :virtualenv_install_with_resources)
find_method_with_args(body_node, :resource, "setuptools") do
problem "Formulae using virtualenvs do not need a `setuptools` resource."
end
end
unless method_called_ever?(body_node, :go_resource)
# processed_source.ast is passed instead of body_node because `require` would be outside body_node
find_method_with_args(processed_source.ast, :require, "language/go") do
problem "require \"language/go\" is unnecessary unless using `go_resource`s"
end
end
find_instance_method_call(body_node, "Formula", :factory) do
problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\""
end
find_every_method_call_by_name(body_node, :xcodebuild).each do |m|
next if parameters_passed?(m, /SYMROOT=/)
problem 'xcodebuild should be passed an explicit "SYMROOT"'
end
find_method_with_args(body_node, :system, "xcodebuild") do
problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args")
end
find_method_with_args(body_node, :system, "scons") do
problem "use \"scons *args\" instead of \"system 'scons', *args\""
end
find_method_with_args(body_node, :system, "go", "get") do
problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s."
end
end
end
end
end
end

51
Library/Homebrew/test/dev-cmd/audit_spec.rb
View File

@ -385,57 +385,6 @@ describe FormulaAuditor do
end
end
describe "#audit_text" do
specify "xcodebuild suggests symroot" do
fa = formula_auditor "foo", <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
xcodebuild "-project", "meow.xcodeproject"
end
end
EOS
fa.audit_text
expect(fa.problems.first)
.to match('xcodebuild should be passed an explicit "SYMROOT"')
end
specify "bare xcodebuild also suggests symroot" do
fa = formula_auditor "foo", <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
xcodebuild
end
end
EOS
fa.audit_text
expect(fa.problems.first)
.to match('xcodebuild should be passed an explicit "SYMROOT"')
end
specify "disallow go get usage" do
fa = formula_auditor "foo", <<-EOS.undent
class Foo <Formula
url "http://example.com/foo-1.0.tgz"
def install
system "go", "get", "bar"
end
end
EOS
fa.audit_text
expect(fa.problems.first)
.to match("Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s.")
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" }

261
Library/Homebrew/test/rubocops/text_cop_spec.rb Normal file
View File

@ -0,0 +1,261 @@
require "rubocop"
require "rubocop/rspec/support"
require_relative "../../extend/string"
require_relative "../../rubocops/text_cop"
describe RuboCop::Cop::FormulaAudit::Text do
subject(:cop) { described_class.new }
context "When auditing formula text" do
it "When xcodebuild is called without SYMROOT" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
xcodebuild "-project", "meow.xcodeproject"
end
end
EOS
expected_offenses = [{ message: "xcodebuild should be passed an explicit \"SYMROOT\"",
severity: :convention,
line: 6,
column: 4,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When xcodebuild is called without any args" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
xcodebuild
end
end
EOS
expected_offenses = [{ message: "xcodebuild should be passed an explicit \"SYMROOT\"",
severity: :convention,
line: 6,
column: 4,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When go get is executed" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
system "go", "get", "bar"
end
end
EOS
expected_offenses = [{ message: "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s.",
severity: :convention,
line: 6,
column: 4,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When xcodebuild is executed" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
system "xcodebuild", "foo", "bar"
end
end
EOS
expected_offenses = [{ message: "use \"xcodebuild *args\" instead of \"system 'xcodebuild', *args\"",
severity: :convention,
line: 6,
column: 4,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When scons is executed" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
system "scons", "foo", "bar"
end
end
EOS
expected_offenses = [{ message: "use \"scons *args\" instead of \"system 'scons', *args\"",
severity: :convention,
line: 6,
column: 4,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When plist_options are not defined when using a formula-defined plist" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
system "xcodebuild", "foo", "bar"
end
def plist; <<-EOS.undent
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>Label</key>
<string>org.nrpe.agent</string>
</dict>
</plist>
\EOS
end
end
EOS
expected_offenses = [{ message: "Please set plist_options when using a formula-defined plist.",
severity: :convention,
line: 9,
column: 2,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When language/go is require'd" do
source = <<-EOS.undent
require "language/go"
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
system "go", "get", "bar"
end
end
EOS
expected_offenses = [{ message: "require \"language/go\" is unnecessary unless using `go_resource`s",
severity: :convention,
line: 1,
column: 0,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When formula uses virtualenv and also `setuptools` resource" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
resource "setuptools" do
url "https://foo.com/foo.tar.gz"
sha256 "db0904a28253cfe53e7dedc765c71596f3c53bb8a866ae50123320ec1a7b73fd"
end
def install
virtualenv_create(libexec)
end
end
EOS
expected_offenses = [{ message: "Formulae using virtualenvs do not need a `setuptools` resource.",
severity: :convention,
line: 5,
column: 2,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
it "When Formula.factory(name) is used" do
source = <<-EOS.undent
class Foo < Formula
url "http://example.com/foo-1.0.tgz"
homepage "http://example.com"
def install
Formula.factory(name)
end
end
EOS
expected_offenses = [{ message: "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"",
severity: :convention,
line: 6,
column: 4,
source: source }]
inspect_source(cop, source)
expected_offenses.zip(cop.offenses).each do |expected, actual|
expect_offense(expected, actual)
end
end
def expect_offense(expected, actual)
expect(actual.message).to eq(expected[:message])
expect(actual.severity).to eq(expected[:severity])
expect(actual.line).to eq(expected[:line])
expect(actual.column).to eq(expected[:column])
end
end
end