From 51f2338dd57eed5a7f18e147ccd4f4b9da19fb52 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Wed, 24 May 2017 00:08:31 +0530 Subject: [PATCH] audit: Port audit_text method to rubocop and add tests --- Library/.rubocop.yml | 3 + Library/Homebrew/dev-cmd/audit.rb | 36 --- Library/Homebrew/rubocops.rb | 1 + Library/Homebrew/rubocops/text_cop.rb | 73 +++++ Library/Homebrew/test/dev-cmd/audit_spec.rb | 51 ---- .../Homebrew/test/rubocops/text_cop_spec.rb | 261 ++++++++++++++++++ 6 files changed, 338 insertions(+), 87 deletions(-) create mode 100644 Library/Homebrew/rubocops/text_cop.rb create mode 100644 Library/Homebrew/test/rubocops/text_cop_spec.rb diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 66e71027e2..8fd64cb1a0 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -8,6 +8,9 @@ AllCops: require: ./Homebrew/rubocops.rb +FormulaAudit/Text: + Enabled: true + FormulaAuditStrict/BottleBlock: Enabled: true diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index d1665ea6f2..4ceff0f3d1 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -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 diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb index 587bf4b202..8627d2c044 100644 --- a/Library/Homebrew/rubocops.rb +++ b/Library/Homebrew/rubocops.rb @@ -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" diff --git a/Library/Homebrew/rubocops/text_cop.rb b/Library/Homebrew/rubocops/text_cop.rb new file mode 100644 index 0000000000..f657dde669 --- /dev/null +++ b/Library/Homebrew/rubocops/text_cop.rb @@ -0,0 +1,73 @@ +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?(body_node, "openssl") && depends_on?(body_node, "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) + resource_calls = find_every_method_call_by_name(body_node, :resource) + resource_calls.each do |m| + if parameters_passed?(m, "setuptools") + problem "Formulae using virtualenvs do not need a `setuptools` resource." + end + 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 + require_calls = find_every_method_call_by_name(processed_source.ast, :require) + require_calls.each do |m| + if parameters_passed?(m, "language/go") + problem "require \"language/go\" is unnecessary unless using `go_resource`s" + end + end + end + + factory_calls = find_every_method_call_by_name(body_node, :factory) + unless factory_calls.nil? + factory_calls.each do |m| + if !m.children.empty? && m.children[0] && string_content(m.children[0]) == "Formula" + offending_node(m) + problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"" + end + end + end + + xcodebuild_calls = find_every_method_call_by_name(body_node, :xcodebuild) + unless xcodebuild_calls.nil? + xcodebuild_calls.each do |m| + params = parameters(m).map { |param| string_content(param) } + if params.none? { |param| param.include?("SYMROOT=") } || params.empty? + offending_node(m) + problem 'xcodebuild should be passed an explicit "SYMROOT"' + end + end + end + + system_calls = find_every_method_call_by_name(body_node, :system) + return if system_calls.nil? + system_calls.each do |m| + if parameters_passed?(m, "go", "get") + problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s." + elsif parameters_passed?(m, "xcodebuild") + problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args") + elsif parameters_passed?(m, "scons") + problem "use \"scons *args\" instead of \"system 'scons', *args\"" + end + end + end + end + end + end +end diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index 97cc0f152e..b90a21b558 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -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 + + + + Label + org.nrpe.agent + + + \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