Load from both files and the API when running brew readall

[Rylan12]

Verification

• This issue's title and/or description do not reference a single formula e.g. brew install wget. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.

Provide a detailed description of the proposed feature

Running brew readall checks different things when running with and without HOMEBREW_NO_INSTALL_FROM_API set. We should either by default or with an option allow brew readall to load formulae from their files and also from the API to check both codepaths.

What is the motivation for the feature?

When running brew readall is CI, we're usually checking to make sure there are no loading errors/invalid formulae or casks. However, if a PR breaks loading from the API but we run brew readall on the formula/cask files, we might not notice the breakage until shipping to users.

How will the feature be relevant to at least 90% of Homebrew users?

By not breaking formula/cask loading.

What alternatives to the feature have been considered?

Doing nothing—this might be okay since we do have other tests that should check these things. They just might not catch all instances, which is why brew readall is a good option since it checks all current formulae/casks in official taps.

[Bo98]

This makes sense to do.

We test API generation via to_hash so I guess it's a case of feeding that round again? I.e. split off Formulary.load_formula_from_api to Formulary.load_formula_from_api_hash and do Formulary.load_formula_from_api_hash(formula.to_hash)

We want to make sure brew readall is as fast as possible (while still useful of course!) so hopefully that works ok. For example, actual JSON parsing and writing is quite slow so we don't do that, and is unlikely to pick up anything not already erroring on Ruby level (or brew CI for non-formula-level changes, which does the slower checks).

[apainintheneck]

I looked into this sort of thing a few months ago and while it's useful I gave up on it for performance reasons. Since then there have been some performance improvements related to the API JSON generation so maybe it's good enough for our purposes. Plus, we already call #to_hash on everything there already so some of the work is already done. @Bo98 makes a good point that writing to and reading from disk is just unnecessary overhead here and can be avoided.

In an ideal world we would test all possible formula/cask reading states whenever merging a PR:

1. User on previous tagged release using the new generated API JSON.
2. User on the new commit using the old generated API JSON.
3. User on the new commit using the new generated API JSON.

Testing all of those seems prohibitive if we just stuff that logic into brew readall and I still have those performance concerns I already mentioned.

I took a stab at something like this a few months and implemented a version which essentially monkey patches the API JSON so that it can be used to test the 3rd scenario mentioned above. It's a bit slow though but might be useful as a reference. Generating the formula API takes a lot longer than generating the cask API.

$ time brew api-readall-test
Generating formulae API ... and mocking formula API loader.
Read core formula and saw no failures!

Generating cask API ... and mocking cask API loader.
Read core casks and saw no failures!

________________________________________________________
Executed in  131.75 secs    fish           external
   usr time  114.82 secs  193.00 micros  114.82 secs
   sys time   12.72 secs  571.00 micros   12.72 secs

• https://github.com/apainintheneck/homebrew-dev-utils#brew-api-readall-test

[MikeMcQuaid]

However, if a PR breaks loading from the API but we run brew readall on the formula/cask files, we might not notice the breakage until shipping to users.

Has this happened before? I think if it has/hasn't adjusted the prioritisation of this.

I agree that I would like to see the test of API generation happening in homebrew/core before merge.

A middle ground would be that we at least test this for the formulae that have changed in the PR.

[apainintheneck]

We'd definitely run into problems in the past with changes to the JSON API causing things to not load correctly. I don't know if this type of check would have caught them though.

I don't know of any cases off the top of my head where a formula or cask file got changed, passed the readall check, and failed later on when loaded from the API.

[Bo98]

I've not really been aware of such incidents. Is there an example of one documented somewhere?

[Rylan12]

Has this happened before? I think if it has/hasn't adjusted the prioritisation of this.

Not one that's shipped, to my knowledge. The motivation for this PR, though, was that when working on #16292 I made some changes that broke formula loading without realizing, and running brew readall on my machine did not catch it since I had HOMEBREW_NO_INSTALL_FROM_API set. In this particular case, Homebrew/brew CI did catch the issue and so it didn't ship, but it raised the possibility.

[MikeMcQuaid]

I've not really been aware of such incidents. Is there an example of one documented somewhere?

The cases I'm thinking of are those where a Homebrew/homebrew-core or Homebrew/homebrew-cask change broke JSON generation on formulae.brew.sh. That was my motivation for e.g. Homebrew/homebrew-test-bot#920 which you improved upon @Bo98.

If we're in a situation where this is no longer possible for Homebrew/brew, Homebrew/homebrew-core and Homebrew/homebrew-cask: I don't think we need anything else here.

In this particular case, Homebrew/brew CI did catch the issue and so it didn't ship, but it raised the possibility.

If the above is addressed but this is a local development improvement: I think we don't need this.

[Bo98]

Homebrew/brew CI AFAIK is already sufficient so any changes on the brew side should be caught.

On the Homebrew/homebrew-core side, we test whether a valid formula hash can be produced. This is almost certainly sufficient to test if formulae.brew.sh generation will pass.

We don't test if the the output JSON is parsable by the API loader, but neither does formulae.brew.sh CI. The scenarios where that happen seem rare - the only one I can think of is deprecate! date: "2023-01-01", because: nil. Though that getting through code review would be questionable on its own.

With that said, I'm still think this isn't a bad idea to do. But only if it adds not too many seconds to the overall runtime.

[MikeMcQuaid]

With that said, I'm still think this isn't a bad idea to do. But only if it adds not too many seconds to the overall runtime.

Agreed. I also think it could be done for the individual formula modified rather than all of them in homebrew/core every time.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.