Okay, I've finally been able to rework this.

I've added Formulary::from_installed that loads a formula directly from the file in /opt/homebrew/opt/NAME/.brew/NAME.rb. I'm not thrilled with the naming of the method, so please suggest improvements.

This method differs from Formulary::from_keg, because the new method loads the formula file that was present at install time. So, any updates that have been made since will not be present in the Formula returned by from_installed.

To prevent potential issues, I've added new methods like Formula#installed_formula_runtime_dependencies that return these types of formulae, whereas Formula#formula_runtime_dependencies will still try to return the newest Formula available, regardless of what's installed.

One known issue is that running brew install foo will download JSON files for all of foo's dependencies, even those that are already installed and up to date. Eventually, this will be improved using the minimal API, but I've left that off for now.

Also, the only place that prefer_stub: is used right now is in named_args.rb, which can probably be cleaned up in a follow-up PR.

@MikeMcQuaid is this approach more what you were picturing?

The only thing I'm not sure I understand is how/why the behaviour changes here compared to e.g. from_rack or from_keg?

Here's an example. Say after I've installed hello, the license of the formula changes to MIT (but a new version/revision isn't released).

rack = HOMEBREW_CELLAR/"hello"
keg = Keg.new(rack/"2.12.2")

Formulary.from_rack(rack).license # => "GPL-3.0-or-later"
Formulary.from_keg(keg).license # => "GPL-3.0-or-later"
Formulary.from_installed("hello").license # => "MIT"

from_installed just reads the keg formula file. from_keg (which is called by from_rack) actually just calls factory and then adjusts a few small things to match the installed formula:

Should e.g. from_keg perhaps be adjusted to make the changes you're looking for here?

Maybe, but I think I'll need to go through all the current calls to from_keg to make sure that the breaking changes won't cause a problem.

Looks ok but one concern I do have is this might be just introducing a problem we have with casks: Ruby syntax can break (typically via deprecations). One of the most common bug reports we receive in homebrew-cask is someone can't uninstall their cask because the Ruby file can't be read anymore due to removed DSL. We're trying to move away from Ruby files in casks now that we install a metadata JSON file, but needs further work due to uninstall_preflight.

Previously this wasn't an issue with formulae as keg Ruby files are normally only read at install time (so will always be the "latest") and also only does so if postinstall_defined is true (so only ~250 formulae).

Does the tab have enough information to get the information we need here? If so, can we use that? If not, is it feasible to add more information to that?

Looks ok but one concern I do have is this might be just introducing a problem we have with casks: Ruby syntax can break (typically via deprecations). One of the most common bug reports we receive in homebrew-cask is someone can't uninstall their cask because the Ruby file can't be read anymore due to removed DSL. We're trying to move away from Ruby files in casks now that we install a metadata JSON file, but needs further work due to uninstall_preflight.

Previously this wasn't an issue with formulae as keg Ruby files are normally only read at install time (so will always be the "latest") and also only does so if postinstall_defined is true (so only ~250 formulae).

Good point, thanks for pointing that out

Does the tab have enough information to get the information we need here? If so, can we use that? If not, is it feasible to add more information to that?

For this PR, the main information that's needed from installed formulae is runtime dependencies and whether they're installed or not, which can be figured out from the tab.

I'll need to look into it more

from_installed just reads the keg formula file. from_keg (which is called by from_rack) actually just calls factory and then adjusts a few small things to match the installed formula:

@Rylan12 I suspect this logic predates us installing formulae into keys 😂

I think it's worth considering just making what you're proposing the new functionality for for_keg. It's very much an internal API anyway.

Maybe, but I think I'll need to go through all the current calls to from_keg to make sure that the breaking changes won't cause a problem.

That's fair. I do trust our test suite plus master build folks to tease this out, though.

Looks ok but one concern I do have is this might be just introducing a problem we have with casks: Ruby syntax can break (typically via deprecations). One of the most common bug reports we receive in homebrew-cask is someone can't uninstall their cask because the Ruby file can't be read anymore due to removed DSL. We're trying to move away from Ruby files in casks now that we install a metadata JSON file, but needs further work due to uninstall_preflight.

@Bo98 we need to do better at handling these errors in Casks. Formulae have been a lot more rock-solid in this way because we don't rely on e.g. a formula file being valid at uninstall time. We shouldn't avoid this approach just because Casks do it poorly.

Previously this wasn't an issue with formulae as keg Ruby files are normally only read at install time (so will always be the "latest") and also only does so if postinstall_defined is true (so only ~250 formulae).

@Bo98 How much is this about pouring from bottles vs. building from source?

Does the tab have enough information to get the information we need here? If so, can we use that? If not, is it feasible to add more information to that?

The tab doesn't solve all our backwards-compatibility/deprecation problems. Arguably it makes them worse because we don't really have a meaningful way to deprecate tabs.

@Bo98 we need to do better at handling these errors in Casks. Formulae have been a lot more rock-solid in this way because we don't rely on e.g. a formula file being valid at uninstall time. We shouldn't avoid this approach just because Casks do it poorly.

This already came up before - maybe Belgium or one of the maintainer calls? There was some consensus towards moving towards the formula model by migrating away from preflight in casks in favour of better DSL and thus remove the need to even load Ruby when uninstalling casks. We started with #17554 (to cover every DSL that's not preflight) and #20421 is the latest example of a step towards removing the need for preflight.

Some of that reasoning was because we have already have attempted error handling in formulae for other areas and this has typically not performed very well or at least not well enough to reliably always work. We do try for brew extract with a large (and not entirely complete anymore) monkey patch:

And for FormulaVersions:

Though in the latter case we just simply skip reading altogether as when a Ruby file fails to load it's hard to recover. Refusing to uninstall a cask because it's unreadable isn't much better of a situation.

@Bo98 How much is this about pouring from bottles vs. building from source?

I'm talking entirely about the pour from bottle case.

If you want to talk about build from source, that's every formula but that's again only at install time and we download the latest from GitHub so not really an issue there.

The tab doesn't solve all our backwards-compatibility/deprecation problems. Arguably it makes them worse because we don't really have a meaningful way to deprecate tabs.

I disagree as the syntax of the tab is much easier to handle backwards compatibility for. Where removing DSL always causes a Ruby error, we have successfully removed things from the tab over the years without even needing a compatiblity layer. We also have a homebrew_version key that we already use for compatibility purposes in advanced cases (e.g. to skip reading parts of old tabs).

Some of that reasoning was because we have already have attempted error handling in formulae for other areas and this has typically not performed very well or at least not well enough to reliably always work.

I disagree with this really. It works very reliably for formulae given how regularly we're deprecating DSLs. It is not 100% reliable and no design ever will be.

Though in the latter case we just simply skip reading altogether as when a Ruby file fails to load it's hard to recover. Refusing to uninstall a cask because it's unreadable isn't much better of a situation.

I agree, it's a worse situation. We should instead be, in the short-term, using the newest cask's uninstall if the older one cannot be read. In the longer-term, we should aim to handle this more gracefully but we can't really take an approach that requires us to never ever break backwards compatibility (including in the JSON tab/receipt files: we have and will need to break backwards compatibility there too).

I disagree as the syntax of the tab is much easier to handle backwards compatibility for. Where removing DSL always causes a Ruby error, we have successfully removed things from the tab over the years without even needing a compatiblity layer. We also have a homebrew_version key that we already use for compatibility purposes in advanced cases (e.g. to skip reading parts of old tabs).

I don't think the homebrew_version conditional version code having to stick around indefinitely is any more graceful than what we're doing for formulae.

The main issue here is a design one: Casks needing to be able to read a Ruby file indefinitely to uninstall is now a poor design choice (it may not have been originally) that's incompatible with our deprecation policy.

Anyone this is all pretty heavily off-topic here.

@Bo98 If you have specific objections with specific lines of code in this PR and how they will break things: please leave comments on them. If it's a general concern with the approach: please specifically propose another one in enough detail that it can be compared with this one.

The main issue here is a design one: Casks needing to be able to read a Ruby file indefinitely to uninstall is now a poor design choice (it may not have been originally) that's incompatible with our deprecation policy.

I'm confused - that's exactly the point I'm making. I'm saying the approach in this PR of having to read a Ruby file indefinitely for formulae wouldn't be great either. Every point I've made here has been centred around this.

I've also been saying the Cask way of doing things sucks and we've made some progress in moving away from reading Ruby files there.

I'm confused - that's exactly the point I'm making. I'm saying the approach in this PR of having to read a Ruby file indefinitely for formulae wouldn't be great either.

@Bo98 Can you elaborate on what specific changes are concerning? The code on this PR does not seem to be adding additional cases where we're reading Ruby from files where we've been explicitly reading them from e.g. JSON before? Again: can you comment on specific lines on code where you see specific behaviour changes and explain in what circumstances (ideally with at least vaguely reproducible test cases) that you see them failing?

I've also been saying the Cask way of doing things sucks and we've made some progress in moving away from reading Ruby files there.

Reading Ruby files indefinitely for formulae is much less problematic than casks. If we're doing an install or upgrade we can use the newer version instead. At uninstall time we can just nuke the keg and the links to it.

With Casks: by design there's non-trivial logic that always wants to use the old version of the Cask at uninstall time. This can make things blow up at uninstall time (something I've not seen in a while that hasn't been very quickly fixed, worst case has been some warnings that are repeated unnecessarily).

If Casks were rock-solid and entirely using the JSON-based approach and we're having new problems with them AND we were proposing moving formulae further away from that approach AND formulae had the same uninstall characteristics: yes, that would be a bad idea.

As-of now, though: this is a piece in trying to get a much more minimal JSON API released soon. @Rylan12 is on a time-limited project here and we're running the very real risk in preventing this work from shipping at all if we're going to blow up the complexity or introduce uncertainty.

Just to repeat and be super explcit, what I'd love to see here:

• analysis of individual lines of code that are changed and examples that Rylan/I could run on our machines before/after this PR to see "oh, yes, this causes an issue or regression"
• a balancing of risk vs. reward here. One of the largest complaints about Homebrew is it's slow. We're trying to mitigate that here. If the primary concern was deprecation-based breakage: we wouldn't be risking that. In this case it seems (to me currently, could be wrong) that we've got speculative/potential deprecation-based downsides that are currently putting a fairly major roadblock in making concrete, measurable performance for all users in the new internal JSON API

Hope the context is helpful. Happy to answer any more questions here or privately as desired ❤️

I'm confused - that's exactly the point I'm making. I'm saying the approach in this PR of having to read a Ruby file indefinitely for formulae wouldn't be great either.

@Bo98 Can you elaborate on what specific changes are concerning? The code on this PR does not seem to be adding additional cases where we're reading Ruby from files where we've been explicitly reading them from e.g. JSON before? Again: can you comment on specific lines on code where you see specific behaviour changes and explain in what circumstances (ideally with at least vaguely reproducible test cases) that you see them failing?

This PR adds Formulary.from_installed which loads the Ruby file from the installed keg only (FromKegLoader) whereas before it would just load the latest via Formulary.factory (Formulary.from_keg/from_rack also calls factory internally). This is now called in various places, including cleanup.rb (for autoremove). The PR title confirms it is using the installed keg formula file and the PR description confirms this affects brew uninstall.

As-of now, though: this is a piece in trying to get a much more minimal JSON API released soon. @Rylan12 is on a time-limited project here and we're running the very real risk in preventing this work from shipping at all if we're going to blow up the complexity or introduce uncertainty.

Rylan seemed to understand my comment fine based on his reply and 👍. I trust if he didn't that he would have either asked for clarification or, given I did not hit "request changes", can still choose to waive the non-blocking comment. After making my initial comment, I made sure to wait for Rylan's reply to confirm if he understood or if I was mistaken (which I always can be, and still can be here).

Given there seems to be a time urgency to ship this ASAP that I was not aware of and I seem to have caused huge problems here, I'll step back from all further code review for now and will self-reflect privately.

Given there seems to be a time urgency to ship this ASAP that I was not aware of and I seem to have caused huge problems here, I'll step back from all further code review for now and will self-reflect privately.

Just to be super explicit here: you definitely haven't caused huge problems here. We really value your feedback here, it's just that I was unable to translate your feedback into direct action items.

I'd like to apologise for my suboptimal communication here too: it has fallen short of the standards I should be held to. Thanks for your input and we'll move forward with it in mind ❤️

Thanks for the discussion @Bo98 and @MikeMcQuaid ❤️

I've just pushed a change that I hope will help alleviate the concern here.

Now, from_rack and from_keg will first try to load directly from the installed keg file. If this is unsuccessful due to a FormulaUnreadableError, fall back to a standard call to factory.

The reason we are trying to avoid factory is to avoid unnecessary downloading of the formula JSON file (eventually the manifest file). However, in the case where a previously installed formula file is unreadable for whatever reason, I think the best solution is just to download the newest version regardless. Eventually, we could probably cache the downloaded JSON file to avoid needing to redownload it each time the invalid formula file is loaded, but that's a future improvement.

I still want to fiddle a little bit more locally before marking this as ready for review, but I think it's very close, assuming the code looks good to everyone

As a consequence of this PR, Formulary::resolve now also uses keg formula files in most cases, which means this PR breaks brew outdated (even without HOMEBREW_USE_INTERNAL_API). I will continue to look into the best way to handle this, but just wanted to post a progress update so it's clear this PR is being worked on.

I know there are still some test failures that I'm trying to pin down, but I think this is now mostly there.

Now, the difference between the Formulary methods is summarized as follows;

• factory retrieves the latest available version of a formula
• resolve, from_rack, and from_keg attempt to load the installed version of a formula, and fall back to factory

Future work: improving brew outdated and brew upgrade with the internal API (it just made sense to push that to a second PR

Thanks @Rylan12! Will look in more detail tomorrow and happy to help with test failures if you get blocked.

Now, the difference between the Formulary methods is summarized as follows;

Sounds good.

• resolve, from_rack, and from_keg attempt to load the installed version of a formula, and fall back to factory

resolve is the only one that maybe could/should default to Not From The Keg, if that's a behaviour change.

Future work: improving brew outdated and brew upgrade with the internal API (it just made sense to push that to a second PR

Makes sense, thanks again!

resolve is the only one that maybe could/should default to Not From The Keg, if that's a behaviour change.

@MikeMcQuaid resolve calls from_rack (and therefore from_keg), so it's a consequence of modifying the from_keg behavior. We can definitely avoid that, but it might mean it's better to leave from_keg as-is and add a new method like what I had originally called from_installed.

Are there specific cases that you're worried about with the modified resolve?

@MikeMcQuaid resolve calls from_rack (and therefore from_keg), so it's a consequence of modifying the from_keg behavior. We can definitely avoid that, but it might mean it's better to leave from_keg as-is and add a new method like what I had originally called from_installed.

Seems fine then. Would rather avoid adding new methods if we can avoid it.

Are there specific cases that you're worried about with the modified resolve?

Nah, just thought if it was very easy it'd be worth persisting existing behaviour. No big deal.

The installed_dependents checks are still failing, so I'm looking into it. Probably won't happen until tomorrow, though. Feel free to take a look if you'd like

Okay, as per usual, the fix ended up being incredibly simple, yet only apparent after days of troubleshooting 🤦

Anyway, I think this is ready now! I want to wait until just after the next release to merge in case there are issues