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

Jump to bottom

formulae_dependents: apply some optimisations #841

Closed
wants to merge 1 commit into from
Closed

formulae_dependents: apply some optimisations #841

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
58 changes: 44 additions & 14 deletions lib/tests/formulae_dependents.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ class FormulaeDependents < TestFormulae
def run!(args:)
@source_tested_dependents = []
@bottle_tested_dependents = []
@testable_formulae = @testing_formulae - skipped_or_failed_formulae
@postinstalled_formulae = []

(@testing_formulae - skipped_or_failed_formulae).each do |f|
@testable_formulae.each do |f|
dependent_formulae!(f, args: args)
puts
end
Expand All @@ -22,19 +24,29 @@ def dependent_formulae!(formula_name, args:)

test_header(:FormulaeDependents, method: "dependent_formulae!(#{formula_name})")

formula = Formulary.factory(formula_name)

source_dependents, bottled_dependents, testable_dependents =
dependents_for_formula(formula, formula_name, args: args)

return if source_dependents.blank? &&
bottled_dependents.blank? &&
testable_dependents.blank?

# Install formula dependencies. These will have been uninstalled after building.
test "brew", "install", "--only-dependencies", formula_name,
env: { "HOMEBREW_DEVELOPER" => nil }
return if steps.last.failed?

# Restore etc/var files that may have been nuked in the build stage.
test "brew", "postinstall", formula_name
formula_dependencies = Utils.safe_popen_read("brew", "deps", formula_name).split("\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we calling brew deps anywhere else? If so, worth caching?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look, but I don't think so it's used elsewhere.

# Some dependencies will need postinstalling too.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why this is the case, can you explain more?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose testing formula includes foo and bar, and both have a common dependent baz.

Whenever we test foo first, we do brew postinstall foo but we skip it for bar, but that's wrong. The reasons why we need to brew postinstall foo apply to needing to brew postinstall bar too.

postinstall_dependencies = @testable_formulae & formula_dependencies
postinstall_formulae = [formula_name, *postinstall_dependencies] - @postinstalled_formulae
test "brew", "postinstall", *postinstall_formulae
return if steps.last.failed?

formula = Formulary.factory(formula_name)

source_dependents, bottled_dependents, testable_dependents =
dependents_for_formula(formula, formula_name, args: args)
@postinstalled_formulae += postinstall_formulae

source_dependents.each do |dependent|
next if @source_tested_dependents.include?(dependent)
Expand All @@ -54,6 +66,21 @@ def dependent_formulae!(formula_name, args:)
end

def dependents_for_formula(formula, formula_name, args:)
# Calling `brew uses` is slow, so let's reserve doing that for
# if we can find a formula in the same tap that declares a dependency
# on the formula we are testing.
has_dependents = formula.tap.potential_formula_dirs.any? do |dir|
next false unless dir.exist?

dir.children.any? do |child|
next false unless child.file?
next false unless child.extname == ".rb"

child.read.include? "depends_on \"#{formula_name}\""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really love to avoid something this hacky as this when we have RuboCop AST methods to do this sort of thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we construct the AST without loading the formula? Because that's part of what's slow here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, doing this instead:

        has_dependents = formula.tap.formula_names.any? do |name|
          Formula[name].deps.map(&:name).include? formula_name
        end

We get:

( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run)  12.84s user 2.96s system 68% cpu 23.128 total

It's faster than just doing brew uses, but also a about 6 times slower than just matching text.

end
end
return unless has_dependents

info_header "Determining dependents..."

uses_args = %w[--formula --eval-all]
Expand All @@ -64,13 +91,16 @@ def dependents_for_formula(formula, formula_name, args:)
.split("\n")
end

# TODO: Consider handling the following case better.
# `foo` has a build dependency on `bar`, and `bar` has a runtime dependency on
# `baz`. When testing `baz` with `--build-dependents-from-source`, `foo` is
# not tested, but maybe should be.
dependents += with_env(HOMEBREW_STDERR: "1") do
Utils.safe_popen_read("brew", "uses", *uses_args, "--include-build", formula_name)
.split("\n")
# We care about build dependents only if we are building them from source.
if args.build_dependents_from_source?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

# TODO: Consider handling the following case better.
# `foo` has a build dependency on `bar`, and `bar` has a runtime dependency on
# `baz`. When testing `baz` with `--build-dependents-from-source`, `foo` is
# not tested, but maybe should be.
dependents += with_env(HOMEBREW_STDERR: "1") do
Utils.safe_popen_read("brew", "uses", *uses_args, "--include-build", formula_name)
.split("\n")
end
end
dependents&.uniq!
dependents&.sort!
Expand Down Expand Up @@ -104,7 +134,7 @@ def dependents_for_formula(formula, formula_name, args:)
next false if OS.linux? && dependent.requirements.exclude?(LinuxRequirement.new)

all_deps_bottled_or_built = deps.all? do |d|
bottled_or_built?(d.to_formula, @testing_formulae - @skipped_or_failed_formulae)
bottled_or_built?(d.to_formula, @testable_formulae)
end
args.build_dependents_from_source? && all_deps_bottled_or_built
end
Expand Down