Merge pull request #23020 from bendrucker/bundle-cyclic-tolerance

Warn instead of aborting brew bundle on stale-tab dependency cycles

Author: MikeMcQuaid

Library/Homebrew/bundle/brew.rb

@@ -4,6 +4,7 @@
 require "json"
 require "tsort"
 require "utils"
+require "utils/topological_hash"
 require "utils/output"
 require "bundle/package_type"
 require "trust"
@@ -426,24 +427,27 @@ def sort!(formulae)
           raise "formulae_by_full_name is nil" if @formulae_by_full_name.nil?
           raise "formulae_by_name is nil" if @formulae_by_name.nil?
 
-          @formulae = topo.tsort
-                          .map { |name| @formulae_by_full_name[name] || @formulae_by_name[name] }
-                          .uniq { |f| f[:full_name] }
-        rescue TSort::Cyclic => e
-          e.message =~ /\["([^"]*)".*"([^"]*)"\]/
-          cycle_first = Regexp.last_match(1)
-          cycle_last = Regexp.last_match(2)
-          odie e.message if !cycle_first || !cycle_last
-
-          odie <<~EOS
-            Formulae dependency graph sorting failed (likely due to a circular dependency):
-            #{cycle_first}: #{topo[cycle_first] if topo}
-            #{cycle_last}: #{topo[cycle_last] if topo}
-            Please run the following commands and try again:
-              brew update
-              brew uninstall --ignore-dependencies --force #{cycle_first} #{cycle_last}
-              brew install #{cycle_first} #{cycle_last}
-          EOS
+          # Stale keg-tab dependency data can form a cycle the live graph does not
+          # have (Homebrew/homebrew-bundle#1513), so warn and continue rather than
+          # aborting the whole bundle.
+          sorted = topo.tsort_with_cycles do |cycles|
+            cyclic = cycles.flatten
+                           .filter_map { |name| @formulae_by_full_name[name] || @formulae_by_name[name] }
+                           .uniq { |f| f[:full_name] }
+                           .map { |f| f[:full_name] }
+            opoo <<~EOS
+              Formulae dependency graph sorting found a circular dependency:
+                #{cyclic.join(", ")}
+              This is usually caused by stale dependency data in installed keg tabs.
+              If it persists, run the following commands and try again:
+                brew update
+                brew uninstall --ignore-dependencies --force #{cyclic.join(" ")}
+                brew install #{cyclic.join(" ")}
+            EOS
+          end
+
+          @formulae = sorted.filter_map { |name| @formulae_by_full_name[name] || @formulae_by_name[name] }
+                            .uniq { |f| f[:full_name] }
         end
       end
 
@@ -773,6 +777,7 @@ def upgrade_formula!(verbose:, force:)
       class Topo < Hash
         extend T::Generic
         include TSort
+        include Utils::CycleTolerantTSort
 
         K = type_member { { fixed: String } }
         V = type_member { { fixed: T::Array[String] } }

Library/Homebrew/cask/installer.rb

@@ -411,13 +411,10 @@ def cask_and_formula_dependencies
 
       ::Utils::TopologicalHash.graph_package_dependencies(pc.dependencies, graph)
 
-      begin
-        @cask_and_formula_dependencies = graph.tsort - [@cask]
-      rescue TSort::Cyclic
-        strongly_connected_components = graph.strongly_connected_components.sort_by(&:count)
-        cyclic_dependencies = strongly_connected_components.last - [@cask]
+      @cask_and_formula_dependencies = graph.tsort_with_cycles do |cycles|
+        cyclic_dependencies = cycles.sort_by(&:count).fetch(-1) - [@cask]
         raise CaskCyclicDependencyError.new(@cask.token, cyclic_dependencies.to_sentence)
-      end
+      end - [@cask]
     end
 
     sig { returns(T::Array[T.any(Formula, ::Cask::Cask)]) }

Library/Homebrew/test/bundle/brew_spec.rb

@@ -160,6 +160,26 @@
       }
     end
 
+    def formula_double_depending_on(name, dependency)
+      instance_double(Formula,
+                      name:,
+                      desc:                   name,
+                      oldnames:               [],
+                      full_name:              name,
+                      any_version_installed?: true,
+                      aliases:                [],
+                      runtime_dependencies:   [instance_double(Dependency, name: dependency)],
+                      deps:                   [],
+                      conflicts:              [],
+                      any_installed_prefix:   nil,
+                      linked?:                false,
+                      keg_only?:              true,
+                      pinned?:                false,
+                      outdated?:              false,
+                      stable:                 instance_double(SoftwareSpec, bottle_defined?: false, bottled?: false),
+                      tap:                    instance_double(Tap, official?: false))
+    end
+
     before do
       described_class.reset!
     end
@@ -180,13 +200,13 @@
         expect(dumper.formulae_by_full_name("bar")).to eql({})
       end
 
-      it "exits on cyclic exceptions" do
-        expect(Formula).to receive(:installed).and_return([foo, bar, baz])
-        expect_any_instance_of(Homebrew::Bundle::Brew::Topo).to receive(:tsort).and_raise(
-          TSort::Cyclic,
-          'topological sort failed: ["foo", "bar"]',
-        )
-        expect { dumper.formulae_by_full_name }.to raise_error(SystemExit)
+      it "warns and continues on cyclic dependencies instead of exiting" do
+        cyclic_foo = formula_double_depending_on("foo", "bar")
+        cyclic_bar = formula_double_depending_on("bar", "foo")
+        expect(Formula).to receive(:installed).and_return([cyclic_foo, cyclic_bar])
+
+        expect { dumper.formulae_by_full_name }.to output(/found a circular dependency/).to_stderr
+        expect(dumper.formulae.map { |f| f[:full_name] }).to contain_exactly("foo", "bar")
       end
 
       it "returns a hash for a formula" do
@@ -916,5 +936,15 @@
 
       expect(topo.tsort).to eq(["libice", "b", "a"])
     end
+
+    it "flattens a cyclic graph via strongly connected components without raising" do
+      topo = described_class.new
+      topo["a"] = ["b"]
+      topo["b"] = ["a"]
+
+      cycles = []
+      expect(topo.tsort_with_cycles { |c| cycles.concat(c) }).to contain_exactly("a", "b")
+      expect(cycles).to eq([["a", "b"]])
+    end
   end
 end

Library/Homebrew/upgrade.rb

@@ -93,13 +93,12 @@ def formula_installers(
         end
 
         dependency_graph = Utils::TopologicalHash.graph_package_dependencies(formulae_to_install)
-        begin
-          formulae_to_install = dependency_graph.tsort & formulae_to_install
-        rescue TSort::Cyclic
-          if Homebrew::EnvConfig.developer?
-            raise CyclicDependencyError, dependency_graph.strongly_connected_components
-          end
+        sorted = dependency_graph.tsort_with_cycles do |cycles|
+          raise CyclicDependencyError, cycles if Homebrew::EnvConfig.developer?
+
+          odebug "Ignoring cyclic dependencies: #{cycles.map(&:to_sentence).join(", ")}"
         end
+        formulae_to_install = sorted & formulae_to_install
 
         # We need to fetch the bottle tabs ahead of the `Install.fetch_formulae`
         # pipeline because we need to first filter out those formulae with all

Library/Homebrew/utils/topological_hash.rb

@@ -4,10 +4,32 @@
 require "tsort"
 
 module Utils
+  # Cycle-tolerant ordering for graphs that include TSort.
+  module CycleTolerantTSort
+    extend T::Helpers
+
+    requires_ancestor { TSort }
+
+    # Orders nodes dependency-first like tsort but, unlike tsort, does not
+    # raise on a cycle: yields cyclic components (size > 1) to the block and
+    # returns the flattened component order.
+    sig {
+      params(on_cycle: T.proc.params(arg0: T::Array[T::Array[T.untyped]]).void)
+        .returns(T::Array[T.untyped])
+    }
+    def tsort_with_cycles(&on_cycle)
+      components = each_strongly_connected_component.to_a
+      cycles = components.select { |component| component.size > 1 }
+      yield(cycles) if cycles.any?
+      components.flatten
+    end
+  end
+
   # Topologically sortable hash map.
   class TopologicalHash < Hash
     extend T::Generic
     include TSort
+    include CycleTolerantTSort
 
     CaskOrFormula = T.type_alias { T.any(Cask::Cask, Formula) }