Merge pull request #7371 from MikeMcQuaid/audit-to-rubocop

dev-cmd/audit: add TODOs for RuboCop migrations.

Library/Homebrew/dev-cmd/audit.rb

@@ -240,39 +240,17 @@ module Homebrew
     end
 
     def audit_file
-      actual_mode = formula.path.stat.mode
+      # TODO: check could be in RuboCop
-      # Check that the file is world-readable.
-      if actual_mode & 0444 != 0444
-        problem format("Incorrect file permissions (%03<actual>o): chmod %<wanted>s %<path>s",
-                       actual: actual_mode & 0777,
-                       wanted: "+r",
-                       path:   formula.path)
-      end
-      # Check that the file is user-writeable.
-      if actual_mode & 0200 != 0200
-        problem format("Incorrect file permissions (%03<actual>o): chmod %<wanted>s %<path>s",
-                       actual: actual_mode & 0777,
-                       wanted: "u+w",
-                       path:   formula.path)
-      end
-      # Check that the file is *not* other-writeable.
-      if actual_mode & 0002 == 002
-        problem format("Incorrect file permissions (%03<actual>o): chmod %<wanted>s %<path>s",
-                       actual: actual_mode & 0777,
-                       wanted: "o-w",
-                       path:   formula.path)
-      end
-
       problem "'DATA' was found, but no '__END__'" if text.data? && !text.end?
 
+      # TODO: check could be in RuboCop
       problem "'__END__' was found, but 'DATA' is not used" if text.end? && !text.data?
 
+      # TODO: check could be in RuboCop
       if text.to_s.match?(/inreplace [^\n]* do [^\n]*\n[^\n]*\.gsub![^\n]*\n\ *end/m)
         problem "'inreplace ... do' was used for a single substitution (use the non-block form instead)."
       end
 
-      problem "File should end with a newline" unless text.trailing_newline?
-
       if formula.core_formula? && @versioned_formula
         unversioned_formula = begin
           # build this ourselves as we want e.g. homebrew/core to be present
@@ -479,6 +457,7 @@ module Homebrew
       first_word = reason.split.first
 
       if reason =~ /\A[A-Z]/ && !reason.start_with?(*whitelist)
+        # TODO: check could be in RuboCop
         problem <<~EOS
           '#{first_word}' from the keg_only reason should be '#{first_word.downcase}'.
         EOS
@@ -486,6 +465,7 @@ module Homebrew
 
       return unless reason.end_with?(".")
 
+      # TODO: check could be in RuboCop
       problem "keg_only reason should not end with a period."
     end
 
@@ -934,58 +914,70 @@ module Homebrew
     def line_problems(line, _lineno)
       # Check for string interpolation of single values.
       if line =~ /(system|inreplace|gsub!|change_make_var!).*[ ,]"#\{([\w.]+)\}"/
+        # TODO: check could be in RuboCop
         problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}"
       end
 
       # Check for string concatenation; prefer interpolation
       if line =~ /(#\{\w+\s*\+\s*['"][^}]+\})/
+        # TODO: check could be in RuboCop
         problem "Try not to concatenate paths in string interpolation:\n   #{Regexp.last_match(1)}"
       end
 
       # Prefer formula path shortcuts in Pathname+
       if line =~ %r{\(\s*(prefix\s*\+\s*(['"])(bin|include|libexec|lib|sbin|share|Frameworks)[/'"])}
+        # TODO: check could be in RuboCop
         problem(
           "\"(#{Regexp.last_match(1)}...#{Regexp.last_match(2)})\" should" \
           " be \"(#{Regexp.last_match(3).downcase}+...)\"",
         )
       end
 
+      # TODO: check could be in RuboCop
       problem "Use separate make calls" if line.include?("make && make")
 
       if line =~ /JAVA_HOME/i &&
          [formula.name, *formula.deps.map(&:name)].none? { |name| name.match?(/^openjdk(@|$)/) } &&
          formula.requirements.none? { |req| req.is_a?(JavaRequirement) }
+        # TODO: check could be in RuboCop
         problem "Use `depends_on :java` to set JAVA_HOME"
       end
 
       return unless @strict
 
+      # TODO: check could be in RuboCop
       problem "`env :userpaths` in formulae is deprecated" if line.include?("env :userpaths")
 
       if line =~ /system ((["'])[^"' ]*(?:\s[^"' ]*)+\2)/
         bad_system = Regexp.last_match(1)
         unless %w[| < > & ; *].any? { |c| bad_system.include? c }
           good_system = bad_system.gsub(" ", "\", \"")
+          # TODO: check could be in RuboCop
           problem "Use `system #{good_system}` instead of `system #{bad_system}` "
         end
       end
 
+      # TODO: check could be in RuboCop
       problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/
 
       if line.match?(%r{#\{share\}/#{Regexp.escape(formula.name)}[/'"]})
+        # TODO: check could be in RuboCop
         problem "Use \#{pkgshare} instead of \#{share}/#{formula.name}"
       end
 
       if !@core_tap && line =~ /depends_on .+ if build\.with(out)?\?\(?["']\w+["']\)?/
+        # TODO: check could be in RuboCop
         problem "`Use :optional` or `:recommended` instead of `#{Regexp.last_match(0)}`"
       end
 
       if line =~ %r{share(\s*[/+]\s*)(['"])#{Regexp.escape(formula.name)}(?:\2|/)}
+        # TODO: check could be in RuboCop
         problem "Use pkgshare instead of (share#{Regexp.last_match(1)}\"#{formula.name}\")"
       end
 
       return unless @core_tap
 
+      # TODO: check could be in RuboCop
       problem "`env :std` in homebrew/core formulae is deprecated" if line.include?("env :std")
     end
 
@@ -1085,6 +1077,7 @@ module Homebrew
       if version.nil?
         problem "missing version"
       elsif version.blank?
+        # TODO: check could be in RuboCop
         problem "version is set to an empty string"
       elsif !version.detected_from_url?
         version_text = version
@@ -1094,21 +1087,25 @@ module Homebrew
         end
       end
 
+      # TODO: check could be in RuboCop
       problem "version #{version} should not have a leading 'v'" if version.to_s.start_with?("v")
 
       return unless version.to_s.match?(/_\d+$/)
 
+      # TODO: check could be in RuboCop
       problem "version #{version} should not end with an underline and a number"
     end
 
     def audit_download_strategy
       if url =~ %r{^(cvs|bzr|hg|fossil)://} || url =~ %r{^(svn)\+http://}
+        # TODO: check could be in RuboCop
         problem "Use of the #{$&} scheme is deprecated, pass `:using => :#{Regexp.last_match(1)}` instead"
       end
 
       url_strategy = DownloadStrategyDetector.detect(url)
 
       if using == :git || url_strategy == GitDownloadStrategy
+        # TODO: check could be in RuboCop
         problem "Git should specify :revision when a :tag is specified." if specs[:tag] && !specs[:revision]
       end
 

Library/Homebrew/rubocops.rb

@@ -19,5 +19,6 @@ require "rubocops/urls"
 require "rubocops/lines"
 require "rubocops/class"
 require "rubocops/uses_from_macos"
+require "rubocops/files"
 
 require "rubocops/rubocop-cask"

Library/Homebrew/rubocops/files.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require "rubocops/extend/formula"
+
+module RuboCop
+  module Cop
+    module FormulaAudit
+      class Files < FormulaCop
+        def audit_formula(node, _class_node, _parent_class_node, _body_node)
+          return unless file_path
+
+          offending_node(node)
+          actual_mode = File.stat(file_path).mode
+          # Check that the file is world-readable.
+          if actual_mode & 0444 != 0444
+            problem format("Incorrect file permissions (%03<actual>o): chmod %<wanted>s %<path>s",
+                           actual: actual_mode & 0777,
+                           wanted: "+r",
+                           path:   file_path)
+          end
+          # Check that the file is user-writeable.
+          if actual_mode & 0200 != 0200
+            problem format("Incorrect file permissions (%03<actual>o): chmod %<wanted>s %<path>s",
+                           actual: actual_mode & 0777,
+                           wanted: "u+w",
+                           path:   file_path)
+          end
+          # Check that the file is *not* other-writeable.
+          return if actual_mode & 0002 != 002
+
+          problem format("Incorrect file permissions (%03<actual>o): chmod %<wanted>s %<path>s",
+                         actual: actual_mode & 0777,
+                         wanted: "o-w",
+                         path:   file_path)
+        end
+      end
+    end
+  end
+end

Library/Homebrew/test/.rubocop_todo.yml

@@ -45,6 +45,7 @@ RSpec/FilePath:
     - 'rubocops/components_redundancy_spec.rb'
     - 'rubocops/conflicts_spec.rb'
     - 'rubocops/dependency_order_spec.rb'
+    - 'rubocops/files_spec.rb'
     - 'rubocops/homepage_spec.rb'
     - 'rubocops/options_spec.rb'
     - 'rubocops/patches_spec.rb'

Library/Homebrew/test/dev-cmd/audit_spec.rb

@@ -96,52 +96,6 @@ module Homebrew
     end
 
     describe "#audit_file" do
-      specify "file permissions" do
-        allow(File).to receive(:umask).and_return(022)
-
-        fa = formula_auditor "foo", <<~RUBY
-          class Foo < Formula
-            url "https://brew.sh/foo-1.0.tgz"
-          end
-        RUBY
-
-        path = fa.formula.path
-
-        path.chmod 0600
-        fa.audit_file
-        expect(fa.problems)
-          .to eq([
-                   "Incorrect file permissions (600): chmod +r #{path}",
-                 ])
-        fa.problems.clear
-
-        path.chmod 0444
-        fa.audit_file
-        expect(fa.problems)
-          .to eq([
-                   "Incorrect file permissions (444): chmod u+w #{path}",
-                 ])
-        fa.problems.clear
-
-        path.chmod 0646
-        fa.audit_file
-        expect(fa.problems)
-          .to eq([
-                   "Incorrect file permissions (646): chmod o-w #{path}",
-                 ])
-        fa.problems.clear
-
-        path.chmod 0002
-        fa.audit_file
-        expect(fa.problems)
-          .to eq([
-                   "Incorrect file permissions (002): chmod +r #{path}",
-                   "Incorrect file permissions (002): chmod u+w #{path}",
-                   "Incorrect file permissions (002): chmod o-w #{path}",
-                 ])
-        fa.problems.clear
-      end
-
       specify "DATA but no __END__" do
         fa = formula_auditor "foo", <<~RUBY
           class Foo < Formula
@@ -167,13 +121,6 @@ module Homebrew
         expect(fa.problems).to eq(["'__END__' was found, but 'DATA' is not used"])
       end
 
-      specify "no trailing newline" do
-        fa = formula_auditor "foo", 'class Foo<Formula; url "file:///foo-1.0.tgz";end'
-
-        fa.audit_file
-        expect(fa.problems).to eq(["File should end with a newline"])
-      end
-
       specify "no issue" do
         fa = formula_auditor "foo", <<~RUBY
           class Foo < Formula

Library/Homebrew/test/rubocops/files_spec.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require "rubocops/files"
+
+describe RuboCop::Cop::FormulaAudit::Files do
+  subject(:cop) { described_class.new }
+
+  context "When auditing files" do
+    it "when the permissions are invalid" do
+      filename = Formulary.core_path("test_formula")
+      File.open(filename, "w") do |file|
+        FileUtils.chmod "-rwx", filename
+
+        expect_offense(<<~RUBY, file)
+          class Foo < Formula
+          ^^^^^^^^^^^^^^^^^^^ Incorrect file permissions (000): chmod +r #{filename}
+            url "https://brew.sh/foo-1.0.tgz"
+          end
+        RUBY
+      end
+    end
+  end
+end