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

dnf5: Statistics: total and downloaded pkgs size, installed and freed space #378

Conversation

jrohel
Copy link
Contributor

@jrohel jrohel commented Mar 20, 2023

Solves issue #342

@jrohel jrohel force-pushed the feature/total_download_installed_size branch from 476982e to 4a7baa1 Compare March 20, 2023 12:37
@jan-kolarik jan-kolarik self-assigned this Mar 21, 2023
@jrohel jrohel force-pushed the feature/total_download_installed_size branch 2 times, most recently from 7391c69 to f3dd6c6 Compare March 22, 2023 06:13
@jrohel
Copy link
Contributor Author

jrohel commented Mar 22, 2023

The code is ready, but we need to agree on the output format. Below are examples of the old DNF output and my suggestion of how the new output might look. However, we need to discuss the actual form of the new output.


dnf swap iftop atop (the package "atop" needs to be downloaded)
Old DNF:

Total download size: 199 k

Mising information about used space.

DNF5:

Total size of incoming pakages is 199 KiB. Need to download 199 KiB.
After unpacking 375 KiB will be used (installed 473 KiB, removed 98 KiB).

dnf swap iftop atop (the package "atop" is cached)
Old DNF:

Total size: 199 k

Mising information about used space.

DNF5:

Total size of incoming pakages is 199 KiB. Need to download 0 B.
After unpacking 375 KiB will be used (installed 473 KiB, removed 98 KiB).

dnf remove iftop
Old DNF:

Freed space: 98 k

DNF5:

Total size of incoming pakages is 0 B. Need to download 0 B.
After unpacking 98 KiB will be freed (installed 0 B, removed 98 KiB).

dnf install atop htop (the package "atop" is cached)
Old DNF:

Total size: 384 k
Total download size: 185 k
Installed size: 914 k

DNF5:

Total size of incoming pakages is 384 KiB. Need to download 185 KiB.
After unpacking 914 KiB will be used (installed 914 KiB, removed 0 B).

@jrohel jrohel force-pushed the feature/total_download_installed_size branch from f3dd6c6 to d4d0148 Compare March 22, 2023 07:21
@jan-kolarik
Copy link
Member

For me, it seems OK in most cases. But for the remove command I suppose the format could be different and less verbose as there will never be any incoming package, right? Btw typo pakages => packages.

@jrohel
Copy link
Contributor Author

jrohel commented Mar 23, 2023

@jan-kolarik Thanks. I thinking about these changes:

  • fix typo pakages => packages
  • (installed 914 KiB, removed 0 B) => (install 914 KiB, remove 0 B)
  • Show Total size of incoming packages is... line only if there are incoming packages.

@jan-kolarik
Copy link
Member

I am also wondering about the line for the remove command: "After unpacking 98 KiB will be freed". Maybe change the phrase to something more general like "After the operation xxx B of space will be used/freed"...

@m-blaha
Copy link
Member

m-blaha commented Mar 23, 2023

FWIW, on the API we use inbound/outbound terminology.
Ubuntu uses this wording:

Need to get 13.0 kB of archives.
After this operation, 53.2 kB of additional disk space will be used.

After this operation, 53.2 kB disk space will be freed.

@m-blaha
Copy link
Member

m-blaha commented Mar 23, 2023

And this is from zypper:

install:
Overall download size: 27.6 KiB. Already cached: 0 B. After the operation, additional 51.7 KiB will be used.

remove:
After the operation, 51.7 KiB will be freed.

@jrohel jrohel force-pushed the feature/total_download_installed_size branch from d4d0148 to 1292eed Compare March 24, 2023 08:10
@jrohel
Copy link
Contributor Author

jrohel commented Mar 24, 2023

I made the above adjustments. And I replaced "incoming" with "inbound" as we have in the API.

The resulting output is:

Total size of inbound packages is 199 KiB. Need to download 199 KiB.
After unpacking 375 KiB will be used (install 473 KiB, remove 98 KiB).

The line Total size of inbound... is displayed only in the case of a non-zero value of inbound packages.

@jrohel
Copy link
Contributor Author

jrohel commented Mar 24, 2023

I was considering whether to use "unpacking" or "operation". Neither is accurate. This is the size after unpacking files from inbound packages and deleting files from outbound (uninstalled/replaced) packages. But the size of the resulting operation may be different - scriptlets will generate/delete additional files (eg. initramfs).
I have no idea which word is better in this case.

@m-blaha
Copy link
Member

m-blaha commented Mar 24, 2023

Although I understand your point about accurateness, I would still prefer After this operation xx will be used/freed wording. This way the line could remain the same for any transaction, regardless it's install or remove.
In the documentation we can describe in detail that these numbers cover only sum of packages sizes from metadata.

jrohel added 6 commits March 27, 2023 10:09
The class template `OnScopeExit` is a general-purpose scope guard
intended to call its exit function when a scope is exited.
Returns `true` if the package is available locally (in cache, command
line package).
`format_size` produces partially aligned output with one decimal place.
Examples: "25.0   B", "4.0 KiB"
Therefore, the function was renamed to `format_size_aligned`.

We often need a different format. That's why the function `to_size` was
created, which returns a pair of float value and unit. The user can then
format the output as desired.
@jrohel jrohel force-pushed the feature/total_download_installed_size branch from 1292eed to 849204a Compare March 27, 2023 08:26
@jrohel
Copy link
Contributor Author

jrohel commented Mar 27, 2023

@m-blaha OK. I rebased the PR and changed the message to After this operation xxx will be used/freed (install xxx, remove xxx)..

/// The class template OnScopeExit is a general-purpose scope guard
/// intended to call its exit function when a scope is exited.
template <typename TExitFunction>
requires requires(TExitFunction f) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, that's a first occurrence of C++20 concepts I've seen in our project 🙂

Copy link
Member

@jan-kolarik jan-kolarik left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@jan-kolarik jan-kolarik merged commit 8830772 into rpm-software-management:main Mar 28, 2023
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