formula_installer: avoid cyclic dependency on Linux

[iMichka]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

Fixes:
==> Installing patchelf dependency: patchelf
==> Installing dependencies for patchelf: patchelf
==> Installing patchelf dependency: patchelf
==> Installing dependencies for patchelf: patchelf
==> Installing patchelf dependency: patchelf
==> Installing dependencies for patchelf: patchelf
...

[iMichka]

CC @maxim-belkin @sjackman

[maxim-belkin]

Just to clarify:

1. There is nothing specific to Linux in this change.
2. This is just a last resort check to prevent cyclic dependencies.

It would be nice to see a message when a cyclic dependency is detected/prevented.
It would also be nice to check that the dependency is actually installed :)

Alternative way to silently remove cyclic dependencies would be to add

# Remove cyclic dependencies
deps.reject! { |dep| dep.to_formula.full_name == formula.full_name }

before the deps.each loop.

[sjackman]

Note that this issue arrises if patchelf were to have a bottle for Linux. It doesn't currently in Homebrew/core, but does in Linuxbrew/core. I'd suggest this fix:

diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb
index d9565ddd7..2c7846ad9 100644
--- a/Library/Homebrew/formula_installer.rb
+++ b/Library/Homebrew/formula_installer.rb
@@ -506,6 +506,7 @@ class FormulaInstaller
       bottle_deps = Keg.relocation_formulae
                        .map { |formula| Dependency.new(formula) }
                        .reject do |dep|
+        next true if dep.name == formula.name
         inherited_options[dep.name] |= inherited_options_for(dep)
         dep.satisfied? inherited_options[dep.name]
       end

@iMichka If you agree, would you update this PR?

[MikeMcQuaid]

I think this should perhaps handle patchelf specifically so it's not added as a dependency of itself. Additionally, this should raise or odie rather than silently removing the exception.

[iMichka]

Yes, my first attempt was to safeguard this only for the "patchelf" formula, because I think it will only be needed for the case.

Something like this (untested) ?:

if formula.full_name == dep.to_formula.full_name
   next if formula.full_name == "patchelf"
   raise/odie + error message

[reitermarkus]

Take a look at

https://github.com/Homebrew/brew/blob/6a1fa87191bfef31ff1b2d47d3ebf281398a210f/Library/Homebrew/cask/lib/hbc/cask_dependencies.rb

It uses TSort to resolve the dependencies, which raises an exception if there is a cyclic dependency.

[MikeMcQuaid]

I think something like odie "Recursive dependency on #{formula.name}!" if dep.name == formula.name would be sufficient.

[sjackman]

This special case occurs only when the requested formula to be installed (formula.name) also occurs in Keg.relocation_formulae. I suggest either the patch I have above (#4622 (comment)), or alternatively…

diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb
index d9565ddd7..23380d043 100644
--- a/Library/Homebrew/formula_installer.rb
+++ b/Library/Homebrew/formula_installer.rb
@@ -503,7 +503,7 @@ class FormulaInstaller
     end
 
     if pour_bottle
-      bottle_deps = Keg.relocation_formulae
+      bottle_deps = (Keg.relocation_formulae - [formula.name])
                        .map { |formula| Dependency.new(formula) }
                        .reject do |dep|
         inherited_options[dep.name] |= inherited_options_for(dep)

In my opinion, the patch should not specifically mention patchelf, as the value of Keg.relocation_formulae differs on Linux and macOS, could possibly change in the future, but mostly for DRY.

@MikeMcQuaid Do you have a preference between these two patches? As above or #4622 (comment)

[MikeMcQuaid]

I'd suggest something that avoids that logic entirely if Keg.relocation_formulae.include?(formula.name)

[sjackman]

diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb
index d9565ddd7..66a44a9cd 100644
--- a/Library/Homebrew/formula_installer.rb
+++ b/Library/Homebrew/formula_installer.rb
@@ -502,7 +502,7 @@ class FormulaInstaller
       end
     end
 
-    if pour_bottle
+    if pour_bottle && !Keg.relocation_formulae.include?(formula.name)
       bottle_deps = Keg.relocation_formulae
                        .map { |formula| Dependency.new(formula) }
                        .reject do |dep|

@iMichka Could you please update this PR?

[MikeMcQuaid]

@sjackman Looks good. You can edit this PR yourself though, too 😎

[iMichka]

I'll do it. Just want to have a closer look at the code and run it locally, so I can learn a little bit more here :)

[iMichka]

Good to go for me.

[maxim-belkin]

I have a feeling that this is not the right fix (it probably works but in a weird way).
The whole infinite loop is caused by the incorrect value of deps variable in the install_dependencies function/method:

brew/Library/Homebrew/formula_installer.rb

Lines 547 to 557 in bb49b50

	def install_dependencies(deps)
	if deps.empty? && only_deps?
	puts "All dependencies for #{formula.full_name} are satisfied."
	elsif !deps.empty?
	oh1 "Installing dependencies for #{formula.full_name}: #{deps.map(&:first).map(&Formatter.method(:identifier)).join(", ")}",
	truncate: false
	deps.each { |dep, options| install_dependency(dep, options) }
	end
	@show_header = true unless deps.empty?
	end

If we look at line 413,

brew/Library/Homebrew/formula_installer.rb

Lines 412 to 415 in bb49b50

	def compute_and_install_dependencies
	deps = compute_dependencies
	install_dependencies(deps)
	end

we'll see that deps is computed by the compute_dependencies method:

brew/Library/Homebrew/formula_installer.rb

Lines 392 to 398 in bb49b50

	def compute_dependencies
	req_map, req_deps = expand_requirements
	check_requirements(req_map)
	deps = expand_dependencies(req_deps + formula.deps)
	deps
	end

Assuming that formula.deps is empty/nil for patchelf, it is more likely the fault of non-empty req_deps variable and, therefore, of the expand_requirements method than of expand_dependencies. I think there is a mistake in the expand_requirements method that we're masking by "fixing" the expand_dependencies method.

[sjackman]

@maxim-belkin I believe the problem is this line:

brew/Library/Homebrew/formula_installer.rb

Line 512 in 7ed3be5

expanded_deps = Dependency.merge_repeats(bottle_deps + expanded_deps) unless bottle_deps.empty?

On Linux bottle_deps is ["patchelf"] which causes expanded_deps to contain patchelf, but the formula that we're installing is patchelf, which causes the infinite loop.

[maxim-belkin]

You're right. req_deps is empty when we install patchelf, butpatchelf is still added via bottle_deps because of the Keg.relocation_formulae: it is hard-coded as["patchelf"] on Linux and an empty list for Mac. So, proposed changes look good then. ☮️

[MikeMcQuaid]

Thanks all!