librespot-org
Discovery: Refactor and add Avahi DBus backend
So... I was mildly annoyed by the
*** WARNING *** The program 'librespot' uses the Apple Bonjour compatibility layer of Avahi.
*** WARNING *** Please fix your application to use the native API of Avahi!
*** WARNING *** For more information see <http://0pointer.de/blog/projects/avahi-compat.html>
warnings from the dns_sd compat library and thought it would be a fun little project to write an additional MDNS/DNS-SD backend that directly talks to Avahi via DBus.
This PR is a minimal working example of that. I made quite some refactorings to the discovery module in order to keep the code readable and not litter it too much with #[cfg(feature = "with-xyz")] guards. This should be fairly easy to review by looking at the individual commits and their messages.
I have a local branch with some more error handling in the avahi_task, but I'd be happy about your opinion on this before making things more complicated (I'd also need to clean that up a bit).
Does this have a place in librespot upstream? If so, I'd add some more documentation around the new feature flags and commandline flag.
Tests seem to pass now (I've had to bump MSRV and improve handling of the feature flags). The one remaining test failure seems to be spurious: It's a timeout when testing Session.connect, which shouldn't be affected by anything in this PR as far as I can tell.
EDIT: Re-based and cleaned up commit history.
Hi, just tried to compile it with cargo build --release --no-default-features --features "pulseaudio-backend", but I get the following compile errors:
Compiling librespot-discovery v0.5.0-dev (/tmp/librespot/discovery)
error[E0432]: unresolved import `self::avahi`
--> discovery/src/lib.rs:26:5
|
26 | avahi::EntryGroupState,
| ^^^^^ could not find `avahi` in the crate root
error[E0433]: failed to resolve: use of undeclared crate or module `zbus`
--> discovery/src/lib.rs:152:11
|
152 | impl From<zbus::Error> for DiscoveryError {
| ^^^^ use of undeclared crate or module `zbus`
error[E0433]: failed to resolve: use of undeclared crate or module `zbus`
--> discovery/src/lib.rs:153:20
|
153 | fn from(error: zbus::Error) -> Self {
| ^^^^ use of undeclared crate or module `zbus`
Some errors have detailed explanations: E0432, E0433.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `librespot-discovery` (lib) due to 3 previous errors
I've build this branch successfully using cargo build --release --no-default-features --features with-avahi. It was even possible to build a static binary (with RUSTFLAGS="-C target-feature=+crt-static").
When I don't specify any 'zeroconf' backend (e.g. cargo build --release --no-default-features) I get the same errors as @meiser79
Sorry, I have fixed that locally already and forgot to push it. Will do so later today!
Building with only --no-default-features works now, but it won't start successful.
[2024-10-06T11:17:43Z ERROR librespot] Invalid `--zeroconf-backend` / `-`: ""
Default: <none>
Building with only
--no-default-featuresworks now, but it won't start successful.[2024-10-06T11:17:43Z ERROR librespot] Invalid `--zeroconf-backend` / `-`: "" Default: <none>
Oh, right: I didn't think about that case. Previously, there was no way to compile librespot without discovery/zeroconf support, but this is a possibility now. (And I guess it's sensible: If people want to build a minimal-size executable, and require only OAuth support, they could compile without zeroconf backend.)
Should be fixed now, with librespot exiting with an error message if the combination of compile-time features and CLI options leaves no way to authenticate.
Works now.
[2024-10-06T15:58:03Z INFO librespot] librespot 0.5.0-dev d07063f (Built on 2024-10-06, Build ID: YkDAkydC, Profile: debug)
[2024-10-06T15:58:03Z ERROR librespot] Credentials are required if discovery and oauth login are disabled.
If you could resolve that last comment, and resolve the conflicts, I'm all for merging this. 👍
If you could resolve that last comment, and resolve the conflicts, I'm all for merging this. 👍
Will do!
You think you could do something about #1379 here?
The new backend should not be affected by that issue, cf. my comment at #1379.
I think there's room to improve documentation around that issue, and maybe also to improve librespot defaults. However, I think that's out of scope here.
Thanks for the update. I think there's something up with the changelog merge. Could you move your changes up to the "Unreleased" section? Also probably should mark the discovery changes as breaking if there were any API changes there that upstream devs should be aware of.
You think you could do something about #1379 here?
The new backend should not be affected by that issue, cf. my comment at #1379.
I think there's room to improve documentation around that issue, and maybe also to improve librespot defaults. However, I think that's out of scope here.
Props for the extensive reply there. All good to me. I thought maybe it was easy to slip that proposal in here, but totally fine to keep that for some other PR.
Thanks for the update. I think there's something up with the changelog merge. Could you move your changes up to the "Unreleased" section? Also probably should mark the
discoverychanges as breaking if there were any API changes there that upstream devs should be aware of.
Completely missed this when rebasing, thanks for the heads-up! Hopefully, this should be fixed now.
You think you could do something about #1379 here?
The new backend should not be affected by that issue, cf. my comment at #1379. I think there's room to improve documentation around that issue, and maybe also to improve librespot defaults. However, I think that's out of scope here.
Props for the extensive reply there. All good to me. I thought maybe it was easy to slip that proposal in here, but totally fine to keep that for some other PR.
:+1:
Merged, thanks a lot! May I ask you to also update the wiki with the new command line options?
With these breaking changes, we're going to up the version to 0.6.0. And hey, since faster releases are requested, would what we have now not be a nice scope?
Merged, thanks a lot! May I ask you to also update the wiki with the new command line options?
Done!
Is there a summary somewhere as to which library to use under which circumstances? What are the pros and cons of them? (I'm not familiar with mDNS here, but providing a librespot based binary to many platforms... Windows, Mac, Linux)
Quick note. If one was building with disabled default features (eg: cargo build --release --no-default-features --features pulseaudio-backend) librespot won't start anymore after this. To recover, explicitly enable mdns, cargo build --release --no-default-features --features pulseaudio-backend,with-libmdns.
Quick note. If one was building with disabled default features (eg:
cargo build --release --no-default-features --features pulseaudio-backend) librespot won't start anymore after this. To recover, explicitly enable mdns,cargo build --release --no-default-features --features pulseaudio-backend,with-libmdns.
What exactly do you mean when you say it "won't start anymore"?
With the changes made after https://github.com/librespot-org/librespot/pull/1347#issuecomment-2395473932 it definitely shouldn't crash but exit with a meaningful error message. Also, see https://github.com/librespot-org/librespot/issues/1390 and updates made to the documentation there.
There's room to further improve documentation; that's still on my todo-list.
What exactly do you mean when you say it "won't start anymore"?
Sorry, indeed; it exits with an error and the message Credentials are required if discovery and oauth login are disabled as mentioned above. It's very clean.
I was just pointing out the change ref --no-default-features, as if I half-understand here the mdns was baked-in (not a feature) and thus always enabled, but that's not true anymore.
Sorry for the noise.