librespot icon indicating copy to clipboard operation
librespot copied to clipboard

Avoid using default features of dependencies

Open kingosticks opened this issue 1 year ago • 10 comments

Is your feature request related to a problem? Please describe. I'm unable to control (disable) unneeded features of librespot's dependencies when pulling librespot (as a library) into other projects. A particular example is rustls which librespot compiles with the default feature set e.g. logging enabled and using aws-lc-sys instead of the more lightweight ring alternative.

Describe the solution you'd like I've read that libraries should avoid enabling default features of their dependencies and enable only what's required. Anything optional should be passed through for the caller to control. You can see this in libraries like ureq and hyper-rustls. From https://github.com/algesten/ureq/issues/765#issuecomment-2218703792

I think our guidance for libraries like ureq is to take rustls as a dependency with no default features enabled, and then let consumers either take their own direct dep on rustls to activate a specific feature flag for a backend, or to have ureq expose its own optional features that enable the relevant rustls features. That's the model hyper-rustls uses as one example.

But I appreciate we want to keep the librespot binary easy to use. So maybe we need to think about separating the deps of the library and the binary. The library wants to provide options, the binary is a concrete example of a particular set of options.

Additional context I think if we did this we could then consider switching to using ring as the default cryptography implementation. I don't believe we have any actual requirement for using aws-lc and it's harder to build on some platforms.

kingosticks avatar Oct 25 '24 14:10 kingosticks

Guess no one can object to that. What would be the best way to do that? Do we need to resurrect librespotd, or do we make a separate workspace here?

Also I agree on using ring instead of aws-lc again. FIPS would be the only reason right? That wouldn't matter.

We should also take another look at needing both ring and rustls. I remember one didn't have AES192 anymore and the other... I forgot.

roderickvd avatar Oct 26 '24 15:10 roderickvd

I think we can just shuffle things about a bit and more or less keep things as they are. That's what I'll attempt to do, at least.

And yes, I think it's mostly just FIPS. I think most projects are choosing to not use the new default.

kingosticks avatar Oct 26 '24 19:10 kingosticks

May I ask what the holdup is on this diff? This would allow several spotify packages to get updated on OpenBSD …

pstumpf avatar Dec 04 '24 18:12 pstumpf

I have merged the changes from https://github.com/kingosticks/librespot/commit/24bbc6314c991c64d1846162f739f223c44e5779 into my code, based on v0.6.0 release. I'm running librespot + changes without any issue. I'm using default features, compiled for Windows, PC/Linux and RPi, 32bit and 64bit (all native builds, no cross-compiling). I really appreciate to not run into build issues related to aws-lc-sys (even though all my systems are also prepared for building with aws-lc-sys).

fivebanger avatar Dec 05 '24 21:12 fivebanger

I’m maintainer of the spot package in Alpine Linux that uses librespot. I just wanted to upgrade the package to 0.5.0 and run into the issue with aws-lc-sys. I checked its dependency tree and I’m horrified – librespot brings an incredible dependency bloat. I’m really tired of this shit – dependency bloat in Rust projects caused by irresponsible approach to dependencies and the default-features antipattern. And cargo makes it extremely hard to fix this mess in downstream. So I’m strongly considering removing the spot package.

jirutka avatar Apr 28 '25 18:04 jirutka

Everybody agrees with getting rid of aws-lc-sys and default features. You can help if you want.

roderickvd avatar Apr 28 '25 18:04 roderickvd

Moved away from AWS-LC to ring in fe7ca0d7009c59d3189d1682154ad63a8fb71332.

roderickvd avatar Aug 13 '25 16:08 roderickvd

I'm a bit surprised this just works, I thought one of the lesser maintained rustls related packages didn't allow you to select provider so it ended up being stuck on aws. I can't remember which, I'll check (much) later. Good work though.

kingosticks avatar Aug 13 '25 17:08 kingosticks

Hopefully courtesy of all the upstream work. A fair number of dependencies were updated, certainly a lot of TLS related ones.

roderickvd avatar Aug 13 '25 18:08 roderickvd

Let’s double check that beyond compiling, it also actually works. At least I can log in again.

roderickvd avatar Aug 13 '25 18:08 roderickvd