cmd/deps: add args to filter output #17129

cho-m · 2024-04-22T19:29:55Z

--direct-build to only show direct build dependencies, which gives information on what formulae are needed when --build-from-source.
--direct-test to only show direct test dependencies when outputting in --tree or --graph format
--skip to prune parts of dependency tree
--level to limit recursion depth. For now, only added to --tree as the support there is simple and idea for arg is similar to what the tree & eza formula provide.

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?

Example:

❯ brew deps --include-build libxml2 --tree --annotate
libxml2
├── python-setuptools  [build]
│   └── [email protected]  [build] [test]
│       ├── pkg-config  [build]
│       ├── mpdecimal
│       ├── openssl@3
│       │   └── ca-certificates
│       ├── sqlite
│       │   └── readline
│       └── xz
├── [email protected]  [build] [test]
│   ├── pkg-config  [build]
│   ├── mpdecimal
│   ├── openssl@3
│   │   └── ca-certificates
│   ├── sqlite
│   │   └── readline
│   └── xz
├── [email protected]  [build] [test]
│   ├── pkg-config  [build]
│   ├── mpdecimal
│   ├── openssl@3
│   │   └── ca-certificates
│   ├── sqlite
│   │   └── readline
│   └── xz
├── icu4c
└── readline

❯ brew deps --include-build libxml2 --tree --annotate --direct-build
libxml2
├── python-setuptools  [build]
├── [email protected]  [build] [test]
│   ├── mpdecimal
│   ├── openssl@3
│   │   └── ca-certificates
│   ├── sqlite
│   │   └── readline
│   └── xz
├── [email protected]  [build] [test]
│   ├── mpdecimal
│   ├── openssl@3
│   │   └── ca-certificates
│   ├── sqlite
│   │   └── readline
│   └── xz
├── icu4c
└── readline

❯ brew deps --include-build libxml2 --tree --annotate --direct-build --skip readline,[email protected]
libxml2
├── python-setuptools  [build]
├── [email protected]  [build] [test]
│   ├── mpdecimal
│   ├── openssl@3
│   │   └── ca-certificates
│   ├── sqlite
│   └── xz
└── icu4c

❯ brew deps --include-build libxml2 --tree --annotate --direct-build --skip readline,[email protected] --level 2
libxml2
├── python-setuptools  [build]
├── [email protected]  [build] [test]
│   ├── mpdecimal
│   ├── openssl@3
│   ├── sqlite
│   └── xz
└── icu4c

❯ brew deps --include-build libxml2 --tree --annotate --direct-build --skip readline,[email protected] --level 1
libxml2
├── python-setuptools  [build]
├── [email protected]  [build] [test]
└── icu4c

* `--direct-build` to only show direct build dependencies, which gives information on what formulae are needed when `--build-from-source`. * `--direct-test` to only show direct test dependencies when outputting in `--tree` or `--graph` format * `--skip` to prune parts of dependency tree * `--level` to limit recursion depth. For now, only added to `--tree` as the support there is simple and idea for arg is similar to what the `tree` & `eza` formula provide. Signed-off-by: Michael Cho <[email protected]>

apainintheneck · 2024-04-23T02:40:17Z

I'm against adding more specific options to brew deps if the same output can be accomplished with a combination of existing options or calling brew deps multiple times.

The --direct-test and --direct-build options could be expressed with --direct --include-build and --direct --include-test, right? If that's true, I don't think they should be added. I also think that as we add more options the interaction and precedence of any set of options becomes harder to reason about.

--skip and --level options could be useful though in some cases and don't seem to overlap with existing options so I'm open to them. Maybe --depth would make more sense as the option name than --level?

cho-m · 2024-04-23T03:08:50Z

The --direct-test and --direct-build options could be expressed with --direct --include-build and --direct --include-test, right?

--direct --include-build will only output a single level. e.g.

❯ brew deps --direct --include-build --tree ruby --annotate
ruby
├── autoconf  [build]
├── pkg-config  [build]
├── rust  [build]
├── libyaml
└── openssl@3

