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

rpm: New callbacks to wrap whole rpm transaction #1250

Closed
wants to merge 3 commits into from

Conversation

m-blaha
Copy link
Member

@m-blaha m-blaha commented Feb 16, 2024

There is currently no way for dnf5daemon user to find out when the rpm
transaction began and when it finished.
The change introduces two new callbacks on rpm::TransactionCallbacks class:

TransactionCallbacks::before_begin(int total)

  • executed right before the rpm transaction is run
  • parameter total contains number of elements in the rpm transaction.

TransactionCallbacks::after_complete(bool success)

  • executed right after the rpm transaction finished
  • parameter success indicates whether the transaction finished successfully.

These callbacks are then used in dnf5daemon-server to send respective signals
to the client.

Resolves #1189

@m-blaha m-blaha requested a review from jrohel February 16, 2024 08:21
@ppisar ppisar self-assigned this Feb 16, 2024
@ppisar
Copy link
Contributor

ppisar commented Feb 16, 2024

The code looks good, but then new signals are missing a documentation in dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.rpm.Rpm.xml. Please add the documentation.

@ppisar
Copy link
Contributor

ppisar commented Feb 16, 2024

The three failing scenarios about installonly packages are not related (a change in libsolv).

@m-blaha
Copy link
Member Author

m-blaha commented Feb 16, 2024

The code looks good, but then new signals are missing a documentation in dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.rpm.Rpm.xml. Please add the documentation.

Oh, right. Will do.

@m-blaha m-blaha force-pushed the mblaha/dnf5-daemon-end-signal branch from 3121dba to 4c8bdb7 Compare February 16, 2024 15:11
@m-blaha
Copy link
Member Author

m-blaha commented Feb 16, 2024

@ppisar Done. Sorry for the rebase, I didn't want to add more noise to git history using new commits.

@m-blaha
Copy link
Member Author

m-blaha commented Feb 16, 2024

Oh, int remained mentioned in the commit message...

New couple of transaction callbacks which are fired around the
rpmtsRun() call.

TransactionCallbacks::before_begin(uint64_t total) - parameter total contains
number of elements in the rpm transaction.

TransactionCallbacks::after_complete(bool success) - success indicates
whether the transaction finished successfully.

For #1189
Use the new callbacks before_begin / after_complete to signal clients
about overall transaction progress.
@m-blaha m-blaha force-pushed the mblaha/dnf5-daemon-end-signal branch from 4c8bdb7 to d9fb3e1 Compare February 16, 2024 15:17
@ppisar
Copy link
Contributor

ppisar commented Feb 16, 2024

Thew new RPM build failure in F41 ("Directory not found: /builddir/build/BUILDROOT/dnf5-5.1.12-1.20240216152113944400.pr1250.22.gd9fb3e11.fc41.x86_64/usr/share/bash-completion") in unrelated.

@@ -69,6 +69,13 @@ class TransactionCallbacks {

virtual ~TransactionCallbacks() = default;

/// Called right before the rpm transaction is run
/// @param total Number of elements in the rpm transaction
virtual void before_begin(uint64_t total) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

/usr/include/libdnf5/rpm/transaction_callbacks.hpp is a public header. Adding virtual methods breaks its ABI. Can this feature be implemented without virtual methods, or in a private class? Otherwise, we need to postpone this pull request until a window for ABI change opens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ech. OK then, moving the PR to 5.2.

@m-blaha
Copy link
Member Author

m-blaha commented Feb 19, 2024

The change breaks ABI, moving to 5.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

dnfdaemon Missing end transaction signal
2 participants