clean up imports & clippy lints

[leshow]

I based this off the tokio_migration branch since that seems to be the most up to date. When I familiarize myself with a codebase I often go through and clean up little things. I cleaned up:

• the imports (merged & formatted)
• removed stuff from edition 2015 that's no longer needed (extern crate)
• removed last instance of error_chain
• applied lots of clippy lints
• StreamLoaderController in audio had methods which didn't need to be &mut self anymore

There should be no real code changes here, it's all just cleanups.

There's more I'd like to do but this is already big enough for a PR. I'd like to start removing the unwrap and expects in the code. The common practice these days is to use a boxed error type for binary, and Error derive (like thiserror) on library code. Given that, I think we should consider anyhow for the binary, we can replace unwrap/expect with .context and use ?.

Now that the code is written in async/await format, there looks to be a lot more we could potentially simplify. Anyway, let me know what you think for now! Thanks!

[Johannesd3]

I'm sorry, but you might have overlooked #649. But wrt error handling I totally agree.

[leshow]

Yeah I didn't look at the open PRs, my bad. Well, I think there's actual not too much overlap here so we should be able to merge one then the other and fix conflicts.

[Johannesd3]

Especially the import sections will be hard to merge, obviously we're fundamentally disagreeing about the best style here :) Nightly rustfmt has options to automatically (un)merge and regroup them, so we could also discuss this later, it will be easy to change.

[ashthespy]

@leshow I've merged in #649 so things should be up to speed with the dev branch.

Totally on board w.t.r thiserror -- it has been on the todo list for a while.. Please feel free to take a crack at it.

[Johannesd3]

As for me, you could go ahead and resolve the conflicts or just reapply your changes, @leshow. Except for rewriting AudioFileFetch as async fn I don't have concrete plans, so there won't be a conflict like now.

[leshow]

Great, thank you! I'll try to get to that later on today. But before I do I wonder if I can quickly try to convince you to merge the imports according to some sort of style.

• we already merge some imports, so the distinction right now is not clear. I'm suggesting we should merge them up to the parentmost point, so:

use crate::diffie_hellman::DHLocalKeys;
use crate::protocol;
use crate::protocol::keyexchange::{APResponseMessage, ClientHello, ClientResponsePlaintext};
use crate::util;

becomes:

use crate::{
    diffie_hellman::DHLocalKeys,
    protocol::{self, keyexchange::{APResponseMessage, ClientHello, ClientResponsePlaintext}},
    util,
};

• with the crates merged as above, we can split out merges into crate/third-party/std. This makes things organized and really easy to add new imports while keeping things clean. It also gives a concrete style going forward to stop things from getting messy in the future.

// any mods would go here
use super::codec::APCodec;
use crate::{
   diffie_hellman::DHLocalKeys,
   protocol::{self, keyexchange::{APResponseMessage, ClientHello, ClientResponsePlaintext}},
   util,
};

// third party
use byteorder::{BigEndian, ByteOrder, WriteBytesExt};
use hmac::{Hmac, Mac, NewMac};
use protobuf::{self, Message};
use rand::thread_rng;
use sha1::Sha1;
use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
use tokio_util::codec::{Decoder, Framed};

// std
use std::io;

Of course this is all very minor things to nitpick on, I just wanted to say my piece and hear your feedback before I re-apply my changes.

If you don't want to merge the imports, I would like to still split the std-imports from the third party imports just to make that spot obvious.

[Johannesd3]

I prefer a "shallow" style where only items of the same module are merged.

• It probably works better with git
• Looks cleaner without those nested brackets
• You don't need to skim multiple lines to get the full path

The imports should definitely be grouped by crate/third-party/std, but I would use another order, as below.

There are two related unstable rustfmt options: imports_granularity and group_imports. The latter has the value StdExternalCrate, so imo it's better to use this order. Unfortunately, we can't enforce these rules in CI if we don't want to use nightly.

What do the maintainers think?

[ashthespy]

Until imports_granularity hits stable, we could just use the old merge_imports.

becomes:

use crate::{
    diffie_hellman::DHLocalKeys,
    protocol::{self, keyexchange::{APResponseMessage, ClientHello, ClientResponsePlaintext}},
    util,
};

I personally prefer this style.. but end of the day it's just a code style, so as long as rustfmt can give us some thing standard (on stable), I'm fine :-)

[Johannesd3]

Was merge_imports ever stable? At least it's now deprecated.

[ashthespy]

You are right - I had forgotten that I had aliased rfmt to cargo +nightly fmt.

Warning: can't set `merge_imports = true`, unstable features are only available in nightly channel.

We can always pick this up later -- consistency (so with rustfmt on stable) wins over any particular style for me..

[leshow]

