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

feat: remove ashpd use zbus directly #66

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Decodetalkers
Copy link

@Decodetalkers Decodetalkers commented Mar 9, 2025

zbus use tokio and async-std together, this cause some buggy thing, for example, in this project, it try to use async-std to run a async flow in blocking flow, but if I induced another zbus, and enable the tokio feature, then it runs with tokio feature, this cause the whole thread panic. So if you want to use blocking function with zbus, I think it is better to use the blocking feature of zbus instead, not with such buggy way

influence: iced-rs/iced#2833

zbus use tokio and async-std together, this cause some buggy thing, for
example, in this project, it try to use async-std to run a async flow in
blocking flow, but if I induced another zbus, and enable the tokio
feature, then it runs with tokio feature, this cause the whole thread
panic. So if you want to use blocking function with zbus, I think it is
better to use the blocking feature of zbus instead, not with such buggy
way
@Be-ing
Copy link
Collaborator

Be-ing commented Mar 9, 2025

How about instead of removing ashpd we expose ashpd's features to select between async-std and tokio?

@Decodetalkers
Copy link
Author

Decodetalkers commented Mar 9, 2025

How about instead of removing ashpd we expose ashpd's features to select between async-std and tokio?

that is impossible. rust cannot do such thing. The problem is that when somewhere, even though not this crate enables the feature of tokio, but other place, as soon as the feature of tokio is enabled, the function will panic

@Be-ing
Copy link
Collaborator

Be-ing commented Mar 9, 2025

Sure it can. ashpd already has Cargo features to select between async-std and tokio.

@Decodetalkers
Copy link
Author

Decodetalkers commented Mar 9, 2025

Sure it can. ashpd already has Cargo features to select between async-std and tokio.

You have not understood the problem. The problem is , for example

[dependencies]
dark-light = "0.2.0"
zbus = {version = "0.5.5", features = ["tokio"]}

then the zbus in ashpd will use tokio to run the action. but now from the snippet of this code, it runs in async-std, and this time tokio is still not started, then the thread will panic.

This problem can even be called as long as one of you dependencies uses the tokio zbus, as long as the feature "tokio" is enabled, the function will panic.

it is not the problem of selecting tokio or selecting async-std, it is the design of the Cargo features, which can only be appended.

and also in this code, in order to run blocking flow, there is no need to change async into sync with async-std, and this api is quite simple, even do not need to use ashpd, just use the blocking api of zbus is enough

@Be-ing
Copy link
Collaborator

Be-ing commented Mar 9, 2025

I understand. That's why I'm suggesting we let the user of dark-light select whether dark-light uses ashpd's tokio or async-std feature. Then if you also use zbus for something else, you can ensure you use the same feature for zbus and dark-light in your Cargo.toml.

@Be-ing
Copy link
Collaborator

Be-ing commented Mar 9, 2025

there is no need to change async into sync

Unfortunately there is, because there is no way for this library to provide a cross platform async API because only the crate that does windowing on macOS is in a position to be able to implement such an async API. We have gone back and forth on this and unfortunately a simple blocking API is the best we can provide :/ #47

@Decodetalkers
Copy link
Author

there is no need to change async into sync

Unfortunately there is, because there is no way for this library to provide a cross platform async API because only the crate that does windowing on macOS is in a position to be able to implement such an async API. We have gone back and forth on this and unfortunately a simple blocking API is the best we can provide :/ #47

So, this is what I am doing, I try to rewrite the logic to a totally sync logic, you can see, there is not sync in my pr, and because I am using linux and writing things about xdg-desktop-portal, you can also let me continue maintain this part

@Decodetalkers
Copy link
Author

Decodetalkers commented Mar 9, 2025

I understand. That's why I'm suggesting we let the user of dark-light select whether dark-light uses ashpd's tokio or async-std feature. Then if you also use zbus for something else, you can ensure you use the same feature for zbus and dark-light in your Cargo.toml.

But why do we need to do such complex thing when we can just simply rewrite this logic with such simple code? There is no need to do that, just rewrite it with the correct blocking zbus, that will be ok.

And as I told above, the api of setting is quite simple that we do not need to induce a ashpd, which does many things that are not need for us

@Be-ing
Copy link
Collaborator

Be-ing commented Mar 9, 2025

Exposing a dependency's Cargo feature via a Cargo feature of our own is trivial. I don't understand why you think that's complicated.

@Decodetalkers
Copy link
Author

Exposing a dependency's Cargo feature via a Cargo feature of our own is trivial. I don't understand why you think that's complicated.

I just cannot understand why you do not want to just use blocking zbus to realize this part. I have told above, this api on linux is quite simple, do not need to induce extra lib, even to induce something like tokio and async-std

@Be-ing
Copy link
Collaborator

Be-ing commented Mar 9, 2025

The original reason for using ashpd was for its async stream API. But since we have discovered we cannot provide such an API on other operating systems, I suppose there is not a strong reason to keep ashpd anymore. I'll think about this... 🤔

@zeenix
Copy link

zeenix commented Mar 9, 2025

While this may be a good change, it doesn't solve the issue of zbus ending up using tokio accidentally, if any other dep decide to use zbus directly and enable the tokio feature.

My recommendation (as the zbus maintainer) is add a tokio feature to proxy to ashpd (or zbus if these changes are taken).

@zeenix
Copy link

zeenix commented Mar 9, 2025

Oops, I somehow missed all the comments before adding my comment. Sorry about that. 🙏

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.

3 participants