Merge pull request #12018 from FnControlOption/upgrade-tsort

upgrade: use topological sort to upgrade formulae

Library/Homebrew/cask.rb

@@ -20,6 +20,5 @@ require "cask/metadata"
 require "cask/pkg"
 require "cask/quarantine"
 require "cask/staged"
-require "cask/topological_hash"
 require "cask/url"
 require "cask/utils"

Library/Homebrew/cask/installer.rb

@@ -3,8 +3,8 @@
 
 require "formula_installer"
 require "unpack_strategy"
+require "utils/topological_hash"
 
-require "cask/topological_hash"
 require "cask/config"
 require "cask/download"
 require "cask/staged"
@@ -294,43 +294,14 @@ module Cask
             "but you are running #{@current_arch}."
     end
 
-    def graph_dependencies(cask_or_formula, acc = TopologicalHash.new)
-      return acc if acc.key?(cask_or_formula)
-
-      if cask_or_formula.is_a?(Cask)
-        formula_deps = cask_or_formula.depends_on.formula.map { |f| Formula[f] }
-        cask_deps = cask_or_formula.depends_on.cask.map { |c| CaskLoader.load(c, config: nil) }
-      else
-        formula_deps = cask_or_formula.deps.reject(&:build?).map(&:to_formula)
-        cask_deps = cask_or_formula.requirements.map(&:cask).compact
-                                   .map { |c| CaskLoader.load(c, config: nil) }
-      end
-
-      acc[cask_or_formula] ||= []
-      acc[cask_or_formula] += formula_deps
-      acc[cask_or_formula] += cask_deps
-
-      formula_deps.each do |f|
-        graph_dependencies(f, acc)
-      end
-
-      cask_deps.each do |c|
-        graph_dependencies(c, acc)
-      end
-
-      acc
-    end
-
     def collect_cask_and_formula_dependencies
       return @cask_and_formula_dependencies if @cask_and_formula_dependencies
 
-      graph = graph_dependencies(@cask)
+      graph = ::Utils::TopologicalHash.graph_package_dependencies(@cask)
 
       raise CaskSelfReferencingDependencyError, cask.token if graph[@cask].include?(@cask)
 
-      primary_container.dependencies.each do |dep|
+      ::Utils::TopologicalHash.graph_package_dependencies(primary_container.dependencies, graph)
-        graph_dependencies(dep, graph)
-      end
 
       begin
         @cask_and_formula_dependencies = graph.tsort - [@cask]

Library/Homebrew/cask/topological_hash.rb

@@ -1,21 +0,0 @@
-# typed: true
-# frozen_string_literal: true
-
-require "tsort"
-
-module Cask
-  # Topologically sortable hash map.
-  class TopologicalHash < Hash
-    include TSort
-
-    private
-
-    def tsort_each_node(&block)
-      each_key(&block)
-    end
-
-    def tsort_each_child(node, &block)
-      fetch(node).each(&block)
-    end
-  end
-end

Library/Homebrew/exceptions.rb

@@ -758,3 +758,13 @@ class ShebangDetectionError < RuntimeError
     super "Cannot detect #{type} shebang: #{reason}."
   end
 end
+
+# Raised when one or more formulae have cyclic dependencies.
+class CyclicDependencyError < RuntimeError
+  def initialize(strongly_connected_components)
+    super <<~EOS
+      The following packages contain cyclic dependencies:
+        #{strongly_connected_components.select { |packages| packages.count > 1 }.map(&:to_sentence).join("\n  ")}
+    EOS
+  end
+end

Library/Homebrew/test/utils/topological_hash_spec.rb

