New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev-cmd/audit: add TODOs for RuboCop migrations. #7371
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -240,39 +240,17 @@ def audit_style | |
end | ||
|
||
def audit_file | ||
actual_mode = formula.path.stat.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: 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 | ||
|
||
# TODO: check could be in RuboCop | ||
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,13 +457,15 @@ def audit_keg_only | |
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 | ||
end | ||
|
||
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 @@ def audit_lines | |
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 @@ def audit_version | |
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 @@ def audit_version | |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MikeMcQuaid could you give me a hint on how to access There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (there may well be other incorrect ones as I am but a fallible human, feel free to shout/ask if you see any others) |
||
# TODO: check could be in RuboCop | ||
problem "Git should specify :revision when a :tag is specified." if specs[:tag] && !specs[:revision] | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to give the those checks a try but I'm not sure how one would access the necessary
text
contents inRubocop
. Can you give me hint please @MikeMcQuaid ?And would it belong in
patches.rb
orfiles.rb
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say probably
patches.rb
. If you check out the definitions inbrew/Library/Homebrew/dev-cmd/audit.rb
Lines 167 to 173 in 2e801c3
rubocop
; there may also be a way to only get the__END__
section from the file fromrubocop
(orparser.rb
that it uses).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my limited knowledge about the codebase and tools I haven't figured it out for now.
I don't see a straight forward solution to read the
__END__
section from the things available inaudit_formula
inRuboCop
, i.e. thebody
only contains all the stuff that makes up theclass
, but not the stuff after it (i.e.__END__
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out: https://www.rubydoc.info/gems/rubocop/0.46.0/RuboCop/ProcessedSource#lines-instance_method
https://www.honeybadger.io/blog/data-and-end-in-ruby/
rubocop/rubocop#1541
As one of them may help. It looks like you may need to look for usage of
patch :DATA
, get the filename and then do a full read of the file looking for^__END__$
. Make sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late follow-up :)
I'm fully back at work and didn't have a time to look at this again so far and probably won't have in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, someone else can pick it up!