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

Print diagnostic messages on stderr, not stdout #1641

Merged

Conversation

evan-goode
Copy link
Member

Resolves #1361

This patchset changes many messages to be printed on stderr instead of stdout.

Opinions vary, but by general convention, the "useful output" of a program (that might be e.g. piped to a pager or consumed by a script), should be on stdout. All other messages, including warnings, progress bars, "Are you sure you want to continue?"-style prompts should be on stderr.

There are gray areas here, especially when you consider the same piece of output in different contexts. You could argue that the "Updating and loading repositories" message and multiprogressbar are "useful output" of the makecache command, but the same messages are not "useful output" of the repoquery command. And DNF 5 commands such as install and upgrade are primarily "imperative" and don't have "useful output" in the same way that commands like repolist and repoquery do. So here, I've tried to use my best judgment and err on the side of keeping the status quo.

The following messages are moved from stdout to stderr:

  • All messages printed via Context::print_info go to stderr, which includes:
    • "Updating and loading repositories:"
    • "Repositories loaded."
    • "Running transaction"
    • "Testing offline transaction"
    • "After this operation, x extra will be used (install x, remove x)."
    • "After this operation, x extra will be freed (install x, remove x)."
    • "Total size of inbound packages is x. Need to download x."
    • "Complete!"
  • Warnings, errors (most were already on stderr), and userconfirm prompts
  • Progress bars
  • Output related to key import

To observe these changes, it's useful to redirect both streams to files:

sudo dnf5 <args> >/tmp/stdout 2>/tmp/stderr

and watch them from other terminals:

tail -Fqf /tmp/stdout /tmp/stderr   # merged stdout stderr
tail -Ff /tmp/stdout                # just stdout
tail -Ff /tmp/stdout                # just stderr
Output of dnf5 --refresh rq rawhide xz after this patch

(before this patch, all output was on stdout.)

merged stdout and stderr
Updating and loading repositories:
 Fedora 40 openh264 (From Cisco) - x86_64    100% |   6.7 KiB/s | 989.0   B |  00m00s
 Fedora 40 - x86_64 - Updates                100% | 223.7 KiB/s |  26.2 KiB |  00m00s
 Fedora 40 - x86_64                          100% | 220.3 KiB/s |  28.0 KiB |  00m00s
 Local Repository                            100% |   0.0   B/s |   1.5 KiB |  00m00s
Repositories loaded.
xz-1:5.4.6-3.fc40.x86_64
stdout
xz-1:5.4.6-3.fc40.x86_64
stderr
Updating and loading repositories:
 Fedora 40 openh264 (From Cisco) - x86_ 100% |  11.9 KiB/s | 989.0   B |  00m00s
 Local Repository                       100% |   0.0   B/s |   1.5 KiB |  00m00s
 Fedora 40 - x86_64 - Updates           100% | 219.9 KiB/s |  26.2 KiB |  00m00s
 Fedora 40 - x86_64                     100% |  87.4 KiB/s |  28.0 KiB |  00m00s
Repositories loaded.
Output of dnf5 install hello after this patch

(before this patch, all output was on stdout.)

merged stdout and stderr
Updating and loading repositories:
Repositories loaded.
Package Arch   Version       Repository      Size
Installing:
 hello  x86_64 2.12.1-4.fc40 fedora     196.4 KiB
Installing weak dependencies:
 info   x86_64 7.1-2.fc40    fedora     357.8 KiB

Transaction Summary:
 Installing:        2 packages

Total size of inbound packages is 269 KiB. Need to download 269 KiB.
After this operation, 554 KiB extra will be used (install 554 KiB, remove 0 B).
Is this ok [y/N]: [1/2] hello-0:2.12.1-4.fc40.x86_64      100% | 239.8 KiB/s |  87.1 KiB |  00m00s
[2/2] info-0:7.1-2.fc40.x86_64          100% | 450.2 KiB/s | 182.3 KiB |  00m00s
--------------------------------------------------------------------------------
[2/2] Total                             100% | 483.7 KiB/s | 269.4 KiB |  00m01s
Running transaction
[1/4] Verify package files              100% |   2.0 KiB/s |   2.0   B |  00m00s
[2/4] Prepare transaction               100% |  28.0   B/s |   2.0   B |  00m00s
[3/4] Installing info-0:7.1-2.fc40.x86_ 100% |  18.4 MiB/s | 358.2 KiB |  00m00s
[4/4] Installing hello-0:2.12.1-4.fc40. 100% | 352.9 KiB/s | 204.4 KiB |  00m01s
Complete!
stdout
Package Arch   Version       Repository      Size
Installing:
 hello  x86_64 2.12.1-4.fc40 fedora     196.4 KiB
