Skip to content

Commit

Permalink
audit: make audit_revision_and_version_scheme faster.
Browse files Browse the repository at this point in the history 
This is really, really slow at the moment for a few reasons:
- it goes through the list of revisions twice
- it checks many more revisions than it needs to

Even after these improvements it's still by far the slowest audit so
am also making it a `--git` only audit.

Additionally, to further improve default `brew audit` performance do not
run `brew style` checks when doing `brew audit` with no arguments.

`brew style` can be run quickly and efficiently on all of a tap (and is
cached) so no need to duplicate it here.
  • Loading branch information
@MikeMcQuaid
MikeMcQuaid committed Jun 8, 2020
1 parent 82e3dea commit 2a94d38
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 141 deletions.
149 changes: 74 additions & 75 deletions Library/Homebrew/dev-cmd/audit.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ def audit_args
Check <formula> for Homebrew coding style violations. This should be run before
submitting a new formula. If no <formula> are provided, check all locally
available formulae. Will exit with a non-zero status if any errors are
found, which can be useful for implementing pre-commit hooks.
available formulae and skip style checks. Will exit with a non-zero status if any
errors are found.
EOS
switch "--strict",
description: "Run additional, stricter style checks."
switch "--git",
description: "Run additional, slower style checks that navigate the Git repository."
switch "--online",
description: "Run additional, slower style checks that require a network connection."
switch "--new-formula",
Expand Down Expand Up @@ -82,17 +84,14 @@ def audit
new_formula = args.new_formula?
strict = new_formula || args.strict?
online = new_formula || args.online?
git = args.git?
skip_style = args.skip_style? || args.no_named?

ENV.activate_extensions!
ENV.setup_build_environment

if args.no_named?
audit_formulae = Formula
style_files = Tap.map(&:formula_dir)
else
audit_formulae = args.resolved_formulae
style_files = args.formulae_paths
end
audit_formulae = args.no_named? ? Formula : args.resolved_formulae
style_files = args.formulae_paths unless skip_style

only_cops = args.only_cops
except_cops = args.except_cops
Expand All @@ -109,13 +108,20 @@ def audit
end

# Check style in a single batch run up front for performance
style_results = Style.check_style_json(style_files, options) unless args.skip_style?
style_results = Style.check_style_json(style_files, options) if style_files

new_formula_problem_lines = []
audit_formulae.sort.each do |f|
only = only_cops ? ["style"] : args.only
options = { new_formula: new_formula, strict: strict, online: online, only: only, except: args.except }
options[:style_offenses] = style_results.file_offenses(f.path) unless args.skip_style?
options = {
new_formula: new_formula,
strict: strict,
online: online,
git: git,
only: only,
except: args.except,
}
options[:style_offenses] = style_results.file_offenses(f.path) if style_results
options[:display_cop_names] = args.display_cop_names?

fa = FormulaAuditor.new(f, options)
Expand All @@ -126,7 +132,7 @@ def audit
formula_count += 1
problem_count += fa.problems.size
problem_lines = format_problem_lines(fa.problems)
corrected_problem_count = options[:style_offenses].count(&:corrected?) unless args.skip_style?
corrected_problem_count = options[:style_offenses].count(&:corrected?) if options[:style_offenses]
new_formula_problem_lines = format_problem_lines(fa.new_formula_problems)
if args.display_filename?
puts problem_lines.map { |s| "#{f.path}: #{s}" }
Expand Down Expand Up @@ -205,6 +211,7 @@ def initialize(formula, options = {})
@new_formula = options[:new_formula] && !@versioned_formula
@strict = options[:strict]
@online = options[:online]
@git = options[:git]
@display_cop_names = options[:display_cop_names]
@only = options[:only]
@except = options[:except]
Expand Down Expand Up @@ -709,86 +716,78 @@ def audit_specs
end

def audit_revision_and_version_scheme
return unless @git
return unless formula.tap # skip formula not from core or any taps
return unless formula.tap.git? # git log is required
return if @new_formula
return if formula.stable.blank?

fv = FormulaVersions.new(formula)

