Refactor brew audit
This commit is contained in:
Jack Nagel
parent
8952bcf315
commit
f970f9ec60
@ -2,9 +2,35 @@ require 'formula'
|
||||
require 'utils'
|
||||
require 'extend/ENV'
|
||||
|
||||
# Some formulae use ENV in patches, so set up an environment
|
||||
ENV.extend(HomebrewEnvExtension)
|
||||
ENV.setup_build_environment
|
||||
module Homebrew extend self
|
||||
def audit
|
||||
formula_count = 0
|
||||
problem_count = 0
|
||||
|
||||
ff = if ARGV.named.empty?
|
||||
Formula.all
|
||||
else
|
||||
ARGV.formulae
|
||||
end
|
||||
|
||||
ff.each do |f|
|
||||
fa = FormulaAuditor.new f
|
||||
fa.audit
|
||||
|
||||
unless fa.problems.empty?
|
||||
puts "#{f.name}:"
|
||||
fa.problems.each { |p| puts " * #{p}" }
|
||||
puts
|
||||
formula_count += 1
|
||||
problem_count += fa.problems.size
|
||||
end
|
||||
end
|
||||
|
||||
unless problem_count.zero?
|
||||
ofail "#{problem_count} problems in #{formula_count} formulae"
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
class Module
|
||||
def redefine_const(name, value)
|
||||
@ -13,365 +39,353 @@ class Module
|
||||
end
|
||||
end
|
||||
|
||||
def ff
|
||||
return Formula.all if ARGV.named.empty?
|
||||
return ARGV.formulae
|
||||
end
|
||||
|
||||
def audit_formula_text name, text
|
||||
problems = []
|
||||
|
||||
if text =~ /<(Formula|AmazonWebServicesFormula|ScriptFileFormula|GithubGistFormula)/
|
||||
problems << " * Use a space in class inheritance: class Foo < #{$1}"
|
||||
end
|
||||
|
||||
# Commented-out cmake support from default template
|
||||
if (text =~ /# depends_on 'cmake'/) or (text =~ /# system "cmake/)
|
||||
problems << " * Commented cmake support found."
|
||||
end
|
||||
|
||||
# 2 (or more in an if block) spaces before depends_on, please
|
||||
if text =~ /^\ ?depends_on/
|
||||
problems << " * Check indentation of 'depends_on'."
|
||||
end
|
||||
|
||||
# build tools should be flagged properly
|
||||
build_deps = %w{autoconf automake boost-build bsdmake
|
||||
cmake imake libtool pkg-config scons smake}
|
||||
if text =~ /depends_on ['"](#{build_deps*'|'})['"]$/
|
||||
problems << " * #{$1} dependency should be \"depends_on '#{$1}' => :build\""
|
||||
end
|
||||
|
||||
# FileUtils is included in Formula
|
||||
if text =~ /FileUtils\.(\w+)/
|
||||
problems << " * Don't need 'FileUtils.' before #{$1}."
|
||||
end
|
||||
|
||||
# Check for long inreplace block vars
|
||||
if text =~ /inreplace .* do \|(.{2,})\|/
|
||||
problems << " * \"inreplace <filenames> do |s|\" is preferred over \"|#{$1}|\"."
|
||||
end
|
||||
|
||||
# Check for string interpolation of single values.
|
||||
if text =~ /(system|inreplace|gsub!|change_make_var!) .* ['"]#\{(\w+(\.\w+)?)\}['"]/
|
||||
problems << " * Don't need to interpolate \"#{$2}\" with #{$1}"
|
||||
end
|
||||
|
||||
# Check for string concatenation; prefer interpolation
|
||||
if text =~ /(#\{\w+\s*\+\s*['"][^}]+\})/
|
||||
problems << " * Try not to concatenate paths in string interpolation:\n #{$1}"
|
||||
end
|
||||
|
||||
# Prefer formula path shortcuts in Pathname+
|
||||
if text =~ %r{\(\s*(prefix\s*\+\s*(['"])(bin|include|libexec|lib|sbin|share)[/'"])}
|
||||
problems << " * \"(#{$1}...#{$2})\" should be \"(#{$3}+...)\""
|
||||
end
|
||||
|
||||
if text =~ %r[((man)\s*\+\s*(['"])(man[1-8])(['"]))]
|
||||
problems << " * \"#{$1}\" should be \"#{$4}\""
|
||||
end
|
||||
|
||||
# Prefer formula path shortcuts in strings
|
||||
if text =~ %r[(\#\{prefix\}/(bin|include|libexec|lib|sbin|share))]
|
||||
problems << " * \"#{$1}\" should be \"\#{#{$2}}\""
|
||||
end
|
||||
|
||||
if text =~ %r[((\#\{prefix\}/share/man/|\#\{man\}/)(man[1-8]))]
|
||||
problems << " * \"#{$1}\" should be \"\#{#{$3}}\""
|
||||
end
|
||||
|
||||
if text =~ %r[((\#\{share\}/(man)))[/'"]]
|
||||
problems << " * \"#{$1}\" should be \"\#{#{$3}}\""
|
||||
end
|
||||
|
||||
if text =~ %r[(\#\{prefix\}/share/(info|man))]
|
||||
problems << " * \"#{$1}\" should be \"\#{#{$2}}\""
|
||||
end
|
||||
|
||||
# Commented-out depends_on
|
||||
if text =~ /#\s*depends_on\s+(.+)\s*$/
|
||||
problems << " * Commented-out dep #{$1}."
|
||||
end
|
||||
|
||||
# No trailing whitespace, please
|
||||
if text =~ /(\t|[ ])+$/
|
||||
problems << " * Trailing whitespace was found."
|
||||
end
|
||||
|
||||
if text =~ /if\s+ARGV\.include\?\s+'--(HEAD|devel)'/
|
||||
problems << " * Use \"if ARGV.build_#{$1.downcase}?\" instead"
|
||||
end
|
||||
|
||||
if text =~ /make && make/
|
||||
problems << " * Use separate make calls."
|
||||
end
|
||||
|
||||
if text =~ /^[ ]*\t/
|
||||
problems << " * Use spaces instead of tabs for indentation"
|
||||
end
|
||||
|
||||
# xcodebuild should specify SYMROOT
|
||||
if text =~ /system\s+['"]xcodebuild/ and not text =~ /SYMROOT=/
|
||||
problems << " * xcodebuild should be passed an explicit \"SYMROOT\""
|
||||
end
|
||||
|
||||
# using ARGV.flag? for formula options is generally a bad thing
|
||||
if text =~ /ARGV\.flag\?/
|
||||
problems << " * Use 'ARGV.include?' instead of 'ARGV.flag?'"
|
||||
end
|
||||
|
||||
# Avoid hard-coding compilers
|
||||
if text =~ %r[(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?(gcc|llvm-gcc|clang)['" ]]
|
||||
problems << " * Use \"\#{ENV.cc}\" instead of hard-coding \"#{$3}\""
|
||||
end
|
||||
|
||||
if text =~ %r[(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?((g|llvm-g|clang)\+\+)['" ]]
|
||||
problems << " * Use \"\#{ENV.cxx}\" instead of hard-coding \"#{$3}\""
|
||||
end
|
||||
|
||||
if text =~ /system\s+['"](env|export)/
|
||||
problems << " * Use ENV instead of invoking '#{$1}' to modify the environment"
|
||||
end
|
||||
|
||||
if text =~ /version == ['"]HEAD['"]/
|
||||
problems << " * Use 'ARGV.build_head?' instead of inspecting 'version'"
|
||||
end
|
||||
|
||||
return problems
|
||||
end
|
||||
|
||||
INSTALL_OPTIONS = %W[
|
||||
--build-from-source
|
||||
--debug
|
||||
--devel
|
||||
--force
|
||||
--fresh
|
||||
--HEAD
|
||||
--ignore-dependencies
|
||||
--interactive
|
||||
--use-clang
|
||||
--use-gcc
|
||||
--use-llvm
|
||||
--verbose
|
||||
].freeze
|
||||
|
||||
def audit_formula_patches f
|
||||
problems = []
|
||||
patches = Patches.new(f.patches)
|
||||
patches.each do |p|
|
||||
next unless p.external?
|
||||
case p.url
|
||||
when %r[raw\.github\.com], %r[gist\.github\.com/raw]
|
||||
problems << " * Using raw GitHub URLs is not recommended:"
|
||||
problems << " #{p.url}"
|
||||
when %r[macports/trunk]
|
||||
problems << " * MacPorts patches should specify a revision instead of trunk:"
|
||||
problems << " #{p.url}"
|
||||
end
|
||||
end
|
||||
return problems
|
||||
end
|
||||
|
||||
def audit_formula_urls f
|
||||
problems = []
|
||||
|
||||
unless f.homepage =~ %r[^https?://]
|
||||
problems << " * The homepage should start with http or https."
|
||||
end
|
||||
|
||||
# Google Code homepages should end in a slash
|
||||
if f.homepage =~ %r[^https?://code\.google\.com/p/[^/]+[^/]$]
|
||||
problems << " * Google Code homepage should end with a slash."
|
||||
end
|
||||
|
||||
urls = [(f.stable.url rescue nil), (f.devel.url rescue nil), (f.head.url rescue nil)].reject {|p| p.nil?}
|
||||
|
||||
# Check GNU urls; doesn't apply to mirrors
|
||||
urls.each do |p|
|
||||
if p =~ %r[^(https?|ftp)://(.+)/gnu/]
|
||||
problems << " * \"ftpmirror.gnu.org\" is preferred for GNU software."
|
||||
end
|
||||
end
|
||||
|
||||
# the rest of the checks apply to mirrors as well
|
||||
mirrors = [(f.stable.mirrors rescue []), (f.devel.mirrors rescue [])].flatten.reject { |m| m.nil? }
|
||||
urls.concat mirrors.map { |m| m[:url] }
|
||||
|
||||
# Check SourceForge urls
|
||||
urls.each do |p|
|
||||
# Is it a filedownload (instead of svnroot)
|
||||
next if p =~ %r[/svnroot/]
|
||||
next if p =~ %r[svn\.sourceforge]
|
||||
|
||||
# Is it a sourceforge http(s) URL?
|
||||
next unless p =~ %r[^https?://.*\bsourceforge\.]
|
||||
|
||||
if p =~ /(\?|&)use_mirror=/
|
||||
problems << " * Update this url (don't use #{$1}use_mirror)."
|
||||
end
|
||||
|
||||
if p =~ /\/download$/
|
||||
problems << " * Update this url (don't use /download)."
|
||||
end
|
||||
|
||||
if p =~ %r[^http://prdownloads\.]
|
||||
problems << " * Update this url (don't use prdownloads)."
|
||||
end
|
||||
|
||||
if p =~ %r[^http://\w+\.dl\.]
|
||||
problems << " * Update this url (don't use specific dl mirrors)."
|
||||
end
|
||||
end
|
||||
|
||||
# Check for git:// urls; https:// is preferred.
|
||||
urls.each do |p|
|
||||
if p =~ %r[^git://github\.com/]
|
||||
problems << " * Use https:// URLs for accessing repositories on GitHub."
|
||||
end
|
||||
end
|
||||
|
||||
return problems
|
||||
end
|
||||
|
||||
def audit_formula_specs f
|
||||
problems = []
|
||||
|
||||
[:stable, :devel].each do |spec|
|
||||
s = f.send(spec)
|
||||
next if s.nil?
|
||||
|
||||
if s.version.to_s.empty?
|
||||
problems << " * invalid or missing #{spec} version"
|
||||
else
|
||||
version_text = s.version if s.explicit_version?
|
||||
version_url = Pathname.new(s.url).version
|
||||
if version_url == version_text
|
||||
problems << " * #{spec} version #{version_text} is redundant with version scanned from URL"
|
||||
end
|
||||
end
|
||||
|
||||
cksum = s.checksum
|
||||
next if cksum.nil?
|
||||
|
||||
len = case cksum.hash_type
|
||||
when :md5 then 32
|
||||
when :sha1 then 40
|
||||
when :sha256 then 64
|
||||
end
|
||||
|
||||
if cksum.empty?
|
||||
problems << " * #{cksum.hash_type} is empty"
|
||||
else
|
||||
problems << " * #{cksum.hash_type} should be #{len} characters" unless cksum.hexdigest.length == len
|
||||
problems << " * #{cksum.hash_type} contains invalid characters" unless cksum.hexdigest =~ /^[a-fA-F0-9]+$/
|
||||
problems << " * #{cksum.hash_type} should be lowercase" unless cksum.hexdigest == cksum.hexdigest.downcase
|
||||
end
|
||||
end
|
||||
|
||||
return problems
|
||||
end
|
||||
|
||||
def audit_formula_instance f
|
||||
problems = []
|
||||
|
||||
# Don't depend_on aliases; use full name
|
||||
aliases = Formula.aliases
|
||||
f.deps.select {|d| aliases.include? d}.each do |d|
|
||||
problems << " * Dep #{d} is an alias; switch to the real name."
|
||||
end
|
||||
|
||||
# Check for things we don't like to depend on.
|
||||
# We allow non-Homebrew installs whenever possible.
|
||||
f.deps.each do |d|
|
||||
begin
|
||||
dep_f = Formula.factory d
|
||||
rescue
|
||||
problems << " * Can't find dependency \"#{d}\"."
|
||||
end
|
||||
|
||||
case d.name
|
||||
when "git", "python", "ruby", "emacs", "mysql", "postgresql", "mercurial"
|
||||
problems << <<-EOS
|
||||
* Don't use #{d} as a dependency. We allow non-Homebrew
|
||||
#{d} installations.
|
||||
EOS
|
||||
when 'gfortran'
|
||||
problems << " * Use ENV.fortran during install instead of depends_on 'gfortran'"
|
||||
|
||||
when 'open-mpi', 'mpich2'
|
||||
problems << <<-EOS.undent
|
||||
* There are multiple conflicting ways to install MPI. Use a MPIDependency:
|
||||
depends_on MPIDependency.new(<lang list>)
|
||||
Where <lang list> is a comma delimited list that can include:
|
||||
:cc, :cxx, :f90, :f77
|
||||
EOS
|
||||
end
|
||||
end
|
||||
|
||||
return problems
|
||||
end
|
||||
|
||||
# Formula extensions for auditing
|
||||
class Formula
|
||||
def head_only?
|
||||
@head and @stable.nil?
|
||||
end
|
||||
|
||||
def formula_text
|
||||
File.open(@path, "r") { |afile| return afile.read }
|
||||
def text
|
||||
@text ||= FormulaText.new(@path)
|
||||
end
|
||||
end
|
||||
|
||||
module Homebrew extend self
|
||||
def audit
|
||||
errors = false
|
||||
class FormulaText
|
||||
def initialize path
|
||||
@text = path.open('r') { |f| f.read }
|
||||
end
|
||||
|
||||
brew_count = 0
|
||||
problem_count = 0
|
||||
def without_patch
|
||||
@text.split("__END__")[0].strip()
|
||||
end
|
||||
|
||||
ff.each do |f|
|
||||
# We need to do this in case the formula defines a patch that uses DATA.
|
||||
f.class.redefine_const :DATA, ""
|
||||
def has_DATA?
|
||||
/\bDATA\b/ =~ @text
|
||||
end
|
||||
|
||||
problems = []
|
||||
problems += [' * head-only formula'] if f.head_only?
|
||||
problems += audit_formula_instance f
|
||||
problems += audit_formula_urls f
|
||||
problems += audit_formula_patches f
|
||||
def has_END?
|
||||
/^__END__$/ =~ @text
|
||||
end
|
||||
|
||||
perms = File.stat(f.path).mode
|
||||
if perms.to_s(8) != "100644"
|
||||
problems << " * permissions wrong; chmod 644 #{f.path}"
|
||||
def has_trailing_newline?
|
||||
/.+\z/ =~ @text
|
||||
end
|
||||
end
|
||||
|
||||
class FormulaAuditor
|
||||
attr :f
|
||||
attr :text
|
||||
attr :problems, true
|
||||
|
||||
BUILD_TIME_DEPS = %W[
|
||||
autoconf
|
||||
automake
|
||||
boost-build
|
||||
bsdmake
|
||||
cmake
|
||||
imake
|
||||
libtool
|
||||
pkg-config
|
||||
scons
|
||||
smake
|
||||
]
|
||||
|
||||
def initialize f
|
||||
@f = f
|
||||
@problems = []
|
||||
@text = f.text.without_patch
|
||||
|
||||
# We need to do this in case the formula defines a patch that uses DATA.
|
||||
f.class.redefine_const :DATA, ""
|
||||
end
|
||||
|
||||
def audit_file
|
||||
unless f.path.stat.mode.to_s(8) == "100644"
|
||||
problem "Incorrect file permissions: chmod 644 #{f.path}"
|
||||
end
|
||||
|
||||
if f.text.has_DATA? and not f.text.has_END?
|
||||
problem "'DATA' was found, but no '__END__'"
|
||||
end
|
||||
|
||||
if f.text.has_END? and not f.text.has_DATA?
|
||||
problem "'__END__' was found, but 'DATA' is not used"
|
||||
end
|
||||
|
||||
if f.text.has_trailing_newline?
|
||||
problem "File should end with a newline"
|
||||
end
|
||||
end
|
||||
|
||||
def audit_deps
|
||||
problems = []
|
||||
|
||||
# Don't depend_on aliases; use full name
|
||||
aliases = Formula.aliases
|
||||
f.deps.select {|d| aliases.include? d}.each do |d|
|
||||
problem "Dependency #{d} is an alias; use the canonical name."
|
||||
end
|
||||
|
||||
# Check for things we don't like to depend on.
|
||||
# We allow non-Homebrew installs whenever possible.
|
||||
f.deps.each do |d|
|
||||
begin
|
||||
dep_f = Formula.factory d
|
||||
rescue
|
||||
problem "Can't find dependency \"#{d}\"."
|
||||
end
|
||||
|
||||
text = f.formula_text
|
||||
case d.name
|
||||
when "git", "python", "ruby", "emacs", "mysql", "postgresql", "mercurial"
|
||||
problem <<-EOS.undent
|
||||
Don't use #{d} as a dependency. We allow non-Homebrew
|
||||
#{d} installations.
|
||||
EOS
|
||||
when 'gfortran'
|
||||
problem "Use ENV.fortran during install instead of depends_on 'gfortran'"
|
||||
when 'open-mpi', 'mpich2'
|
||||
problem <<-EOS.undent
|
||||
There are multiple conflicting ways to install MPI. Use an MPIDependency:
|
||||
depends_on MPIDependency.new(<lang list>)
|
||||
Where <lang list> is a comma delimited list that can include:
|
||||
:cc, :cxx, :f90, :f77
|
||||
EOS
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
# DATA with no __END__
|
||||
if (text =~ /\bDATA\b/) and not (text =~ /^\s*__END__\s*$/)
|
||||
problems << " * 'DATA' was found, but no '__END__'"
|
||||
|
||||
def audit_urls
|
||||
unless f.homepage =~ %r[^https?://]
|
||||
problem "The homepage should start with http or https."
|
||||
end
|
||||
|
||||
# Google Code homepages should end in a slash
|
||||
if f.homepage =~ %r[^https?://code\.google\.com/p/[^/]+[^/]$]
|
||||
problem "Google Code homepage should end with a slash."
|
||||
end
|
||||
|
||||
urls = [(f.stable.url rescue nil), (f.devel.url rescue nil), (f.head.url rescue nil)].compact
|
||||
|
||||
# Check GNU urls; doesn't apply to mirrors
|
||||
if urls.any? { |p| p =~ %r[^(https?|ftp)://(.+)/gnu/] }
|
||||
problem "\"ftpmirror.gnu.org\" is preferred for GNU software."
|
||||
end
|
||||
|
||||
# the rest of the checks apply to mirrors as well
|
||||
urls.concat([(f.stable.mirrors rescue nil), (f.devel.mirrors rescue nil)].flatten.compact)
|
||||
|
||||
# Check SourceForge urls
|
||||
urls.each do |p|
|
||||
# Is it a filedownload (instead of svnroot)
|
||||
next if p =~ %r[/svnroot/]
|
||||
next if p =~ %r[svn\.sourceforge]
|
||||
|
||||
# Is it a sourceforge http(s) URL?
|
||||
next unless p =~ %r[^https?://.*\bsourceforge\.]
|
||||
|
||||
if p =~ /(\?|&)use_mirror=/
|
||||
problem "Update this url (don't use #{$1}use_mirror)."
|
||||
end
|
||||
|
||||
# files should end with a newline
|
||||
if text =~ /.+\z/
|
||||
problems << " * File should end with a newline"
|
||||
if p =~ /\/download$/
|
||||
problem "Update this url (don't use /download)."
|
||||
end
|
||||
|
||||
# Don't try remaining audits on text in __END__
|
||||
text_without_patch = (text.split("__END__")[0]).strip()
|
||||
if p =~ %r[^http://prdownloads\.]
|
||||
problem "Update this url (don't use prdownloads)."
|
||||
end
|
||||
|
||||
problems += audit_formula_text(f.name, text_without_patch)
|
||||
problems += audit_formula_specs(f)
|
||||
|
||||
unless problems.empty?
|
||||
errors = true
|
||||
puts "#{f.name}:"
|
||||
puts problems * "\n"
|
||||
puts
|
||||
brew_count += 1
|
||||
problem_count += problems.select{ |p| p.start_with? ' *' }.size
|
||||
if p =~ %r[^http://\w+\.dl\.]
|
||||
problem "Update this url (don't use specific dl mirrors)."
|
||||
end
|
||||
end
|
||||
|
||||
ofail "#{problem_count} problems in #{brew_count} brews" if errors
|
||||
# Check for git:// urls; https:// is preferred.
|
||||
if urls.any? { |p| p =~ %r[^git://github\.com/] }
|
||||
problem "Use https:// URLs for accessing GitHub repositories."
|
||||
end
|
||||
end
|
||||
|
||||
def audit_specs
|
||||
problem "Head-only (no stable download)" if f.head_only?
|
||||
|
||||
[:stable, :devel].each do |spec|
|
||||
s = f.send(spec)
|
||||
next if s.nil?
|
||||
|
||||
if s.version.to_s.empty?
|
||||
problem "Invalid or missing #{spec} version"
|
||||
else
|
||||
version_text = s.version if s.explicit_version?
|
||||
version_url = Pathname.new(s.url).version
|
||||
if version_url == version_text
|
||||
problem "#{spec} version #{version_text} is redundant with version scanned from URL"
|
||||
end
|
||||
end
|
||||
|
||||
cksum = s.checksum
|
||||
next if cksum.nil?
|
||||
|
||||
len = case cksum.hash_type
|
||||
when :md5 then 32
|
||||
when :sha1 then 40
|
||||
when :sha256 then 64
|
||||
end
|
||||
|
||||
if cksum.empty?
|
||||
problem "#{cksum.hash_type} is empty"
|
||||
else
|
||||
problem "#{cksum.hash_type} should be #{len} characters" unless cksum.hexdigest.length == len
|
||||
problem "#{cksum.hash_type} contains invalid characters" unless cksum.hexdigest =~ /^[a-fA-F0-9]+$/
|
||||
problem "#{cksum.hash_type} should be lowercase" unless cksum.hexdigest == cksum.hexdigest.downcase
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def audit_patches
|
||||
# Some formulae use ENV in patches, so set up an environment
|
||||
ENV.extend(HomebrewEnvExtension)
|
||||
ENV.setup_build_environment
|
||||
|
||||
Patches.new(f.patches).select { |p| p.external? }.each do |p|
|
||||
case p.url
|
||||
when %r[raw\.github\.com], %r[gist\.github\.com/raw]
|
||||
problem "Using raw GitHub URLs is not recommended:\n#{p.url}"
|
||||
when %r[macports/trunk]
|
||||
problem "MacPorts patches should specify a revision instead of trunk:\n#{p.url}"
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def audit_text
|
||||
if text =~ /<(Formula|AmazonWebServicesFormula|ScriptFileFormula|GithubGistFormula)/
|
||||
problem "Use a space in class inheritance: class Foo < #{$1}"
|
||||
end
|
||||
|
||||
# Commented-out cmake support from default template
|
||||
if (text =~ /# depends_on 'cmake'/) or (text =~ /# system "cmake/)
|
||||
problem "Commented cmake support found."
|
||||
end
|
||||
|
||||
# 2 (or more in an if block) spaces before depends_on, please
|
||||
if text =~ /^\ ?depends_on/
|
||||
problem "Check indentation of 'depends_on'."
|
||||
end
|
||||
|
||||
# build tools should be flagged properly
|
||||
if text =~ /depends_on ['"](#{BUILD_TIME_DEPS*'|'})['"]$/
|
||||
problem "#{$1} dependency should be \"depends_on '#{$1}' => :build\""
|
||||
end
|
||||
|
||||
# FileUtils is included in Formula
|
||||
if text =~ /FileUtils\.(\w+)/
|
||||
problem "Don't need 'FileUtils.' before #{$1}."
|
||||
end
|
||||
|
||||
# Check for long inreplace block vars
|
||||
if text =~ /inreplace .* do \|(.{2,})\|/
|
||||
problem "\"inreplace <filenames> do |s|\" is preferred over \"|#{$1}|\"."
|
||||
end
|
||||
|
||||
# Check for string interpolation of single values.
|
||||
if text =~ /(system|inreplace|gsub!|change_make_var!) .* ['"]#\{(\w+(\.\w+)?)\}['"]/
|
||||
problem "Don't need to interpolate \"#{$2}\" with #{$1}"
|
||||
end
|
||||
|
||||
# Check for string concatenation; prefer interpolation
|
||||
if text =~ /(#\{\w+\s*\+\s*['"][^}]+\})/
|
||||
problem "Try not to concatenate paths in string interpolation:\n #{$1}"
|
||||
end
|
||||
|
||||
# Prefer formula path shortcuts in Pathname+
|
||||
if text =~ %r{\(\s*(prefix\s*\+\s*(['"])(bin|include|libexec|lib|sbin|share)[/'"])}
|
||||
problem "\"(#{$1}...#{$2})\" should be \"(#{$3}+...)\""
|
||||
end
|
||||
|
||||
if text =~ %r[((man)\s*\+\s*(['"])(man[1-8])(['"]))]
|
||||
problem "\"#{$1}\" should be \"#{$4}\""
|
||||
end
|
||||
|
||||
# Prefer formula path shortcuts in strings
|
||||
if text =~ %r[(\#\{prefix\}/(bin|include|libexec|lib|sbin|share))]
|
||||
problem "\"#{$1}\" should be \"\#{#{$2}}\""
|
||||
end
|
||||
|
||||
if text =~ %r[((\#\{prefix\}/share/man/|\#\{man\}/)(man[1-8]))]
|
||||
problem "\"#{$1}\" should be \"\#{#{$3}}\""
|
||||
end
|
||||
|
||||
if text =~ %r[((\#\{share\}/(man)))[/'"]]
|
||||
problem "\"#{$1}\" should be \"\#{#{$3}}\""
|
||||
end
|
||||
|
||||
if text =~ %r[(\#\{prefix\}/share/(info|man))]
|
||||
problem "\"#{$1}\" should be \"\#{#{$2}}\""
|
||||
end
|
||||
|
||||
# Commented-out depends_on
|
||||
if text =~ /#\s*depends_on\s+(.+)\s*$/
|
||||
problem "Commented-out dep #{$1}."
|
||||
end
|
||||
|
||||
# No trailing whitespace, please
|
||||
if text =~ /(\t|[ ])+$/
|
||||
problem "Trailing whitespace was found."
|
||||
end
|
||||
|
||||
if text =~ /if\s+ARGV\.include\?\s+'--(HEAD|devel)'/
|
||||
problem "Use \"if ARGV.build_#{$1.downcase}?\" instead"
|
||||
end
|
||||
|
||||
if text =~ /make && make/
|
||||
problem "Use separate make calls."
|
||||
end
|
||||
|
||||
if text =~ /^[ ]*\t/
|
||||
problem "Use spaces instead of tabs for indentation"
|
||||
end
|
||||
|
||||
# xcodebuild should specify SYMROOT
|
||||
if text =~ /system\s+['"]xcodebuild/ and not text =~ /SYMROOT=/
|
||||
problem "xcodebuild should be passed an explicit \"SYMROOT\""
|
||||
end
|
||||
|
||||
# using ARGV.flag? for formula options is generally a bad thing
|
||||
if text =~ /ARGV\.flag\?/
|
||||
problem "Use 'ARGV.include?' instead of 'ARGV.flag?'"
|
||||
end
|
||||
|
||||
# Avoid hard-coding compilers
|
||||
if text =~ %r[(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?(gcc|llvm-gcc|clang)['" ]]
|
||||
problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{$3}\""
|
||||
end
|
||||
|
||||
if text =~ %r[(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?((g|llvm-g|clang)\+\+)['" ]]
|
||||
problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{$3}\""
|
||||
end
|
||||
|
||||
if text =~ /system\s+['"](env|export)/
|
||||
problem "Use ENV instead of invoking '#{$1}' to modify the environment"
|
||||
end
|
||||
|
||||
if text =~ /version == ['"]HEAD['"]/
|
||||
problem "Use 'ARGV.build_head?' instead of inspecting 'version'"
|
||||
end
|
||||
end
|
||||
|
||||
def audit
|
||||
audit_file
|
||||
audit_specs
|
||||
audit_urls
|
||||
audit_deps
|
||||
audit_patches
|
||||
audit_text
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def problem p
|
||||
@problems << p
|
||||
end
|
||||
end
|
||||
|
||||
@ -1,4 +1,6 @@
|
||||
class Patches
|
||||
include Enumerable
|
||||
|
||||
# The patches defined in a formula and the DATA from that file
|
||||
def initialize patches
|
||||
@patches = []
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user