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

info: print requirements #1004

Merged
merged 2 commits into from Sep 27, 2016

Conversation

Projects
None yet
5 participants
@apjanke
Copy link
Contributor

apjanke commented Sep 18, 2016

Picking up #770
Closes #775
Closes Homebrew/homebrew-core#4049

@apjanke apjanke added the in progress label Sep 18, 2016

@apjanke apjanke referenced this pull request Sep 18, 2016

Closed

info: print requirements #770

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

One bummer: requirements like MinimumMacOS have specific version numbers attached to them, but they're not included in the output yet. How much do we care about this?

Fixing it the Right Way probably means adding a method or two to Requirement, so subclasses can add their parameterizations in a custom way.

screen shot 2016-09-18 at 12 17 09 am

@apjanke apjanke added the features label Sep 18, 2016

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

I've added another commit with a display_s method in Requirement. (display for "thing to display to users"; _s for "string", which seems to be a conventional Ruby suffix.) Implemented it for the :java, :ruby, :perl, language_module, minimum_macos and maximum_macos requirements.

Output examples:

==> Requirements
Recommended: ruby = 1.8 ✔

==> Requirements
Required: ruby = 1.8 ✔, pygments.rb => :ruby ✘

==> Requirements
Optional: x11 ✔, perl = 5.5 ✔

==> Requirements
Required: lpeg => :lua ✘, luaexpat => :lua ✘, luafilesystem => :lua ✘

==> Requirements
Required: java = 1.7+ ✔

@apjanke apjanke referenced this pull request Sep 18, 2016

Closed

Requirements Are Not Listed in `brew info` Output #775

2 of 2 tasks complete

@apjanke apjanke force-pushed the apjanke:print-requirements branch from c1d80a4 to 7dd7e81 Sep 18, 2016

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

Amended to improve readability of min/max OS X requirements.

==> Requirements
Required: MinimumMacOS = 10.7 ✔

==> Requirements
Required: MaximumMacOS = 10.6 ✘
@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

For testing, here's formulae to try out with brew info <formula>. They exercise the various requirements and their options.

phantomjs
valgrind
libstfl
sile
sbt
imagemagick
@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 18, 2016

Nit: I think "optional requirements"

==> Requirements
Optional: x11

are very very weird...

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 18, 2016

Not sure we can do any better though.

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

Hah. Yeah, that does sound weird. (Especially because that means the default is a "required requirement"?) But I think that's a consequence of the terminology Homebrew has already chosen. Probably better to make this just a UI change that sticks with the established wording for now.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 18, 2016

Got a better word than "requirements" (for UI)?

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

Been thinking about it a bit, and so far, no. Anything I've come up with is too technical-sounding.

Plus, changing that word now would mean a more extensive code change. Would rather just make this UI change as is, and figure out a better word later after discussion with everyone, if it matters.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 18, 2016

changing that word now would mean a more extensive code change

I was just thinking about a better word to stick into the ohai

==> _______
Required: MinimumMacOS = 10.7 ✔
Optional: x11

but can't think of anything either.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 18, 2016

Current coverage is 83.16% (diff: 100%)

Merging #1004 into master will decrease coverage by 0.08%

@@             master      #1004   diff @@
==========================================
  Files           340        340          
  Lines         14050      14080    +30   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          11697      11710    +13   
- Misses         2353       2370    +17   
  Partials          0          0          

Powered by Codecov. Last update 8e9a9b6...3335677

@Eitot

This comment has been minimized.

Copy link
Contributor

Eitot commented Sep 18, 2016

Why the distinction between dependencies and requirements?

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

That's a good question!

In Homebrew-speak, a Dependency is a dependency on another specific formula that can be installed by Homebrew; it's pretty simple. And a Requirement is a dependency on something that can be provided from outside Homebrew, or satisfied in multiple ways, like :x11 being provided by XQuartz, or Perl being installed, or a minimum/maximum version of OS X or Xcode, or even another formula being installed with particular language options.

This is why zmwangx and I were talking about the wording for this stuff; It's not immediately clear. We're interested in suggestions on how to word this more intuitively or document it better.

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 18, 2016

Put another way: Dependencies are things that brew can take care of automatically for you, and Requirements are things that you will (probably) have to deal with manually yourself.

@@ -16,4 +16,9 @@ def message
versions newer than #{@version.pretty_name} due to an upstream incompatibility.
EOS
end

def display_s
"MaximumMacOS = #{@version}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

Maybe just macOS <?

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Done.

Required: macOS <= 10.6 ✘

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 19, 2016

Member

Do you think <= is as understood as <? I wasn't sure but happy to defer to you here.

@@ -13,4 +13,8 @@ def initialize(tags)
def message
"OS X #{@version.pretty_name} or newer is required."
end

def display_s
"MinimumMacOS = #{@version}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

Maybe just macOS >?

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Done. Using >= instead of >, since that matches the version specified in the requirement.

Required: macOS >= 10.7 ✔

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 19, 2016

Member

Do you think >= is as understood as >? I wasn't sure but happy to defer to you here.

This comment has been minimized.

@zmwangx

zmwangx Sep 19, 2016

Contributor

Oh definitely, plus the fact that > (strictly greater than) is just wrong here.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 19, 2016

Member

@zmwangx Obviously I'd mean making the > value correct 😆


def display_s
if @version
"#{name} = #{@version}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

> rather than =, no?

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Actually, >=, I think. Done.


def display_s
if @version
"#{name} = #{@version}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

> rather than =, no?

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Also >=, i think. Done.

Required: ruby >= 1.9 ✔

def display_s
if @version
"#{name} = #{@version}"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

> rather than =, no? Also this should probably differentiate between 1.7 and 1.7+

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Would be >= or =, depending on whether the + is present. Done.

Required: java = 1.6 ✘

Required: java >= 1.6 ✔

def decorate_requirements(requirements)
req_status = requirements.collect do |req|
req.satisfied? ? pretty_installed(req.display_s) : pretty_uninstalled(req.display_s)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

As over 80 characters I'd just make this an if/else and pull the req.display_s into a variable.

This comment has been minimized.

@apjanke

apjanke Sep 18, 2016

Contributor

Anything wrong with keeping the ?: if just pulling req.display_s out to a variable gets it under 80 characters?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 19, 2016

Member

Fine with that if under 80 chars

req_status = requirements.collect do |req|
req.satisfied? ? pretty_installed(req.display_s) : pretty_uninstalled(req.display_s)
end
req_status * ", "

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

I'd prefer .join here.

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Done.

ohai "Requirements"
%w[build required recommended optional].map do |type|
reqs = f.requirements.select(&:"#{type}?")
puts "#{type.capitalize}: #{decorate_requirements(reqs)}" unless reqs.to_a.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

next if reqs.to_a.empty?

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Done.

@@ -45,7 +45,7 @@ def print_info
end
else
ARGV.named.each_with_index do |f, i|
puts unless i.zero?
puts unless i == 0

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 18, 2016

Member

Rubocop disagrees.

This comment has been minimized.

@apjanke

apjanke Sep 19, 2016

Contributor

Fixed.

@apjanke apjanke force-pushed the apjanke:print-requirements branch from 7dd7e81 to 3335677 Sep 19, 2016

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 19, 2016

Addressed Mike's review. Now the displays look like this.

==> Requirements
Build: git ✔, xcode ✔
Required: macOS >= 10.7 ✔

==> Requirements
Required: macOS >= 10.6 ✔, macOS <= 10.11 ✔

==> Requirements
Recommended: ruby >= 1.8 ✔

==> Requirements
Required: lpeg => :lua ✘, luaexpat => :lua ✘, luafilesystem => :lua ✘

==> Requirements
Required: java >= 1.6 ✔

==> Requirements
Optional: x11 ✔, perl >= 5.5 ✔
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 19, 2016

Required: lpeg => :lua ✘, luaexpat => :lua ✘, luafilesystem => :lua ✘

This is the only bit that still reads a bit weird. Maybe lpeg (lua) or even lpeg (lua module)?

@apjanke apjanke force-pushed the apjanke:print-requirements branch from 3335677 to 9798625 Sep 22, 2016

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Sep 22, 2016

Yeah, I think lpeg (lua module) reads better. Amended to do so.

Now displays as:

$ brew info sile
sile: stable 0.9.4, HEAD
...
==> Requirements
Required: lpeg (lua module) ✘, luaexpat (lua module) ✘, luafilesystem (lua module) ✘
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 24, 2016

👍 when 💚; a few style complaints currently.

@apjanke apjanke force-pushed the apjanke:print-requirements branch from 9798625 to 5aaccba Sep 25, 2016

@MikeMcQuaid MikeMcQuaid merged commit b432f8e into Homebrew:master Sep 27, 2016

2 of 4 checks passed

codecov/patch 29.41% of diff hit (target 65.71%)
Details
codecov/project 65.71% (-0.01%) compared to 03e568e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 27, 2016

Thanks @apjanke!

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

@apjanke apjanke deleted the apjanke:print-requirements branch Jun 11, 2018

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