-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
audit: detect partial component order compliance #807
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 |
---|---|---|
|
@@ -72,7 +72,7 @@ def audit | |
fa.audit | ||
|
||
next if fa.problems.empty? | ||
|
||
fa.problems | ||
formula_count += 1 | ||
problem_count += fa.problems.size | ||
problem_lines = fa.problems.map { |p| "* #{p.chomp.gsub("\n", "\n ")}" } | ||
|
@@ -121,10 +121,15 @@ def include?(s) | |
@text.include? s | ||
end | ||
|
||
def line_number(regex) | ||
index = @lines.index { |line| line =~ regex } | ||
def line_number(regex, skip = 0) | ||
index = @lines.drop(skip).index { |line| line =~ regex } | ||
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. The following will avoid constructing an intermediate array with index = @lines.each_with_index.find_index { |line, index| index >= skip && line =~ regex } (But if you prefer to stick with your version, it has a bug as 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. I add offset back in later which is why the code currently works, but absolute line number is probably better. |
||
index ? index + 1 : nil | ||
end | ||
|
||
def reverse_line_number(regex) | ||
index = @lines.reverse.index { |line| line =~ regex } | ||
index ? @lines.count - index : nil | ||
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. The following should also do the trick and avoids building and immediately discarding a new array: index = @lines.rindex { |line| line =~ regex }
index ? index + 1 : nil |
||
end | ||
end | ||
|
||
class FormulaAuditor | ||
|
@@ -171,35 +176,14 @@ def audit_style | |
end | ||
end | ||
|
||
def audit_file | ||
# Under normal circumstances (umask 0022), we expect a file mode of 644. If | ||
# the user's umask is more restrictive, respect that by masking out the | ||
# corresponding bits. (The also included 0100000 flag means regular file.) | ||
wanted_mode = 0100644 & ~File.umask | ||
actual_mode = formula.path.stat.mode | ||
unless actual_mode == wanted_mode | ||
problem format("Incorrect file permissions (%03o): chmod %03o %s", | ||
actual_mode & 0777, wanted_mode & 0777, formula.path) | ||
end | ||
|
||
if text.has_DATA? && !text.has_END? | ||
problem "'DATA' was found, but no '__END__'" | ||
end | ||
|
||
if text.has_END? && !text.has_DATA? | ||
problem "'__END__' was found, but 'DATA' is not used" | ||
end | ||
|
||
if text =~ /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 | ||
|
||
unless text.has_trailing_newline? | ||
problem "File should end with a newline" | ||
end | ||
|
||
return unless @strict | ||
def component_problem(before, after, offset = 0) | ||
problem "`#{before[1]}` (line #{before[0] + offset}) should be put before `#{after[1]}` (line #{after[0] + offset})" | ||
end | ||
|
||
# scan in the reverse direction for remaining problems but report problems | ||
# in the forward direction so that contributors don't reverse the order of | ||
# lines in the file simply by following instructions | ||
def audit_components(reverse = true, previous_pair = nil) | ||
component_list = [ | ||
[/^ include Language::/, "include directive"], | ||
[/^ desc ["'][\S\ ]+["']/, "desc"], | ||
|
@@ -226,17 +210,67 @@ def audit_file | |
[/^ (plist_options|def plist)/, "plist block"], | ||
[/^ test do/, "test block"], | ||
] | ||
|
||
if previous_pair | ||
previous_before = previous_pair[0] | ||
previous_after = previous_pair[1] | ||
end | ||
offset = (previous_after && previous_after[0] && previous_after[0] >= 1) ? previous_after[0] - 1 : 0 | ||
present = component_list.map do |regex, name| | ||
lineno = text.line_number regex | ||
lineno = if reverse | ||
text.reverse_line_number regex | ||
else | ||
text.line_number regex, offset | ||
end | ||
next unless lineno | ||
[lineno, name] | ||
end.compact | ||
no_problem = true | ||
present.each_cons(2) do |c1, c2| | ||
unless c1[0] < c2[0] | ||
problem "`#{c1[1]}` (line #{c1[0]}) should be put before `#{c2[1]}` (line #{c2[0]})" | ||
if reverse | ||
# scan in the forward direction from the offset | ||
audit_components(false, [c1, c2]) if c1[0] > c2[0] # at least one more offense | ||
elsif c1[0] > c2[0] && (offset == 0 || previous_pair.nil? || (c1[0] + offset) != previous_before[0] || (c2[0] + offset) != previous_after[0]) | ||
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. I'm afraid I don't understand this condition. Can it be modified so that it is easier to parse and understand? 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. It should be simpler to understand when the offsets don't have to be added back in here, but it's just saying, current found problem != previous pair. |
||
component_problem c1, c2, offset | ||
no_problem = false | ||
end | ||
end | ||
if no_problem && previous_pair | ||
component_problem previous_before, previous_after | ||
end | ||
present | ||
end | ||
|
||
def audit_file | ||
# Under normal circumstances (umask 0022), we expect a file mode of 644. If | ||
# the user's umask is more restrictive, respect that by masking out the | ||
# corresponding bits. (The also included 0100000 flag means regular file.) | ||
wanted_mode = 0100644 & ~File.umask | ||
actual_mode = formula.path.stat.mode | ||
unless actual_mode == wanted_mode | ||
problem format("Incorrect file permissions (%03o): chmod %03o %s", | ||
actual_mode & 0777, wanted_mode & 0777, formula.path) | ||
end | ||
|
||
if text.has_DATA? && !text.has_END? | ||
problem "'DATA' was found, but no '__END__'" | ||
end | ||
|
||
if text.has_END? && !text.has_DATA? | ||
problem "'__END__' was found, but 'DATA' is not used" | ||
end | ||
|
||
if text =~ /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 | ||
|
||
unless text.has_trailing_newline? | ||
problem "File should end with a newline" | ||
end | ||
|
||
return unless @strict | ||
|
||
present = audit_components | ||
|
||
present.map!(&:last) | ||
if present.include?("stable block") | ||
%w[url checksum mirror].each do |component| | ||
|
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.
What does this do? This looks like a no-op to me.
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.
yeah that's dead code. it used to be fa.problems.uniq! which is no longer needed. thanks for spotting it.