Discovery: Refactor and add Avahi DBus backend #1347

wisp3rwind · 2024-09-23T16:36:34Z

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.

wisp3rwind · 2024-09-24T10:10:40Z

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.

meiser79 · 2024-10-05T07:45:34Z

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

yubiuser · 2024-10-05T12:30:08Z

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

wisp3rwind · 2024-10-05T12:47:41Z

Sorry, I have fixed that locally already and forgot to push it. Will do so later today!

wisp3rwind · 2024-10-06T10:28:27Z

Build should be fixed now for any feature combination :)

yubiuser · 2024-10-06T11:19:40Z

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>

wisp3rwind · 2024-10-06T15:10:53Z

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>

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.

yubiuser · 2024-10-06T15:59:07Z

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.

roderickvd

I think this is awesome! Please note that I'm the process of releasing 0.5.0 as we speak, so please move your changelog entries into a post-0.5.0 "Unreleased" section that should be there momentarily.

src/main.rs

discovery/src/server.rs

discovery/src/avahi.rs

wisp3rwind

Thanks for the review 👍 I left some comments inline, and will push an update in a minute.

discovery/src/avahi.rs

src/main.rs

discovery/src/avahi.rs

discovery/src/server.rs

roderickvd · 2024-10-19T18:28:54Z

If you could resolve that last comment, and resolve the conflicts, I'm all for merging this. 👍

roderickvd · 2024-10-20T18:50:10Z

You think you could do something about #1379 here?

wisp3rwind · 2024-10-20T22:06:21Z

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.

roderickvd · 2024-10-21T20:11:04Z

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.

This helps to decouple discovery and core by not leaking implementation details of the zeroconf backend into Error conversion impls in core.

previously, libmdns errors would use a generic conversion from std::io::Error to core::Error

in preparation for adding another backend: The only purpose of this is to unregister the service on drop. Thus, it is much easier to work with an opaque type, which avoids proliferation #[cfg(...)] gates.

in preparation for re-working feature flags and adding another backend

i.e. add with-libmdns instead of using not(with-dns-sd). This is in preparation for adding another backend, and in general recommended, cf. https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features The logic is such that enabling with-dns-sd in addition to the default with-libmdns will still end up using dns-sd, as before. If only with-libmdns is enabled, that will be the default. If none of the features is enabled, attempting to build a `Discovery` will yield an error.

required by zbus >= 4

Previously, on drop the the shutdown_tx/close_tx, it wasn't guaranteed the corresponding tasks would continue to be polled until they actually completed their shutdown. Since dns_sd::Service is not Send and non-async, and because libmdns is non-async, put them on their own threads.

This deals gracefully with the case where the Avahi daemon is restarted or not running initially.

...but exit with an error if there's no way to authenticate

wisp3rwind · 2024-10-22T09:48:38Z

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.

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.

👍

roderickvd · 2024-10-26T14:46:46Z

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?

wisp3rwind · 2024-10-29T09:33:08Z

Merged, thanks a lot! May I ask you to also update the wiki with the new command line options?

Done!

michaelherger · 2024-10-31T08:11:08Z

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)

wisp3rwind mentioned this pull request Sep 23, 2024

Segmentation fault when running static build with with-dns-sd #1276

Closed

wisp3rwind force-pushed the avahi-dbus-v2 branch 2 times, most recently from ae578f5 to e49dde6 Compare September 24, 2024 09:30

wisp3rwind force-pushed the avahi-dbus-v2 branch 3 times, most recently from 561fada to 862e73e Compare October 1, 2024 09:37

yubiuser mentioned this pull request Oct 4, 2024

Remove librespot's avahi backend and build small-as-possible static binary yubiuser/librespot-shairport-snapserver#83

Merged

wisp3rwind force-pushed the avahi-dbus-v2 branch 3 times, most recently from 99e5ed4 to 33964da Compare October 6, 2024 09:47

wisp3rwind force-pushed the avahi-dbus-v2 branch from 012dac9 to c6873ba Compare October 6, 2024 15:02

wisp3rwind force-pushed the avahi-dbus-v2 branch from d07063f to 2579003 Compare October 15, 2024 13:30

wisp3rwind marked this pull request as ready for review October 15, 2024 14:22

roderickvd reviewed Oct 15, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

discovery/src/server.rs Outdated Show resolved Hide resolved

discovery/src/avahi.rs Show resolved Hide resolved

discovery/src/avahi.rs Show resolved Hide resolved

wisp3rwind commented Oct 15, 2024

View reviewed changes

discovery/src/avahi.rs Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

discovery/src/avahi.rs Show resolved Hide resolved

discovery/src/server.rs Outdated Show resolved Hide resolved

wisp3rwind force-pushed the avahi-dbus-v2 branch from 828ce4c to 3d28d93 Compare October 16, 2024 07:04

roderickvd linked an issue Oct 20, 2024 that may be closed by this pull request

librespot mdns reponder competes with avahi -> forces avahi to add '-2' to the hostname #1379

Closed

wisp3rwind mentioned this pull request Oct 20, 2024

librespot mdns reponder competes with avahi -> forces avahi to add '-2' to the hostname #1379

Closed

wisp3rwind force-pushed the avahi-dbus-v2 branch from 3d28d93 to 878d64d Compare October 21, 2024 09:03

wisp3rwind force-pushed the avahi-dbus-v2 branch from 878d64d to 63a9579 Compare October 21, 2024 09:04

wisp3rwind force-pushed the avahi-dbus-v2 branch from 63a9579 to c8a00b9 Compare October 21, 2024 23:47

wisp3rwind added 17 commits October 22, 2024 01:48

discovery: use opaque error type for DnsSdError

903843f

This helps to decouple discovery and core by not leaking implementation details of the zeroconf backend into Error conversion impls in core.

discovery: map all MDNS/DNS-SD errors to DiscoveryError::DnsSdError

09a6171

previously, libmdns errors would use a generic conversion from std::io::Error to core::Error

discovery: de-duplicate zerconf data into module consts

62e14e5

discovery: use an opaque type for the handle to the DNS-SD service

9e6c615

in preparation for adding another backend: The only purpose of this is to unregister the service on drop. Thus, it is much easier to work with an opaque type, which avoids proliferation #[cfg(...)] gates.

discovery: move service registration into separate functions

5383be1

in preparation for re-working feature flags and adding another backend

discovery: add --zeroconf-backend CLI flag

c8744a5

discovery: Add minimal Avahi zeroconf backend

512c558

bump MSRV to 1.75

c9f0aeb

required by zbus >= 4

update Cargo.lock and docs

582537a

discovery: use a shared channel for server and zeroconf status messages

b340f3b

discovery: add Avahi reconnection logic

692cde0

This deals gracefully with the case where the Avahi daemon is restarted or not running initially.

discovery: allow running when compiled without zeroconf backend...

36083fc

...but exit with an error if there's no way to authenticate

better error messages for invalid options with no short flag

c112ab3

address review

80e5fc0

discovery: revise shutdown handling after review

e6645f1

wisp3rwind force-pushed the avahi-dbus-v2 branch from c8a00b9 to e6645f1 Compare October 21, 2024 23:49

roderickvd approved these changes Oct 26, 2024

View reviewed changes

roderickvd merged commit 94d174c into librespot-org:dev Oct 26, 2024
13 checks passed

roderickvd mentioned this pull request Oct 26, 2024

Undiscoverable when launched at the same time as avahi-daemon #1133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discovery: Refactor and add Avahi DBus backend #1347

Discovery: Refactor and add Avahi DBus backend #1347

wisp3rwind commented Sep 23, 2024

wisp3rwind commented Sep 24, 2024 •

edited

Loading

meiser79 commented Oct 5, 2024

yubiuser commented Oct 5, 2024

wisp3rwind commented Oct 5, 2024

wisp3rwind commented Oct 6, 2024

yubiuser commented Oct 6, 2024

wisp3rwind commented Oct 6, 2024

yubiuser commented Oct 6, 2024

roderickvd left a comment

wisp3rwind left a comment

roderickvd commented Oct 19, 2024

roderickvd commented Oct 20, 2024

wisp3rwind commented Oct 20, 2024

roderickvd commented Oct 21, 2024

wisp3rwind commented Oct 22, 2024

roderickvd commented Oct 26, 2024

wisp3rwind commented Oct 29, 2024

michaelherger commented Oct 31, 2024

Discovery: Refactor and add Avahi DBus backend #1347

Discovery: Refactor and add Avahi DBus backend #1347

Conversation

wisp3rwind commented Sep 23, 2024

wisp3rwind commented Sep 24, 2024 • edited Loading

meiser79 commented Oct 5, 2024

yubiuser commented Oct 5, 2024

wisp3rwind commented Oct 5, 2024

wisp3rwind commented Oct 6, 2024

yubiuser commented Oct 6, 2024

wisp3rwind commented Oct 6, 2024

yubiuser commented Oct 6, 2024

roderickvd left a comment

Choose a reason for hiding this comment

wisp3rwind left a comment

Choose a reason for hiding this comment

roderickvd commented Oct 19, 2024

roderickvd commented Oct 20, 2024

wisp3rwind commented Oct 20, 2024

roderickvd commented Oct 21, 2024

wisp3rwind commented Oct 22, 2024

roderickvd commented Oct 26, 2024

wisp3rwind commented Oct 29, 2024

michaelherger commented Oct 31, 2024

wisp3rwind commented Sep 24, 2024 •

edited

Loading