Merge pull request #6698 from MikeMcQuaid/upgrade-dependents-performance

Improve upgrade dependents check performance, reliability and readability

Library/Homebrew/cmd/deps.rb

@@ -58,6 +58,9 @@ module Homebrew
 
   def deps
     deps_args.parse
+
+    Formulary.enable_factory_cache!
+
     mode = OpenStruct.new(
       installed?:  args.installed?,
       tree?:       args.tree?,

Library/Homebrew/cmd/upgrade.rb

@@ -209,100 +209,6 @@ module Homebrew
     end
   end
 
-  def upgradable_dependents(kegs, formulae)
-    formulae_to_upgrade = Set.new
-    formulae_pinned = Set.new
-
-    formulae_to_check = formulae
-    checked_formulae = Set.new
-
-    until formulae_to_check.empty?
-      descendants = Set.new
-
-      formulae_to_check.each do |formula|
-        next if checked_formulae.include?(formula)
-
-        dependents = kegs.select do |keg|
-          keg.runtime_dependencies
-             .any? { |d| d["full_name"] == formula.full_name }
-        end
-
-        next if dependents.empty?
-
-        dependent_formulae = dependents.map(&:to_formula)
-
-        dependent_formulae.each do |f|
-          next if formulae_to_upgrade.include?(f)
-          next if formulae_pinned.include?(f)
-
-          if f.outdated?(fetch_head: args.fetch_HEAD?)
-            if f.pinned?
-              formulae_pinned << f
-            else
-              formulae_to_upgrade << f
-            end
-          end
-
-          descendants << f
-        end
-
-        checked_formulae << formula
-      end
-
-      formulae_to_check = descendants
-    end
-
-    [formulae_to_upgrade, formulae_pinned]
-  end
-
-  def broken_dependents(kegs, formulae, scanned = Set.new)
-    formulae_to_reinstall = Set.new
-    formulae_pinned_and_outdated = Set.new
-
-    CacheStoreDatabase.use(:linkage) do |db|
-      formulae.each do |formula|
-        descendants = Set.new
-
-        dependents = kegs.select do |keg|
-          keg = keg.runtime_dependencies
-                   .any? { |d| d["full_name"] == formula.full_name }
-          keg unless scanned.include?(keg)
-        end
-
-        next if dependents.empty?
-
-        dependents.each do |keg|
-          f = keg.to_formula
-
-          next if formulae_to_reinstall.include?(f)
-          next if formulae_pinned_and_outdated.include?(f)
-
-          checker = LinkageChecker.new(keg, cache_db: db)
-
-          if checker.broken_library_linkage?
-            if f.outdated?(fetch_head: args.fetch_HEAD?)
-              # Outdated formulae = pinned formulae (see function above)
-              formulae_pinned_and_outdated << f
-            else
-              formulae_to_reinstall << f
-            end
-          end
-
-          descendants << f
-        end
-
-        scanned.merge dependents
-
-        descendants_to_reinstall, descendants_pinned = broken_dependents(kegs, descendants, scanned)
-
-        formulae_to_reinstall.merge descendants_to_reinstall
-        formulae_pinned_and_outdated.merge descendants_pinned
-      end
-    end
-
-    [formulae_to_reinstall, formulae_pinned_and_outdated]
-  end
-
   # @private
   def depends_on(a, b)
     if a.opt_or_installed_prefix_keg
@@ -314,81 +220,88 @@ module Homebrew
     end
   end
 
-  # @private
+  def check_dependents(formulae_to_install)
-  def formulae_with_runtime_dependencies
+    return if formulae_to_install.empty?
-    Formula.installed
-           .map(&:opt_or_installed_prefix_keg)
-           .reject(&:nil?)
-           .reject { |f| f.runtime_dependencies.to_a.empty? }
-  end
-
-  def check_dependents(formulae)
-    return if formulae.empty?
-
-    # First find all the outdated dependents.
-    kegs = formulae_with_runtime_dependencies
-
-    return if kegs.empty?
 
     oh1 "Checking dependents for outdated formulae" if args.verbose?
-    upgradable, pinned = upgradable_dependents(kegs, formulae).map(&:to_a)
+    dependents =
+      formulae_to_install.flat_map(&:runtime_installed_formula_dependents)
+    return if dependents.empty?
 
-    upgradable.sort! { |a, b| depends_on(a, b) }
+    upgradeable_dependents = dependents.select(&:outdated?)
+                                       .sort { |a, b| depends_on(a, b) }
+    pinned_dependents = dependents.select(&:pinned?)
+                                  .sort { |a, b| depends_on(a, b) }
 
-    pinned.sort! { |a, b| depends_on(a, b) }
+    if pinned_dependents.present?
-
+      plural = "dependent".pluralize(pinned_dependents.count)
-    # Print the pinned dependents.
+      ohai "Not upgrading #{pinned_dependents.count} pinned #{plural}:"
-    unless pinned.empty?
+      puts pinned_dependents.map do |f|
-      ohai "Not upgrading #{pinned.count} pinned #{"dependent".pluralize(pinned.count)}:"
+        "#{f.full_specified_name} #{f.pkg_version}"
-      puts pinned.map { |f| "#{f.full_specified_name} #{f.pkg_version}" } * ", "
+      end.join(", ")
     end
 
     # Print the upgradable dependents.
-    if upgradable.empty?
+    if upgradeable_dependents.blank?
       ohai "No dependents to upgrade" if args.verbose?
     else
-      ohai "Upgrading #{upgradable.count} #{"dependent".pluralize(upgradable.count)}:"
+      plural = "dependent".pluralize(upgradeable_dependents.count)
-      formulae_upgrades = upgradable.map do |f|
+      ohai "Upgrading #{upgradable.count} #{plural}:"
+      formulae_upgrades = upgradeable_dependents.map do |f|
+        name = f.full_specified_name
         if f.optlinked?
-          "#{f.full_specified_name} #{Keg.new(f.opt_prefix).version} -> #{f.pkg_version}"
+          "#{name} #{Keg.new(f.opt_prefix).version} -> #{f.pkg_version}"
         else
-          "#{f.full_specified_name} #{f.pkg_version}"
+          "#{name} #{f.pkg_version}"
         end
       end
       puts formulae_upgrades.join(", ")
     end
 
-    upgrade_formulae(upgradable)
+    upgrade_formulae(upgradeable_dependents)
-
-    # TODO: re-enable when this code actually works.
-    # https://github.com/Homebrew/brew/issues/6671
-    return unless ENV["HOMEBREW_BROKEN_DEPENDENTS"]
-
-    # Assess the dependents tree again.
-    kegs = formulae_with_runtime_dependencies
 
+    # Assess the dependents tree again now we've upgraded.
     oh1 "Checking dependents for broken library links" if args.verbose?
-    reinstallable, pinned = broken_dependents(kegs, formulae).map(&:to_a)
+    broken_dependents = CacheStoreDatabase.use(:linkage) do |db|
+      formulae_to_install.flat_map(&:runtime_installed_formula_dependents)
+                         .map(&:opt_or_installed_prefix_keg)
+                         .compact
+                         .select do |keg|
+        LinkageChecker.new(keg, cache_db: db)
+                      .broken_library_linkage?
+      end
+    end
+    return if broken_dependents.empty?
 
-    reinstallable.sort! { |a, b| depends_on(a, b) }
+    reinstallable_broken_dependents =
-
+      broken_dependents.select(&:outdated?)
-    pinned.sort! { |a, b| depends_on(a, b) }
+                       .sort { |a, b| depends_on(a, b) }
+    pinned_broken_dependents =
+      broken_dependents.select(&:pinned?)
+                       .sort { |a, b| depends_on(a, b) }
 
     # Print the pinned dependents.
-    unless pinned.empty?
+    if pinned_broken_dependents.present?
-      onoe "Not reinstalling #{pinned.count} broken and outdated, but pinned #{"dependent".pluralize(pinned.count)}:"
+      count = pinned_broken_dependents.count
-      $stderr.puts pinned.map { |f| "#{f.full_specified_name} #{f.pkg_version}" } * ", "
+      plural = "dependent".pluralize(pinned_broken_dependents.count)
+      onoe "Not reinstalling #{count} broken and outdated, but pinned #{plural}:"
+      $stderr.puts pinned_broken_dependents.map do |f|
+        "#{f.full_specified_name} #{f.pkg_version}"
+      end.join(", ")
     end
 
     # Print the broken dependents.
-    if reinstallable.empty?
+    if reinstallable_broken_dependents.empty?
       ohai "No broken dependents to reinstall" if args.verbose?
     else
-      ohai "Reinstalling #{reinstallable.count} broken #{"dependent".pluralize(reinstallable.count)} from source:"
+      count = reinstallable_broken_dependents.count
-      puts reinstallable.map(&:full_specified_name).join(", ")
+      plural = "dependent".pluralize(reinstallable_broken_dependents.count)
+      ohai "Reinstalling #{count} broken #{plural} from source:"
+      puts reinstallable_broken_dependents.map(&:full_specified_name)
+                                          .join(", ")
     end
 
-    reinstallable.each do |f|
+    reinstallable_broken_dependents.each do |f|
       reinstall_formula(f, build_from_source: true)
     rescue FormulaInstallationAlreadyAttemptedError
       # We already attempted to reinstall f as part of the dependency tree of

Library/Homebrew/cmd/uses.rb

@@ -46,6 +46,8 @@ module Homebrew
 
     raise FormulaUnspecifiedError if args.remaining.empty?
 
+    Formulary.enable_factory_cache!
+
     used_formulae_missing = false
     used_formulae = begin
       ARGV.formulae
@@ -56,34 +58,43 @@ module Homebrew
       ARGV.named.map { |name| OpenStruct.new name: name, full_name: name }
     end
 
-    formulae = args.installed? ? Formula.installed : Formula
-    recursive = args.recursive?
     only_installed_arg = args.installed? &&
                          !args.include_build? &&
                          !args.include_test? &&
                          !args.include_optional? &&
                          !args.skip_recommended?
 
-    includes, ignores = argv_includes_ignores(ARGV)
+    uses = if only_installed_arg && !used_formulae_missing
+      used_formulae.map(&:runtime_installed_formula_dependents)
+                   .reduce(&:&)
+    else
+      formulae = args.installed? ? Formula.installed : Formula
+      recursive = args.recursive?
+      includes, ignores = argv_includes_ignores(ARGV)
 
-    uses = formulae.select do |f|
+      formulae.select do |f|
-      used_formulae.all? do |ff|
+        deps = if recursive
-        deps = f.runtime_dependencies if only_installed_arg
-        deps ||= if recursive
           recursive_includes(Dependency, f, includes, ignores)
         else
           reject_ignores(f.deps, ignores, includes)
         end
 
-        deps.any? do |dep|
+        used_formulae.all? do |ff|
-          dep.to_formula.full_name == ff.full_name
+          deps.any? do |dep|
-        rescue
+            match = begin
-          dep.name == ff.name
+              dep.to_formula.full_name == ff.full_name if dep.name.include?("/")
+            rescue
+              nil
+            end
+            next match unless match.nil?
+
+            dep.name == ff.name
+          end
+        rescue FormulaUnavailableError
+          # Silently ignore this case as we don't care about things used in
+          # taps that aren't currently tapped.
+          next
         end
-      rescue FormulaUnavailableError
-        # Silently ignore this case as we don't care about things used in
-        # taps that aren't currently tapped.
-        next
       end
     end
 

Library/Homebrew/dev-cmd/bottle.rb

@@ -263,6 +263,8 @@ module Homebrew
 
         changed_files = keg.replace_locations_with_placeholders unless args.skip_relocation?
 
+        Formula.clear_cache
+        Keg.clear_cache
         Tab.clear_cache
         tab = Tab.for_keg(keg)
         original_tab = tab.dup

Library/Homebrew/formula.rb

@@ -52,6 +52,7 @@ class Formula
   include Utils::Shell
   extend Enumerable
   extend Forwardable
+  extend Cachable
 
   # @!method inreplace(paths, before = nil, after = nil)
   # Actually implemented in {Utils::Inreplace.inreplace}.
@@ -1528,7 +1529,8 @@ class Formula
   # If not, return nil.
   # @private
   def opt_or_installed_prefix_keg
-    if optlinked? && opt_prefix.exist?
+    Formula.cache[:opt_or_installed_prefix_keg] ||= {}
+    Formula.cache[:opt_or_installed_prefix_keg][name] ||= if optlinked? && opt_prefix.exist?
       Keg.new(opt_prefix)
     elsif installed_prefix.directory?
       Keg.new(installed_prefix)
@@ -1538,27 +1540,27 @@ class Formula
   # Returns a list of Dependency objects that are required at runtime.
   # @private
   def runtime_dependencies(read_from_tab: true, undeclared: true)
-    if read_from_tab &&
+    deps = if read_from_tab && undeclared &&
-       undeclared &&
+              (tab_deps = opt_or_installed_prefix_keg&.runtime_dependencies)
-       (keg = opt_or_installed_prefix_keg) &&
+      tab_deps.map do |d|
-       (tab_deps = keg.runtime_dependencies)
-      return tab_deps.map do |d|
         full_name = d["full_name"]
         next unless full_name
 
         Dependency.new full_name
       end.compact
     end
-
+    deps ||= declared_runtime_dependencies unless undeclared
-    return declared_runtime_dependencies unless undeclared
+    deps ||= (declared_runtime_dependencies | undeclared_runtime_dependencies)
-
+    deps
-    declared_runtime_dependencies | undeclared_runtime_dependencies
   end
 
   # Returns a list of Formula objects that are required at runtime.
   # @private
   def runtime_formula_dependencies(read_from_tab: true, undeclared: true)
-    runtime_dependencies(
+    cache_key = "#{name}-#{read_from_tab}-#{undeclared}"
+
+    Formula.cache[:runtime_formula_dependencies] ||= {}
+    Formula.cache[:runtime_formula_dependencies][cache_key] ||= runtime_dependencies(
       read_from_tab: read_from_tab,
       undeclared:    undeclared,
     ).map do |d|
@@ -1568,6 +1570,23 @@ class Formula
     end.compact
   end
 
+  def runtime_installed_formula_dependents
+    # `opt_or_installed_prefix_keg` and `runtime_dependencies` `select`s ensure
+    # that we don't end up with something `Formula#runtime_dependencies` can't
+    # read from a `Tab`.
+    Formula.cache[:runtime_installed_formula_dependents] = {}
+    Formula.cache[:runtime_installed_formula_dependents][name] ||= Formula.installed
+                                                                          .select(&:opt_or_installed_prefix_keg)
+                                                                          .select(&:runtime_dependencies)
+                                                                          .select do |f|
+      f.runtime_formula_dependencies.any? do |dep|
+        full_name == dep.full_name
+      rescue
+        name == dep.name
+      end
+    end
+  end
+
   # Returns a list of formulae depended on by this formula that aren't
   # installed
   def missing_dependencies(hide: nil)

Library/Homebrew/formula_installer.rb

@@ -782,6 +782,7 @@ class FormulaInstaller
     unless link_keg
       begin
         keg.optlink
+        Formula.clear_cache
       rescue Keg::LinkError => e
         onoe "Failed to create #{formula.opt_prefix}"
         puts "Things that depend on #{formula.full_name} will probably not build."

Library/Homebrew/formulary.rb

@@ -9,6 +9,14 @@ require "extend/cachable"
 module Formulary
   extend Cachable
 
+  def self.enable_factory_cache!
+    @factory_cache = true
+  end
+
+  def self.factory_cached?
+    !@factory_cache.nil?
+  end
+
   def self.formula_class_defined?(path)
     cache.key?(path)
   end
@@ -314,7 +322,18 @@ module Formulary
   def self.factory(ref, spec = :stable, alias_path: nil, from: nil)
     raise ArgumentError, "Formulae must have a ref!" unless ref
 
-    loader_for(ref, from: from).get_formula(spec, alias_path: alias_path)
+    cache_key = "#{ref}-#{spec}-#{alias_path}-#{from}"
+    if factory_cached? && cache[:formulary_factory] &&
+       cache[:formulary_factory][cache_key]
+      return cache[:formulary_factory][cache_key]
+    end
+
+    formula = loader_for(ref, from: from).get_formula(spec, alias_path: alias_path)
+    if factory_cached?
+      cache[:formulary_factory] ||= {}
+      cache[:formulary_factory][cache_key] ||= formula
+    end
+    formula
   end
 
   # Return a Formula instance for the given rack.

Library/Homebrew/keg.rb

@@ -6,6 +6,8 @@ require "lock_file"
 require "ostruct"
 
 class Keg
+  extend Cachable
+
   class AlreadyLinkedError < RuntimeError
     def initialize(keg)
       super <<~EOS
@@ -519,7 +521,8 @@ class Keg
   end
 
   def runtime_dependencies
-    tab.runtime_dependencies
+    Keg.cache[:runtime_dependencies] ||= {}
+    Keg.cache[:runtime_dependencies][path] ||= tab.runtime_dependencies
   end
 
   def aliases

Library/Homebrew/test/dependency_collector_spec.rb

@@ -13,10 +13,6 @@ describe DependencyCollector do
     subject.requirements.find { |req| req.is_a? klass }
   end
 
-  after do
-    described_class.clear_cache
-  end
-
   describe "#add" do
     specify "dependency creation" do
       subject.add "foo" => :build

Library/Homebrew/test/missing_formula_spec.rb

@@ -38,7 +38,6 @@ describe Homebrew::MissingFormula do
     subject { described_class.tap_migration_reason(formula) }
 
     before do
-      Tap.clear_cache
       tap_path = Tap::TAP_DIRECTORY/"homebrew/homebrew-foo"
       tap_path.mkpath
       (tap_path/"tap_migrations.json").write <<~JSON
@@ -63,7 +62,6 @@ describe Homebrew::MissingFormula do
     subject { described_class.deleted_reason(formula, silent: true) }
 
     before do
-      Tap.clear_cache
       tap_path = Tap::TAP_DIRECTORY/"homebrew/homebrew-foo"
       tap_path.mkpath
       (tap_path/"deleted-formula.rb").write "placeholder"

Library/Homebrew/test/os/linux/dependency_collector_spec.rb

@@ -5,10 +5,6 @@ require "dependency_collector"
 describe DependencyCollector do
   alias_matcher :be_a_build_requirement, :be_build
 
-  after do
-    described_class.clear_cache
-  end
-
   describe "#add" do
     resource = Resource.new
 

Library/Homebrew/test/os/mac/dependency_collector_spec.rb

@@ -5,10 +5,6 @@ require "dependency_collector"
 describe DependencyCollector do
   alias_matcher :need_tar_xz_dependency, :be_tar_needs_xz_dependency
 
-  after do
-    described_class.clear_cache
-  end
-
   specify "Resource dependency from a '.xz' URL" do
     resource = Resource.new
     resource.url("https://brew.sh/foo.tar.xz")

Library/Homebrew/test/spec_helper.rb

@@ -145,7 +145,13 @@ RSpec.configure do |config|
 
     begin
       Homebrew.raise_deprecation_exceptions = true
+
+      Formulary.clear_cache
       Tap.clear_cache
+      DependencyCollector.clear_cache
+      Formula.clear_cache
+      Keg.clear_cache
+      Tab.clear_cache
       FormulaInstaller.clear_attempted
 
       TEST_DIRECTORIES.each(&:mkpath)
@@ -176,6 +182,11 @@ RSpec.configure do |config|
         @__stderr.close
       end
 
+      Formulary.clear_cache
+      Tap.clear_cache
+      DependencyCollector.clear_cache
+      Formula.clear_cache
+      Keg.clear_cache
       Tab.clear_cache
 
       FileUtils.rm_rf [

Library/Homebrew/test/tap_spec.rb

@@ -78,8 +78,6 @@ describe Tap do
     expect {
       described_class.fetch("homebrew", "homebrew/baz")
     }.to raise_error(/Invalid tap name/)
-  ensure
-    described_class.clear_cache
   end
 
   describe "::from_path" do