Add description field to Formulae

[adzenith]

• Add a desc field to the Formula class
• Fill out the desc field for each formula in Homebrew/homebrew
• brew audit checks for desc
• brew info shows the desc
• brew search --desc searches on desc (preliminary)

[telemachus]

I would love for this to go in with 100% coverage for formulas in master. @adzenith Can you add these two to the fork, please?

• stiftlint: Experimental tool to enforce Swift style and conventions
• xtail: Watch growth of multiple files or directories (like tail -f)

[adzenith]

Updated, ran the script again, and committed. So unless anyone adds more formulae while we chat, we're good to go. Thanks!

[pthariensflame]

I am greatly in favor this pull request! This is the first big step to including brew desc in Homebrew core!

[adzenith]

@pthariensflame: We've been discussing just folding brew desc into brew info and brew search, which this pull request does. You can get the description by running brew info, and you can search names normally with brew search or descriptions with brew search --desc.
The only thing that's missing here is the ability to search both the names and the descriptions simultaneously (is that something that's desired?).

[pthariensflame]

@adzenith No, I think the way it currently is in this PR is fine. :)

[luzpaz]

👍 for this feature

[telemachus]

The only thing that's missing here is the ability to search both the names and the descriptions simultaneously (is that something that's desired?)

My two cents: Yes, that would be a good thing. It's been the default behavior of brew desc, so many of the current users of that will likely expect it. It wouldn't be too hard to build variable searches into brew search, however.

Here's what I'm imagining:

• brew search checks both names and descriptions
• brew search --name checks only names
• brew search --desc checks only descriptions

What do other people think of this? (I will likely begin to work up a version of this soon.)

[adzenith]

That sounds good to me. If I'm using brew search, I probably don't know the name of what I'm trying to find (why then would I be searching for it?) so it seems like it would be nicest to search name and desc by default.

[jacknagel]

Please don't commit changes to every formula at once, it will start a massive CI job that will always timeout. It needs to be done incrementally over time.

[telemachus]

@jacknagel The nature of this change is that it covers all formulas. (The change was originally put off partly because it wouldn't cover all the formulas.) Isn't it possible to adjust the CI job rather than force the PR to be split?

If not, how many formulas at once is a reasonable guideline to change?

Or alternatively since the changes to the formulas here can't break anything (I don't think), then couldn't it be pulled regardless of the CI failure?

[adzenith]

Or, alternatively, the CI job has already been cancelled. Are we ok with the commit staying in the PR?

[mistydemeo]

brew search checks both names and descriptions

I don't think this is going to be feasible, unfortunately - a desc search that requires instantiating every Formula object is much slower than the filename-based search used by Formula.names. Making this the default would slow down brew search substantially. For example, compare the following results (with networked tap searches disabled in brew search to ensure we're comparing apples to apples):

$ time brew search --desc mdx
mdxmini: Plays music in X68000 MDX chiptune format
        5.32 real         3.99 user         0.48 sys
$ time brew search mdx
mdxmini
        0.18 real         0.14 user         0.03 sys

[jacknagel]

There are several reasons that we don't make changes to every formula at once, CI Is only one of them. It will need to be rolled out in increments so that the brew update output doesn't show 3000 changed formulae, for example.

I killed the CI job manually. Please don't push again without removing that commit first.

[telemachus]

A few questions:

1. Is the idea of adding descriptions to every formula (in increments) something that the team is willing to consider?
2. Is the team open to changing brew search to offer a search of descriptions (not as the default, based on @mistydemeo's point)?
3. Is the team open to including the descriptions in the output of brew info?
4. Does it make sense to include a check for descriptions in brew audit going forward, as this PR adds?

If the answer to those are no, then there's not much sense moving forward. If the answers are yes, but with heavy changes to this PR, then that would be helpful to know.

[mistydemeo]

I'm personally very much in favour of finding a way to integrate descriptions to every formula. I think making it an option to brew search is a good first step, and if we can make it both performant and consistent with user expectations then moving forward with that would be great!

[mistydemeo]

As proposed in IRC, a solution may be to generate a cache in HOMEBREW_CACHE/desc.cache when, say, brew update is run; for example:

require "csv"
csv_cache = Formula.map {|f| CSV.generate_line [f.name, f.desc]}.join
File.open("#{HOMEBREW_CACHE}/desc.cache", "w") {|f| f.write csv_cache}

# ...

# and then in cmd/search
query = query_regexp(ARGV.next)
descriptions = CSV.parse File.read "#{HOMEBREW_CACHE}/desc.cache"
descriptions.each do |name, desc|
  puts "#{name}: #{desc}" if desc =~ query
end

This performs well:

$ time brew search --desc mdx
mdxmini: Plays music in X68000 MDX chiptune format
        0.33 real         0.21 user         0.07 sys

[jacknagel]

To be clear, I haven't expressed any opinions about this proposal, I'm simply pointing out the reality of (a) our current CI situation and (b) how we roll out changes to a large number of formulae (in this case, all of them). It's hard to review 3000 formulae at a time, even if the change seems harmless.

[telemachus]

@jacknagel Fair enough. But I'd like to move forward. So if you have any opinions about the general idea, please share. (Also, if you have a rough estimate of how many formulas it would be best to change at once, let us know that too. )

[xu-cheng]

Although I'm very much like brew-desc, IMHO putting desc information inside Formula is not the best practice. If we end up to search in the cache for performance, why we bother to put it inside Formula. At the same time, I think if we come a better way to store and query this kind of meta data(an official homebrew-metadata tap?), it may benefit beyond brew-desc. (e.g. homebrew-command-not-found created by @bfontaine)

To be clear, I'm in favor of such feature and integrate it into brew search/info. But I think we need a better approach.

[chdiza]

I strongly agree with @xu-cheng that the descriptions shouldn't go in the formulas.

[MikeMcQuaid]

I think we should add descriptions to formulae and to the DSL.

@jacknagel Fair enough. But I'd like to move forward. So if you have any opinions about the general idea, please share. (Also, if you have a rough estimate of how many formulas it would be best to change at once, let us know that too. )

I think the best thing to do in this PR would be to add the DSL and change a single formula that's quick to build (e.g. ack). Then after that create a branch that changes every formula and, as it's a DSL-only change, as long as brew readall passes I'll just merge the change as-is.

The problem we've had in these cases before which may need addressed before is that if we change every formula then they will all show up in brew update which is less than ideal.

[MikeMcQuaid]

I think if we come a better way to store and query this kind of meta data(an official homebrew-metadata tap?), it may benefit beyond brew-desc. (e.g. homebrew-command-not-found created by @bfontaine)

I do think we should add support for storing additional metadata like this and having a search cache and I think it'll provide other benefits. I don't think adding a new single string DSL field to formulae needs this, though, and I'm happy to add this to search as brew search --desc for now and it to just be much slower if you pass that option. I'm aware that big changes like this from non-maintainers will basically never get merged if we put too many requirements on them and I think that would be a shame. Better that we add this in a way that won't break any existing functionality and iterate from there.

So tl;dr:

• Let's merge the DSL change and the change to a single formula for now
• Let's work out a way to update every formula without spamming everyone's brew update
• Let's just merge all the commits without a PR job when that's the case

[telemachus]

@MikeMcQuaid Thanks for the plan.

For the time being, I will continue to add new items (and remove deleted items) from brew desc until all of the formulas' descriptions are moved over. That way, people from the Homebrew team can use @adzenith's Python script to bring them into newer formulas.

Once all the descriptions are moved fully into formulas, I'll likely retire brew desc. Thanks everyone for suggestions and feedback.

[adzenith]

Thanks for all the comments! From what I've read (and what @MikeMcQuaid summarized), I will remove all of the descriptions except for ack's, and leave all the other commits as-is. This will add desc to the DSL, add brew search --desc, and add desc to brew info and brew audit. If it gets merged, we can merge in the rest of the descriptions as well.

If that all goes well, we can then look at what @mistydemeo was saying about caching descs, and potentially search descriptions by default.

Sound good to everyone? I'll fix up the commits.

[adzenith]

Here are the new commits, only touching ack this time. I've also moved the brew audit changes to the end in case we don't want to merge those yet—it'll show every formula as needing a desc

[MikeMcQuaid]

I've also moved the brew audit changes to the end in case we don't want to merge those yet—it'll show every formula as needing a desc

Let's make that only apply to brew audit --strict for now and we can make it apply to brew audit globally after we've merged them in for all of core.

[MikeMcQuaid]

Add a space between # and M

[adzenith]

Fixed the comment, and now brew audit skips checking the desc unless it's --strict. And rebased onto origin/master.

[xu-cheng]

A minor question here. Which style is better? Personally, I prefer homepage is at the top.

[adzenith]

The homepage is more canonical, so to speak, but the desc is more relevant (to me as someone who doesn't know what the formula is). That's why I put the desc first. Obviously it's easy to move second if that's preferred. I put it first in brew create to match this, but can easily move it all below.

[DomT4]

My own personal strong preference is that we put the desc as the last line of the initial block, i.e. under either the sha or revision. For some reason, having it as the top line is making me very uncomfortable, but I'm just weird.

[adzenith]

I would prefer to keep it above the sha and version, because the sha and version apply to a single version of the software, whereas the description deals with the software regardless of version (like the homepage). Is there a reason you'd want to have it down there?

[DomT4]

For me at least, the initial formula block is in order of things I need to know most, that are most important to most people/formula authors.

I want to know where the formula is from first, I want to know where it's being download from next, and then I want to know what the checksum is for security. The formula description feels less important to me than any of those three things.

[bfontaine]

I first was in favor of “homepage then desc then the rest” but I read @adzenith’s comment and now agree with the “description first, then homepage, then the rest”. The desc is indeed the more relevant here, and the homepage’s URL doesn’t usually give you much information about the software (say the formula is “foobar”, the URL will likely be foobar.sourceforge.net or some-academic-website.edu/~doe/software/foobar) and you can open it quickly in your browser with brew home <formula>.

[telemachus]

My two cents: Let's please not let this PR get bogged down with bike-shedding. As far as I know, brew does not enforce any rules about the order of items in the top of the class. So I can't see why we need to worry so very much about it. (My point being that once we start asking for opinions, everyone will have one, but really: is it that important?)

[DomT4]

As far as I know, brew does not enforce any rules about the order of items in the top of the class.

This isn't quite true. It's not in the audit but the only things that move around in the initial block unless there's need for stable do is the version line, if there is one. That can go either below the URL or below the sha. But generally, although not audit-enforced, deviation from that tends to get asked to change. Whether that's just because formulae become a pain to update/compare/etc if all of them have different layouts or whether that's for another reason, I defer to the people who wrote the implementation.

[telemachus]

This isn't quite true....

I didn't realize. Thanks for clarifying.

In that case, I suppose if we could get a quick ruling from the team, that would be great. I don't think it much matters where they pick, but I agree that having a convention is useful.

Again, my goal is to avoid protracted delay of the PR being merged because of bike-shedding.

[MikeMcQuaid]

Yes, let's hold off the ordering discussion for now and just leave it as-is.

[telemachus]

Thanks for doing this, @adzenith. I'm glad this is finally happening.

[adzenith]

My pleasure! And hey, thanks for making brew desc in the first place! 🌟

[MikeMcQuaid]

Let me know when and how and I can submit PRs for the rest of the descriptions.

Hi @adzenith! You can do this now that #40090 is merged but instead of submitting a PR just create a branch on your fork with all the descriptions and I'll merge them in (to avoid killing the bot unnecessarily).

[MikeMcQuaid]

branch on your fork with all the descriptions

Oh, and post a link here to that branch. Thanks!

[telemachus]

@MikeMcQuaid First, thanks you so much for your efforts to get this merged. I really appreciate it. Second, if you could, please ping me when you merge @adzenith's branch. That way, I can begin the process of phasing out brew desc.

Thanks again, @adzenith as well.

[MikeMcQuaid]

@telemachus A pleasure. Still need to get anytap merged in here too though 😉

[telemachus]

@MikeMcQuaid Funny you say that since I've been noodling with it just now.

[MikeMcQuaid]

@telemachus It should be relatively easy now to add a --any argument to brew tap so you can brew tap --any telemachus myjunk https://telemachus@bitbucket.org/telemachus/brew.git or something. @xu-cheng may also be interested or want to try that. We'd definitely accept that. Also, we should probably stop assuming tap directories need to start with homebrew- too.

[telemachus]

@MikeMcQuaid The last conversation I recall was that people wanted to avoid a new flag and switch on number of arguments. For example:

• If given one argument, handle in the traditional brew tap style: assume GitHub, add homebrew- as needed, etc.
• If given three arguments, handle in the brew any-tap style: assume nothing, combine first and second argument for directory structure, and feed third argument to git clone --depth 1.

My school year is nearly over, so I'd be willing to put together a PR for either version, but do you think now that an explicit flag is better than checking the size of ARGV? Given the way brew handles arguments, I suspect an explicit flag might be safer/easier, but I'm not 100% sure.

[MikeMcQuaid]

Yeh, I don't personally mind. It feels weird that we don't handle the two argument case? Maybe user/tapname url as two arguments instead or something? Eiterh approach seems fine though, do what you personally think is better.

[telemachus]

It feels weird that we don't handle the two argument case? Maybe user/tapname url as two arguments instead or something?

Now that I look again, I'm realizing that the super-explicit three argument form is a relic of when the tap structure was different. I think you're right and there's no longer any reason for three.

I'll work on a PR early next week, I hope. Thanks.

[adzenith]

@MikeMcQuaid: check out this branch. Everything should be there!

[xu-cheng]

@adzenith Can you rebase your branch against homebrew/master and make sure there won't be duplicated desc field? Thanks.

[MikeMcQuaid]

@xu-cheng Just to triple-check: if I merge that branch then brew upgrade won't show any of the changes as the versions will be the same.

[MikeMcQuaid]

@xu-cheng Right?

[xu-cheng]

Just to triple-check: if I merge that branch then brew upgrade won't show any of the changes as the versions will be the same.

They won't be sure. If you want to be absolute safe, you can split it to multiple commits as test, which can be helpful to code review as well.

[MikeMcQuaid]

@adzenith Yeh, once it's rebased I'll get it merged in fairly quickly (and resolve any conflicts then myself). Thanks!

[adzenith]

@xu-cheng, @MikeMcQuaid: Just rebased again! And double-checked that there are no duplicate descs.

[MikeMcQuaid]

@adzenith Merged!

[tdsmith]

Thanks for all your work on this, @telemachus and Nik (and Mike and Xu Cheng); this is great! 🎉

[adzenith]

I come back to my computer after a week away, and not only is it merged but there's Tim! Hi Tim!

Thanks @MikeMcQuaid, @xu-cheng, and especially @telemachus for doing all the hard work in the first place!