New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

brew info: include options to dependencies in display #1308

Merged
merged 1 commit into from Oct 20, 2016

Conversation

Projects
None yet
2 participants
@apjanke
Copy link
Contributor

apjanke commented Oct 17, 2016

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

Currently, brew info does not take the options of dependencies in to account when displaying them.

For example, libgroove depends on ffmpeg --with-libvorbis, not plain ffmpeg. But the brew info output looks like it needs plain ffmpeg.

Before:

before

This PR changes brew info to list the options for dependencies which have them, and to take those options in to account when determining whether the dependency is satisfied.

After:

after

Note that not only does it add --with-libvorbis to the display, but it's now a red X, which I think is correct here, because this is run on a system with the default ffmpeg formula installed, and --with-libvorbis was not included in the build options. Now the display correctly reflects a formula which will need to be (re-)installed during the brew install libgroove process.

Other formulae you can test this on:

anjuta.rb: depends_on "libxml2" => "with-python"
cmu-sphinxbase.rb: depends_on "libsamplerate" => "with-libsndfile"
freeling.rb: depends_on "boost" => "with-icu4c"
git-ftp.rb: depends_on "curl" => "with-libssh2"
libgroove.rb: depends_on "ffmpeg" => "with-libvorbis"

If people are in favor of this change, similar options display could be added to brew deps.

@apjanke apjanke added the in progress label Oct 17, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 17, 2016

Seems good 👍

If people are in favor of this change, similar options display could be added to brew deps.

This will likely need to be a non-default option as people are likely relying on parsing the current format and we want brew install $(brew deps foo) to work.

@@ -165,9 +165,9 @@ def info_formula(f)

def decorate_dependencies(dependencies)
deps_status = dependencies.collect do |dep|
dep.installed? ? pretty_installed(dep) : pretty_uninstalled(dep)
dep.satisfied?([]) ? pretty_installed(dep_display_s(dep)) : pretty_uninstalled(dep_display_s(dep))

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 17, 2016

Member

Probably worth splitting onto multiple lines now.

This comment has been minimized.

@apjanke

apjanke Oct 17, 2016

Contributor

What's the right style for a muti-line trinary expression? Or should it be converted to an if ... else ... end?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 17, 2016

Member

Yeh, the latter, thanks.

This comment has been minimized.

@apjanke

apjanke Oct 17, 2016

Contributor

Amended to do so.


def dep_display_s(dep)
if dep.option_tags.empty?
dep.name

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 17, 2016

Member

return dep.name if dep.option_tags.empty? to avoid the else.

This comment has been minimized.

@apjanke

apjanke Oct 17, 2016

Contributor

Amended to fix.

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Oct 17, 2016

This will likely need to be a non-default option as people are likely relying on parsing the current format and we want brew install $(brew deps foo) to work.

Yeah, I was thinking like a --verbose option for the main listings. It could probably be on by default in --tree mode, since that's unlikely to be parsed, though maybe it should be optional there too for consistency.

@apjanke apjanke force-pushed the apjanke:info-with-formula-options branch from 56895e5 to 85eedcb Oct 17, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 17, 2016

Yeah, I was thinking like a --verbose option for the main listings. It could probably be on by default in --tree mode, since that's unlikely to be parsed, though maybe it should be optional there too for consistency.

Sounds good.

@apjanke apjanke force-pushed the apjanke:info-with-formula-options branch from 85eedcb to 05a0274 Oct 17, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 19, 2016

@apjanke apjanke merged commit 551ce2b into Homebrew:master Oct 20, 2016

2 of 4 checks passed

codecov/patch 14.28% of diff hit (target 63.15%)
Details
codecov/project 63.14% (-0.01%) compared to 0296439
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Oct 20, 2016

Merged!

@apjanke apjanke removed the in progress label Oct 20, 2016

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Oct 20, 2016

Merged!

@apjanke apjanke deleted the apjanke:info-with-formula-options branch Oct 20, 2016

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.