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 RPM messages to the user #1728

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Print RPM messages to the user #1728

merged 7 commits into from
Oct 1, 2024

Conversation

m-blaha
Copy link
Member

@m-blaha m-blaha commented Sep 25, 2024

The RPM messages contain vital information for debugging RPM transaction.
Currently are these messages only written to the dnf5.log file. This patch
also prints them to the user during the transaction execution.

Resolves: #1710
Related: https://bugzilla.redhat.com/show_bug.cgi?id=2312906

@m-blaha m-blaha marked this pull request as draft September 27, 2024 06:29
Currently, RPM log messages are written directly to the log file.
The new `extract_rpm_logs_buffer()` method allows libdnf5 to retrieve
log messages emitted since the last method call and handle them.
This will be used in RPM transaction callbacks to display RPM logs to the user.
Adds new public API to retrieve recent RPM log messages.
The setter is only used internally and thus is private.
@m-blaha m-blaha marked this pull request as ready for review September 27, 2024 08:32
Copy link
Member

@evan-goode evan-goode left a comment

Choose a reason for hiding this comment

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

Looks great overall!

@@ -321,6 +321,10 @@ class Transaction {
/// @param file_path new file path
void set_script_out_file(const std::string & file_path);

/// Retrieve recently emitted rpm log messages. After the method is called,
/// the stored rpm log messages are cleared.
std::vector<std::string> get_rpm_messages();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be called something other than get_rpm_messages since it mutates the object. I guess it is not marked const so maybe that is enough. But what about calling it extract_rpm_messages or something?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "pop" or "dequeue" would make the behavior clearer (the latter is annoying to type 😅 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks guys, I'll go with extract which is consistent with what RpmLogGuard has.

Copy link
Member Author

Choose a reason for hiding this comment

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

@evan-goode Done.

At the end of each RPM transaction "block" (before the transaction_stop,
(un)install_stop, unpack_error, and script_stop callbacks are
triggered), the captured RPM log messages are stored in the
base.Transaction object, making them accessible for transaction
callbacks to handle.
Change the context.script_output_to_progress() to return the number of
lines actually printed.
- In case of an error, do not print the "Stop scriptlet" message, as
  it's redundant and always followed by the "Error in scriptlet" message.

- For better code clarity, concentrate all scriptlet status printing in
  the `script_stop()` callback, leaving `script_error()` empty.

- Drop the "return code XX" part of the message. The return code is only
  used to distinguish between success, non-critical errors, and fatal
  errors, which are already indicated by the existing messages.

- Even successful scriptlet executions may produce important output.
  Always forward the scriptlet output to the user.
In the end callback of each RPM transaction section, print all RPM log
messages to the user.
Some RPM transaction callback functionality (e.g. reporting RPM logs and
scriptlet outputs) depends on the current transaction being set in the
context.
@evan-goode
Copy link
Member

The F39 tests are failing in the CI due to https://bugzilla.redhat.com/show_bug.cgi?id=2315859, but they pass locally (after rebasing to main). Merging.

@evan-goode evan-goode added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit 61a0e35 Oct 1, 2024
17 of 20 checks passed
@evan-goode evan-goode deleted the mblaha/rpm-logs branch October 1, 2024 17:50
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.

Present rpm logs to the user in case of problems
3 participants