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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display collected caveats at end of `install` or `upgrade` #4361

Merged
merged 2 commits into from Jul 13, 2018

Conversation

Projects
None yet
3 participants
@apjanke
Copy link
Contributor

apjanke commented Jun 20, 2018

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

Fixes #4057.

This PR collects all caveats encountered during an install, upgrade, or reinstall, and outputs them together at the end of the build.

It also keeps the per-formula caveats output with each individual formula, so when a user is scanning through a long build output, they notice the caveats for each formula, instead of having to skip down to the end to check if there were caveats for that build.

Uses a generic message-collection design, per @ilovezfs's suggestion at #4359 (comment), so this could be combined with a build times output, or other output that combines messages from multiple formulae across a multi-step operation.

Example:

$ brew install vf vice
==> Downloading https://github.com/glejeune/vf/archive/0.0.1.tar.gz
Already downloaded: /Users/janke/Library/Caches/Homebrew/vf-0.0.1.tar.gz
==> Caveats
To complete installation, add the following line to your shell's rc file:
  source /usr/local/opt/vf/vf.sh
==> Summary
馃嵑  /usr/local/Cellar/vf/0.0.1: 6 files, 14.2KB, built in 2 seconds
==> Downloading https://homebrew.bintray.com/bottles/vice-3.1_1.high_sierra.bott
Already downloaded: /Users/janke/Library/Caches/Homebrew/vice-3.1_1.high_sierra.bottle.1.tar.gz
==> Pouring vice-3.1_1.high_sierra.bottle.1.tar.gz
==> Caveats
Apps for these emulators have been installed to /usr/local/opt/vice.
==> Summary
馃嵑  /usr/local/Cellar/vice/3.1_1: 1,493 files, 143.6MB
==> Caveats
==> vf
To complete installation, add the following line to your shell's rc file:
  source /usr/local/opt/vf/vf.sh
==> vice
Apps for these emulators have been installed to /usr/local/opt/vice.
@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

I'm liking this approach 馃憤

end
end

module Homebrew

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

I'd be tempted to put this in global.rb

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Yeah, that sounds better. Amended.

@@ -0,0 +1,36 @@
# A Messages object collects messages that may need to be displayed together
# at the end of a multi-step `brew` command run
class Messages

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

Could we get some basic unit tests for this class?

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Sure. Give me a little while...

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Do you want to see the Messages class tested in isolation in-process, or an example of shelling out to a brew command that produces collected caveats?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

@apjanke Tested in process/unit tests please, thanks!

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2018

Member

@apjanke Gentle ping on some unit tests, thanks!

This comment has been minimized.

@apjanke

apjanke Jul 13, 2018

Contributor

Added unit tests.

end

def display_caveats
return unless @formula_count > 1

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

return if @formula_count < 1

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Amended.

This comment has been minimized.

@RandomDSdevel

RandomDSdevel Jun 20, 2018

Contributor

Repository lurker sharp-sighting @apjanke and @MikeMcQuaid: Pardon me, but shouldn't that be '<=,' not just '<' (to properly be the true reverse of '>?')

This comment has been minimized.

@apjanke

apjanke Jun 21, 2018

Contributor

Yep. The amended commit uses return if @formula_count <= 1.

end

def record_caveats(f, caveats)
@caveats.push([f, caveats])

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

A hash might be nicer here?

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Not sure a hash would be best 鈥 I'd like to preserve the ordering of the messages wrt how they were first encountered. (I'm thinking ahead to build times here.) Or maybe alphabetical would be better?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

@caveats.push({f.name => caveats}) perhaps?

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

That gets kind of ugly when doing the reading:

  ohai c.keys.first, c[c.keys.first]

Maybe @caveats.push({"name" => f.name, "caveats" => caveats})?

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Ah, how's this? Amended.

  def record_caveats(f, caveats)
    @caveats.push(formula: f, caveats: caveats)
  end
...
    @caveats.each do |c|
      ohai c[:formula].name, c[:caveats]
    end

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

Yeh, that looks nicer. I'd still be tempted to just store the formula name, though, if that's all you're using?

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Amended to store just the formula name.

@apjanke apjanke force-pushed the apjanke:collected-caveats branch 2 times, most recently from eb4ad3e to ecb721e Jun 20, 2018

@@ -30,6 +31,9 @@
module Homebrew
extend FileUtils

@messages = Messages.new
class << self; attr_accessor :messages; end

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

There's a class << self just below here it might be nicer for all this to live in (and perhaps a lazy initialisation of @messages

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Amended to address.

@apjanke apjanke force-pushed the apjanke:collected-caveats branch 3 times, most recently from 7317484 to ec9b57b Jun 20, 2018

@stale

This comment has been minimized.

Copy link

stale bot commented Jul 12, 2018

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 added stale and removed stale labels Jul 12, 2018

@apjanke apjanke force-pushed the apjanke:collected-caveats branch from ec9b57b to 3b01f45 Jul 12, 2018

@apjanke apjanke force-pushed the apjanke:collected-caveats branch from 3b01f45 to c644392 Jul 13, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 13, 2018

@apjanke Can you rebase on origin/master? Thanks!

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 13, 2018

@apjanke Ignore me, all green now, thanks!

@MikeMcQuaid MikeMcQuaid merged commit 060c615 into Homebrew:master Jul 13, 2018

3 checks passed

codecov/patch 80% of diff hit (target 69.61%)
Details
codecov/project Absolute coverage decreased by -0.1% but relative coverage increased by +10.38% compared to 1f115fa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 13, 2018

Thanks again @apjanke!

@wafflebot wafflebot bot removed the in progress label Jul 13, 2018

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jul 13, 2018

馃憤

@apjanke apjanke deleted the apjanke:collected-caveats branch Jul 13, 2018

@lock lock bot added the outdated label Aug 12, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 2018

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