@@ -0,0 +1,99 @@
+# typed: false
+# frozen_string_literal: true
+
+require "utils/topological_hash"
+
+describe Utils::TopologicalHash do
+  describe "#tsort" do
+    it "returns a topologically sorted array" do
+      hash = described_class.new
+      hash[1] = [2, 3]
+      hash[2] = [3]
+      hash[3] = []
+      hash[4] = []
+      expect(hash.tsort).to eq [3, 2, 1, 4]
+    end
+  end
+
+  describe "#strongly_connected_components" do
+    it "returns an array of arrays" do
+      hash = described_class.new
+      hash[1] = [2]
+      hash[2] = [3, 4]
+      hash[3] = [2]
+      hash[4] = []
+      expect(hash.strongly_connected_components).to eq [[4], [2, 3], [1]]
+    end
+  end
+
+  describe "::graph_package_dependencies" do
+    it "returns a topological hash" do
+      formula1 = formula "homebrew-test-formula1" do
+        url "foo"
+        version "0.5"
+      end
+
+      formula2 = formula "homebrew-test-formula2" do
+        url "foo"
+        version "0.5"
+        depends_on "homebrew-test-formula1"
+      end
+
+      formula3 = formula "homebrew-test-formula3" do
+        url "foo"
+        version "0.5"
+        depends_on "homebrew-test-formula4"
+      end
+
+      formula4 = formula "homebrew-test-formula4" do
+        url "foo"
+        version "0.5"
+        depends_on "homebrew-test-formula3"
+      end
+
+      cask1 = Cask::Cask.new("homebrew-test-cask1") do
+        url "foo"
+        version "1.2.3"
+      end
+
+      cask2 = Cask::Cask.new("homebrew-test-cask2") do
+        url "foo"
+        version "1.2.3"
+        depends_on cask: "homebrew-test-cask1"
+        depends_on formula: "homebrew-test-formula1"
+      end
+
+      cask3 = Cask::Cask.new("homebrew-test-cask3") do
+        url "foo"
+        version "1.2.3"
+        depends_on cask: "homebrew-test-cask2"
+      end
+
+      stub_formula_loader formula1
+      stub_formula_loader formula2
+      stub_formula_loader formula3
+      stub_formula_loader formula4
+
+      stub_cask_loader cask1
+      stub_cask_loader cask2
+      stub_cask_loader cask3
+
+      packages = [formula1, formula2, formula3, formula4, cask1, cask2, cask3]
+      expect(described_class.graph_package_dependencies(packages)).to eq({
+        formula1 => [],
+        formula2 => [formula1],
+        formula3 => [formula4],
+        formula4 => [formula3],
+        cask1    => [],
+        cask2    => [formula1, cask1],
+        cask3    => [cask2],
+      })
+
+      sorted = [formula1, cask1, cask2, cask3, formula2]
+      expect(described_class.graph_package_dependencies([cask3, cask2, cask1, formula2, formula1]).tsort).to eq sorted
+      expect(described_class.graph_package_dependencies([cask3, formula2]).tsort).to eq sorted
+
+      expect { described_class.graph_package_dependencies([formula3, formula4]).tsort }.to raise_error TSort::Cyclic
+    end
+  end
+end

Library/Homebrew/upgrade.rb

@@ -6,6 +6,7 @@ require "formula_installer"
 require "development_tools"
 require "messages"
 require "cleanup"
+require "utils/topological_hash"
 
 module Homebrew
   # Helper functions for upgrading formulae.
@@ -42,6 +43,13 @@ module Homebrew
         end
       end
 
+      dependency_graph = Utils::TopologicalHash.graph_package_dependencies(formulae_to_install)
+      begin
+        formulae_to_install = dependency_graph.tsort & formulae_to_install
+      rescue TSort::Cyclic
+        raise CyclicDependencyError, dependency_graph.strongly_connected_components if Homebrew::EnvConfig.developer?
+      end
+
       formula_installers = formulae_to_install.map do |formula|
         Migrator.migrate_if_needed(formula, force: force, dry_run: dry_run)
         begin

Library/Homebrew/utils/topological_hash.rb

@@ -0,0 +1,63 @@
+# typed: true
+# frozen_string_literal: true
+
+require "tsort"
+
+module Utils
+  # Topologically sortable hash map.
+  class TopologicalHash < Hash
+    extend T::Sig
+
+    include TSort
+
+    sig {
+      params(
+        packages:    T.any(Cask::Cask, Formula, T::Array[T.any(Cask::Cask, Formula)]),
+        accumulator: TopologicalHash,
+      ).returns(TopologicalHash)
+    }
+    def self.graph_package_dependencies(packages, accumulator = TopologicalHash.new)
+      packages = Array(packages)
+
+      packages.each do |cask_or_formula|
+        next accumulator if accumulator.key?(cask_or_formula)
+
+        if cask_or_formula.is_a?(Cask::Cask)
+          formula_deps = cask_or_formula.depends_on
+                                        .formula
+                                        .map { |f| Formula[f] }
+          cask_deps = cask_or_formula.depends_on
+                                     .cask
+                                     .map { |c| Cask::CaskLoader.load(c, config: nil) }
+        else
+          formula_deps = cask_or_formula.deps
+                                        .reject(&:build?)
+                                        .map(&:to_formula)
+          cask_deps = cask_or_formula.requirements
+                                     .map(&:cask)
+                                     .compact
+                                     .map { |c| Cask::CaskLoader.load(c, config: nil) }
+        end
+
+        accumulator[cask_or_formula] ||= []
+        accumulator[cask_or_formula] += formula_deps
+        accumulator[cask_or_formula] += cask_deps
+
+        graph_package_dependencies(formula_deps, accumulator)
+        graph_package_dependencies(cask_deps, accumulator)
+      end
+
+      accumulator
+    end
+
+    private
+
+    def tsort_each_node(&block)
+      each_key(&block)
+    end
+
+    def tsort_each_child(node, &block)
+      fetch(node).each(&block)
+    end
+  end
+end