Make dnf5 compatible with sdbus-cpp version 2 #1888

m-blaha · 2024-11-22T09:13:16Z

The new and incompatible version of sdbus-cpp library has been released upstream.
I'd like to upgrade also version in Fedora rawhide, but to not break dnf5
building we fist need to adjust it.
This patch makes dnf5 buildable with both sdbus-cpp versions.

Conan-Kudo

This fixed compiling against sdbus-cpp v2 in openSUSE Tumblweed.

kontura

This looks reasonable to me but the dnf5daemon CI tests fail, I have verified that also locally.
I didn't investigate it further but the error is: [System.Error.ENXIO] Failed to enter a variant (No such device or address) when sdbus-cpp < 2.

With sdbus-cpp >= 2 (I used your build from https://copr.fedorainfracloud.org/coprs/mblaha/dnf5-sdbus-cpp-2/) I am getting some [org.rpm.dnf.v0.rpm.Rpm.TransactionError] rpm transaction failed with code 6.

kontura · 2024-12-19T08:27:07Z

cmake/sdbus_cpp.cmake

@@ -0,0 +1,11 @@
+


I think we could drop this new line.

kontura · 2024-12-19T09:45:10Z

dnf5daemon-server/session_manager.cpp

-        "org.freedesktop.DBus", "NameOwnerChanged", [this](sdbus::Signal signal) -> void {
+        SDBUS_INTERFACE_NAME_TYPE{"org.freedesktop.DBus"},
+        SDBUS_SIGNAL_NAME_TYPE{"NameOwnerChanged"},
+        [this](sdbus::Signal signal) -> void {


I guess this is in the wrong commit, but it doesn't matter all that much.

Also set SDBUS_CPP_VERSION_2 macro in case sdbus-c++ was found in version 2. This enables conditional compilation according the version of the library found.

- Use InterfaceName type instead of char * for interface names - Use SignalName type instead of char * for signal names - Use ServiceName instead of char * for bus names - Use Error::Name type instead of char * for errors

In version 2 the sdbus::Variant constuctor was made explicit.

In version 2, conversion of sdbus::Variant to the underlying type is explicit.

sdbus-cpp version 2 requires signal handler to have signature (sdbus::Signal signal) -> void, which is also acceptable for version 1.

sdbus-cpp-2 uses different approach to registering methods and signals on the interface.

m-blaha · 2024-12-20T10:28:02Z

This looks reasonable to me but the dnf5daemon CI tests fail, I have verified that also locally. I didn't investigate it further but the error is: [System.Error.ENXIO] Failed to enter a variant (No such device or address) when sdbus-cpp < 2.

With sdbus-cpp >= 2 (I used your build from https://copr.fedorainfracloud.org/coprs/mblaha/dnf5-sdbus-cpp-2/) I am getting some [org.rpm.dnf.v0.rpm.Rpm.TransactionError] rpm transaction failed with code 6.

Thanks, the [System.Error.ENXIO] Failed to enter a variant (No such device or address) when sdbus-cpp < 2 issue should be resolved.

Still working on coredumps with sdbus-cpp version 2. Let me convert this back to draft until the issues are investigated and resolved.

Exit the event loop and properly join the serving thread.

To address race conditions between the lifetime of the context and callbacks, it is safer to manage them separately. With sdbus-cpp version 2, I observed various failures in dnf5daemon-client, typically following this pattern: - The main thread has already completed its work and is in the process of destructing the Context class. - Meanwhile, the D-Bus event loop thread is still handling messages, and the handler attempts to access the context instance that is currently being destroyed.

Outdated

jonathanspw · 2025-01-31T12:58:15Z

Is this PR ready or should it remain a draft?

m-blaha · 2025-01-31T13:19:53Z

Oh, sorry, I forget to switch it back.

kontura · 2025-02-05T13:55:51Z

I am still getting some crashes with sdbus-cpp > 2.
Specifically it crashes and the CI hangs for me in:

distro-sync.feature: Scenario: updating a signed pkg
gpg-noeol.feature: Scenario: Install signed package from repository
gpg.feature: Scenario Outline: Install <offline> masterkey signed package and check GPG key was imported
gpg.feature: Scenario Outline: Install <offline> subkey signed package with masterkey signed dependency

It seems like some problem with signing.

Plus I am getting some crashes that result in failing tests in:

dnf/clean-cachefiles.feature
dnf/gpg.feature
dnf/install-path.feature
dnf/install-provides.feature
dnf/upgrade-dependent.feature

However since @m-blaha is the maintainer of sdbus-cpp in fedora I am thinking about merging this anyway. Perhaps fixing the crashes will be easier when we don't have to deal with so many patches?

m-blaha · 2025-02-05T13:58:40Z

Hmm, maybe. The crashes (at least those I'm aware of) were all related to the client side of tha dnf5daemon. The server seems to work correctly.

kontura · 2025-02-06T08:31:49Z

The crashes (at least those I'm aware of) were all related to the client side of tha dnf5daemon.

I see, so is the plan to fix those in subsequent PR?

The server seems to work correctly.

My bad, I didn't check why it hangs properly, the server indeed doesn't crash.
For me it gets stuck here: https://github.com/rpm-software-management/dnf5/blob/main/dnf5daemon-server/session.cpp#L193 (presumably it would timed out after 5 min).
As far as I can tell the key_import callback is never called in the client.

m-blaha · 2025-02-06T09:57:42Z

I've already tried to fix them, but apparently the issues are still there. As our testing uses the dnf5daemon-client, we need to fix them before the sdbus-cpp upgrade.

m-blaha · 2025-02-06T10:55:06Z

Created a specialized tracking issue for fixing dnf5daemon-client with sdbus-cpp 2: #2040

Conan-Kudo previously approved these changes Dec 8, 2024

View reviewed changes

kontura self-assigned this Dec 16, 2024

kontura reviewed Dec 19, 2024

View reviewed changes

m-blaha added 5 commits December 19, 2024 12:27

cmake: Move sdbus-c++ check to one place

8f137a9

Also set SDBUS_CPP_VERSION_2 macro in case sdbus-c++ was found in version 2. This enables conditional compilation according the version of the library found.

dnfdaemon: sdbus-cpp v. 2 requires strong types

40c84f5

- Use InterfaceName type instead of char * for interface names - Use SignalName type instead of char * for signal names - Use ServiceName instead of char * for bus names - Use Error::Name type instead of char * for errors

dnfdaemon: sdbus::Variant constructor is explicit

c6cb059

In version 2 the sdbus::Variant constuctor was made explicit.

dnfdaemon: Explicit sdbus::Variant conversion

316abd1

In version 2, conversion of sdbus::Variant to the underlying type is explicit.

dnfdaemon: Make signal handlers compatible

fa191fa

sdbus-cpp version 2 requires signal handler to have signature (sdbus::Signal signal) -> void, which is also acceptable for version 1.

m-blaha force-pushed the mblaha/sdbus-cpp-2 branch from 2f18a16 to 919d5b9 Compare December 20, 2024 07:22

dnfdaemon: Register interface methods for sdbus-cpp-2

14ee083

sdbus-cpp-2 uses different approach to registering methods and signals on the interface.

m-blaha force-pushed the mblaha/sdbus-cpp-2 branch from 919d5b9 to 14ee083 Compare December 20, 2024 10:24

m-blaha marked this pull request as draft December 20, 2024 10:28

m-blaha added 3 commits January 2, 2025 14:38

dnfdaemon-client: Use correct data type for callbacks

8f7af5e

dnfdaemon: Properly leave event loop

7ba21cc

Exit the event loop and properly join the serving thread.

m-blaha force-pushed the mblaha/sdbus-cpp-2 branch from 45934c7 to 80c6e46 Compare January 7, 2025 09:13

Conan-Kudo self-requested a review January 31, 2025 12:47

m-blaha marked this pull request as ready for review January 31, 2025 13:19

Conan-Kudo self-assigned this Feb 5, 2025

kontura added this pull request to the merge queue Feb 6, 2025

Merged via the queue into main with commit 4eb8ef2 Feb 6, 2025
17 checks passed

kontura deleted the mblaha/sdbus-cpp-2 branch February 6, 2025 10:48

m-blaha mentioned this pull request Feb 6, 2025

Fix dnf5daemon-client issues on sdbus-cpp version 2 #2040

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make dnf5 compatible with sdbus-cpp version 2 #1888

Make dnf5 compatible with sdbus-cpp version 2 #1888

m-blaha commented Nov 22, 2024

Conan-Kudo left a comment

kontura left a comment •

edited

Loading

kontura Dec 19, 2024

kontura Dec 19, 2024

m-blaha commented Dec 20, 2024

jonathanspw commented Jan 31, 2025

m-blaha commented Jan 31, 2025

kontura commented Feb 5, 2025

m-blaha commented Feb 5, 2025

kontura commented Feb 6, 2025

m-blaha commented Feb 6, 2025

m-blaha commented Feb 6, 2025

Make dnf5 compatible with sdbus-cpp version 2 #1888

Make dnf5 compatible with sdbus-cpp version 2 #1888

Conversation

m-blaha commented Nov 22, 2024

Conan-Kudo left a comment

Choose a reason for hiding this comment

kontura left a comment • edited Loading

Choose a reason for hiding this comment

kontura Dec 19, 2024

Choose a reason for hiding this comment

kontura Dec 19, 2024

Choose a reason for hiding this comment

m-blaha commented Dec 20, 2024

jonathanspw commented Jan 31, 2025

m-blaha commented Jan 31, 2025

kontura commented Feb 5, 2025

m-blaha commented Feb 5, 2025

kontura commented Feb 6, 2025

m-blaha commented Feb 6, 2025

m-blaha commented Feb 6, 2025

kontura left a comment •

edited

Loading