Implement `brew desc`, with a cache #42281

hivehand · 2015-07-31T00:58:28Z

This is an attempt to recreate @telemachus’s brew desc tool, using the desc component now built into formulae. Like the original brew desc, it lets you look up formulae either by their exact names, or by searching through some combination of name and description.

Notes:

It follows the suggestion made by @mistydemeo, in the discussion over the pull request that added desc to formulae, and uses a cache for performance.
Said cache tries to be “pay-as-you-go”: the cache isn't created, or updated, until you either explicitly do so with brew desc --cache, or perform a search. This allows people who don’t expect to use brew desc to avoid paying ~5 extra seconds with every brew update.
A slight performance tweak to utils.rb’s maximum-length-determination logic, with input from @bfontaine, came along for the ride.

Caveats:

I have almost no personal experience with Kegs or Casks, and so can’t guarantee that desc and the cache work properly with them.
I’m not well-versed with brew’s model for handling ARGV, so it’s possible that I’m making a lot of unnecessary noise in the desc.rb command handling.

Open Questions:

brew search --desc is exactly equivalent to brew desc -d. I’m not sure it makes sense to preserve both, particularly since the former was never documented, but for the time being I’ve rewritten it to take advantage of the cache.
The cache-generation code is completely silent. I don’t know if we want to notify the user to the effect that “It’s OK! Something’s actually happening!” when the first attempt to run brew desc takes several seconds.

Feedback is more than welcome.

dunn · 2015-07-31T01:21:24Z

The tests are failing on Mountain Lion—are you using any Ruby that's not valid in 1.8?

hivehand · 2015-07-31T01:44:11Z