previous_version_and_checksum = fv.previous_version_and_checksum("origin/master")
[:stable, :devel].each do |spec_sym|
next unless spec = formula.send(spec_sym)
next unless previous_version_and_checksum[spec_sym][:version] == spec.version
next if previous_version_and_checksum[spec_sym][:checksum] == spec.checksum
current_version = formula.stable.version
current_checksum = formula.stable.checksum
current_version_scheme = formula.version_scheme
current_revision = formula.revision

problem(
"#{spec_sym}: sha256 changed without the version also changing; " \
"please create an issue upstream to rule out malicious " \
"circumstances and to find out why the file changed.",
)
end
previous_version = nil
previous_checksum = nil
previous_version_scheme = nil
previous_revision = nil

attributes = [:revision, :version_scheme]
attributes_map = fv.version_attributes_map(attributes, "origin/master")
newest_committed_version = nil
newest_committed_revision = nil

current_version_scheme = formula.version_scheme
[:stable, :devel].each do |spec|
spec_version_scheme_map = attributes_map[:version_scheme][spec]
next if spec_version_scheme_map.empty?

version_schemes = spec_version_scheme_map.values.flatten
max_version_scheme = version_schemes.max
max_version = spec_version_scheme_map.select do |_, version_scheme|
version_scheme.first == max_version_scheme
end.keys.max

if max_version_scheme && current_version_scheme < max_version_scheme
problem "version_scheme should not decrease (from #{max_version_scheme} to #{current_version_scheme})"
end
fv.rev_list("origin/master") do |rev|
fv.formula_at_revision(rev) do |f|
stable = f.stable
next if stable.blank?

if max_version_scheme && current_version_scheme >= max_version_scheme &&
current_version_scheme > 1 &&
!version_schemes.include?(current_version_scheme - 1)
problem "version_schemes should only increment by 1"
end
previous_version = stable.version
previous_checksum = stable.checksum
previous_version_scheme = f.version_scheme
previous_revision = f.revision

formula_spec = formula.send(spec)
next unless formula_spec
newest_committed_version ||= previous_version
newest_committed_revision ||= previous_revision
end

spec_version = formula_spec.version
next unless max_version
next if spec_version >= max_version
break if previous_version && current_version != previous_version
end

above_max_version_scheme = current_version_scheme > max_version_scheme
map_includes_version = spec_version_scheme_map.key?(spec_version)
next if !current_version_scheme.zero? &&
(above_max_version_scheme || map_includes_version)
if current_version == previous_version &&
current_checksum != previous_checksum
problem(
"stable sha256 changed without the version also changing; " \
"please create an issue upstream to rule out malicious " \
"circumstances and to find out why the file changed.",
)
end

problem "#{spec} version should not decrease (from #{max_version} to #{spec_version})"
if !newest_committed_version.nil? &&
current_version < newest_committed_version &&
current_version_scheme == previous_version_scheme
problem "stable version should not decrease (from #{newest_committed_version} to #{current_version})"
end

current_revision = formula.revision
revision_map = attributes_map[:revision][:stable]
if formula.stable && !revision_map.empty?
stable_revisions = revision_map[formula.stable.version]
stable_revisions ||= []
max_revision = stable_revisions.max || 0

if current_revision < max_revision
problem "revision should not decrease (from #{max_revision} to #{current_revision})"
unless previous_version_scheme.nil?
if current_version_scheme < previous_version_scheme
problem "version_scheme should not decrease (from #{previous_version_scheme} " \
"to #{current_version_scheme})"
elsif current_version_scheme > (previous_version_scheme + 1)
problem "version_schemes should only increment by 1"
end
end

stable_revisions -= [formula.revision]
if !current_revision.zero? && stable_revisions.empty? &&
revision_map.keys.length > 1
problem "'revision #{formula.revision}' should be removed"
elsif current_revision > 1 &&
current_revision != max_revision &&
!stable_revisions.include?(current_revision - 1)
problem "revisions should only increment by 1"
end
elsif !current_revision.zero? # head/devel-only formula
if previous_version != newest_committed_version &&
!current_revision.zero? &&
current_revision == newest_committed_revision &&
current_revision == previous_revision
problem "'revision #{current_revision}' should be removed"
elsif current_version == previous_version &&
!previous_revision.nil? &&
current_revision < previous_revision
problem "revision should not decrease (from #{previous_revision} to #{current_revision})"
elsif current_revision > (newest_committed_revision + 1)
problem "revisions should only increment by 1"
end
end