This is not the full set of formulae installed when you run brew install --build-from-source ruby (i.e. recursive runtime deps and direct build's recursive runtime deps)

❯ brew deps --include-build --tree ruby --annotate --direct-build
ruby
├── autoconf  [build]
│   └── m4
├── pkg-config  [build]
├── rust  [build]
│   ├── libgit2
│   │   ├── libssh2
│   │   │   └── openssl@3
│   │   │       └── ca-certificates
│   │   └── openssl@3
│   │       └── ca-certificates
│   ├── libssh2
│   │   └── openssl@3
│   │       └── ca-certificates
│   ├── llvm@17
│   │   └── zstd
│   │       ├── lz4
│   │       └── xz
│   ├── openssl@3
│   │   └── ca-certificates
│   └── pkg-config
├── libyaml
└── openssl@3
    └── ca-certificates

The usage of this is analyzing problems that occur when particular formulae are installed in build environment, which often happens in Homebrew/core.

In most cases, I've fallen back to grepping and scripting as this doesn't seem possible in current brew commands without excessive calls to brew with extra post processing.

Maybe --depth would make more sense as the option name than --level?

I don't mind either way. I went with option I was used to using for directory tree printing, e.g.

❯ tree --help | rg ' -L'
  -L level      Descend only level directories deep.

❯ eza --help | rg ' -L'
  -L, --level DEPTH          limit the depth of recursion

MikeMcQuaid

If you, @cho-m, personally need all of these flags doing the Homebrew CI analysis you've mentioned: I'm 👍🏻 to adding it.

If you don't need any of these: let's hold off adding them.

Code-wise: I'm 👍🏻 bar the one suggestion I've made, this can be merged with no re-review after that.

MikeMcQuaid · 2024-04-23T07:37:46Z

Library/Homebrew/cmd/deps.rb

@@ -88,6 +102,9 @@ class Deps < AbstractCommand
 def run
 raise UsageError, "`brew deps --os=all` is not supported" if args.os == "all"
 raise UsageError, "`brew deps --arch=all` is not supported" if args.arch == "all"
+ if (level = args.level) && !level.match?(/^[1-9][0-9]*$/)


Suggested change

if (level = args.level) && !level.match?(/^[1-9][0-9]*$/)

if (level = args.level) && level.to_i > 0

maybe with exception rescue required

Doing some basic testing: it doesn't look like a rescue is required as invalid input will just end up being 0 from to_i.

Using String#to_i is slightly more permissive of garbage input but probably fine for this use case.

irb(main):001:0> "skdfjsdklj100".to_i => 0 irb(main):002:0> "100alskdfsdkjfs".to_i => 100

apainintheneck · 2024-04-23T21:49:37Z

The usage of this is analyzing problems that occur when particular formulae are installed in build environment, which often happens in Homebrew/core.

In most cases, I've fallen back to grepping and scripting as this doesn't seem possible in current brew commands without excessive calls to brew with extra post processing.

In that case, maybe brew deps --build-from-source would make more sense than brew deps --include-build --direct-build. I think that this option should conflict with the existing --include-* and --skip-* options as well since it's not easy to determine precedence here.

apainintheneck

Another option would be to have --direct take a list of dependencies types. That would allow you to run brew deps --include-build --direct=build NAME to get the build from source dependencies.

apainintheneck · 2024-04-23T21:11:29Z

Library/Homebrew/cmd/deps.rb

@@ -88,6 +102,9 @@ class Deps < AbstractCommand
 def run
 raise UsageError, "`brew deps --os=all` is not supported" if args.os == "all"
 raise UsageError, "`brew deps --arch=all` is not supported" if args.arch == "all"
+ if (level = args.level) && !level.match?(/^[1-9][0-9]*$/)


Using String#to_i is slightly more permissive of garbage input but probably fine for this use case.

irb(main):001:0> "skdfjsdklj100".to_i => 0 irb(main):002:0> "100alskdfsdkjfs".to_i => 100

apainintheneck · 2024-04-23T21:29:56Z

Library/Homebrew/cmd/deps.rb

 switch "--include-test",
- description: "Include `:test` dependencies for <formula> (non-recursive)."
+ description: "Include `:test` dependencies for <formula> (non-recursive for flat output)."
+ switch "--direct-test",
+ depends_on: "--include-test",
+ description: "Only include `:test` dependencies that are direct dependencies declared " \
+ "in the formula when showing non-flat output like `--tree` or `--graph`."


Maybe we'd be better off just making the --direct-test behavior the default for the --include-test option instead. It's always been a bit weird that the output is different depending on whether you're using the tree, graph or flat output.

Library/Homebrew/dependencies_helpers.rb

MikeMcQuaid · 2024-04-24T07:39:21Z

In that case, maybe brew deps --build-from-source would make more sense than brew deps --include-build --direct-build

Yeh, I really like this idea, nice one @apainintheneck 💡

cho-m force-pushed the deps-filters branch from 99bc830 to 649ff41 Compare April 22, 2024 19:51

MikeMcQuaid approved these changes Apr 23, 2024

View reviewed changes

apainintheneck reviewed Apr 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/deps: add args to filter output #17129

cmd/deps: add args to filter output #17129

cho-m commented Apr 22, 2024

apainintheneck commented Apr 23, 2024

cho-m commented Apr 23, 2024 •

edited

MikeMcQuaid left a comment

MikeMcQuaid Apr 23, 2024

MikeMcQuaid Apr 23, 2024

apainintheneck Apr 23, 2024

apainintheneck commented Apr 23, 2024

apainintheneck left a comment

apainintheneck Apr 23, 2024

apainintheneck Apr 23, 2024

MikeMcQuaid commented Apr 24, 2024

	if (level = args.level) && !level.match?(/^[1-9][0-9]*$/)
	if (level = args.level) && level.to_i > 0

cmd/deps: add args to filter output #17129

Are you sure you want to change the base?

cmd/deps: add args to filter output #17129

Conversation

cho-m commented Apr 22, 2024

apainintheneck commented Apr 23, 2024

cho-m commented Apr 23, 2024 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Apr 23, 2024

Choose a reason for hiding this comment

MikeMcQuaid Apr 23, 2024

Choose a reason for hiding this comment

apainintheneck Apr 23, 2024

Choose a reason for hiding this comment

apainintheneck commented Apr 23, 2024

apainintheneck left a comment

Choose a reason for hiding this comment

apainintheneck Apr 23, 2024

Choose a reason for hiding this comment

apainintheneck Apr 23, 2024

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 24, 2024

cho-m commented Apr 23, 2024 •

edited