storaged-project / udisks

The UDisks project provides a daemon, tools and libraries to access and manipulate disks, storage devices and technologies.
https://storaged.org/doc/udisks2-api/latest/
Other
349 stars 142 forks source link

Rust client library #1183

Open pothos opened 1 year ago

pothos commented 1 year ago

Hey, in the past there were some attempts of maintaining a client library in Rust but they are not active anymore: https://github.com/csnewman/udisks-rs https://github.com/pop-os/dbus-udisks2

The C client library consists of both generated D-Bus code and the hand-written helper functions for working with the D-Bus objects or converting IDs to human readable strings.

There are two approaches possible, one is to generate Rust code from the D-Bus interface with zbus and write the helper functions by hand, one is to generate foreign language bindings to the C library with GIR. Here is the second approach: https://github.com/FineFindus/udisks-rs/ However, it is not so nice because the code is not ergonomic. The zbus generation would be nicer.

Anyway, the result needs to be maintained somewhere and crates published at the same time as a new UDisks release happens.

Do you already have an approach for a supported Rust client library? Are you interested in exploring how to make a nice one? Are you also interested in it living in the main repo?

Background is that some standalone parts of GNOME Disks are planned to be implemented in Rust, @FineFindus is looking into that.

tbzatek commented 1 year ago

We haven't really thought about Rust bindings. Nobody from the core team has any experience with Rust and we're also rather short on manpower these days, so this would need to be a community contribution with at least some degree of ownership/maintainership until we embrace it fully. I like the idea of having Rust bindings though!

A question of API guarantees comes in mind. For the start we may mark the Rust bindings and unstable until it proves to be working and adopted by other projects. Once we pronounce it as stable, there's no way out. I don't expect UDisks3 anytime near and there's really no reason for that. We generally try to keep the API stable and rock solid, in line with glib2 API stability guarantees.

Considered from the other side - now that three incompatible bindings exist, it's better to officially recommend one until they all get broader adoption and it's too late to change anything.

Here is the second approach: https://github.com/FineFindus/udisks-rs/

I like this approach the best from a quick stroll through the sources. The more generated code (from gobject-introspection) the better, the lesser the maintenance burden. The number of imported crates is quite scary but that's the Rust way.

I haven't really followed Gnome for a while and have no idea what is the recommended way of creating bindings for foreign projects. I trust you to make the right decision!

vojtechtrefny commented 1 year ago

and the hand-written helper functions for working with the D-Bus objects or converting IDs to human readable strings

It might be also a good idea to check these helper functions and possibly add them to the DBus API so any future bindings could get the functionality for free.

tbzatek commented 1 year ago

and the hand-written helper functions for working with the D-Bus objects or converting IDs to human readable strings

It might be also a good idea to check these helper functions and possibly add them to the DBus API so any future bindings could get the functionality for free.

Most of them are constant translations and you don't really want to do any I/O just for that ;-)

FineFindus commented 1 year ago

I haven't really followed Gnome for a while and have no idea what is the recommended way of creating bindings for foreign projects. I trust you to make the right decision!

One of the [gtk-rs] (https://gtk-rs.org/) maintainers recommended using D-Bus over GIR-based generation. In addition, using GIR-based also requires a certain amount of handwritten Rust code. I'm not sure exactly how much code is required, as I've only generated the bindings so far, but haven't used them yet. Since handwritten code is required in any case, I would prefer to use a D-Bus generated API, as it leads to more idiomatic Rust code.

How much work is involved in writing and maintaining the non-generated code? I'm happy to help port it from C and possibly maintain it for a while.

tbzatek commented 1 year ago

One of the [gtk-rs] (https://gtk-rs.org/) maintainers recommended using D-Bus over GIR-based generation.

Sounds good! Feel free to submit any annotations that are missing for the bindings.

How much work is involved in writing and maintaining the non-generated code? I'm happy to help port it from C and possibly maintain it for a while.

I'm not sure if there's a need for 1:1 translation of the libudisks2 API, at least not from the beginning. These object lookup convenient functions may become handy, there's a question of how objects are represented in Rust however. Might be easier to reimplement these in Rust rather than wrapping around the C-code.

FineFindus commented 1 year ago

Sorry for the long silence. I did a little side quest to add support for generating interfaces in different files in zbus, for easier maintenance of the different interface files. I've moved the previous GIR-generated code to a new branch, and started working on a new, zbus-based implementation. The generated code is already available, right now I'm working on porting the client to Rust.

I'm not sure if there's a need for 1:1 translation of the libudisks2 API, at least not from the beginning

I don't think so, especially since the generated code already differs from the C code (e.g. using idiomatic Rust function names).

I will try to include some more status updates and questions when they come up :)

FineFindus commented 9 months ago

Sorry again for the long silence, the Client is now functionally fully ported to Rust🎉. This means that most functions have been ported to C. I excluded some that did not make sense in a Rust context. Of course, the work is far from finished, the code is often non-idiomatic Rust and lacks the usual type safety expected in Rust. An example would be the RotationRate property, which has three different states represented by a single integer. In idiomatic Rust, this would be expressed as an enum instead. One problem is the reliance on GIcon for icon support in e.g. udisks_object_info_get_icon. While a Rust crate for gio exists, it feels a bit wasteful to have it as an additional dependency for a single type. Another open issue is that gettext support is not currently implemented. Ideally, it would be possible to reuse the existing translation, though I'm not entirely sure if and how this is possible, since the Rust version has different string placeholders as well as different line numbers.

I also have a few questions about the C code, it would be great if you could answer them:

tbzatek commented 8 months ago

Sorry again for the long silence, the Client is now functionally fully ported to Rust🎉.

Very nice!

One problem is the reliance on GIcon for icon support in e.g. udisks_object_info_get_icon. While a Rust crate for gio exists, it feels a bit wasteful to have it as an additional dependency for a single type.

There are many more libraries that return GIcon at some point. Try looking at their Rust bindings how is this done. I.e. the bindings should be consistent with the rest of the platform (Gnome).

Another open issue is that gettext support is not currently implemented. Ideally, it would be possible to reuse the existing translation, though I'm not entirely sure if and how this is possible, since the Rust version has different string placeholders as well as different line numbers.

Hmm, the library will return strings according to your process active locale. Is there something extra to be translated with Rust bindings?

udisks_client_get_partition_type_infos returns a list of UDisksPartitionTypeInfo, which seems to be the same as the struct for known_partition_types, except that it doesn't expose the name property. I'm not sure what the reason for this is. I'm currently using the same type for both and have set the name to private.

The name field here is a display name - a string that should not be used as a key. It's common to separate that as in many cases clients don't need them. See related udisks_client_get_partition_type_for_display().

The documentation for partition_table_type and partition_table_subtype in udisks_client_get_partition_table_subtype_for_display is identical. Is that intentional?

Ha, you've found a bug!

In udisks_client_get_object_info_for_drive, DRIVE_TYPE_UNSET is marked as g_assert_not_reached. Is this because the media_data doesn't contain it, and it is used as a sentinel value?

A common practice to guard against programmer errors. Theoretically should never happen unless something goes really wrong. You may want to raise an exception instead.

FineFindus commented 5 months ago

Hmm, the library will return strings according to your process active locale. Is there something extra to be translated with Rust bindings?

Not necessarily extra, the gettext bindings for Rust aren't really mature yet, so things like string interpolation aren't well-supported. Also, I'm not sure if gettext is able to detect that the same string now exists in two different files/languages, or if any additional configuration is required.

A common practice to guard against programmer errors. Theoretically should never happen unless something goes really wrong. You may want to raise an exception instead.

Ah, okay. I've replaced it with None (Rust NULL equivalent), which is more appropriate in a Rust context.

A few new questions I've had so far:

Many items contain the phrase "known", e.g. the Drive.Configuration property. Does "known" mean that the listed values are all possible values, or are there more possible unknown values?

Can / am I allowed to include the documentation in the Rust code (as doc comments above the functions, just like in the C version)? Since the Rust version has the same license, I think it should be okay from a legal standpoint, though I'm not a lawyer.

tbzatek commented 5 months ago

Hmm, the library will return strings according to your process active locale. Is there something extra to be translated with Rust bindings?

Not necessarily extra, the gettext bindings for Rust aren't really mature yet, so things like string interpolation aren't well-supported. Also, I'm not sure if gettext is able to detect that the same string now exists in two different files/languages, or if any additional configuration is required.

Hmm, I wouldn't worry too much about that for the first public version. The point is, the C library should give you already processed/translated strings. If you're talking about translating string constants from the header files, then we should reevaluate the C API perhaps.

A few new questions I've had so far:

Many items contain the phrase "known", e.g. the Drive.Configuration property. Does "known" mean that the listed values are all possible values, or are there more possible unknown values?

The term known should be treated as supported values for the given version the documentation was generated. If the docs or the bindings were written for certain UDisks API version, and the installed daemon or library is a newer version, there might be possibly some added values. However, we're strict in API stability and never remove any symbols, arguments or constants.

So known values may be added in the future but never removed.

Can / am I allowed to include the documentation in the Rust code (as doc comments above the functions, just like in the C version)? Since the Rust version has the same license, I think it should be okay from a legal standpoint, though I'm not a lawyer.

Of course, everything is covered by the same license. Keep in mind you'd have to somehow synchronize every change or a new addition (or perhaps some bindings generator or a script should automate this).

FineFindus commented 5 months ago

The translations are now implemented and work (and are compatible with the translations from the C version). Two quick questions: As far as I know, libraries that use gettext need to call bindtextdomain, but I couldn't find any code to do this. Is it just set by a compile flag? I assume the distribution of translations is the responsibility of the distros, so I don't have to do anything in that regard?

Keep in mind you'd have to somehow synchronize every change or a new addition (or perhaps some bindings generator or a script should automate this).

That's a good point, I hadn't thought of it that way. I'm not quite sure what the best solution is yet, ideally the docs would be idiomatic Rustdocs, which can be quite different. Also, I would like to convert more types to native Rust types (e.g. using an enum for bitflag ints instead of a single int) that a script would have to deal with.

I don't think I've said this before, but thanks for your help, I appreciate it!

FineFindus commented 4 weeks ago

Gentle ping for the questions in the previous comment.

Since the last update, we have merged the first Rust code into GNOME Disks. :tada:

I'm a bit confused about how Udisks.Job works. The D-Bus interface doesn't support setting properties, so I'm not quite sure how they are set/how it's possible to do that from the Rust side.

tbzatek commented 2 weeks ago

Sorry, this slipped through my fingers...

Since the last update, we have merged the first Rust code into GNOME Disks. 🎉

Congratulations!

As far as I know, libraries that use gettext need to call bindtextdomain, but I couldn't find any code to do this. Is it just set by a compile flag?

Hmm, good point. I have no idea :-) This is described in https://docs.gtk.org/glib/i18n.html for example. However, I haven't found any such traces in UDisks. Only udisksctl calls setlocale (LC_ALL, ""). My assumption is that for libudisks the burden is on the process using the library. The udisks code is compiled with #define GETTEXT_PACKAGE "udisks2" which I expect is used by some <glib/gi18n.h> macros.

