uses: add --formula and --casks flags by Rylan12 · Pull Request #9437 · Homebrew/brew

Rylan12 · 2020-12-06T16:51:26Z

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 tests with your changes locally?
Have you successfully run brew man locally and committed any changes?

This PR adds --formula and --cask flags to brew uses to filter results by only formulae or casks.

I'm marking this as critical because it is causing issues with our CI in homebrew/core for a few formulae (see Homebrew/homebrew-core#66268)

CC: @fxcoudert

fxcoudert · 2020-12-06T16:56:30Z

Thanks @Rylan12, can you explain how that fixes the CI workflow? (and why it was broken) I'm not very familiar with this codebase

fxcoudert · 2020-12-06T17:06:01Z

I'm thinking: maybe the issue is just that the CI nodes are not supposed to have casks tapped. I'm gonna try to untap it, see if it helps.

carlocab · 2020-12-06T17:13:25Z

@fxcoudert see Homebrew/homebrew-core#66268 (comment)

The CI error was that test-bot couldn't find a formula called aptible. (Which there isn't; it's a cask.)

Rylan12 · 2020-12-06T17:14:33Z

It sounds like the CI workflow issue was that brew uses was picking up casks which are not compatible with the test-bot that followed. For example, brew uses --include-test --include-build --recursive python@3.9 picked up the cask aptible. However, when test-bot then tried to run Formulary.factory(d) on all the results from brew uses, it fails for casks with a message saying No available formula with the name "aptible" (which is the error we were seeing that was being silenced by test-bot).

This PR adds --formula and --cask to brew uses that allows us to request only formula dependants. Then, in Homebrew/homebrew-test-bot#532, I changed the test-bot code to call brew uses --include-test --include-build --recursive --formula python@3.9 to get only formulae

carlocab · 2020-12-06T17:16:09Z

I will say though that I like this PR anyway even if it didn't fix any CI errors.

fxcoudert · 2020-12-06T17:20:31Z

Yes I understand, and I think it makes sense anyway. But I'm hoping that my other fix (to the CI nodes) works, so we can take time to properly review this PR (by someone more capable than myself!).

We now have 4 Catalina and 4 Mojave nodes back up, and hopefully the Catalina ones behave better. Let me know if that's not the case.

Rylan12 · 2020-12-06T17:24:17Z

I'm thinking: maybe the issue is just that the CI nodes are not supposed to have casks tapped. I'm gonna try to untap it, see if it helps.

Thanks, @fxcoudert! I agree, it seems that homebrew/cask doesn't need to be tapped based on this section of the setup-homebrew action which automatically taps homebrew/cask only if the repo being tested is homebrew/cask.

If the other runners don't have homebrew/cask tapped, then it makes sense not to tap it on 10.15 either. I'll go rerun the two affected PRs. If the problems go away, I'll remove the critical label here. I'll probably keep the test-bot PR open for some more reviews as it's a good way to ensure that this won't happen in the future, but it won't be critical anymore.

Rylan12 · 2020-12-06T21:00:35Z

Okay, this is no longer breaking CI so I'm going to remove the critical label. I still think that adding these flags is still a good idea, so I'm going to keep this PR open.

Review period has not ended yet.

BrewTestBot · 2020-12-06T21:00:55Z

Review period will end on 2020-12-08 at 00:00:00 UTC.

MikeMcQuaid

Makes sense to me but some suggested cleanup.

MikeMcQuaid · 2020-12-07T15:04:04Z

+    only = if args.formula?
+      :formula


I actually think this might be more readable just using args.formula? etc. everywhere.

I don't think it's more readable when every other command uses only already for this.

Hate to be picky about this but there are several commands that don't use only (deps, list, outdated, create).

The main difference that I can see is that these commands don't call external methods that take an only parameter. I agree, if we're going to be passing the value of only with e.g. args.named.to_formulae_and_casks(only: only) it makes sense to be only.

However, the distinction (especially with uses) is that --formula and --cask mean that we are expecting to output only formulae or casks rather than input formulae or casks. All of the logic is handled within uses.rb, so I see no reason why we should hold ourselves to a convention being used in some commands with a totally different use case.

To me, reading if show_formulae_and_casks || args.formula? is much easier to understand than if only != :cask. There are too many negatives in the second one. It's like saying "are we not excluding" instead of "are we including"

"if we are not restricting ourselves to casks" versus "if we are including formulae"

Hate to be picky about this but there are several commands that don't use only (deps, list, outdated, create).