That’s entirely possible. I apologize. I’ll install 1.8.7 and see about finding and fixing the issue. (Off the top of my head, I'm guessing that it might be due to my use of proc rather than Proc.new, but we’ll see.)

hivehand · 2015-07-31T02:19:13Z

Aieeee, no. Close, but the actual culprit was my using { <sym_name>: <value> } instead of { :<sym_name> => <value> }. I’ll fix that, and update after proper testing.

bfontaine · 2015-07-31T07:30:08Z

Library/Homebrew/cmd/desc.rb

+  def desc
+    options = ARGV.options_only
+
+    if options.include?("--decache")


You can drop the parentheses here.

--decache feels bit jargon.

It was meant to be either a portmanteau of “delete” and “cache”, or analogous to “decompress”. I’m open to clearer suggestions, though. --delete_cache?

I’d prefer not to mix underscores and hyphens, so maybe --delete-cache.

MikeMcQuaid · 2015-07-31T07:37:04Z

@hivehand Can you show the output of time brew readall and time brew desc --cache? Thanks!

bfontaine · 2015-07-31T07:38:33Z

Library/Homebrew/manpages/brew.1.md

+    descriptions (`-d`) for `<pattern>`. `<pattern>` is by default  interpreted
+    as a literal string; if flanked by slashes, it is instead interpreted as a
+    regular expression. Formula descriptions are cached, and the cache is
+    created on the first search, making it slower than succeeding ones.


I don’t think “succeeding” is the good word here 😉

How about "subsequent" or "later"?

I agree that “succeeding” is not the good word. I like “subsequent”: I’ll use that.

telemachus · 2015-07-31T09:59:39Z

@hivehand Thanks for doing this!

xu-cheng · 2015-07-31T10:00:40Z

Library/Homebrew/cmd/desc.rb

+require 'descriptions'
+require 'cmd/search'
+
+USAGE =<<EOM


Since none of internal commands has a usage message. I think we should drop this. Manpage should be sufficiency.

Fair enough. I will make it behave like brew deps just raise a FormulaUnspecifiedError instead.

MikeMcQuaid · 2015-07-31T11:39:54Z

I recommend this be profiled with something like ruby-prof --printer=call_stack --file=call_stack.html -- brew desc --cache to try and tweak performance.

hivehand · 2015-07-31T14:00:33Z

The first benchmarks, as requested by @MikeMcQuaid:

$ time brew readall

real    0m2.721s
user    0m2.577s
sys 0m0.139s

$ time brew desc --recache

real    0m2.925s
user    0m2.775s
sys 0m0.143s

More to come as soon as I familiarize myself with ruby-prof.

I’ll be addressing and/or replying to specific comments shortly. Thank you all for the careful proofreading and detailed feedback!

bfontaine · 2015-07-31T14:13:33Z

Library/Homebrew/descriptions.rb

+
+    matchers = {
+      :name   => proc { |name, desc| name =~ regex },
+      :desc   => proc { |name, desc| desc =~ regex },


You can use _ for the parameters you’re no using, e.g. { |_, desc| desc =~ regex }.

hivehand · 2015-07-31T22:09:50Z

I just pushed an update. I believe that this addresses all issues raised and suggestions made, with the exception of @bfontaine’s proposal that I use case instead of a hash of procs when performing a search. (I’m not yet sure that I properly understand how to use case concisely here.)

On the profiling front,

ruby-prof --printer=call_stack --file=call_stack.html -- brew desc --cache

fails with

/usr/local/lib/ruby/gems/2.2.0/gems/ruby-prof-0.15.8/lib/ruby-prof/compatibility.rb:104:in `load': cannot load such file -- brew (LoadError)
    from /usr/local/lib/ruby/gems/2.2.0/gems/ruby-prof-0.15.8/lib/ruby-prof/compatibility.rb:104:in `start_script'
    from /usr/local/lib/ruby/gems/2.2.0/gems/ruby-prof-0.15.8/bin/ruby-prof:292:in `run'
    from /usr/local/lib/ruby/gems/2.2.0/gems/ruby-prof-0.15.8/bin/ruby-prof:340:in `<top (required)>'
    from /usr/local/bin/ruby-prof:23:in `load'
    from /usr/local/bin/ruby-prof:23:in `<main>’

which stands to reason, since brew is a shell script rather than Ruby. I will see if I can teach myself enough ruby-prof to obtain useful results; in the meantime, hints would be welcome.

xu-cheng · 2015-08-01T07:31:14Z

@hivehand I wrote a small script to help you profile homebrew.

If you install it with brew tap homebrew/dev-tools. You should be able to run brew prof desc --cache to generate the result.

MikeMcQuaid · 2015-08-01T10:38:54Z

Maintainers: please hold off merging this: I haven't had the chance to review it yet but would like it before we merge it.

xu-cheng · 2015-08-01T11:05:45Z

Library/Homebrew/descriptions.rb

+    @cache = {}
+    CSV.open(CACHE_FILE, 'w') do |csv|
+      Formula.map do |f|
+        name, desc = f.name, f.desc


f.full_name

MikeMcQuaid · 2015-08-15T17:18:15Z

@hivehand That approach sounds good!

1. Resurrect the late `brew desc` as a means of geting descriptions for an arbitrary set of formulae via the command line. 2. Create the Descriptions class to do most of the heavy lifting. 3. Update the man page.

Use traditional Hash-construction notation, and deal with the fact that Hash#select in Ruby 1.8.7 and earlier returns an Array of Arrays, instead of a Hash. While we're here: - Make `ruby desc` without arguments print usage, rather than all formulae. - Rename --cache to --recache, and update the manual accordingly. - Sort formulae by name when printing. - Flag formulae with missing descriptions when printing the list.

@bfontaine

- Add zsh autocompletion support. - Dispose of the verbose usage message. - Adopt too many tweaks and refinements courtesy of @bfontaine to enumerate. - Replace `--decache` with `--delete-cache` and, for consistency, `--recache` with `--create-cache`. - Update the man page accordingly.

- Use the update report to update only the affected formulae. - Also, refactor the cache-saving code.

- Also refactor using Filepaths

- Refactor update_cache, splitting cache addition and deletion into separate methods. Use those methods to update the cache when tapping or untapping. - Also revise update_cache, and move cache_fresh? while we're at it. - Realign methods' descriptions with reality.

Also remove the Descriptions::delete_cache method, since nothing now uses it.

hivehand · 2015-09-01T20:56:15Z

Apologies in advance if I’m being a pest, but are there any outstanding issues regarding this pull request? The changes have been working well for me, locally, for the past two weeks and change, with minimal impact on performance. I’d still be happy to share prof results, once I know where and in what format to submit them.

hivehand · 2015-09-02T23:54:25Z

And, of course, a day after my last comment, I discover the newly-added support for formula renaming in update.rb. I’ve added the corresponding support to the description-cache updating logic.

MikeMcQuaid · 2015-09-06T11:45:40Z

The diff looks good to me. If @xu-cheng is happy with it as-is then I'll do some more local testing and merge.

xu-cheng · 2015-09-06T11:58:29Z

LGTM

MikeMcQuaid · 2015-09-06T13:03:43Z

Library/Homebrew/cmd/desc.rb

+      searches = {
+        '-s' => :either,
+        '-n' => :name,
+        '-d' => :desc


Would be nice to also have --search, --name etc. here.

Added. (In the process, I replaced the somewhat convoluted Hash logic with a case statement, which should please @bfontaine. 😉)

MikeMcQuaid · 2015-09-06T13:04:39Z

A couple of final nits but otherwise looks and works great for me 👍

Add the '--search', '--name', and '--description' options, and update bash completion accordingly.

MikeMcQuaid · 2015-09-08T14:24:16Z

Thanks for the great work here @hivehand! Merged!

hivehand · 2015-09-08T15:33:13Z

Thank you, collectively, for the many excellent suggestions and your patience!

telemachus · 2015-09-08T21:48:55Z

@hivehand Thanks for getting this into brew.

hivehand · 2015-09-08T23:19:58Z

@telemachus, you’re welcome. Thanks for hopelessly addicting me to formula descriptions in the first place!

MikeMcQuaid · 2015-09-09T07:42:55Z

Such a great example of a feature that we couldn't have implemented well in core as a one-off that the community picked up and got it back into core as a full-fledged, improving feature. 👏 to both of you @telemachus, @hivehand.

dunn added the features label Jul 31, 2015

bfontaine reviewed Jul 31, 2015
View reviewed changes

xu-cheng reviewed Jul 31, 2015
View reviewed changes

bfontaine reviewed Jul 31, 2015
View reviewed changes

xu-cheng reviewed Aug 1, 2015
View reviewed changes

hivehand added 11 commits August 15, 2015 12:26

Add autocompletion support for brew desc

c608ebc

Tweak utils' maximum-length-computation efficiency

e724e21

Add brew desc and the Descriptions class

5c585e5

1. Resurrect the late `brew desc` as a means of geting descriptions for an arbitrary set of formulae via the command line. 2. Create the Descriptions class to do most of the heavy lifting. 3. Update the man page.

Ensure that HOMEBREW_CACHE exists before writing

803a171

Add incremental cache updates

05c00a6

- Use the update report to update only the affected formulae. - Also, refactor the cache-saving code.

Add determination of cache freshness

91799cd

- Also refactor using Filepaths

Remove blank lines and manpage minutiae

ff6546e

Remove the cache-manipulation flags

b30c659

Also remove the Descriptions::delete_cache method, since nothing now uses it.

Add support for renamings

660c126

MikeMcQuaid reviewed Sep 6, 2015
View reviewed changes

Add long search flags, and completion support

d24d6a1

Add the '--search', '--name', and '--description' options, and update bash completion accordingly.

MikeMcQuaid closed this in 84b5e7f Sep 8, 2015

hivehand deleted the redesc branch September 10, 2015 18:17

Homebrew locked and limited conversation to collaborators Jul 10, 2016

Implement brew desc, with a cache #42281

Implement brew desc, with a cache #42281

Conversation

hivehand commented Jul 31, 2015

dunn commented Jul 31, 2015

hivehand commented Jul 31, 2015

hivehand commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telemachus commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jul 31, 2015

hivehand commented Jul 31, 2015

Choose a reason for hiding this comment

hivehand commented Jul 31, 2015

xu-cheng commented Aug 1, 2015

MikeMcQuaid commented Aug 1, 2015

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 15, 2015

hivehand commented Sep 1, 2015

hivehand commented Sep 2, 2015

MikeMcQuaid commented Sep 6, 2015

xu-cheng commented Sep 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Sep 6, 2015

MikeMcQuaid commented Sep 8, 2015

hivehand commented Sep 8, 2015

telemachus commented Sep 8, 2015

hivehand commented Sep 8, 2015

MikeMcQuaid commented Sep 9, 2015

Implement `brew desc`, with a cache #42281

Implement `brew desc`, with a cache #42281