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

New release broke gvariant feature #1125

Closed
bilelmoussaoui opened this issue Nov 4, 2024 · 9 comments · Fixed by #1143
Closed

New release broke gvariant feature #1125

bilelmoussaoui opened this issue Nov 4, 2024 · 9 comments · Fixed by #1143

Comments

@bilelmoussaoui
Copy link
Contributor

If you build zbus from git using cargo build --features gvariant it would fail with the following

user@toolbox ~/P/zbus (main)> cargo build --features gvariant
    Blocking waiting for file lock on build directory
   Compiling zvariant v5.1.0 (/home/user/Projects/zbus/zvariant)
error[E0004]: non-exhaustive patterns: `Format::GVariant` not covered
   --> zvariant/src/serialized/data.rs:234:28
    |
234 |         let mut de = match self.context.format() {
    |                            ^^^^^^^^^^^^^^^^^^^^^ pattern `Format::GVariant` not covered
    |
note: `Format` defined here
   --> /home/user/Projects/zbus/zvariant_utils/src/serialized.rs:5:1
    |
5   | pub enum Format {
    | ^^^^^^^^^^^^^^^
...
12  |     GVariant,
    |     -------- not covered
    = note: the matched value is of type `Format`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
267 ~             .map(Deserializer::DBus)?,
268 ~             Format::GVariant => todo!(),
    |

error[E0004]: non-exhaustive patterns: `Format::GVariant` not covered
   --> zvariant/src/serialized/data.rs:307:28
    |
307 |         let mut de = match self.context.format() {
    |                            ^^^^^^^^^^^^^^^^^^^^^ pattern `Format::GVariant` not covered
    |
note: `Format` defined here
   --> /home/user/Projects/zbus/zvariant_utils/src/serialized.rs:5:1
    |
5   | pub enum Format {
    | ^^^^^^^^^^^^^^^
...
12  |     GVariant,
    |     -------- not covered
    = note: the matched value is of type `Format`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
340 ~             .map(Deserializer::DBus)?,
341 ~             Format::GVariant => todo!(),
    |

error[E0004]: non-exhaustive patterns: `Format::GVariant` not covered
  --> zvariant/src/ser.rs:58:21
   |
58 |     let len = match ctxt.format() {
   |                     ^^^^^^^^^^^^^ pattern `Format::GVariant` not covered
   |
note: `Format` defined here
  --> /home/user/Projects/zbus/zvariant_utils/src/serialized.rs:5:1
   |
5  | pub enum Format {
   | ^^^^^^^^^^^^^^^
...
12 |     GVariant,
   |     -------- not covered
   = note: the matched value is of type `Format`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
   |
69 ~         },
70 +         Format::GVariant => todo!()
   |

error[E0004]: non-exhaustive patterns: `Format::GVariant` not covered
   --> zvariant/src/ser.rs:172:21
    |
172 |     let len = match ctxt.format() {
    |                     ^^^^^^^^^^^^^ pattern `Format::GVariant` not covered
    |
note: `Format` defined here
   --> /home/user/Projects/zbus/zvariant_utils/src/serialized.rs:5:1
    |
5   | pub enum Format {
    | ^^^^^^^^^^^^^^^
...
12  |     GVariant,
    |     -------- not covered
    = note: the matched value is of type `Format`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
183 ~         },
184 +         Format::GVariant => todo!()
    |

error[E0004]: non-exhaustive patterns: `&zvariant_utils::signature::Signature::Maybe(_)` not covered
   --> zvariant/src/de.rs:198:11
    |
198 |     match signature {
    |           ^^^^^^^^^ pattern `&zvariant_utils::signature::Signature::Maybe(_)` not covered
    |
note: `zvariant_utils::signature::Signature` defined here
   --> /home/user/Projects/zbus/zvariant_utils/src/signature/mod.rs:42:1
    |
42  | pub enum Signature {
    | ^^^^^^^^^^^^^^^^^^
...
99  |     Maybe(Child),
    |     ----- not covered
    = note: the matched value is of type `&zvariant_utils::signature::Signature`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
217 ~         Signature::Structure { .. } => de.deserialize_seq(visitor),
218 ~         &zvariant_utils::signature::Signature::Maybe(_) => todo!(),
    |

It is something I have noticed from the CI bump of zbus at bilelmoussaoui/oo7#154.

Note this happens even if I have

zvariant = { version = "5.1", default-features = false, features = ["gvariant"]}
@bilelmoussaoui
Copy link
Contributor Author

The error doesn't happen on zbus-5.0.1 tag, so very likely a change since then caused it. I haven't investigated more yet

@zeenix
Copy link
Contributor

zeenix commented Nov 5, 2024

If you build zbus from git using cargo build --features gvariant

Yeah, I saw that but it's some weird cargo dependency feature management mess that I couldn't figure out but I did not realize that affects our users so didn't care much and worked around it (72ccc73).

Interestingly, cargo build --tests --features gvariant works just fine. 🤔

This hack makes cargo build --features gvariant work though:

diff --git a/zbus_macros/Cargo.toml b/zbus_macros/Cargo.toml
index 1f35d35d..c0a4a302 100644
--- a/zbus_macros/Cargo.toml
+++ b/zbus_macros/Cargo.toml
@@ -20,6 +20,7 @@ readme = "README.md"
 default = []
 # Enable blocking API.
 blocking-api = ["zbus/blocking-api"]
+gvariant = ["zvariant/gvariant", "zvariant_utils/gvariant"]
 
 [lib]
 proc-macro = true

I don't see any harm in that so if you could check if that makes things work for you, I'll commit it and hope for the best.


P.S. This whole gvariant support is getting too much to handle for me, especially mixed with zbus etc. I hope to someday split it into a separate crate since I've long abandoned the plans of using gvariant as a dbus2 protocol (which was its original intention).

@zeenix
Copy link
Contributor

zeenix commented Nov 13, 2024

P.S. This whole gvariant support is getting too much to handle for me, especially mixed with zbus etc. I hope to someday split it into a separate crate since I've long abandoned the plans of using gvariant as a dbus2 protocol (which was its original intention).

While thinking about it, I had another look at gvariant crate and realized that not only it is used by ostree folks, it actually lives in the ostree space and seems to be the officially blessed implementation.

So I'm wondering to just deprecate the gvariant support in favour of that crate.

@bilelmoussaoui
Copy link
Contributor Author

Thanks for the fix! Sorry for not reporting earlier, been quite busy with other stuff.

While thinking about it, I had another look at gvariant crate and realized that not only it is used by ostree folks, it actually lives in the ostree space and seems to be the officially blessed implementation.

I will investigate the crate and see if it can work out and eventually switch to using it.

@zeenix
Copy link
Contributor

zeenix commented Nov 13, 2024

I will investigate the crate and see if it can work out and eventually switch to using it.

Cool, thanks for your understanding. Rest assured, even if I do go ahead with dropping the support, it'll only be in zvariant 6.0 and that's not happening for at least some months (probably more). So you've plenty of time and you'll get deprecation warning long before then.

@bilelmoussaoui
Copy link
Contributor Author

@zeenix would it be possible to include the fix for this in the upcoming release please?:)

@zeenix
Copy link
Contributor

zeenix commented Dec 17, 2024

@zeenix would it be possible to include the fix for this in the upcoming release please?:

I released yesterday exactly for this but soon after the release, Maximiliano told me that the fix doesn't work.

I had assumed from our conversation here that you tested the fix. If you could rejoin the matrix channel, it'd help avoid these misunderstandings. 😉

Having said that, I really am not sure how to fix the issue. Perhaps re-addition of gvariant feature to the zbus crate is the only way here. I really don't want to do that but if that's the only way, I'd be willing to do that.

Maximiliano said he'll dig deeper so I'm now waiting on him.

@bilelmoussaoui
Copy link
Contributor Author

@zeenix would it be possible to include the fix for this in the upcoming release please?:

I released yesterday exactly for this but soon after the release, Maximiliano told me that the fix doesn't work.

I had assumed from our conversation here that you tested the fix. If you could rejoin the matrix channel, it'd help avoid these misunderstandings. 😉

Having said that, I really am not sure how to fix the issue. Perhaps re-addition of gvariant feature to the zbus crate is the only way here. I really don't want to do that but if that's the only way, I'd be willing to do that.

Maximiliano said he'll dig deeper so I'm now waiting on him.

Interesting because I tested the changes locally and they worked. I will investigate further later :)

@zeenix
Copy link
Contributor

zeenix commented Dec 17, 2024

Interesting because I tested the changes locally and they worked. I will investigate further later :)

Oh that is indeed interesting. I assume you tested against oo7 and not just the zbus repo itself (the latter was indeed "fixed" by #1143).

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 a pull request may close this issue.

2 participants