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

Add --display-times option to `install`, `upgrade`, and `reinstall` #4359

Merged
merged 1 commit into from Jul 15, 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?

Would you consider a --display-build-times option for brew install and brew upgrade? This could be helpful to developers who are wrangling big installation trees.

I'm working on a big build with lots of dependencies (like 40 minutes total), and I'd like to see which of the dependencies are taking a lot of compilation time, so I know where to spend my efforts at making relocatable bottles, removing dependencies, or something else. I don't see a good way to do this with a custom brew command, since there aren't public API hooks into the installation process.

Adds output like this:

$ brew install --display-build-times -s arpack cowsay libpng                                                   display-build-times
==> Downloading https://github.com/opencollab/arpack-ng/archive/3.6.0.tar.gz
...
==> make test
==> make install
馃嵑  /usr/local/Cellar/libpng/1.6.34: 26 files, 1.2MB, built in 33 seconds
==> Build times
arpack               28.326 s
cowsay               3.063 s
libpng               33.318 s
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 20, 2018

A comment from left field: this looks similar to what will be needed for #4057

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jun 20, 2018

Indeed it does. Though if we're going to both, might want to implement them differently to avoid cluttering up the main install/update code.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 20, 2018

Yup.

@apjanke apjanke force-pushed the apjanke:display-build-times branch from b63f98f to ca8c294 Jun 20, 2018

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jun 20, 2018

Maybe we could consider merging this now, and refactor if/when the collected-caveats gets done? I'm thinking the "right way" to do that might be to have a hash of "collected message groups" in the main Homebrew module; have the install_formula stuff push messages to a caveats or timings member of that hash, and then have a single display_collected_messages method to run at the end of the process. But that seems like overkill if we only have one kind of message implemented (it's starting to sound like Java).

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jun 20, 2018

Stand by... a PR for a Caveats with a common message-collector design is incoming...

@apjanke apjanke referenced this pull request Jun 20, 2018

Merged

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

5 of 6 tasks complete
@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jun 20, 2018

Here's a design for a generic-ish message collector, used for Caveats, and could be extended for build times: #4361

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

I think if you find this useful others will too. I'd like to see #4361 get used for this.

formulae.each do |f|
Migrator.migrate_if_needed(f)
t0 = Time.now

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

Technically this won't be a "build time" but a "installation time" as it includes e.g. pouring, post-install.

This comment has been minimized.

@apjanke

apjanke Jul 13, 2018

Contributor

Changed the terminology to "install times".

@@ -273,6 +273,9 @@ With `--verbose` or `-v`, many commands print extra debugging information. Note

If `--verbose` (or `-v`) is passed, print the verification and postinstall steps.

If `--display-build-times` is passed, build times for each formula are printed
at the end of the upgrade run.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2018

Member

Not an upgrade here.

This comment has been minimized.

@apjanke

apjanke Jun 20, 2018

Contributor

Amended to fix.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 20, 2018

not reinstall?

@apjanke apjanke force-pushed the apjanke:display-build-times branch from ca8c294 to 2573cb0 Jun 20, 2018

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jun 20, 2018

Added reinstall.

Changed all the names to use "install times" instead of "build times".

Will change this to use #4361 once that gets merged.

@apjanke apjanke force-pushed the apjanke:display-build-times branch 3 times, most recently from 8fa5a24 to a566282 Jun 20, 2018

@apjanke apjanke changed the title Add --display-build-times option to `install` and `upgrade` Add --display-install-times option to `install`, `upgrade`, and `reinstall` Jun 20, 2018

@stale

This comment has been minimized.

Copy link

stale bot commented Jul 11, 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 the stale label Jul 11, 2018

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jul 11, 2018

Go away stalebot; I'm still working on this.

@stale stale bot removed the stale label Jul 11, 2018

@apjanke apjanke force-pushed the apjanke:display-build-times branch 2 times, most recently from d1b860a to 6739936 Jul 13, 2018

@apjanke apjanke changed the title Add --display-install-times option to `install`, `upgrade`, and `reinstall` Add --display-times option to `install`, `upgrade`, and `reinstall` Jul 13, 2018

@apjanke apjanke force-pushed the apjanke:display-build-times branch from 6739936 to b8634b2 Jul 13, 2018

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jul 13, 2018

I've rewritten this to use the message-collector mechanism from #4361.

Added unit test.

Also changed --display-install-times to a more generic --display-times, so if we end up having other operations that support displaying timings, they can just reuse the same option.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

A few nits but looking good 馃憤

@@ -221,6 +221,7 @@ def build_bottle_postinstall
end

def install
t0 = Time.now

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

start_time

@@ -349,7 +350,8 @@ def install
build_bottle_postinstall if build_bottle?

opoo "Nothing was installed to #{formula.prefix}" unless formula.installed?
Homebrew.messages.formula_installed(formula)
t1 = Time.now

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

end_time

def display_install_times
return if @install_times.empty?
oh1 "Installation times"
@install_times.each do |t|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

install_times.each; may as well use that attribute reader.

@@ -28,4 +33,12 @@ def display_caveats
ohai c[:formula], c[:caveats]
end
end

def display_install_times
return if @install_times.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

if install_times; may as well use that attribute reader.

@@ -245,6 +248,7 @@ def install
return if formulae.empty?
Install.perform_preinstall_checks

install_times = []

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

Where is this array used?

@@ -11,6 +14,7 @@ module Homebrew
def reinstall
FormulaInstaller.prevent_build_flags unless DevelopmentTools.installed?

install_times = []

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

Where is this array used?

@@ -91,6 +94,7 @@ def upgrade
end
end

install_times = []

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

Where is this array used?

end

def display_messages
display_caveats
if ARGV.include?("--display-times")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 14, 2018

Member

display_install_times if ARGV.include?("--display-times")

@apjanke apjanke force-pushed the apjanke:display-build-times branch from b8634b2 to 0a2d8c3 Jul 14, 2018

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jul 14, 2018

Nits addressed!

@MikeMcQuaid MikeMcQuaid merged commit af204c8 into Homebrew:master Jul 15, 2018

2 of 3 checks passed

codecov/patch 69.23% of diff hit (target 69.63%)
Details
codecov/project 69.63% (+<.01%) compared to 526bece
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 15, 2018

Thanks again @apjanke!

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jul 15, 2018

You're welcome! 馃帀

@apjanke apjanke deleted the apjanke:display-build-times branch Jul 15, 2018

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

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

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