Expand Down
62 changes: 0 additions & 62 deletions Library/Homebrew/formula_versions.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,66 +65,4 @@ def bottle_version_map(branch)
end
map
end

def previous_version_and_checksum(branch)
map = {}

rev_list(branch) do |rev|
formula_at_revision(rev) do |f|
[:stable, :devel].each do |spec_sym|
next unless spec = f.send(spec_sym)

map[spec_sym] ||= { version: spec.version, checksum: spec.checksum }
end
end

break if map[:stable] || map[:devel]
end

map[:stable] ||= {}
map[:devel] ||= {}

map
end

def version_attributes_map(attributes, branch)
attributes_map = {}
return attributes_map if attributes.empty?

attributes.each do |attribute|
attributes_map[attribute] ||= {
stable: {},
devel: {},
}
end

stable_versions_seen = 0
rev_list(branch) do |rev|
formula_at_revision(rev) do |f|
attributes.each do |attribute|
map = attributes_map[attribute]
set_attribute_map(map, f, attribute)

stable_keys_length = (map[:stable].keys + [f.version]).uniq.length
stable_versions_seen = [stable_versions_seen, stable_keys_length].max
end
end
break if stable_versions_seen > MAX_VERSIONS_DEPTH
end

attributes_map
end

private

def set_attribute_map(map, f, attribute)
if f.stable
map[:stable][f.stable.version] ||= []
map[:stable][f.stable.version] << f.send(attribute)
end
return unless f.devel

map[:devel][f.devel.version] ||= []
map[:devel][f.devel.version] << f.send(attribute)
end
end
2 changes: 1 addition & 1 deletion Library/Homebrew/test/dev-cmd/audit_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ class Foo < Formula

describe "#audit_revision_and_version_scheme" do
subject {
fa = described_class.new(Formulary.factory(formula_path))
fa = described_class.new(Formulary.factory(formula_path), git: true)
fa.audit_revision_and_version_scheme
fa.problems.first
}
Expand Down
6 changes: 4 additions & 2 deletions docs/Manpage.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,11 +634,13 @@ Homebrew/homebrew-cask (if tapped) to standard output.

Check *`formula`* for Homebrew coding style violations. This should be run before
submitting a new formula. If no *`formula`* are provided, check all locally
available formulae. Will exit with a non-zero status if any errors are found,
which can be useful for implementing pre-commit hooks.
available formulae and skip style checks. Will exit with a non-zero status if
any errors are found.

* `--strict`:
Run additional, stricter style checks.
* `--git`:
Run additional, slower style checks that navigate the Git repository.
* `--online`:
Run additional, slower style checks that require a network connection.
* `--new-formula`:
Expand Down
6 changes: 5 additions & 1 deletion manpages/brew.1
View file
Original file line number Diff line number Diff line change
Expand Up @@ -801,13 +801,17 @@ Print the version numbers of Homebrew, Homebrew/homebrew\-core and Homebrew/home
.SH "DEVELOPER COMMANDS"
.
.SS "\fBaudit\fR [\fIoptions\fR] [\fIformula\fR]"
Check \fIformula\fR for Homebrew coding style violations\. This should be run before submitting a new formula\. If no \fIformula\fR are provided, check all locally available formulae\. Will exit with a non\-zero status if any errors are found, which can be useful for implementing pre\-commit hooks\.
Check \fIformula\fR for Homebrew coding style violations\. This should be run before submitting a new formula\. If no \fIformula\fR are provided, check all locally available formulae and skip style checks\. Will exit with a non\-zero status if any errors are found\.
.
.TP
\fB\-\-strict\fR
Run additional, stricter style checks\.
.
.TP
\fB\-\-git\fR
Run additional, slower style checks that navigate the Git repository\.
.
.TP
\fB\-\-online\fR
Run additional, slower style checks that require a network connection\.
.
Expand Down

0 comments on commit 2a94d38

Please sign in to comment.