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

Release/v2.0 #364

Merged
merged 52 commits into from
Apr 24, 2024
Merged

Release/v2.0 #364

merged 52 commits into from
Apr 24, 2024

Conversation

sangelovic
Copy link
Collaborator

This is an ongoing, WIP, draft MR. Once all the work on v2 API is finished, the master branch will get branched off to the release/v1.x branch and the release/v2.0 branch will get merged to master. v2 will become the default development version.

Stanislav Angelovic and others added 13 commits April 24, 2024 20:13
This will also install the library as `libsdbus-c++.so.2.0.0`, which
represents the actual version of this library
Signatures of callbacks async_reply_handler, signal_handler, message_handler and property_set_callback were modified to take input message objects by value, as opposed to non-const ref.

The callee assumes ownership of the message. This API is more idiomatic, more expressive, cleaner and safer. Move semantics is used to pass messages to the callback handlers. In some cases, this also improves performance.
Since synchronous D-Bus calls are simplified in v2.0 and no more implemented in terms of an async call, the issue reported in #362 is no more relevant, and we can return to the simple boolean flag for indicating a finished call.
The test now works with Clang, libc++ and -O2 optimization, since the underlying implementation has been completely re-designed and doesn't suffer from that problem anymore.
This makes the library more robust and prone to user's errors when the user writes an extension for their custom type. In case they forget to implement a serialization function for that type and yet insert an object of that type into sdbus::Message, the current behavior is that, surprisingly, the library masks the error as it resolves the call to the Variant overload, because Variant provides an implicit template converting constructor, so the library tries to construct first the Variant object from the object of custom type, and then inserting into the message that Variant object. Variant constructor serializes the underlying object into its internal message object, which resolves to the same message insertion overload, creating an infinite recursion and ultimately the stack overflow. This is undesired and plain wrong. Marking this Variant converting constructor solves these problems, plus in overall it makes the code a little safer and more verbose. With explicit Variant constructor, when the user forgets to implement a serialization function for their type, the call of such function will fail with an expressive compilation error, and will produce no undesired, surprising results.
In sdbus-c++ v1, new virtual functions (e.g. overloads of existing virtual functions) were always placed at the end of the class to keep backwards ABI compatibility. Now, with v2, these functions are reordered and functions forming a logical group are together.
This PR makes things around connection factories a little more consistent and more intuitive:

    * createConnection() has been removed. One shall call more expressive createSystemConnection() instead to get a connection to the system bus.
    * createDefaultBusConnection() has been renamed to createBusConnection(), so as not to be confused with libsystemd's default_bus, which is a different thing (a reusable thread-local bus).

Proxies still by default call createBusConnection() to get a connection when the connection is not provided explicitly by the caller, but now createBusConnection() does a different thing, so now the proxies connect to either session bus or system bus depending on the context (as opposed to always to system bus like before).

The integration tests were modified to use createBusConnection().
A new CMake configuration variable SDBUSCPP_SDBUS_LIB has been introduced that enables clients to specify which sd-bus implementation library shall be used by sdbus-c++.
As of now sdbus-c++ supports C++20 but does not require it, and the used C++20 features are conditionally compiled depending on whether they are available or not.
This introduces strong types for `std::string`-based D-Bus types. This facilitates safer, less error-prone and more expressive API.

What previously was `auto proxy = createProxy("org.sdbuscpp.concatenator", "/org/sdbuscpp/concatenator");` is now written like `auto proxy = createProxy(ServiceName{"org.sdbuscpp.concatenator"}, ObjectPath{"/org/sdbuscpp/concatenator"});`.

These types are:
  * `ObjectPath` type for the object path (the type has been around already but now is also used consistently in sdbus-c++ API for object path strings),
  * `InterfaceName` type for D-Bus interface names,
  * `BusName` (and its aliases `ServiceName` and `ConnectionName`) type for bus/service/connection names,
  * `MemberName` (and its aliases `MethodName`, `SignalName` and `PropertyName`) type for D-Bus method, signal and property names,
  * `Signature` type for the D-Bus signature (the type has been around already but now is also used consistently in sdbus-c++ API for signature strings),
  * `Error::Name` type for D-Bus error names.
Having explicit conversion operator is a good practice according to the C++ core guidelines, as it makes the code safer and better follows the principle of least astonishment. Also, it is consistent with the standard library style, where wrappers like std::variant, std::any, std::optional... also do not provide an implicit conversion to the underlying type. Last but not least, it paves the way for the upcoming std::variant <-> sdbus::Variant implicit conversions without surprising behavior in some edge cases.
Since sdbus-c++ knows types of D-Bus call/signal/property input/output parameters at compile time, why not assemble the D-Bus signature strings at compile time, too, instead of assembling them at run time as std::string with likely cost of (unnecessary) heap allocations...

std::array is well supported constexpr type in C++20, so we harness it to build the D-Bus signature string and store it into the binary at compile time.
Moving adaptor or proxy instances changes their `this` pointer. But `this` is captured by value in closures used by those instances, and this remains unchanged on move, leading to accessing an invalid instance when a lambda expression executes. Supporting move semantics would require unsubscribing/unregistering vtable, handlers, etc. and re-subscribing and re-registering all that, which is too complicated and may have side effects. Hence it has been decided that these classes are not moveable. One may use an indirection with e.g. `std::unique_ptr` to get move semantics.
This makes the signature of addObjectManager() function overloads consistent with common API design of the library.
)

Make all the API consistent by using return_slot_t-based overloads for returning the slots to clients, and overloads without that tag for "floating" slots. floating_slot_t tag was previously added for API backwards compatibility reasons, but now is a (counter-)duplicate to the return_slot_t.
@sangelovic sangelovic marked this pull request as ready for review April 24, 2024 18:19
@sangelovic sangelovic merged commit 798eaf8 into master Apr 24, 2024
6 checks passed
@sangelovic sangelovic deleted the release/v2.0 branch April 24, 2024 18:20
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.

2 participants