From 507686e8ac55e6581dc09984c13bb70f81bde391 Mon Sep 17 00:00:00 2001 From: Carlo Cabrera <30379873+carlocab@users.noreply.github.com> Date: Mon, 26 Sep 2022 16:58:56 +0800 Subject: [PATCH] formulae_dependents: apply some optimisations 1. Doing `brew install --only-dependencies` and `brew postinstall` can be quite expensive. Let's defer this to when we're sure we have dependents to test. 2. Calling `brew uses` is slow, because this requires traversing the dependency tree. [*] Let's avoid doing this unless we know we really need to. Here, we test for "needing to" by checking if another `.rb` file in the tap that might be a formula contains a `depends_on` line declaring a dependency on the formula being tested. 3. Restrict the second `brew uses` call to when we are building dependents from source, since that is the only instance where we are interested in the build dependents. While we're here, make sure to call `brew postinstall` on all dependencies that were rebuilt, and not just the one being tested currently. This may address #805. Locally, this results in the following speed up for a formula with no dependents: Before ( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run) 28.95s user 6.70s system 76% cpu 46.875 total After ( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run) 0.91s user 1.02s system 51% cpu 3.738 total This makes testing formulae with dependents slightly slower. However, the vast majority of formulae in Homebrew/core have no dependents (on macOS, at least), so this is likely a net win for the average workflow. [*] Potential future optimisation: calling `Dependency.expand` directly might give us better opportunities to exploit caching. --- lib/tests/formulae_dependents.rb | 58 ++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/lib/tests/formulae_dependents.rb b/lib/tests/formulae_dependents.rb index faf29740..a97813c7 100644 --- a/lib/tests/formulae_dependents.rb +++ b/lib/tests/formulae_dependents.rb @@ -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 @@ -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") + # Some dependencies will need postinstalling 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) @@ -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}\"" + end + end + return unless has_dependents + info_header "Determining dependents..." uses_args = %w[--formula --eval-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? + # 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! @@ -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