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

Show the output of failed scriptlets to the user #1652

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

m-blaha
Copy link
Member

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

Currently, the output of RPM scriptlets is somewhat hidden in the log. This patch introduces a new libdnf5::base::Transaction::get_last_script_output() method, which returns the outputs of the most recently executed RPM scriptlet. This method is then utilized in the transaction callbacks in DNF5 to display the output to the user.

Although it would be more intuitive to add the scriptlet output as a new parameter in script_end and script_error, doing so would break the existing API.

The scriptlet output is only printed in the event of a failure; it is not displayed when the execution is successful.

Resolves: #522
Replaces: #1645

Changes:

5771571 (Marek Blaha, 5 minutes ago)
dnf5: Print rpm scriptlets outputs to the user

If a scriptlet fails, its output is displayed to the user in addition to
being written to the log. For scriptlets that complete successfully, the
output is only logged.

27d322a (Marek Blaha, 9 minutes ago)
transaction: Store outputs of the last rpm scriptlet

The outputs of the most recently executed RPM scriptlet in the transaction
are stored and can be accessed by the user through the
Base::Transaction::get_last_script_output() method.

55cf66f (Marek Blaha, 21 minutes ago)
rpm::transaction: Base::Transaction to callback holder

Handling scriptlet outputs requires rpm transaction callbacks to have
access to the libdnf5::Base::Transaction instance the rpm transaction
belongs to.

Copy link
Contributor

@jrohel jrohel left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. Just a minor change, please. See notes in review.

@@ -30,6 +30,7 @@ along with libdnf. If not, see <https://www.gnu.org/licenses/>.
#include "libdnf5/rpm/transaction_callbacks.hpp"

#include <optional>
#include <string_view>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the string_view file included to the public header?
It is not used in this header. Please move the line to the transaction_impl.hpp file.

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, I will fix it.

@@ -193,12 +194,17 @@ class LIBDNF_API Transaction {
void set_download_local_pkgs(bool value);
bool get_download_local_pkgs() const noexcept;

/// Retrieve output of the last executed rpm scriptlet.
std::string get_last_script_output();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we need to make a copy in the future or if it would be better to just return a constant reference.
const std::string & get_last_script_output();
If you change here to a reference, change in transaction_impl.hpp as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I intended to make a copy here. The thing is that this string is regularly changed from another thread and I wanted to ensure that the string the client received will remain the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the user calls get_last_script_output in a script_error or script_stop callback, the script has already finished and a new script will be started after the next script_start callback. The rpm executes scripts serially (there is no parallelization). So the contents of the string should not change while the script_error and script_stop callbacks are being serviced. And if the user needs to keep it for longer, he will make his own copy.

Your solution allows the user to call the get_last_script_output method from another thread. We don't generally support multithreading - it is not supported by the libraries used, for example libsolv. So I still don't know if this copy will help us in the future. But I will merge it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was specifically thinking about situations where the client keeps the output longer then during script_start/script_end execution. You are right that it can be handled on the client side. Thank you.

Handling scriptlet outputs requires rpm transaction callbacks to have
access to the `libdnf5::Base::Transaction` instance the rpm transaction
belongs to.
The outputs of the most recently executed RPM scriptlet in the
transaction are stored and can be accessed by the user through the
Base::Transaction::get_last_script_output() method.
If a scriptlet fails, its output is displayed to the user in addition to
being written to the log. For scriptlets that complete successfully, the
output is only logged.
@jrohel jrohel added this pull request to the merge queue Aug 28, 2024
Merged via the queue into main with commit be2e6be Aug 28, 2024
11 of 20 checks passed
@jrohel jrohel deleted the mblaha/scriptlet-output-2 branch August 28, 2024 21:38
@jrohel jrohel self-assigned this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dnf5 eats stdout/stderr from the underlying scriptlets
2 participants