Use API for description searches

[rrotter]

Both brew search --desc and brew desc --search use API for cask and formula searches unless --eval-all or HOMEBREW_EVAL_ALL set. Description searches do not use the description cache or eval any formulas/casks.

With --eval-all, description search reverts to the old behavior.

This will resolve #16237.

• 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?

---

This is intentionally a draft, looking for comments on this direction.

[MikeMcQuaid]

This is looking great so far, nice work @rrotter!

Assuming you agree with suggestions: only thing I think is needed is to get some test coverage and to decide how to message the non-eval-all case if there's other taps.

[apainintheneck]

Does this change the behavior of cask description searches? I remember previously that it was using both the names and descriptions in search and it looks like it's now only using descriptions. This is useful when people want to surface cask results for alternative names.

[apainintheneck]

It'd also be nice to get some benchmarks at some point too.

[rrotter]

It'd also be nice to get some benchmarks at some point too.

In my testing this search takes about 1s, and the old method was 0.5s cached and 5s uncached. (M2 Mac Studio)

---

edit, to be more precise:

~ % rm ~/Library/Caches/Homebrew/*descriptions.json # rm desc cache
~ % time brew search --desc foo --eval-all > /dev/null # uncached search w/ old codepath
brew search --desc foo --eval-all > /dev/null  5.15s user 0.32s system 95% cpu 5.734 total
~ % time brew search --desc foo --eval-all > /dev/null # cached search w/ old codepath
brew search --desc foo --eval-all > /dev/null  0.28s user 0.07s system 65% cpu 0.539 total
~ % time brew search --desc foo > /dev/null # cached search w/ new codepath
brew search --desc foo > /dev/null  0.74s user 0.12s system 82% cpu 1.048 total

[rrotter]

Does this change the behavior of cask description searches?

Good catch. This is using exactly the same code to perform a search, but I rewrote the code that generates the hash that is being searched. I didn't realize that the data structure was slightly different for formulae and casks. The cask data structure actually has a third field.

# formula description hash
{"name"=>"description"}
# cask description hash
{"token"=>["list, of, names", "description"]}

Fixed in my latest commit.

[rrotter]

Here's the current warning language that I'm printing if there are third party taps installed. Can't paste w/ color here, but I'm using opoo.

~ % brew desc -s foo
==> Formulae
Warning: Use `--eval-all` to search 6 additional formulae in third party taps.
chibi-scheme: Small footprint Scheme for use as a C Extension Language
duktape: Embeddable Javascript engine with compact footprint
foremost: Console program to recover files based on their headers and footers
...

Trying to keep it clear and concise. Could add something about why you might not want to use --eval-all, but that would get long. Currently just under 80 chars (obv length varies depending upon number of external formulae).

[rrotter]

I'm a little stuck on testing:

• The one test I added (checks that search --desc works w/o --eval-all) just checks that the search completes w/o failure, but doesn't check the search results because, AFAIK, we don't have a way to mock the API to include "testball". Maybe this is the best I can do for now? (probably relates to Run the test suite in the default API mode #16895)
• I'd like to test that the warning is printed or not printed as appropriate. I think the way to do this is to add a test tap w/ a test formula in it, but I can't quite get this to work (and I don't see any similar examples in the existing test code). Here's what I've got so far:

tap = Tap.from_path(setup_test_tap)
setup_test_formula "testballexternal", tap: tap

Failure/Error: setup_test_formula "testballexternal", tap: tap

Errno::ENOENT:
   No such file or directory @ rb_sysopen - /private/tmp/homebrew-tests-20240629-39807-bbynmd/prefix/Library/Taps/homebrew/homebrew-foo/Formula/testballexternal.rb

Any tips would be appreciated. Other than adding tests I should be done pushing on this branch for now.

[apainintheneck]

The performance looks good and adding cask names was a simple fix which was nice.

I'm a little stuck on testing:

• The one test I added (checks that search --desc works w/o --eval-all) just checks that the search completes w/o failure, but doesn't check the search results because, AFAIK, we don't have a way to mock the API to include "testball". Maybe this is the best I can do for now? (probably relates to Run the test suite in the default API mode #16895)
• I'd like to test that the warning is printed or not printed as appropriate. I think the way to do this is to add a test tap w/ a test formula in it, but I can't quite get this to work (and I don't see any similar examples in the existing test code). Here's what I've got so far:

tap = Tap.from_path(setup_test_tap)
setup_test_formula "testballexternal", tap: tap

Failure/Error: setup_test_formula "testballexternal", tap: tap

Errno::ENOENT:
  No such file or directory @ rb_sysopen - /private/tmp/homebrew-tests-20240629-39807-bbynmd/prefix/Library/Taps/homebrew/homebrew-foo/Formula/testballexternal.rb

Any tips would be appreciated. Other than adding tests I should be done pushing on this branch for now.

In terms of integration tests, nobody's been able to find a good way to mock and sign the API JSON yet. I think it'd make more sense to add unit tests for Homebrew::Search.search_descriptions instead. It would likely require mocking the API though which we do already in a few places Certainly doable though maybe a bit tedious. That being said unit tests would be nice to have but aren't a blocker here since the surface area of the change is pretty small.

• Some API unit tests for reference
  

  brew/Library/Homebrew/test/formulary_spec.rb

  Line 271 in fda555d

  context "when loading from the API" do

  
  

  brew/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb

  Line 3 in fda555d

  RSpec.describe Cask::CaskLoader::FromAPILoader, :cask do

[rrotter]

Pushed a couple of tests, including an API mock. They do pass, but CI is failing on the linter, see comment above.

Other than the lint, does this look okay?

[apainintheneck]

Pushed a couple of tests, including an API mock. They do pass, but CI is failing on the linter, see comment above.

Other than the lint, does this look okay?

@rrotter Yep, the code looks good. I left an optional nit as well but feel free to ignore that.

[rrotter]

Thanks, @apainintheneck, added your suggestions, squashed and rebased.

[apainintheneck]

LGTM, thanks for updating this!

[MikeMcQuaid]

Thanks @rrotter!