Installing weak dependencies:
 info   x86_64 7.1-2.fc40    fedora     357.8 KiB

Transaction Summary:
 Installing:        2 packages

stderr
Updating and loading repositories:
Repositories loaded.
Total size of inbound packages is 269 KiB. Need to download 269 KiB.
After this operation, 554 KiB extra will be used (install 554 KiB, remove 0 B).
Is this ok [y/N]: [1/2] hello-0:2.12.1-4.fc40.x86_64      100% | 608.8 KiB/s |  87.1 KiB |  00m00s
[2/2] info-0:7.1-2.fc40.x86_64          100% |   1.0 MiB/s | 182.3 KiB |  00m00s
--------------------------------------------------------------------------------
[2/2] Total                             100% | 844.5 KiB/s | 269.4 KiB |  00m00s
Running transaction
[1/4] Verify package files              100% |   2.0 KiB/s |   2.0   B |  00m00s
[2/4] Prepare transaction               100% |  28.0   B/s |   2.0   B |  00m00s
[3/4] Installing info-0:7.1-2.fc40.x86_ 100% |  17.5 MiB/s | 358.2 KiB |  00m00s
[4/4] Installing hello-0:2.12.1-4.fc40. 100% | 364.3 KiB/s | 204.4 KiB |  00m01s
Complete!

I anticipate this PR needing a large ci-dnf-test patch, so I've marked it as a draft.

Move some messages that are not strictly part of the "useful output" of
the program to stderr, including warnings, errors, userconfirm prompts,
and output related to key import.
@jan-kolarik jan-kolarik self-assigned this Aug 21, 2024
dnf5/context.cpp Show resolved Hide resolved
dnf5/context.cpp Outdated Show resolved Hide resolved
dnf5/main.cpp Outdated Show resolved Hide resolved
dnf5/context.cpp Outdated Show resolved Hide resolved
@jan-kolarik
Copy link
Member

And thanks for the great description with examples!

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.

LGTM

@evan-goode
Copy link
Member Author

Marking as RFR. CI PR: rpm-software-management/ci-dnf-stack#1540.

@jan-kolarik
Copy link
Member

Marking as RFR. CI PR: rpm-software-management/ci-dnf-stack#1540.

I've run the tests locally. It should be OK, there were few failures, but this should be due to different dnf5 bases here in the PR vs all the tests included in the CI PR.

@jan-kolarik jan-kolarik added this pull request to the merge queue Sep 2, 2024
Merged via the queue into rpm-software-management:main with commit 58cfa3f Sep 2, 2024
9 of 20 checks passed
jan-kolarik pushed a commit to rpm-software-management/ci-dnf-stack that referenced this pull request Sep 2, 2024
psss added a commit to teemtee/tmt that referenced this pull request Sep 19, 2024
We don't care that much where `dnf` puts the info about which
packages were installed. Plus in different `dnf` versions the
output goes into different output streams. See also:

* rpm-software-management/dnf5#1641
psss added a commit to teemtee/tmt that referenced this pull request Sep 20, 2024
We don't care that much where `dnf` puts the info about which
packages were installed. Plus in different `dnf` versions the
output goes into different output streams. See also:

* rpm-software-management/dnf5#1641
psss added a commit to teemtee/tmt that referenced this pull request Sep 20, 2024
We don't care that much where `dnf` puts the info about which
packages were installed. Plus in different `dnf` versions the
output goes into different output streams. See also:

* rpm-software-management/dnf5#1641
psss added a commit to teemtee/tmt that referenced this pull request Sep 20, 2024
We don't care that much where `dnf` puts the info about which
packages were installed. Plus in different `dnf` versions the
output goes into different output streams. See also:

* rpm-software-management/dnf5#1641
psss added a commit to teemtee/tmt that referenced this pull request Sep 20, 2024
We don't care that much where `dnf` puts the info about which
packages were installed. Plus in different `dnf` versions the
output goes into different output streams. See also:

* rpm-software-management/dnf5#1641
psss added a commit to teemtee/tmt that referenced this pull request Sep 20, 2024
We don't care that much where `dnf` puts the info about which
packages were installed. Plus in different `dnf` versions the
output goes into different output streams. See also:

* rpm-software-management/dnf5#1641
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

dnf5 repoquery writes repo update output to stdout not stderr
2 participants