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

Display formula conflict reasons #2470

Merged
merged 5 commits into from May 16, 2017

Conversation

Projects
None yet
2 participants
@johnhawkinson
Copy link
Contributor

johnhawkinson commented Apr 9, 2017

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

Scenario

The enduser story for dealing with formula conflicts is not very good right now.
For instance, this morning I wanted a sed with better regexp support than the stock OSX version. brew search sed told me gnu-sed existed, and so of course I reviewed brew info gnu-sed first to see what I was getting:

pb3:Homebrew jhawk$ brew info gnu-sed
gnu-sed: stable 4.4 (bottled)
GNU implementation of the famous stream editor
https://www.gnu.org/software/sed/
Conflicts with: ssed

Hrmm. It conflicts with ssed? Why is that, and also, maybe I should be looking more closely at ssed too... brew info ssed is no help:

pb3:extractor jhawk$ brew info ssed
ssed: stable 3.62 (bottled)
Super sed stream editor
https://sed.sourceforge.io/grabbag/ssed/
Conflicts with: gnu-sed

If I were willing to review the formula source code, I would find:

  conflicts_with "ssed", :because => "both install share/info/sed.info"
  conflicts_with "gnu-sed", :because => "both install share/info/sed.info"

but that's not a reasonable thing for endusers to do. And also, if I have already installed gnu-sed and try to install ssed, the installation will fail and tell me why:

pb3:Homebrew jhawk$ brew install ssed
Updating Homebrew...
==> Auto-updated Homebrew!
Updated 2 taps (caskroom/cask, homebrew/core).
No changes to formulae.

Error: Cannot install ssed because conflicting formulae are installed.

  gnu-sed: because both install share/info/sed.info

But this also can't be the answer -- if I don't know why 2 formulas conflict, I should not have to install one in order to read the conflict message from the other. I should be able to figure this out nondestructively without installing formulas.

Solution

brew info should show me conflict information, ala:

pb3:Formula jhawk$ brew info gnu-sed
gnu-sed: stable 4.4 (bottled)
GNU implementation of the famous stream editor
https://www.gnu.org/software/sed/
Conflicts with: ssed (because both install share/info/sed.info)
/usr/local/Cellar/gnu-sed/4.4 (12 files, 491.5KB) *

Here's an example with a formula with multiple conflicts:

pb3:Formula jhawk$ brew info app-engine-go-32
Unsetting PERL5LIB to protect you from yourself
app-engine-go-32: stable 1.9.46
Google App Engine SDK for Go (i386)
https://cloud.google.com/appengine/docs/go/
Conflicts.with: app-engine-go-64 (because both install the same binaries),
                app-engine-python (because both install the same binaries)
Not installed

And here's one that doesn't specify a reason (is that a bug?):

pb3:Formula jhawk$ brew info watch
watch: stable 3.3.12 (bottled), HEAD
Executes a program periodically, showing output fullscreen
https://gitlab.com/procps-ng/procps
Conflicts.with: visionmedia-watch
Not installed

And while we're here, the documentation for conflicts_with has a particularly poor example; improve it.

Tests

I'm not sure what sort of test would be appropriate here? Should I add a faux conflicts_with to Library/Homebrew/test/support/fixtures/testball.rb and then change cmd/info_spec.rb? I don't see any examples that care about the specifity of command output to this degree.

johnhawkinson added some commits Apr 9, 2017

formula: conflicts_with doc: give realistic :because
The documentation should help a developer understand what the
parameter is for.  "stupid example" doesn't cut it.
@johnhawkinson

This comment has been minimized.

Copy link
Contributor

johnhawkinson commented Apr 9, 2017

Whoops, forgot brew style. Maybe that should be in the PR template?
Complaints about formula.rb are preexisting.
Fixed the ones in info.rb.

Why is Rubocop configured with this?:

pb3:cmd jhawk$ brew style -v info.rb
rubocop --force-exclusion --config /usr/local/Homebrew/Library/.rubocop.yml info.rb --format simple
== info.rb ==
C: 27:  1: Module has too many lines. [143/100]

1 file inspected, 1 offense detected

It's an unattainable standard for small edits, and causes unfortunate CI failures?
Edit: Or not? I guess the CI is configured differently? Not sure how that works...

puts "Conflicts with: #{conflicts*", "}" unless conflicts.empty?
conflicts = f.conflicts.map do |c|
c.name +
(c.reason ? " (because #{c.reason})" : "")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 17, 2017

Member

Make this all a single interpolated string (i.e. "#{c.name}#{reason}") and also do something like reason = " (because #{c.reason})" if c.reason to allow the reason to be interpolated.

This comment has been minimized.

@johnhawkinson

johnhawkinson May 14, 2017

Contributor

Sorry for the delay, got distracted. This is done.

(c.reason ? " (because #{c.reason})" : "")
end.sort!
msg="Conflicts with: "
puts msg+conflicts*(",\n"+" "*msg.length) unless conflicts.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 17, 2017

Member

Also make this a single interpolated string (i.e. "Conflicts with: #{conflicts.join ", "}") and use .join.

This comment has been minimized.

@johnhawkinson

johnhawkinson May 14, 2017

Contributor

I'm not sure how to do as you request and make the alignment across multiple lines work right. By setting msg first, the code as written can use the length of that line to pad the continuation lines with 13 blanks so it lines up properly, but without hardcoding the count. I'm not sure how to do what you ask without losing that. Maybe I want to be too fancy?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 15, 2017

Member

I've changed this to have a pretty simple solution that has the best of both, I think, and will work better on 80 character terminals. Thanks for caring, though ❤️

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 17, 2017

This seems like an improvement, nice work.

I'm not sure what sort of test would be appropriate here? Should I add a faux conflicts_with to Library/Homebrew/test/support/fixtures/testball.rb and then change cmd/info_spec.rb? I don't see any examples that care about the specifity of command output to this degree.

Using formula do would probably be more appropriate. As long as we can increase the % of the diff hit > 50% I think there doesn't need to be a test for the specific output format.

Why is Rubocop configured with this?:

It's a nudge to consider not making modules longer when possible. This case is fine I think, though.

@stale stale bot added the stale label May 8, 2017

@stale

This comment has been minimized.

Copy link

stale bot commented May 8, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot removed the stale label May 14, 2017

@MikeMcQuaid MikeMcQuaid merged commit aeaf9af into Homebrew:master May 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 16, 2017

Thanks again @johnhawkinson!

@johnhawkinson

This comment has been minimized.

Copy link
Contributor

johnhawkinson commented May 16, 2017

Oops, that's not quite right:

pb3:cmd jhawk$ brew info app-engine-go-32
Unsetting PERL5LIB to protect you from yourself
app-engine-go-32: stable 1.9.46
Google App Engine SDK for Go (i386)
https://cloud.google.com/appengine/docs/go/
Conflicts with:
  app-engine-go-64 (because both install the same binaries)  
app-engine-python (because both install the same binaries)
Not installed
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/app-engine-go-32.rb

Looks like 8e2198f should have been:

diff --git a/Library/Homebrew/cmd/info.rb b/Library/Homebrew/cmd/info.rb
index ba920d005b..826f31bbd7 100644
--- a/Library/Homebrew/cmd/info.rb
+++ b/Library/Homebrew/cmd/info.rb
@@ -130,7 +130,7 @@ module Homebrew
     unless conflicts.empty?
       puts <<-EOS.undent
         Conflicts with:
-          #{conflicts.join("  \n")}
+          #{conflicts.join("\n  ")}
       EOS
     end
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 16, 2017

@johnhawkinson My bad, sorry. Fixed in 2b72638

@lock lock bot added the outdated label May 11, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 11, 2018

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