Refactor zbus_macros and zvariant_derive to use common code.

[zeenix]

In GitLab by @turbocooler on Feb 14, 2023, 18:39

I've noticed there are similarities between what zvariant_derive and zbus_macros crates do. In particular both crates:

1. Parse structure/enum/field attributes. zvariant_derive was recently refactored to use cleaner code in !639, but zbus_macros has been left without any attention.
2. Convert cases. Case conversion code for zvariant_derive was copied with little changes from zbus_macros.

I suggest adding a new crate that contains utilities used by both crates.

Benefits:

1. Cleaner code that is easier to understand in both crates.
2. Less code duplication and faster compile times.
3. Easier changes to supported attributes in zbus_macros.

Drawbacks:

1. A new dependency for these crates. This dependency is fully controlled though so I don't think this is an issue.

Questions:

• What will the name of the crate be?
• Are there any requirements on the architecture for such a crate?

I can submit a MR in case you like the idea.

[zeenix]

Hmm.. Given we already have quite a few subcrates, I would rather avoid creating yet another one. Can't zvariant_derive expose the needed utils API and zbus_macros can depend on it? In practice, this won't be a new dep ad zbus already depends on zvariant_derive.

[zeenix]

In GitLab by @turbocooler on Feb 14, 2023, 22:59

Makes sense. I'll make a MR with this change then. Won't exporting utility macros from the root of zvariant_derive be a concern?

[zeenix]

Makes sense. I'll make a MR with this change then.

Awesome!

Won't exporting utility macros from the root of zvariant_derive be a concern?

Yeah, best we export in mod, maybe utils should be exposed? Also I think it should be marked #[doc(hidden)] so it's not considered pub API exactly and hence we don't need to care about breaking changes.

[zeenix]

In GitLab by @turbocooler on Feb 15, 2023, 13:23

I've tried experimenting with it and turns out declarative macros can not be exported from proc macro crates. Here's the error output from rustc when adding #[macro_export] to the def_attrs! macro from zvariant_derive.

error: cannot export macro_rules! macros from a `proc-macro` crate type currently

I don't think generating these structures with attributes using proc macros makes sense.

[zeenix]

I don't think generating these structures with attributes using proc macros makes sense.

Right. I don't then know what could be the best solution. We face a similar issue as !486. However, tbh I'm not super bothered by a bit of code duplication between the macro crates. It's not ideal for sure but I wouldn't want to create another crate only for sharing the utils code.

[zeenix]

In GitLab by @turbocooler on Feb 16, 2023, 20:11

Is there a reason you wouldn't like to add another crate? I feel like these two situations are good enough reasons since there is currently no other way to do this in Rust.

There are a couple of examples of major projects creating crates used solely to contain some common utilities for other crates:

• proc-macro-api from the Rust Analyzer project;
• serde_derive_internals from the Serde project.

In my personal projects consisting of multiple crates I usually split off modules that are independent from the other code in their crate into separate crates. That sometimes leads to them becoming standalone libraries like with my latest mapgraph crate.

The signature parser implementation and macro utilities technically could be grouped into the same crate since these are all utilities used by zbus and zvariant.

[zeenix]

Is there a reason you wouldn't like to add another crate?

Well, mostly greater cognitive load and harder to maintain. I am not sure how long you intend to stick around but often people add new code and disappear at some point and everything falls on my shoulders. :( Then there are people who are concerned with number of dependencies only.

Hence my reluctance for having more crates.

There are a couple of examples of major projects creating crates used solely to contain some common utilities for other crates:

I'm aware of the practice and I'm sure there are a lot of projects out there doing this. The important factor for me here are the benefits.

The signature parser implementation and macro utilities technically could be grouped into the same crate since these are all utilities used by zbus and zvariant.

That would be very convincing argument for me. :) Let's call it zvariant_marcos_utils?

[zeenix]

In GitLab by @turbocooler on Feb 17, 2023, 15:41

Well, mostly greater cognitive load and harder to maintain.

The reason behind the attribute-related changes I recently submitted to zvariant_derive and this issue was mostly to reduce cognitive load when reading code of the macro crates. This is just something I struggled with while using these. For me understanding what exactly the macros do is crucial to using them correctly. I believe that moving attribute parsing logic into a separate module/crate makes the code that does the actual code generation more visible and easier to work with.

Then there are people who are concerned with number of dependencies only.

I see, this is a valid concern. The new crate would exist in the same zbus git repo and have the same publisher though, so from the point of view of a reviewer this is effectively a piece of code moved into a different directory. The new approach with structures also simplifies incremental review.

I am not sure how long you intend to stick around but often people add new code and disappear at some point and everything falls on my shoulders.

Initially I wanted to close this issue in bluer, but seems like I will provide an alternative crate in the end since there are just too many changes and the code just ended up being mostly rewritten. I'm planning to contribute some features that I think will make using zbus more convenient at least until I finish that project. Then I'll be happy to maintain this small part if you don't mind some help with this.

That would be very convincing argument for me. :) Let's call it zvariant_marcos_utils?

If you're not joking, I like the name. :) It doesn't reflect that the crate will be used as the default signature parser though so maybe something like zvariant_utils?

[zeenix]

believe that moving attribute parsing logic into a separate module/crate makes the code that does the actual code generation more visible and easier to work with.

When you are working on that code, yes. As a maintainer I've to deal with many other factors too that you (or most contributors) will probably not even know of. E.g releasing and stuff that affects every crate. For releasing, I've to do at least 3 things manually.

Having said that, I guess the lesson here is to automate releasing as much as possible. I recently automated a part of release notes creation at least but it could be done better I'm sure.

Then there are people who are concerned with number of dependencies only.

I see, this is a valid concern. The new crate would exist in the same zbus git repo and have the same publisher though, so from the point of view of a reviewer this is effectively a piece of code moved into a different directory. The new approach with structures also simplifies incremental review.

Those are very good points but if people were reasonable, they wouldn't care if the crate is external or not. What we could do still is make this very clear, not only by the name but also in the README/crate description.

I am not sure how long you intend to stick around but often people add new code and disappear at some point and everything falls on my shoulders.

Initially I wanted to close this issue in bluer

Nice!

But seems like I will provide an alternative crate in the end since there are just too many changes and the code just ended up being mostly rewritten.

You mean alternative to zbus or bluer? The only really viable and (sort of) maintained alternative to zbus is dbus-rs. I would strongly advice not to use any other alternative (unless you found/created a new one that I've not seen yet).

I'm planning to contribute some features that I think will make using zbus more convenient at least until I finish that project. Then I'll be happy to maintain this small part if you don't mind some help with this.

That would be great, thanks!

That would be very convincing argument for me. :) Let's call it zvariant_marcos_utils?

If you're not joking, I like the name. :smile:

I'm not. :) I meant to write zvariant_derive_utils though.

It doesn't reflect that the crate will be used as the default signature parser though so maybe something like zvariant_utils?

I don't think it has to reflect that since it's mainly there for us. The more pressing problem is that ideally we'd just want it to provide the signature proc macro itself but then it can not prov provide any other type of API than proc macro and we need to do that.

So we've two options:

1. 2 additional crates, zvariant_utils and zvariant_utils_macros?
2. Only zvariant_utils, which provides the signature checking function (this can be even #[doc(hidden)]). put the signature proc macro in zvariant_derive.

Although the _derive suffix isn't great for non-derive macros, we need to keep in mind that people will use this API through the re-export in zvariant anyway.

Note that in !486, things got very hairy because I was hoping to have also macros for zbus_names in the same crate. I now think that should go in its own separate crate (zbus_names_macros the zbus_names depend on and re-exports).

[zeenix]

In GitLab by @turbocooler on Feb 17, 2023, 18:37

You mean alternative to zbus or bluer? The only really viable and (sort of) maintained alternative to zbus is dbus-rs. I would strongly advice not to use any other alternative (unless you found/created a new one that I've not seen yet).

No-no, not zbus :smile:. zbus is actually very good and that's the reason a lot of code that was meant to make dbus-rs work can be removed.

I don't think it has to reflect that since it's mainly there for us.

Makes sense.

Only zvariant_utils, which provides the signature checking function (this can be even #[doc(hidden)]). put the signature proc macro in zvariant_derive.

I think this option is better. I don't see why the signature macro can't go into zvariant_derive.

Although the _derive suffix isn't great for non-derive macros, we need to keep in mind that people will use this API through the re-export in zvariant anyway.

You could rename it to zvariant_macros. IIRC zbus_macros was named zbus_derive previously. This may be a breaking change though.

I now think that should go in its own separate crate (zbus_names_macros the zbus_names depend on and re-exports)

Not sure about that, I didn't reach this part of zbus yet :).

Since everything seems to be settled, is it OK if I submit a MR?

[zeenix]

You mean alternative to zbus or bluer? The only really viable and (sort of) maintained alternative to zbus is dbus-rs. I would strongly advice not to use any other alternative (unless you found/created a new one that I've not seen yet).

No-no, not zbus :smile:. zbus is actually very good and that's the reason a lot of code that was meant to make dbus-rs work can be removed.

Good to hear. :smile:

Only zvariant_utils, which provides the signature checking function (this can be even #[doc(hidden)]). put the signature proc macro in zvariant_derive.

I think this option is better. I don't see why the signature macro can't go into zvariant_derive.

Glad we agree.

Although the _derive suffix isn't great for non-derive macros, we need to keep in mind that people will use this API through the re-export in zvariant anyway.

You could rename it to zvariant_macros. IIRC zbus_macros was named zbus_derive previously. This may be a breaking change though.

That's why we can't do it. :smile: Maybe in the future.

Since everything seems to be settled, is it OK if I submit a MR?

:thumbsup: