brew deps: add --include-requirements, plus some fixes #2996

apjanke · 2017-08-03T05:51:32Z

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 tests with your changes locally?

Adds a --include-requirements option to brew deps, to display all requirements, including ones without a default formula. Ones with a default formula are displayed as a requirement, with the default formula noted in parentheses. Per a suggestion by @ilovezfs.

Mostly useful in conjunction with --tree. This helps hunt down where a deeply-nested requirement is coming from. For example, octave has an indirect dependency on :x11, but it has so many dependencies it's hard to tell where they're coming from. Or it lets you determine where the MinimumMacOS requirement for mytop is coming from.

Also fixes a bug in --tree where the dependencies of a Requirement's default formula were not recursed into.

Also fixes a bug where the --tree mode ignored the --include-optional, --include-build, and --skip-recommended filters, instead always displaying build and recommended dependencies, and never optionals.

Also adds an --annotate option to the --tree mode, marking up optional, recommended, and build dependencies as such in the display.

Also adds an undocumented --for-each switch to help debug the --all/--installed display mode, which couldn't previously be gotten to for a list of specified formulae.

Before:

$ brew deps --tree xpdf
xpdf (required dependencies)
├── openmotif
│   ├── automake
│   │   └── autoconf
│   ├── autoconf
...

$ brew deps --tree mytop
mytop (required dependencies)
├── :mysql
└── openssl
    └── makedepend
        └── pkg-config

After:

$ brew deps --tree --include-requirements xpdf
xpdf
├── :x11
├── openmotif
│   ├── :x11
│   ├── fontconfig
│   │   └── freetype
...

$ brew deps --tree mytop
mytop
├── mysql
│   └── openssl
└── openssl

$ brew deps --tree mytop --include-requirements
mytop
├── :mysql (mysql)
│   ├── :macOS >= 10.7
│   └── openssl
└── openssl

$ brew deps --tree mytop --include-requirements --include-build
mytop
├── :mysql (mysql)
│   ├── :macOS >= 10.7
│   ├── cmake
│   │   └── sphinx-doc
│   └── openssl
│       └── makedepend
│           └── pkg-config
└── openssl
    └── makedepend
        └── pkg-config

Removing the colon before the default formula when not doing --include-requirements makes the --tree output more consistent with the non-tree output.

Note that if you include all of --tree --include-optional --include-build --include-requirements on large formulae (like brew deps octave --tree --include-optional --include-build --include-requirements), you can get a stack overflow error. I suspect this is because there's a circular dependency in the formula definitions somewhere. This could be worked around by using a stack to detect circular dependencies inside the code here, but I haven't put it in yet.

Test failure

I'm getting this failure when I run brew tests locally, but it's also happening for me on master. Don't think it's introduced by this PR.

Failures:

  1) OS::Mac::language can be parsed by Locale::parse
     Failure/Error: expect { Locale.parse(subject.language) }.not_to raise_error

       expected no Exception, got #<Locale::ParserError: '' cannot be parsed to a Locale> with backtrace:
         # ./locale.rb:15:in `parse'
         # ./test/os/mac_spec.rb:19:in `block (4 levels) in <top (required)>'
         # ./test/os/mac_spec.rb:19:in `block (3 levels) in <top (required)>'
         # ./test/spec_helper.rb:96:in `block (2 levels) in <top (required)>'
         # ./vendor/bundle/ruby/2.0.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'
     # ./test/os/mac_spec.rb:19:in `block (3 levels) in <top (required)>'
     # ./test/spec_helper.rb:96:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.0.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

Finished in 56.57 seconds (files took 1.49 seconds to load)
446 examples, 1 failure

Failed examples:

rspec ./test/os/mac_spec.rb:18 # OS::Mac::language can be parsed by Locale::parse

ilovezfs · 2017-08-04T07:18:04Z

Thanks for this PR @apjanke! It seems to work as expected in my testing.

you can get a stack overflow error.

It would be good if this were fixed before this goes in. brew deps --tree -include-optional ffmpeg caused my terminal to lose its mind. :)

(Also, it would be neat if --tree respected --1.)

apjanke · 2017-08-04T07:49:19Z

Added --1 support to --tree. I don't see how that would be useful in practice, but it's nice in that it unifies the behavior of the different display modes; I like it.

I see how to implement cycle detection to avoid the stack overflow. I'll add it. The brew deps --tree -include-optional ffmpeg blow-up reproduces for me too, and that's not an obscure case.

apjanke · 2017-08-04T08:25:28Z

Added circular-dependency detection. This fixes the stack overflow error.

Now brew deps --tree --include-optional ffmpeg works:

$ brew deps --tree --include-optional ffmpeg
ffmpeg
├── lame
├── x264
│   └── l-smash
├── xvid
...
│       │   ├── jpeg
│       │   ├── libpng
│       │   └── libtiff
│       │       ├── jpeg
│       │       └── xz
│       └── ffmpeg (CIRCULAR DEPENDENCY)

and brew deps octave --tree ... is merely insane, not broken:

$ brew deps octave --tree --include-optional --include-build --include-requirements | wc -l
   25843

ilovezfs · 2017-08-04T08:55:08Z

and brew deps octave --tree ... is merely insane, not broken:
$ brew deps octave --tree --include-optional --include-build --include-requirements | wc -l
  25843

A generic --depth option would probably help to tame that particular crazy.

MikeMcQuaid

Looks good! Some style pedantry (some of which isn't your fault) but: nice work!

MikeMcQuaid · 2017-08-05T14:44:36Z

Library/Homebrew/cmd/deps.rb

+    @with_optional = ARGV.include?("--include-optional")
+    @with_build = ARGV.include?("--include-build")
+    @skip_recommended = ARGV.include?("--skip-recommended")
+    @annotate = ARGV.include?("--annotate")


Feels weird to use class variables for some of ARGV and not others. I'd lean on the side of never using them.

So, I tried writing this to use function parameters for everything, and it got pretty hard to ready. Maybe it should fall back to using ARGV.include? everywhere? Or a single class instance variable for all the options? My impression is we're trying to move away from scattering global ARGV references everywhere.

Maybe it should fall back to using ARGV.include? everywhere?

Yeh, I agree.

My impression is we're trying to move away from scattering global ARGV references everywhere.

I think broadly but at this point it's probably reliant on having a decent Command DSL first.

I've updated it to use direct ARGV references everywhere, and removed the class instance variables.

MikeMcQuaid · 2017-08-05T14:44:57Z

Library/Homebrew/cmd/deps.rb

+    if @with_reqs
+      deps
+    else
+      deps.map do |d|


deps.map do |dep|

MikeMcQuaid · 2017-08-05T14:45:19Z

Library/Homebrew/cmd/deps.rb

+      end.compact
+    end
+  end
+
  def dep_display_name(d)


def dep_display_name(dep)

MikeMcQuaid · 2017-08-05T14:46:22Z

Library/Homebrew/cmd/deps.rb

-      prefix_ext = (i == max) ? "    " : "│   "
-      puts prefix + "#{chr} #{dep_display_name(dep)}"
-      recursive_deps_tree(Formulary.factory(dep.name), prefix + prefix_ext)
+      display_s = "#{chr} #{dep_display_name(dep)}"


Can we figure out a better name than chr here? line, maybe?

how about tree_lines, to distinguish the graphical lines of the tree structure from a "line" of text?

MikeMcQuaid · 2017-08-05T14:46:39Z

Library/Homebrew/cmd/deps.rb

+      display_s = "#{chr} #{dep_display_name(dep)}"
+      is_circular = @dep_stack.include?(dep.name)
+      display_s = "#{display_s} (CIRCULAR DEPENDENCY)" if is_circular
+      puts prefix + display_s


puts "#{prefix}#{display_s}

MikeMcQuaid · 2017-08-05T14:47:01Z

Library/Homebrew/cmd/deps.rb

+      display_s = "#{display_s} (CIRCULAR DEPENDENCY)" if is_circular
+      puts prefix + display_s
+      if recursive && !is_circular
+        prefix_ext = (i == max) ? "    " : "│   "


prefix_extension = if i == max

apjanke · 2017-08-07T04:14:45Z

Addressed most of the style pedantry, except for the use of class instance variables. Not sure what the right fix to do there is; using function parameters makes the code look pretty gross IMHO. I think class instance variables should be okay in the case of commands, since the modules are only loaded one per process, used once, and then the process ends. Deferring to the judgment of other maintainers here though.

apjanke · 2017-08-07T04:15:03Z

And, thanks!

apjanke · 2017-08-07T04:20:19Z

Updated the zsh completions to reflect the new brew deps --tree behavior in this PR.

MikeMcQuaid

Once brew style passes 👍

apjanke · 2017-08-11T20:41:21Z

Arg! brew style-ing now...

MikeMcQuaid

Looks good

apjanke · 2017-08-15T00:19:48Z

Merged. Thanks for the feedback!

chdiza · 2017-08-15T17:36:17Z

For example, octave has an indirect dependency on :x11, but it has so many dependencies it's hard to tell where they're coming from.

I just tried brew deps octave --tree --include-requirements, and no such :x11 appears in the output. Am I doing it wrong?

ilovezfs · 2017-08-15T18:09:54Z

@chdiza no Homebrew/homebrew-core#16193

apjanke changed the title ~~brew deps --tree: add --include-requirements~~ brew deps: add --include-requirements Aug 3, 2017

apjanke changed the title ~~brew deps: add --include-requirements~~ brew deps: add --include-requirements, plus some fixes Aug 3, 2017

ilovezfs approved these changes Aug 4, 2017

View reviewed changes

ilovezfs requested a review from MikeMcQuaid August 4, 2017 08:55

MikeMcQuaid requested changes Aug 5, 2017

View reviewed changes

MikeMcQuaid approved these changes Aug 11, 2017

View reviewed changes

brew deps: add --include-requirements

bb85581

MikeMcQuaid approved these changes Aug 12, 2017

View reviewed changes

apjanke merged commit 60d8218 into Homebrew:master Aug 15, 2017

apjanke deleted the non-formula-reqs-in-brew-deps-tree branch August 15, 2017 00:19

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

brew deps: add --include-requirements, plus some fixes #2996

brew deps: add --include-requirements, plus some fixes #2996

apjanke commented Aug 3, 2017 •

edited

Loading

ilovezfs commented Aug 4, 2017

apjanke commented Aug 4, 2017

apjanke commented Aug 4, 2017 •

edited

Loading

ilovezfs commented Aug 4, 2017

MikeMcQuaid left a comment

MikeMcQuaid Aug 5, 2017

apjanke Aug 7, 2017

MikeMcQuaid Aug 7, 2017

apjanke Aug 10, 2017

MikeMcQuaid Aug 5, 2017

apjanke Aug 7, 2017

MikeMcQuaid Aug 5, 2017

apjanke Aug 7, 2017

MikeMcQuaid Aug 5, 2017

apjanke Aug 7, 2017

MikeMcQuaid Aug 5, 2017

apjanke Aug 7, 2017

MikeMcQuaid Aug 5, 2017

apjanke Aug 7, 2017

apjanke commented Aug 7, 2017

apjanke commented Aug 7, 2017

apjanke commented Aug 7, 2017

MikeMcQuaid left a comment •

edited

Loading

apjanke commented Aug 11, 2017

MikeMcQuaid left a comment

apjanke commented Aug 15, 2017

chdiza commented Aug 15, 2017

ilovezfs commented Aug 15, 2017

brew deps: add --include-requirements, plus some fixes #2996

brew deps: add --include-requirements, plus some fixes #2996

Conversation

apjanke commented Aug 3, 2017 • edited Loading

Test failure

ilovezfs commented Aug 4, 2017

apjanke commented Aug 4, 2017

apjanke commented Aug 4, 2017 • edited Loading

ilovezfs commented Aug 4, 2017

MikeMcQuaid left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apjanke commented Aug 7, 2017

apjanke commented Aug 7, 2017

apjanke commented Aug 7, 2017

MikeMcQuaid left a comment • edited Loading

Choose a reason for hiding this comment

apjanke commented Aug 11, 2017

MikeMcQuaid left a comment

Choose a reason for hiding this comment

apjanke commented Aug 15, 2017

chdiza commented Aug 15, 2017

ilovezfs commented Aug 15, 2017

apjanke commented Aug 3, 2017 •

edited

Loading

apjanke commented Aug 4, 2017 •

edited

Loading

MikeMcQuaid left a comment •

edited

Loading