formula_installer: use cached fetched formula instance when available #15778
Conversation
| sig { returns(T.nilable(Formula)) } | ||
| def previously_fetched_formula | ||
| # We intentionally don't compare classes here. | ||
| self.class.fetched.find do |fetched_formula| | ||
| fetched_formula.full_name == formula.full_name && fetched_formula.active_spec_sym == formula.active_spec_sym | ||
| end | ||
| end |
There was a problem hiding this comment.
Nit: Would it make sense to memoize this if it could get called 3 times but we don't expect the result to change? It should be a cheap check either way so not a big deal.
There was a problem hiding this comment.
Yeh, that'd be another option instead of using a e.g. Hash here
There was a problem hiding this comment.
Looks good so far, thanks @Bo98! If you're confident in this fixing #15765 (comment): could also roll those changes into this PR, too.
| @@ -1179,9 +1182,17 @@ def fetch_dependencies | |||
| deps.each { |dep, _options| fetch_dependency(dep) } | |||
| end | |||
|
|
|||
| sig { returns(T.nilable(Formula)) } | |||
| def previously_fetched_formula | |||
| # We intentionally don't compare classes here. | |||
There was a problem hiding this comment.
Would be good to explain why we don't.
There was a problem hiding this comment.
@Bo98 Merging as-is but would love you to address this (and any other comments you feel like) in a follow-up when you get the chance! Thanks for creating it ❤️
There was a problem hiding this comment.
(I would do it myself but I'm not sure I know the answer 😅)
There was a problem hiding this comment.
Quick answer is: while each formula definition is its own subclass, a API and from-source formula class are separate classes but we want to equate them to be the same thing here given mixing bottle and from-source installs of the same formula within the same operation doesn't make sense.
| sig { returns(T.nilable(Formula)) } | ||
| def previously_fetched_formula | ||
| # We intentionally don't compare classes here. | ||
| self.class.fetched.find do |fetched_formula| |
There was a problem hiding this comment.
Might be nicer to use a Hash instead of Set to make this sort of lookup a bit nicer.
| sig { returns(T.nilable(Formula)) } | ||
| def previously_fetched_formula | ||
| # We intentionally don't compare classes here. | ||
| self.class.fetched.find do |fetched_formula| | ||
| fetched_formula.full_name == formula.full_name && fetched_formula.active_spec_sym == formula.active_spec_sym | ||
| end | ||
| end |
There was a problem hiding this comment.
Yeh, that'd be another option instead of using a e.g. Hash here
I've tried this PR with a cherry-pick of #15765; can confirm it fixes the problem from my comment. |
Had just about enough energy to get this one done tonight too.
We detect whether we want an API formula or need a Ruby source formula fairly late on (after the formula installer has been first created), so we swap the formula instance within the installer.
Unfortunately, dependencies use separate instances of the installer for their fetch and install stages so we can't rely on the
fetchmethod being called like we could normally. The dependency handling should probably be refactored to not create separate instances like that as it'll eliminate these type of bugs occurring in the future (or we alternatively commit to separate fetcher and installer classes), as well as help tackle the growingly fragile amount of assumptions FormulaInstaller currently has on the way it is called (e.g. some steps are incorrectly cached without considering some options, but we get away with it based on how things are set up on the CLI side and how things like formula options are rarely used in complex dependency trees). But that's a non-trivial task that I definitely won't have time for this week.Fixes #15765 (comment)