As udisksd is a system daemon, it runs with system locale, no matter what locale is used on the other/user end. Messages are transferred as strings through D-Bus. Polkit might employ another mechanism, e.g. I've noticed setting a polkit.gettext_domain property when asking for authorization.

I assume the distribution of translations is the responsibility of the distros, so I don't have to do anything in that regard?

I don't see any further steps necessary here.

I'm a bit confused about how Udisks.Job works. The D-Bus interface doesn't support setting properties, so I'm not quite sure how they are set/how it's possible to do that from the Rust side.

The semantics with jobs is that the caller starts an operation via a method call and exposes a new job object tracking progress that stays on the bus for the whole method call run. There's generally no properties to set from user side, other than calling Cancel() or such, the object is ready with all information provided by the method call. Users that don't need to track progress or interact with cancellation may safely ignore job objects.

FineFindus commented 3 days ago

No problem and thanks :)

It also seems to work without any initialization, so it seems like there isn't a problem (just wanted to make sure, if there isn't anything I'd miss).

The semantics with jobs is that the caller starts an operation via a method call and exposes a new job object tracking progress that stays on the bus for the whole method call run. There's generally no properties to set from user side, other than calling Cancel() or such, the object is ready with all information provided by the method call. Users that don't need to track progress or interact with cancellation may safely ignore job objects.

The method call is what I'm confused about. I'm assuming it is done in the client lib and the job is then 'passed' to the server? With setting properties I meant calls like udisks_job_set_rate or udisks_job_set_bytes, which are required in GNOME Disks (currently these are the main blockers left for two new dialogs to be ported :)).