Skip to content
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

cmd/info: show size information #18172

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Aug 26, 2024

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Follow up to #18144

Proof of concept for now on getting size info into brew info.

May need to see if any better ways to get this information out of the manifest. A bit trickier than just tab info.

brew info cling git-town --github-manifest
==> cling: stable 1.0 (bottled)
C++ interpreter
https://root.cern/cling/
Not installed
Bottle size: 264.3MB
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/c/cling.rb
License: (LGPL-2.1-only or NCSA) and (Apache-2.0 with LLVM-exception)
==> Dependencies
Build: cmake ✘

==> git-town: stable 15.3.0 (bottled)
High-level command-line interface for Git
https://www.git-town.com/
Not installed
Bottle size: 5MB
Installed size: 16.0MB
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/g/git-town.rb
License: MIT
==> Dependencies
Build: go ✔


private

def manifest_annotations
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe to cache?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think so?

Comment on lines +311 to +314
bottle_size = bottle.bottle_size
puts "Bottle size: #{disk_usage_readable(bottle_size)}" if bottle_size.positive?
installed_size = bottle.installed_size
puts "Installed size: #{disk_usage_readable(installed_size)}" if installed_size.positive?
Copy link
Member Author

@cho-m cho-m Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently looks like

Bottle size: 5MB
Installed size: 16.0MB

For comparison

Also, mainly an approximate if pouring bottle into non-standard prefix.

@@ -45,6 +45,8 @@ class Info < AbstractCommand
switch "--github",
description: "Open the GitHub source page for <formula> and <cask> in a browser. " \
"To view the history locally: `brew log -p` <formula> or <cask>"
switch "--github-manifest",
description: "Fetch Github package manifest for extra information when <formula> is not installed."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: "Fetch Github package manifest for extra information when <formula> is not installed."
description: "Fetch GitHub package manifest for extra information when <formula> is not installed."

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me so far, nice work!

Would be interested (post-merge) in benchmarks for this so we can consider how to get to being able to put these in the API.

CC @reitermarkus also for thoughts on whether the parallel downloading work overlaps here and when it might be ready.

@@ -45,6 +45,8 @@ class Info < AbstractCommand
switch "--github",
description: "Open the GitHub source page for <formula> and <cask> in a browser. " \
"To view the history locally: `brew log -p` <formula> or <cask>"
switch "--github-manifest",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work with --json?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet. Haven't had time to look at that code path. Also need to figure out the path to get information to formulae.brew.sh, which may be similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, it's that code path. If it doesn't work for now: might be worth marking as conflicting?


private

def manifest_annotations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think so?

end

def bottle_size
manifest_annotations["sh.brew.bottle.size"].to_i
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
manifest_annotations["sh.brew.bottle.size"].to_i
manifest_annotations.fetch("sh.brew.bottle.size").to_i

or

Suggested change
manifest_annotations["sh.brew.bottle.size"].to_i
manifest_annotations["sh.brew.bottle.size"]&.to_i

depending on whether manifest_annotations is supposed to always have that key.

@reitermarkus
Copy link
Member

on whether the parallel downloading work overlaps here

Yes, slight overlap, but the changes to Resource here seem to be exactly the same, so shouldn't conflict too hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants