diff --git a/Library/Homebrew/cmd/audit.rb b/Library/Homebrew/cmd/audit.rb index af7ddf890c..bb3a538a47 100755 --- a/Library/Homebrew/cmd/audit.rb +++ b/Library/Homebrew/cmd/audit.rb @@ -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 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() - Where 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() + Where 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 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 diff --git a/Library/Homebrew/patches.rb b/Library/Homebrew/patches.rb index 6c71ac190a..9abea188f6 100644 --- a/Library/Homebrew/patches.rb +++ b/Library/Homebrew/patches.rb @@ -1,4 +1,6 @@ class Patches + include Enumerable + # The patches defined in a formula and the DATA from that file def initialize patches @patches = []