Reset requirement cache (again) after recursive_dependencies.map(&:to_formula) invalidates singleton cache

[maschwenk]

• 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 typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

brew deps <my custom tap>/<my custom formula>

can blow up with

Error: undefined method `[]=' for nil:NilClass
Please report this issue:
  https://docs.brew.sh/Troubleshooting
/opt/homebrew/Library/Homebrew/requirement.rb:242:in `expand'
/opt/homebrew/Library/Homebrew/dependencies_helpers.rb:43:in `recursive_includes'
/opt/homebrew/Library/Homebrew/cmd/deps.rb:198:in `deps_for_dependent'
/opt/homebrew/Library/Homebrew/cmd/deps.rb:208:in `block in deps_for_dependents'
/opt/homebrew/Library/Homebrew/cmd/deps.rb:208:in `map'
/opt/homebrew/Library/Homebrew/cmd/deps.rb:208:in `deps_for_dependents'
/opt/homebrew/Library/Homebrew/cmd/deps.rb:148:in `deps'
/opt/homebrew/Library/Homebrew/brew.rb:94:in `<main>'

I can't provide a reproduction case at this moment, but the source of the bug seems to be that:

1. Yes, we do already set cache[cache_key] ||= {} on line 225
2. But, formulae = dependent.recursive_dependencies.map(&:to_formula) if you look under the hood, to_formula seems to clear the singleton cache
3. Therefore when we get to the point that we're setting a new key, the entire cache is empty.

[maschwenk]

class ExampleFormula < Formula
  desc "Test"
  depends_on "sqitchers/sqitch/sqitch" => "with-postgres-support"
end

It looks like this is all it takes to repro (a depends_on for a custom tap). Stripped out other changes in our custom Formula I thought were the issue

[apainintheneck]

This part of the codebase was changed last week in #15892 but it's not obvious to me at a glance how those changes are related.

I wasn't able to get that example working locally for me because I was getting other loading errors (missing url/version). I tried the following but I didn't see any errors.

class ExampleFormula < Formula
  desc "Test"
  url "url"
  version "1.1.1"
  depends_on "nlohmann/json/nlohmann_json" => "with_cmake"
end

I also tried brew deps --eval-all --include-requirements but that didn't show any errors either.

The call to Dependency#to_formula should not affect the cache in the Requirement class. Both of them extend the Cachable module separately so the cache shouldn't be shared between them.

Wait a second, which version of brew are you running? Those line numbers in the stack trace don't line up with what I'm seeing locally and I'm on 4.1.8.

[apainintheneck]

You're probably on version 4.1.7 or earlier. Run brew update and see if you can still reproduce this bug. It means it's not directly caused by the changes mentioned in my previous comment but I doubt those changes will fix things either.

[Bo98]

The call to Dependency#to_formula should not affect the cache in the Requirement class.

I think the formula.build = can call Requirement.clear_cache. In theory it should also affect the equivalent code in dependency.rb so not sure why it doesn't happen there.

[maschwenk]

Yep still getting the same on 4.1.8. You can see the stacktrace here:

brew/Library/Homebrew/dependency.rb

Lines 43 to 47 in d5d7478

	def to_formula
	formula = Formulary.factory(name)
	formula.build = BuildOptions.new(options, formula.options)
	formula
	end

brew/Library/Homebrew/formula.rb

Lines 271 to 280 in d5d7478

	def build=(build_options)
	old_options = @build
	@build = build_options
	return if old_options.used_options == build_options.used_options &&
	old_options.unused_options == build_options.unused_options
	Dependency.clear_cache
	Requirement.clear_cache
	end

[apainintheneck]

Ah yeah, of course. Not sure how I missed that the first time.

I'm guessing that this hasn't come up before because we don't use --optional and --recommended in core formulae.

[Bo98]

Ah yeah, of course. Not sure how I missed that the first time.

I'm guessing that this hasn't come up before because we don't use --optional and --recommended in core formulae.

I think it's not even as simple as optional/recommended anyway, and I struggled to reproduce the issue. It seems to be some combination of options that cause it. It's odd enough that it doesn't make sense we haven't seen it in dependency.rb as well

[apainintheneck]

@maschwenk Would it be possible to share the full formula that's causing problems here? Neither me or @Bo98 can reproduce right now.

I think it's not a problem in Dependency.expand because even if the cache gets invalidated it gets added back in during the next recursive call. Requirement.expand is not really recursive so if the cache disappears in the middle of the method it just dies.

[maschwenk]

@maschwenk Would it be possible to share the full formula that's causing problems here? Neither me or @Bo98 can reproduce right now.

I think it's not a problem in Dependency.expand because even if the cache gets invalidated it gets added back in during the next recursive call. Requirement.expand is not really recursive so if the cache disappears in the middle of the method it just dies.

@apainintheneck Yep i’ll try to extract a reproducible repo, unfortunately current repro is in something I can’t make public.

[apainintheneck]

@maschwenk In that case, no worries. I just wonder if there's a better way to decide when to bust the cache. Fine with this as is though once tests pass. I looked at core formulae and casks and it seems like we don't bust the cache during brew dips normally which explains why this hasn't come up before.

[MikeMcQuaid]

Thanks @maschwenk!