The main difference that I can see is that these commands don't call external methods that take an only parameter. I agree, if we're going to be passing the value of only with e.g. args.named.to_formulae_and_casks(only: only) it makes sense to be only.

I feel like there's definitely a shared abstraction that all these commands could use to reduce the boilerplate.

Sorry, are you talking about the commands that do use only or the commands that don't. If the former, I think that already exists for the most part. We could add an option to args that figures out the value of only. It would probably save two lines of code in each file but would reduce duplication. If the latter, I think the reason that they don't use only is that their functions are different enough that it doesn't apply (so there likely isn't much logic to be pulled out)

Sorry, are you talking about the commands that do use only or the commands that don't. If the former, I think that already exists for the most part.

Either! 😁

We could add an option to args that figures out the value of only. It would probably save two lines of code in each file but would reduce duplication.

This sounds good 👍🏻

Okay, I'll look into it. I'm going to merge this, though, because those changes are not likely to affect the uses command.

(Of course, if they do, this can be modified then)

@Rylan12 Agreed, this was definitely non-blocking feedback 👍🏻

Rylan12 · 2020-12-07T16:14:00Z

Thanks! I cleaned up the logic a little bit. It's definitely cleaner now.

BrewTestBot · 2020-12-08T00:14:38Z

Review period ended.

Refs. - https://github.com/Homebrew/brew/releases/tag/2.6.1 - Homebrew/brew#9437 - Homebrew/brew@e3427fa - https://github.com/Homebrew/brew/releases/tag/2.6.0 - Homebrew/brew#9247 - Homebrew/brew@3c2ec1c

Warning message was: ``` Warning: Calling brew cask install is deprecated! Use brew install [--cask] instead. ``` Refs. - https://github.com/Homebrew/brew/releases/tag/2.6.1 - Homebrew/brew#9437 - Homebrew/brew@e3427fa - https://github.com/Homebrew/brew/releases/tag/2.6.0 - Homebrew/brew#9247 - Homebrew/brew@3c2ec1c

uses: add --formula and --casks flags

e3427fa

Rylan12 added the critical Critical change which should be shipped as soon as possible. label Dec 6, 2020

BrewTestBot previously approved these changes Dec 6, 2020

View reviewed changes

This was referenced Dec 6, 2020

Exclude casks when determining dependants Homebrew/homebrew-test-bot#532

Merged

python@3.9: update wheel Homebrew/homebrew-core#66268

Closed

Rylan12 removed the critical Critical change which should be shipped as soon as possible. label Dec 6, 2020

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 6, 2020

reitermarkus reviewed Dec 7, 2020

View reviewed changes

Comment thread Library/Homebrew/cmd/uses.rb Outdated

uses: cleanup formula/cask argument handling

eab1c2c

MikeMcQuaid approved these changes Dec 7, 2020

View reviewed changes

uses: further cleanup formula/cask argument handling

cb4e473

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 8, 2020

BrewTestBot approved these changes Dec 8, 2020

View reviewed changes

MikeMcQuaid approved these changes Dec 8, 2020

View reviewed changes

Rylan12 merged commit fbcf943 into Homebrew:master Dec 9, 2020

Rylan12 deleted the uses-flags branch December 9, 2020 13:59

machupicchubeta mentioned this pull request Dec 14, 2020

Follow brew cask warning machupicchubeta/dotfiles#150

Merged

BrewTestBot added the outdated PR was locked due to age label Jan 9, 2021

Homebrew locked as resolved and limited conversation to collaborators Jan 9, 2021

Uh oh!

Conversation

Rylan12 commented Dec 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fxcoudert commented Dec 6, 2020

Uh oh!

fxcoudert commented Dec 6, 2020

Uh oh!

carlocab commented Dec 6, 2020

Uh oh!

Rylan12 commented Dec 6, 2020

Uh oh!

carlocab commented Dec 6, 2020

Uh oh!

fxcoudert commented Dec 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rylan12 commented Dec 6, 2020

Uh oh!

Rylan12 commented Dec 6, 2020

Uh oh!

BrewTestBot commented Dec 6, 2020

Uh oh!

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MikeMcQuaid Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

reitermarkus Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

Rylan12 Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

Rylan12 Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

Rylan12 Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

Rylan12 commented Dec 7, 2020

Uh oh!

BrewTestBot commented Dec 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Rylan12 commented Dec 6, 2020 •

edited

Loading

fxcoudert commented Dec 6, 2020 •

edited

Loading

Rylan12 Dec 7, 2020 •

edited

Loading