Skip to content
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.

Sign up for GitHub

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: make audit_revision_and_version_scheme faster. #7684

Merged
merged 1 commit into from
Jun 8, 2020
Merged

audit: make audit_revision_and_version_scheme faster. #7684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 74 additions & 75 deletions Library/Homebrew/dev-cmd/audit.rb
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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