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 #1645

Closed
wants to merge 3 commits into from

Conversation

m-blaha
Copy link
Member

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

Currently, the output of RPM scriptlets is kind of buried in the log. This patch introduces a new script_output callback, which is executed just before script_end or script_error. While it would be more intuitive to include the scriptlet output directly as a new parameter in script_end and script_error, I opted to add a new callback to maintain compatibility.

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

Resolves: #522

Changes:

eaf5dd9 (Marek Blaha, 39 minutes ago)
dnf5: Handle script_output callback for transaction

0d6e2cf (Marek Blaha, 41 minutes ago)
transaction: New script_output callback

Used to pass the outputs (stdout and stderr) of an executed scriptlet to
the user.

b915955 (Marek Blaha, 26 hours ago)
transaction: Move scriptlet outputs to rpm::Transaction

The code works on rpm level anyway and I'd like to use this mechanism in
transaction callback to present the outputs to the user. Also adds
rpm::Transaction class member to store the output of the last scriptlet.

The code works on rpm level anyway and I'd like to use this mechanism in
transaction callback to present the outputs to the user.
Also adds rpm::Transaction class member to store the output of the last
scriptlet.
Used to pass the outputs (stdout and stderr) of an executed scriptlet to
the user.
@m-blaha
Copy link
Member Author

m-blaha commented Aug 21, 2024

@jrohel @jan-kolarik may I ask you for your opinion?

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.

PR breaks ABI. And in a very bad way.
I described it in the review with an example solution.

/// @param type Type of the scriptlet
/// @param return_code Return code of the scriptlet: OK, non-critical error, error
/// @param output Output produced by the scriptlet
virtual void 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.

PR inserts a new virtual method into the public callback class. This does not break the API.

However, adding a virtual function will definitely change the vtable and how it changes depends on the compiler.
The result will be a broken ABI. And in a very bad way. An application using a library with an added or missing virtual callback method will run and then crash during the callback function call.

If we want to keep ABI backward compatibility, we need to do it differently.

Example solution:
Instead of adding a new callback script_output (a virtual method), add a non-virtual API method libdnf5::base::Transaction::get_last_scriptlet_output that the library user will be able to call while servicing the existing callback script_stop .
This solution came to my mind first. Maybe there is a better place for a new non-virtual method. This is an example.

@m-blaha m-blaha marked this pull request as draft August 22, 2024 10:31
@m-blaha m-blaha closed this Aug 26, 2024
@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