Okay, no problem, thanks for your thoughts.

[leshow]

Okay, I've re-applied some of my changes on top of the tokio_migration branch.

This has nothing to do with my current changes but just something I noticed:

The way that the cargo features are set up here is not great. Just opening the workspace in VSCode will give you the error "Cannot use two decoders at the same time". If you try and compile like playback individually, you get a whole stack of errors.

In general I just don't think the way these features are set up is a tenable situation. features are meant to be additive, not exclusive, and on top of that, we should be looking to not have a compile error when used with rust-analyzer. Unless I'm the only one getting this error? Reading some of these crates, it feels like there is a lot of accidental complexity that could be done away with.

I think we could make some quick good improvements to this codebase by adding a CI that does clippy lint, rustfmt, etc.

[leshow]

I've also pushed a change to use flavor = "current_thread" also, as running this binary on my machine spawns 16 threads.

[Johannesd3]

I'm not satisfied with the features for the decoders as well: Previously it was solved a different way where compilation could not fail (they had sone kind of precedence tremor > vorbis > lewton), but it had the drawback that lewton was always in the dependency tree, so I changed it.

I don't get errors in rust-analyzer, maybe you've activated a setting to check with all-features.

Would it be an option to drop vorbis and tremor support? Or are there any advantages I don't know of?

[leshow]

Yep, I forgot I have all-features on because when developing I want to see any potential compile errors for optional features.

Did lewton really add much to the compile time? Without any of the optional features, it doesn't look too big. Although it looks like it could use an update as well.

In any case, why are there are different ogg vorbis backends to begin with? Does one work better than the other? As you said it seems like we should just pick one. However, if do still want all three, then I wouldn't put them behind cargo features like this, it's just not designed for the sort of mutually exclusive selection we're doing.

Also, our version of vorbis is out of date (even though 0.1.0 was released like 5 years ago)

vorbis = { version ="0.0.14", optional = true }

looking through lewtons issues list doesn't inspire a lot of confidence: https://github.com/RustAudio/lewton/issues perhaps we'd be better off just using vorbis as it's just a wrapper around libvorbis?

[Johannesd3]

Last time I used vorbis, it panicked. tremor used to panic until a few weeks ago, but it was fixed. lewton works reliable, AFAIK.

The same question should be asked regarding libmdns/dns-sd in the connect crate.

[leshow]

If they don't actually work then I don't know why we would keep them. Perhaps we can get some input from the project maintainers on this.

[roderickvd]

I think there's a place for tremor for lower CPU and RAM usage on ARMv6 targets such as the Raspberry Pi Zero. In which case libvorbis support is basically a freebie as it's a 99% drop-in replacement.

I don't think looking at a library's issue list paints a complete enough to picture. Yes, lewton has 21 open. But rodio has no less than 82 spread over 4 pages and that didn't stop it from becoming the default backend.

As for libmdns / dns-sd, I'm not super-experienced but cursory Googling show no reasons why the pure Rust libmdns library wouldn't suffice. I think it's mostly historic from back when it wasn't up to par with external libraries. In which case you could drop it.

[leshow]

I haven't taken a look at connect in much detail yet, but why not just use trust-dns? It's been around for a while and has a client you can use to resolve DNS.

[Johannesd3]

@leshow I think you're confusing DNS and mDNS, so trust-dns is probably not the right choice.

I'm really keen to hear the opinion of the maintainers who probably remember why these alternatives were introduced.

[leshow]

I'm pretty sure trust-dns has an option to resolve multicast also. Check out: https://github.com/bluejekyll/trust-dns/blob/main/crates/proto/src/multicast/mdns_client_stream.rs#L24

It's got a higher-level wrapper in their client library also behind the mdns feature.

[Johannesd3]

@willstott101 Are you maybe able to tell us if libmdns simply can be replaced by trust-dns?

[leshow]

We've sidetracked a bit from what this PR actually does, but I'm glad these convos are happening!

Once these things are decided perhaps we can make some issues to split the work up, don't want to get into a situation where we duplicate effort again.

[willstott101]

Many of the optional features require a non-rust library which adds non-cargo dependencies onto the build. If they're not behind features then there'd have to be some other way to choose between them. It's my strong preference for librespot to be pure-rust by default due to the ease of Windows support if that's the case.

One of the main issues with maintaining libmdns has been platform support, which is not always trivial to add CI checking for. trust-dns still claims mDNS support is experimental, but if it works and supports all the same (and maybe even more) platforms than libmdns then it's no skin off my teeth.

It looks like no one is maintaining the dns-sd bindings, and it doesn't have incredible cross-platform support. But I have no evidence to suggest it doesn't work, nor even any evidence to suggest it doesn't work better.