zbus 4.0 port #155
zbus 4.0 port #155
Conversation
|
I needed explicit type annotations to compile on stable. Unsure if you've had the same issues. It seems the bus is getting timed out... But I can't find the reason immediately. |
|
Hello there, We should of course upgrade to zbus 4 at some point, but now is way too soon for AccessKit. We just bumped our MSRV to 1.68 so I wouldn't want to be stuck on the current version of this crate for a while because of that. |
No problem. Totally understand. I'll make a release right before we merge this so that you have everything we do up to but not including the zbus version upgrade. Hope that is ok? |
|
@TTWNO It all depends on when this will be ready and if I find things to add or fix here in the meantime. I guess we'll all have to agree on how and when to bump our MSRV in sync at some point. |
|
I understand you cannot bump MSVR now, however we do want to move on. For the time being I have been pondering if a types-and-traits crate in the workspace could be something worth exploring. |
Would this just be |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
==========================================
- Coverage 89.97% 88.97% -1.01%
==========================================
Files 39 39
Lines 3173 3228 +55
==========================================
+ Hits 2855 2872 +17
- Misses 318 356 +38 ☔ View full report in Codecov by Sentry. |
That would make things easier if we could.. Let's see:
Then we'd need to carefully look at which types and traits we need to move over still. |
This sounds like more things to keep straight in our heads. Personally, I don't think that's a good idea. |
| @@ -131,7 +103,7 @@ impl Default for EventBodyQT { | |||
| /// Standard event body (GTK, `egui`, etc.) | |||
| /// NOTE: Qt has its own signature: [`EventBodyQT`]. | |||
| /// Signature `(siiva{sv})`, | |||
| #[derive(Clone, Debug, Serialize, Deserialize, Type, PartialEq)] | |||
| #[derive(Debug, Serialize, Deserialize, Type, PartialEq)] | |||
There was a problem hiding this comment.
Why was the Clone derive removed here?
There was a problem hiding this comment.
EventBodyOwned has fields of type OwnedValue.
In zvariant 4, Values no longer have Clone because of the safe Fd work. Cloning is now achieved through Value::try_clone.
I manually re-implemented Clone using OwnedValue::try_clone to not break our API, because:
- Suddenly missing
Cloneis pretty rough. - There is only a remote chance that we will hit the error path in this context, because it requires the current process to exceed the maximum number of open files: (from
Value::try_clonedocs)
Errors
This method can currently only fail on Unix platforms for [Value::Fd](https://docs.rs/zvariant/latest/zvariant/enum.Value.html#variant.Fd) variant containing an [Fd::Owned](https://docs.rs/zvariant/latest/zvariant/enum.Fd.html#variant.Owned) variant. This happens when the current process exceeds the maximum number of open file descriptors.
[edit: updated safe Fd work link.]
| properties: HashMap::new(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Manual implementation to account for the remote chance that either `any_data` or `properties` | ||
| // fail to clone because the clone would exceed the open file limit. | ||
| impl Clone for EventBodyOwned { |
There was a problem hiding this comment.
Oh I see... Makes sense now.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we just let the users of atspi call .try_clone() instead? And let them decide whether or not to fail?
There was a problem hiding this comment.
I decided to 'lust be practical' considering the context:
Did or will EventBodyOwned.any_data ever contain a file?
Did or will EventBodyOwned.properties ever contain a file?
I don't think so and I think the chances are slim they ever will and should they ever, the chances of triggering the failure path are slim.
It is expected that a type has Clone.
| let qt_body = body.deserialize::<EventBodyQT>()?; | ||
| Ok(EventBodyOwned::from(qt_body)) | ||
| } else if signature == ATSPI_EVENT_SIGNATURE { | ||
| Ok(body.deserialize::<EventBodyOwned>()?) |
There was a problem hiding this comment.
Please add a test so that all three cases are covered (QT is currently not).
| let with_parentheses = &Signature::from_static_str_unchecked("((ii)(ii))"); | ||
| let without_parentheses = &Signature::from_static_str_unchecked("((((ii)(ii))))"); | ||
| assert!(!signatures_are_eq(with_parentheses, without_parentheses)); | ||
| } |
There was a problem hiding this comment.
Glad to see this logic upstreamed.
atspi-common/src/events/object.rs
Outdated
| property.clone(), | ||
| value.try_clone().unwrap_or_else(|err| { | ||
| eprintln!("Failed to clone value: {err:?}"); | ||
| u8::default().into() |
There was a problem hiding this comment.
Again, I would argue that we should give the .try_clone() option to users and let them handle errors.
On the other hand, it's not a trait (in std anyways), and it is exceedingly rare that this would actually be triggered.... Hmmmm
Does cloning a file descriptor do anything? Wouldn't the "limit" only be exceeded if we tried to open it?
There was a problem hiding this comment.
AFAIU, having the OwnedFd is having an open file, otherwise cloning these would not be limited?
I fully agree that the chances are slim. Have we ever seen file descriptors being sent?
If we can, I would like to not burden our users with try_clone() but just keep a Clone implementation.
|
@TTWNO Do you need I recently enforced MSRV for AccessKit:
- name: install stable toolchain
uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ needs.find-msrv.outputs.version }}If your CI fail with compile errors, then you know you broke your MSRV policy. It's usually enough since you also have a clippy job able to catch regular compile errors, should there be some. You might lose a bit of efficiency since you won't reuse the cache of the clippy job in the test jobs, not a big deal IMO. |
I think it would be a good idea to have some (even vague) policy about it. Just for the record, I bumped MSRV in zbus to latest of the time only because it was a major version bump. I will be a lot more reluctant to bump it going forward until the next API break. |
|
@DataTriny great idea! I'll try that today and commit it. |
@zeenix Do you have something in mind? There is of course versioning policy: Considering we are <v1.0.0 and |
It depends entirely on the maintainers according to the project's needs and I don't have enough context to suggest something specific. My suggestion was only that there should be policy (even if vague). In zbus, I'd try my best to not bump MSRV to anything newer than 6 months in a non-major release. |
|
@luukvanderduim I'd be curious to know what features you absolutely need from |
You would be using
|
|
Ok, I'm coming around to your point on the However, I see there are a couple places where if you can not
Should these not also panic? I want to have consistent behaviour for library users. Even if that does mean that it can technically panic in more places. |
|
Or, would you argue, like in |
```Rust
loop {
let to = events.try_next().await;
let event = to
.expect("stream timed out")
.expect("stream closed")
.expect("conversion to `Event` failed");
if let Event::Cache(CacheEvents::Remove(event)) = event {
let ObjectRef { name, path } = event.node_removed;
assert_eq!(name.as_str(), ":69.420");
assert_eq!(path.as_str(), "/org/a11y/atspi/accessible/remove");
}
}
```
In the above pattern we could receive our synthetic signal, but the loop would
carry on until the stream would timeout - or until an unrelated `Cache` event
would be received.
A simple `break;` fixes this.
Secondly to prevent unrelated `Cache` events from triggering our event assertions,
we now check whether the event sender name corresponds our connections' unique
name.
Cargo format
Since zvariant 4's changes on `Signature as PartialEq::eq` changes, `assert_eq!` is now sufficient.
|
@DataTriny I've released atspi Any future additions to the crate will be gated behind |
|
Understood @TTWNO, thank you. All of this is very unfortunate... |
I don't see the problem here? accesskit is often 6 months or so behind the latest version anyways; is there a reason you need us to keep compatibility with zbus 3.x for that long? Or is there some other issue I'm not seeing here? |
|
Maybe the README is outdated but given the beta status of this project, I don't really see why bumping MSRV is a big problem here. 🤔 |
|
I just don't want to propose bumping AccessKit's MSRV to 1.75 before our biggest downstream users bring the subject, namely egui (currently on Rust 1.72) and slint (currently on Rust 1.70). |
Then don't. It's ok. Like I said, I often see accesskit being many months behind the latest atspi version. Accesskit's dependence on us has been really mostly the most fundemental types. Those aren't likely to change in the near futute. I see no problem bumping this project, even if accesskit is not ready to. |
On line object.rs:135 We no longer return a default but panic with helpful message. On line object.rs:235 In this conversion, there are only `Strings` and a single `OwnedValue`, which, at the end is just handed over and does not need to be cloned. We should never see that `expect()` message.
- A few items needed backticks. - An unneeded import.
Good work so far, Luuk!
I'm just opening this so we can catalogue some of the issues we've had porting.
( Addresses #154 )