dependency_helpers: rework recursive dependency resolution

[apainintheneck]

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

---

Fixes #15842

This is a refactor/reworking of the dependency resolution methods in the DependencyHelpers module. These methods are used by both the brew deps and brew uses commands to get a specific set of dependencies for the user based on multiple criteria.

Additive Options:
--include-build
--include-test
--include-optional

Subtractive Options:
--skip-recommended
--missing

When a user runs either command the only dependencies that are included by default are recommended and runtime dependencies. This is largely unchanged though we only used to include recommended dependencies if they were build dependencies before. I'm not entirely sure why these were included by default before though.

The biggest change is that all installed dependencies are always removed from the list now if the --missing option is passed. This could get skipped before depending on the other options that were passed. Essentially subtractive options now will always be evaluated before additive ones (the docs will need to be updated to make this clear).

Beyond that we have no special handling for the optional command anymore. We used to check that the optional dependency was not needed to build the formula but that seems redundant and confusing. Essentially, the #recursive_includes command now behaves much more like the #reject_ignores command (essentially the non-recursive version) which is a good thing for consistency's sake. I updated the #reject_ignores command to make sure it aligns with the new goal of giving subtractive options priority here and also renamed it to #select_includes after refactoring.

I also updated the man pages for both the brew uses and brew deps commands to clarify how they work.

Note: These are technically breaking changes. I think these commands were broken before but still it's possible that someone is counting on the previous behavior. This should probably wait until the next minor release at the very least.

Todo:

• Compare the results against the previous version of the command
  • I've only tested a handful of packages so far.
  • I've tested different variations of the brew deps command with the --for-each flag set and this returns the same results except for when --missing is set.
  • I've tested ~20 packages with brew uses, most of which are recommended or optional, and things look good here.
• Add tests for expected behavior.
• Update docs for brew uses and brew deps to make things clearer.

[apainintheneck]

There's a part of me that wonders if we'd be better off just having a --include-recommended flag instead of --skip-recommended and just stop including recommended dependencies by default. It'd be a breaking change though so it'd be beyond the scope of this PR.

[MikeMcQuaid]

There's a part of me that wonders if we'd be better off just having a --include-recommended flag instead of --skip-recommended and just stop including recommended dependencies by default.

I don't think so as recommended are installed by default so it would be weird for a default brew install and brew deps or brew uses to get different output.

[MikeMcQuaid]

There is, though, an argument whether it even makes sense to support :recommended or even :optional based on wider ecosystem usage and whether they should be deprecated.

[MikeMcQuaid]

This looks great so far, I really like the refactoring approach.

[Bo98]

There is, though, an argument whether it even makes sense to support :recommended or even :optional based on wider ecosystem usage and whether they should be deprecated.

They're quite highly used in third-party taps, which makes sense given they don't necessarily use bottles like we do for homebrew-core, and are also usually only a few formulae rather than a few thousand.

homebrew-ffmpeg being a popular example (and the use of optional is exactly the reason that tap exists).

[MikeMcQuaid]

They're quite highly used in third-party taps, which makes sense given they don't necessarily use bottles like we do for homebrew-core, and are also usually only a few formulae rather than a few thousand.

Yeh, I'm interested whether :recommended is used as I do see :optional usage now in e.g. the ffmpeg case.

[Bo98]

Yeh, I'm interested whether :recommended is used as I do see :optional usage now in e.g. the ffmpeg case.

Not as common as optional for sure. Here's a few that do: https://github.com/search?q=%3Arecommended+%22%3C+Formula%22+lang%3ARuby+-repo%3AHomebrew%2Fbrew+-repo%3Amistydemeo%2Ftigerbrew+-is%3Afork&type=code&p=1

[MikeMcQuaid]

Cool, yeh, probably worth keeping both then, thanks @Bo98.

[apainintheneck]

Thanks for feedback everyone.

I did some testing on the core repo and it seems like everything behaves the same way for the brew deps command on the core repos. I'm going to do some more testing of the brew uses commands though.

Beyond that I'll take a look at other third-party taps for testing how optional and recommended dependencies respond to these changes. I was able to find a few good ones that should be representative I hope.

I'll also change the code to use public_send instead of send.

[apainintheneck]

Never mind, my earlier testing wasn't thorough enough. It looks like one of things the new algorithm wasn't handling well was depends_on cask: <cask_name>. I was not handling required dependencies correctly but now everything should be good.

I did a bit more looking around and it seems like the main difference will be with the missing flag. Other than that things should be pretty similar to before.

[apainintheneck]

I updated the integration tests to cover more of the expected behavior. I thought about adding unit tests but that would involve lots of mocking of formulae so I'm not sure it's worth it. I'm surprised that while the behavior of the brew deps command seems very similar to before but a lot more results are appearing for the brew uses command. Looking at the results I'm not entirely sure why most of these didn't appear in the result before my changes (all of them seem legit).

For example:

/u/l/Homebrew (master|✔) $ brew deps chapel --tree --include-build
chapel
├── cmake
├── gmp
├── llvm@14
│   ├── cmake
│   └── [email protected]
│       ├── pkg-config
│       ├── mpdecimal
│       ├── openssl@3
│       │   └── ca-certificates
│       ├── sqlite
│       │   └── readline
│       └── xz
└── [email protected]
    ├── pkg-config
    ├── mpdecimal
    ├── openssl@3
    │   └── ca-certificates
    ├── sqlite
    │   └── readline
    └── xz

The chapel formula depends on cmake and I don't have it installed locally. Yet it doesn't show up with the brew uses command when paired with --missing.

/u/l/Homebrew (master|✔) $ brew uses cmake --eval-all --include-build --missing | grep chapel

I assume that brew uses should apply the --missing criteria only to the results of the search (that is to say the packages that use the named formula) instead of to the named formula itself. Otherwise, brew uses cmake --missing would always return zero results for me since I already have it installed locally. So I just decided to filter out all of the installed packages before checking their dependencies in that command.

[apainintheneck]

I would have liked to test the installed package behavior with --missing but that increased the time taken by the integration test from 5 to 15 seconds on my machine. I wonder if there's a faster way to mock installation in this case. All I want is for one of the formula to respond with true when Formula#any_version_installed? is called.

[MikeMcQuaid]

I wonder if there's a faster way to mock installation in this case.

Could try to just write the keg's directory on disk, that will likely be enough?

[apainintheneck]

I updated the pr description with some more info, added more stuff to the integration tests and updated the man pages.

These are breaking changes so I think they should probably wait until the next release though the previous behavior was somewhat broken to begin with.

[MikeMcQuaid]

Looks great, thanks again @apainintheneck!

[MikeMcQuaid]

Don't think it's worth waiting for next release or considering these changes breaking considering these are non-default options on a developer command. If enough people loudly complain: we can revert or gate until the next release (but I appreciate the caution nonetheless @apainintheneck).

[MikeMcQuaid]

@carlocab When you're ✅ here: feel free to 🚢

[Bo98]

To clarify: does this mean formula_options in brew deps no longer does anything?

a developer command

I'm fine with changing, but just to clarify here: this is not a dev-cmd.

[MikeMcQuaid]

TIL that brew deps even had formula_options. I would be frankly amazed if that's widely used as it is undocumented.

[MikeMcQuaid]

I'm fine with changing, but just to clarify here: this is not a dev-cmd.

Yes, fair point. It feels like a borderline one.

[apainintheneck]

Good catch @Bo98! That was something that hadn't crossed my mind at all.

It looks like this was added in at user request to match how some undocumented behavior used to work (discussion: #5587; pr: #5648). I can look into adding this back in though I believe it's a super niche use case. It'd be nice to have a test for this too.

Note: This never worked before with the --direct option. It only worked when evaluating dependencies recursively.

[MikeMcQuaid]

It looks like this was added in at user request to match how some undocumented behavior used to work (discussion: #5587; pr: #5648).

The relevant folks here no longer seem to be maintaining taps with options and this was shortly around the time Homebrew/homebrew-core removed all options.

Gonna merge as-is. We can adjust or revert as needed. Thanks @apainintheneck!

[apainintheneck]

In that case, we should remove the formula_options from the arg parser and the docs should be updated too.

diff --git a/Library/Homebrew/cmd/deps.rb b/Library/Homebrew/cmd/deps.rb
index bd14a89c9..4ea97820d 100644
--- a/Library/Homebrew/cmd/deps.rb
+++ b/Library/Homebrew/cmd/deps.rb
@@ -13,8 +13,7 @@ module Homebrew
   def self.deps_args
     Homebrew::CLI::Parser.new do
       description <<~EOS
-        Show dependencies for <formula>. Additional options specific to <formula>
-        may be appended to the command. When given multiple formula arguments,
+        Show dependencies for <formula>. When given multiple formula arguments,
         show the intersection of dependencies for each formula. By default, `deps`
         shows all required and recommended dependencies.
 
@@ -71,7 +70,6 @@ module Homebrew
       conflicts "--installed", "--eval-all"
       conflicts "--installed", "--all"
       conflicts "--formula", "--cask"
-      formula_options
 
       named_args [:formula, :cask]
     end

CC: @blogabe and @RandomDSdevel since you both were involved in the original discussion to add formula_options to the brew deps command

[MikeMcQuaid]

@apainintheneck whoops, sorry, follow-up PR in #15922

[RandomDSdevel]

@MikeMcQuaid:

(Apologies for the mild delay in replying.)

CC: … @RandomDSdevel since you both were involved in the original discussion to add formula_options to the brew deps command

     My efforts remain stalled in extended development purgatory; including due to external factors; this can get revisited if/as needed.