[GH-ISSUE #130] API review for librespot-core

kerem commented

2026-02-27 19:28:50 +03:00

Owner

Originally created by @plietar on GitHub (Feb 8, 2018).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/130

As a first step we want to publish librespot-core to crates.io and start versioning it.

While we can still make breaking changes by publishing a new major version, we should do a first review pass to make sure we're not publishing in our first version anything stupid we'll want to change immediately.

Nice resource to have in mind while doing so: https://rust-lang-nursery.github.io/api-guidelines/

List of things to do

FileId's field should be private and method to_raw/from_raw should be added, similar to SpotifyId's

Things that have been done

authentication::get_credentials should take a custom prompt function rather than using stderr/rpassword directly.
SpotifyId::from_base16 / SpotifyId::from_base62 / SpotifyId::from_raw should return Option or Result
Stuff in util/mod.rs should either be removed, moved to the crate/module they are use in, or , not exposed. (Partially Done)
int128::u128 does not need to be exposed. Worth noting that native u128 will hopefully be in stable soon (https://github.com/rust-lang/rust/issues/35118#issuecomment-361380874).
The new method generated by the component! macro does not need to be exposed
session::device_id probably does not belong in librespot-core at all
Session::spawn may not need to be exposed
component does not need to be exposed (macro is already internal only, but Lazy needs to be made more private)
SessionData does not need to be exposed
SessionInternal does not need to be exposed
MercuryManager::request and MercuryRequest/MercuryMethod may not need to be exposed
SpotifyId and FileId should be located in a more central place than util. Potentially at the root of the crate in lib.rs, or in their own module.
apresolve does not need to be exposed (#136)
AudioKeyManager::dispatch does not need to be exposed. (#136)
Credentials::from_reader and similar probably do not need to be exposed (#136)
cache is fine, could be moved to cache.rs rather than being in a subdirectory (#136)
ChannelManager::dispatch does not need to be exposed. (#136)
connection does not need to be exposed (#136)
MercuryManager::dispatch does not need to be exposed (#136)
subfile belongs to the same crate as the player
MercurySender::new does not need to be exposed
SessionWeak and Session::weak may not need to be exposed
Everything in config except SessionConfig can be moved to the relevant crate. SessionConfig can move to the session module.
~~diffie_hellman does not need to be exposed~~ It is used by discovery
~~Session::send_packet may not need to be exposed~~ It is used by at least audio and metadata
keymaster is fine like that
version is (probably) fine like that

For everything which "does not need to be exposed", this generally means switching the item's visibility from `pub` to `pub(crate)`.

Originally created by @plietar on GitHub (Feb 8, 2018). Original GitHub issue: https://github.com/librespot-org/librespot/issues/130 As a first step we want to publish librespot-core to crates.io and start versioning it. While we can still make breaking changes by publishing a new major version, we should do a first review pass to make sure we're not publishing in our first version anything stupid we'll want to change immediately. Nice resource to have in mind while doing so: https://rust-lang-nursery.github.io/api-guidelines/ <details> <summary>List of things to do</summary> - [ ] `FileId`'s field should be private and method `to_raw`/`from_raw` should be added, similar to `SpotifyId`'s </details> <br /> <details> <summary>Things that have been done</summary> - [x] `authentication::get_credentials` should take a custom prompt function rather than using stderr/rpassword directly. - [x] `SpotifyId::from_base16` / `SpotifyId::from_base62` / `SpotifyId::from_raw` should return `Option` or `Result` - [x] Stuff in `util/mod.rs` should either be removed, moved to the crate/module they are use in, or , not exposed. (Partially Done) - [x] `int128::u128` does not need to be exposed. Worth noting that native u128 will hopefully be in stable soon (https://github.com/rust-lang/rust/issues/35118#issuecomment-361380874). - [x] The `new` method generated by the `component!` macro does not need to be exposed - [x] `session::device_id` probably does not belong in `librespot-core` at all - [x] `Session::spawn` may not need to be exposed - [x] `component` does not need to be exposed (macro is already internal only, but `Lazy` needs to be made more private) - [x] `SessionData` does not need to be exposed - [x] `SessionInternal` does not need to be exposed - [x] `MercuryManager::request` and `MercuryRequest`/`MercuryMethod` may not need to be exposed - [x] `SpotifyId` and `FileId` should be located in a more central place than `util`. Potentially at the root of the crate in `lib.rs`, or in their own module. - [X] `apresolve` does not need to be exposed (#136) - [X] `AudioKeyManager::dispatch` does not need to be exposed. (#136) - [X] `Credentials::from_reader` and similar probably do not need to be exposed (#136) - [X] `cache` is fine, could be moved to `cache.rs` rather than being in a subdirectory (#136) - [X] `ChannelManager::dispatch` does not need to be exposed. (#136) - [X] `connection` does not need to be exposed (#136) - [X] `MercuryManager::dispatch` does not need to be exposed (#136) - [X] `subfile` belongs to the same crate as the player - [X] `MercurySender::new` does not need to be exposed - [X] `SessionWeak` and `Session::weak` may not need to be exposed - [x] Everything in `config` except `SessionConfig` can be moved to the relevant crate. `SessionConfig` can move to the `session` module. - ~~`diffie_hellman` does not need to be exposed~~ It is used by discovery - ~~`Session::send_packet` may not need to be exposed~~ It is used by at least audio and metadata - `keymaster` is fine like that - `version` is (probably) fine like that </details> <br /> For everything which "does not need to be exposed", this generally means switching the item's visibility from `pub` to `pub(crate)`.

kerem

2026-02-27 19:28:50 +03:00

closed this issue
added the
high priority

good first issue
labels

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@plietar commented on GitHub (Feb 8, 2018):

These are pretty easy to do and can be done individually, so anyone feel free to take a few and fix them.

@plietar commented on GitHub (Feb 8, 2018): These are pretty easy to do and can be done individually, so anyone feel free to take a few and fix them.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Feb 8, 2018):

Before we start this review, are you ok with spirc going into the core crate, or do you want it in a seperate one?

@sashahilton00 commented on GitHub (Feb 8, 2018): Before we start this review, are you ok with spirc going into the core crate, or do you want it in a seperate one?

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@plietar commented on GitHub (Feb 8, 2018):

No, IMO spirc belongs to its own crate, librespot-connect, which could potentially be merged with the discovery stuff.

@plietar commented on GitHub (Feb 8, 2018): No, IMO spirc belongs to its own crate, librespot-connect, which could potentially be merged with the discovery stuff.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@kingosticks commented on GitHub (Feb 8, 2018):

Since this crate won't include spirc, can I ask the possibly naive question, in what projects would the core crate be useful?

@kingosticks commented on GitHub (Feb 8, 2018): Since this crate won't include spirc, can I ask the possibly naive question, in what projects would the core crate be useful?

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@plietar commented on GitHub (Feb 8, 2018):

@kingosticks This is just a starting point, I agree core is pretty useless on it's own, but it must be done first.
The next steps are probably metadata, audio and player. These are the crates with reasonably well defined APIs, and still have some value to 3rd party projects. spirc and discovery would come last

@plietar commented on GitHub (Feb 8, 2018): @kingosticks This is just a starting point, I agree core is pretty useless on it's own, but it must be done first. The next steps are probably metadata, audio and player. These are the crates with reasonably well defined APIs, and still have some value to 3rd party projects. spirc and discovery would come last

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@awiouy commented on GitHub (Feb 9, 2018):

@plietar core::diffie_hellman is used in discovery

@awiouy commented on GitHub (Feb 9, 2018): @plietar core::diffie_hellman is used in discovery

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@plietar commented on GitHub (Feb 9, 2018):

@awiouy Could point, I've updated the list

@plietar commented on GitHub (Feb 9, 2018): @awiouy Could point, I've updated the list

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@plietar commented on GitHub (Feb 9, 2018):

Updated the list with #136. Thanks @awiouy

@plietar commented on GitHub (Feb 9, 2018): Updated the list with #136. Thanks @awiouy

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@awiouy commented on GitHub (Feb 13, 2018):

@plietar
apresolve.rs could be moved to session.rs and constants.
What do you think?

@awiouy commented on GitHub (Feb 13, 2018): @plietar `apresolve.rs` could be moved to `session.rs` and constants. What do you think?

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Feb 22, 2018):

I believe u128 will be in stable within a week, so once that lands, we can remove the int128 dependency.
Most of the stuff that was in util/mod.rs has been removed, I don't think it needs anything else being moved, as we'll be duplicating code into the connect crate otherwise.
Where do you think device_id should be if not in the core crate? Since it's a simple function, one could maybe stick it in main.rs?
imo keeping apresolve.rs in it's own file keeps things more readable.

@sashahilton00 commented on GitHub (Feb 22, 2018): I believe ```u128``` will be in stable within a week, so once that lands, we can remove the ```int128 ``` dependency. Most of the stuff that was in ```util/mod.rs``` has been removed, I don't think it needs anything else being moved, as we'll be duplicating code into the connect crate otherwise. Where do you think ```device_id``` should be if not in the core crate? Since it's a simple function, one could maybe stick it in main.rs? imo keeping apresolve.rs in it's own file keeps things more readable.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Feb 26, 2018):

@plietar have done the last couple of things, haven't touched the FileId stuff as I'm not too familiar with it, but the ball is now pretty much in your court in terms of a final code review before publishing. What are your thoughts with regards to #134? It's a bug that seems to be cropping up in quite a few use cases, and thus I'm wondering if we shouldn't hold off publishing to crates.io until we have this sorted out.

Furthermore, before we publish, we probably ought to think about how we want to handle development and releases from this point on, as once it's on crates.io, we can't really continue on the rolling release style that the project has used thus far, at least not in librespot (possibly in librespotd though). We probably need to think about properly tagging Github releases, and move development into a develop branch, which we periodically merge with master and increment the version. Any thoughts on this or an alternative release setup would be great.

Once it's published, I'll look at spending a bit more time on librespotd, and also documenting the codebase, since documentation is fairly sparse at the moment.

@sashahilton00 commented on GitHub (Feb 26, 2018): @plietar have done the last couple of things, haven't touched the ```FileId``` stuff as I'm not too familiar with it, but the ball is now pretty much in your court in terms of a final code review before publishing. What are your thoughts with regards to #134? It's a bug that seems to be cropping up in quite a few use cases, and thus I'm wondering if we shouldn't hold off publishing to crates.io until we have this sorted out. Furthermore, before we publish, we probably ought to think about how we want to handle development and releases from this point on, as once it's on crates.io, we can't really continue on the rolling release style that the project has used thus far, at least not in librespot (possibly in librespotd though). We probably need to think about properly tagging Github releases, and move development into a ```develop``` branch, which we periodically merge with ```master``` and increment the version. Any thoughts on this or an alternative release setup would be great. Once it's published, I'll look at spending a bit more time on librespotd, and also documenting the codebase, since documentation is fairly sparse at the moment.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@plietar commented on GitHub (Feb 27, 2018):

as once it's on crates.io, we can't really continue on the rolling release style that the project has used thus far

I don't see why not. Continuous development on master with tags whenever a release is done is fine. Unless you want to backport bugfixes to the latest release.

My only concern is how to deal with releases of individual crates. Say core and metadata are at version 0.5.0, and we add a new (backwards compatible) feature to metedata, what happens with core's version number? Do we bump both crates (and all the others) to 0.5.1 and tag that, or does metadata become 0.5.1 but core stays at 0.5.0? How does tagging work in this case?

I'd be curious to see how other projects with multliple crates in the same repo deal with this.

@plietar commented on GitHub (Feb 27, 2018): > as once it's on crates.io, we can't really continue on the rolling release style that the project has used thus far I don't see why not. Continuous development on master with tags whenever a release is done is fine. Unless you want to backport bugfixes to the latest release. My only concern is how to deal with releases of individual crates. Say `core` and `metadata` are at version 0.5.0, and we add a new (backwards compatible) feature to `metedata`, what happens with `core`'s version number? Do we bump both crates (and all the others) to 0.5.1 and tag that, or does `metadata` become 0.5.1 but `core` stays at 0.5.0? How does tagging work in this case? I'd be curious to see how other projects with multliple crates in the same repo deal with this.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@plietar commented on GitHub (Feb 27, 2018):

#134 can come later and shouldn't block the first release of core. It's a non-trivial change and will take some time, especially since I don't have time/can't write code for librespot.

@plietar commented on GitHub (Feb 27, 2018): #134 can come later and shouldn't block the first release of `core`. It's a non-trivial change and will take some time, especially since I don't have time/can't write code for librespot.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Feb 27, 2018):

I don't see why not. Continuous development on master with tags whenever a release is done is fine. Unless you want to backport bugfixes to the latest release.

What I intended to convey was that we can't have a rolling release on crates.io. Your point about rolling on master is valid though, and thinking about it, having a develop branch is pointless if we are tagging releases.

My only concern is how to deal with releases of individual crates. Say core and metadata are at version 0.5.0, and we add a new (backwards compatible) feature to metedata, what happens with core's version number? Do we bump both crates (and all the others) to 0.5.1 and tag that, or does metadata become 0.5.1 but core stays at 0.5.0? How does tagging work in this case?

I believe this is how serde handles things. If we take the metadata example, and implement a (backwards compatible) feature and then bump it from 0.5.0 -> 0.5.1, then core gets bumped from 0.5.0 -> 0.5.1 as well. If a change is then made in the connect crate, and the version gets bumped from 0.5.0 -> 0.5.1, then the core crate, which uses the connect crate, gets bumped from 0.5.1 -> 0.5.2.

If we stick to the semver Major.Minor.Patch logic, then whenever a fix is implemented in a crate, the patch number increases for that crate, and the patch number then also increases for crates which utilise that crate. Similarly, if we publish a (small) breaking change, we can increase the minor, and that propagates through, same thing again for the major. Over time this might mean versions become differentiated amongst the crates, but as long as one applies this logic, it's pretty easy to understand and implement.

#134 can come later and shouldn't block the first release of core. It's a non-trivial change and will take some time, especially since I don't have time/can't write code for librespot.

Good to know. Didn't realise you were still unable to contribute code. Anyway, I believe #134 and #21 will both require substantial portions of code to be rewritten, so perhaps this can be implementent in another minor at a later date.

@sashahilton00 commented on GitHub (Feb 27, 2018): > I don't see why not. Continuous development on master with tags whenever a release is done is fine. Unless you want to backport bugfixes to the latest release. What I intended to convey was that we can't have a rolling release on crates.io. Your point about rolling on master is valid though, and thinking about it, having a develop branch is pointless if we are tagging releases. > My only concern is how to deal with releases of individual crates. Say core and metadata are at version 0.5.0, and we add a new (backwards compatible) feature to metedata, what happens with core's version number? Do we bump both crates (and all the others) to 0.5.1 and tag that, or does metadata become 0.5.1 but core stays at 0.5.0? How does tagging work in this case? I believe this is how serde handles things. If we take the metadata example, and implement a (backwards compatible) feature and then bump it from ```0.5.0 -> 0.5.1```, then core gets bumped from ```0.5.0 -> 0.5.1``` as well. If a change is then made in the connect crate, and the version gets bumped from ```0.5.0 -> 0.5.1```, then the core crate, which uses the connect crate, gets bumped from ```0.5.1 -> 0.5.2```. If we stick to the semver Major.Minor.Patch logic, then whenever a fix is implemented in a crate, the patch number increases for that crate, and the patch number then also increases for crates which utilise that crate. Similarly, if we publish a (small) breaking change, we can increase the minor, and that propagates through, same thing again for the major. Over time this might mean versions become differentiated amongst the crates, but as long as one applies this logic, it's pretty easy to understand and implement. > #134 can come later and shouldn't block the first release of core. It's a non-trivial change and will take some time, especially since I don't have time/can't write code for librespot. Good to know. Didn't realise you were still unable to contribute code. Anyway, I believe #134 and #21 will both require substantial portions of code to be rewritten, so perhaps this can be implementent in another minor at a later date.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Feb 28, 2018):

@plietar are you happy to sign off on 0.1.0 being deployed to crates.io, and are you happy with the versioning described in the previous comment?

@sashahilton00 commented on GitHub (Feb 28, 2018): @plietar are you happy to sign off on ```0.1.0``` being deployed to crates.io, and are you happy with the versioning described in the previous comment?

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Mar 8, 2018):

I propose we get v0.1.0 shipped to crates.io, as enough of the bugs have been fixed to make this a plausible crate. Please tick your relevant box below if you agree that we are ready to ship the code, with the current codebase and the versioning suggested above. If you have any reservations, please make them known. I propose that we prepare ship this at the end of next week, assuming the majority agrees we're ready to ship.

@ComlOnline
@plietar
@sashahilton00

If anyone else has anything to contribute, please leave a note.

@sashahilton00 commented on GitHub (Mar 8, 2018): I propose we get v0.1.0 shipped to crates.io, as enough of the bugs have been fixed to make this a plausible crate. Please tick your relevant box below if you agree that we are ready to ship the code, with the current codebase and the versioning suggested above. If you have any reservations, please make them known. I propose that we prepare ship this at the end of next week, assuming the majority agrees we're ready to ship. - [x] @ComlOnline - [ ] @plietar - [x] @sashahilton00 If anyone else has anything to contribute, please leave a note.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@ComlOnline commented on GitHub (Mar 8, 2018):

I'm happy, the bugs we do have are nothing major, the only thing I would suggest we do is to make releases at the same time that we publish a crate.

@ComlOnline commented on GitHub (Mar 8, 2018): I'm happy, the bugs we do have are nothing major, the only thing I would suggest we do is to make releases at the same time that we publish a crate.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Mar 14, 2018):

I'm not sure building releases is that important, but I do think we definitely need to tag releases from the moment we publish and then going forwards.

@sashahilton00 commented on GitHub (Mar 14, 2018): I'm not sure building releases is that important, but I do think we definitely need to tag releases from the moment we publish and then going forwards.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@ComlOnline commented on GitHub (Mar 14, 2018):

Yeah that's what I was thinking. No building, just the tags and so that you can get the source code from a release.

@ComlOnline commented on GitHub (Mar 14, 2018): Yeah that's what I was thinking. No building, just the tags and so that you can get the source code from a release.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@sashahilton00 commented on GitHub (Mar 20, 2018):

I'll upload librespot to crates.io later this week, need to get my head around multi crate uploads, think I'll have to publish a few dummy versions before I can use [replace] to point it to git. for the initial compilation. Will give it some thought.

@sashahilton00 commented on GitHub (Mar 20, 2018): I'll upload librespot to crates.io later this week, need to get my head around multi crate uploads, think I'll have to publish a few dummy versions before I can use ```[replace]``` to point it to git. for the initial compilation. Will give it some thought.

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@hrkfdn commented on GitHub (Mar 8, 2019):

Any news on the publishing? Would be nice if we could publish our own crates using librespot :)

@hrkfdn commented on GitHub (Mar 8, 2019): Any news on the publishing? Would be nice if we could publish our own crates using librespot :)

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@ashthespy commented on GitHub (Nov 6, 2019):

@hrkfdn https://crates.io/crates/librespot is up and running!

@ashthespy commented on GitHub (Nov 6, 2019): @hrkfdn https://crates.io/crates/librespot is up and running!

kerem commented

2026-02-27 19:28:51 +03:00

Author

Owner

@hrkfdn commented on GitHub (Nov 6, 2019):

Awesome!

@hrkfdn commented on GitHub (Nov 6, 2019): Awesome!

Rows
Columns

[GH-ISSUE #130] API review for librespot-core #102