Open shymega opened 6 years ago
Hey @shymega, sorry for taking so long to get back to you. Would be happy to add Libre.fm support. Not something I'd work on until post-v1.0 (I simply don't use Libre.fm), but always happy to take PRs if you're still interested in adding this.
No problem about the delay.
I'm just looking at the code now. Looking in particular at the source file src/scrobbler.rs
, function new
on line 20.
The problem is where to specify the endpoint. I suppose we could add an endpoint
field to the client
struct, and allow the developer to specify the endpoint after creating the an instance of client
, which would also mean extra line(s) of code as well.
The alternative is to specify another argument to new()
for the endpoint, which, IMHO, would not be desirable - it would require existing usages to take that into account, I think...
Personally, I think putting an endpoint
field to the client
struct is the best approach. What are your thoughts?
I just tried my approach I outlined in my last comment, which compiles fine, but Libre.fm doesn't seem to recognise the api_key
& api_secret
parameters. Indeed, my current scrobbler - mpdscribble - only needs a username and password in the configuration file.
Its possible this isn't going to be a quick fix. I've tried to find some documentation for Libre.fm, but it is lacking.
Looks like they don't require an API key & secret - which I guess makes sense for a free & open platform? If that's the case, it may be a fiddle of a refactor.
Yeah, that's the conclusion I'm coming to here as well. I had hoped it would be as simple as a endpoint update, but it doesn't look that simple. As well as that, the website doesn't have an API registration page either.
It might be easier to develop a whole new library for Libre.fm, but that's extra dependencies for developers..
Right, after asking in Libre.fm's IRC channel, API key and secret doesn't matter - we can just use a 32 digit placeholder.
If its as simple as a 32 digit random/static number for the key and secret, we can move forward with that.
I've had a look at implementing it in the existing codebase, but it is a bit hacky, and I haven't worked on it for a while... had some data loss, so I'm not even sure if I still have it.
HI @shymega, I'm happy to look over any PRs and work a bit on this but don't have a lot of time to dedicate. Maybe we could land this for Hacktoberfest?
@bobbo Sounds good to me. I might forget, but if you think I've forgotten, feel free to give me a prod!
Thanks.
Still taking a look at this. Apologies for the delay.. will keep you updated :sweat_smile:
No worries at all @shymega, there's no rush! If you've got any questions / anything you want to run past me just shout, happy to lend a hand.
Hey @dmfutcher - I don't know if you saw @InputUsername's comment about Libre.fm support on the referenced issue, and their scrobbler is using an unmaintained Listenbrainz library,
What would you say about adding both Libre.fm and Listenbrainz support to this library? Would it be in the library's scope? Personally, I can see a unified approach being better and less confusing for new developers.
@shymega I believe many scrobbling services like ListenBrainz and Maloja support access through a Last.fm-like API, so adding configurable API URLs could potentially be a catch-all solution for that?
One thing that I did run into with my listenbrainz
crate was that ureq
(which rustfm-scrobble
currently uses) sometimes errors out when connecting to api.listenbrainz.org -- and this seems to be an issue with ureq itself, so replacing ureq might be necessary for ListenBrainz support.
Potentially, but I'd also rather support the ListenBrainz native API, in case other hosted/self-hosted ListenBrainz instances don't have the Last.fm-like API. IMHO, it would be best to support their standard API. Unless I'm missing something here?
Interesting that ureq
has a timeout issue. Have you measured the connection time when connecting to the endpoint? How long is it, if at all?
I'm planning to use reqwest
for my bank library...
@shymega fair enough - in that case I'd be happy to accept PRs etc on listenbrainz-rs
if adding native support to rustfm-scrobble
isn't feasible.
I'm not sure it's a timeout issue for ureq
, I vaguely remember there being some (long-standing) issues with supported TLS certs that pop up for some websites but not others. It caused about half my scrobbles to ListenBrainz to not go through when I tested it.
reqwest
is fine - I personally prefer not to use it because it pulls in a bunch of crates from the Tokio ecosystem and doubles dependency count/compile times.
@shymega fair enough - in that case I'd be happy to accept PRs etc on
listenbrainz-rs
if adding native support torustfm-scrobble
isn't feasible.
I think it would be, it just might need refactoring...
I'm not sure it's a timeout issue for
ureq
, I vaguely remember there being some (long-standing) issues with supported TLS certs that pop up for some websites but not others. It caused about half my scrobbles to ListenBrainz to not go through when I tested it.
Hm. LMK if you remember anything else!I
reqwest
is fine - I personally prefer not to use it because it pulls in a bunch of crates from the Tokio ecosystem and doubles dependency count/compile times.
Yeah, I have used it on my bank library, but as you said, it does pull in a bunch of Tokio crates. I just need a blocking HTTP client.
@shymega sorry for not getting back to you sooner - I've run into all of the following issues using ureq with ListenBrainz's API before: https://github.com/algesten/ureq/issues/317, https://github.com/algesten/ureq/issues/318, https://github.com/algesten/ureq/issues/325
So, if rustfm-scrobble is turned into a unified scrobbler, I would recommend moving away from ureq
Hi all,
On the HTTP client, until very recently we were using reqwest
but to sort out the huge dependency tree that comes with it, we switched to ureq
. Is there a similarly minimal HTTP client that could be used in ureq
's place? Or I would have no issue with a compile option that would swap reqwest
back in (but wouldn't want that to be the default). If it helps, the last commit before ureq
was switched in was 914c6f0364b
.
Compile time options for enabling the other scrobbler backends may also be a good way of cutting out bloat if you only need Last.fm support vs supporting n-different scrobblers out of the box. Haven't really used them too much though, so could be totally unwieldy or unnecessary.
On the HTTP client, until very recently we were using
reqwest
but to sort out the huge dependency tree that comes with it, we switched toureq
. Is there a similarly minimal HTTP client that could be used inureq
's place? Or I would have no issue with a compile option that would swapreqwest
back in (but wouldn't want that to be the default). If it helps, the last commit beforeureq
was switched in was914c6f0364b
.
I found isahc
which wraps Curl but otherwise looks pretty similar to ureq
. It doesn't use rustls
so it avoids the issues ureq
has.
Compile time options for enabling the other scrobbler backends may also be a good way of cutting out bloat if you only need Last.fm support vs supporting n-different scrobblers out of the box. Haven't really used them too much though, so could be totally unwieldy or unnecessary.
That sounds reasonable. The API of ListenBrainz is a bit different in that authenticated requests simply need a user token instead of requiring a more complex login flow, so I'm not sure how compatible it is with the way things are currently done in rustfm-scrobble
.
AFAIK Libre.fm uses Last.fm's API though, so that would be a matter of allowing configurable API URLs, right?
On Sat, Jul 31, 2021, at 12:57 PM, Koen Bolhuis wrote:
On the HTTP client, until very recently we were using
reqwest
but to sort out the huge dependency tree that comes with it, we switched toureq
. Is there a similarly minimal HTTP client that could be used inureq
's place? Or I would have no issue with a compile option that would swapreqwest
back in (but wouldn't want that to be the default). If it helps, the last commit beforeureq
was switched in was914c6f0364b
.I found
isahc
https://crates.io/crates/isahc which wraps Curl but otherwise looks pretty similar toureq
. It doesn't userustls
so it avoids the issuesureq
has.
The only thing I'd say is that if the curl headers aren't available, surely it wouldn't be able to compile? In terms of cross-platform compatibility, it'd be necessary to consider macOS and Windows targets.
Compile time options for enabling the other scrobbler backends may also be a good way of cutting out bloat if you only need Last.fm support vs supporting n-different scrobblers out of the box. Haven't really used them too much though, so could be totally unwieldy or unnecessary.
That sounds reasonable. The API of ListenBrainz is a bit different in that authenticated requests simply need a user token instead of requiring a more complex login flow, so I'm not sure how compatible it is with the way things are currently done in
rustfm-scrobble
.
Yeah, I'm wondering if it should be split out into a separate crate. My ideal scenario is one scrobbling crate fits all, but I think it might become hacky without a little refactoring.
AFAIK Libre.fm uses Last.fm's API though, so that would be a matter of allowing configurable API URLs, right?
Yes, but with GNU FM, the authentication token, the api_key/api_secret can be any value, IIRC (32 len, iirc), its only username and password you need. GNU FM being what Libre.fm runs on.
I did ask in the GNU FM IRC channel, got a response from mattl, but seeing as Freenode has imploded, I'm not even sure if that channel exists now.
Dom Rodriguez (also known as shymega)
Yeah, I'm wondering if it should be split out into a separate crate. My ideal scenario is one scrobbling crate fits all, but I think it might become hacky without a little refactoring.
Agreed, honestly I think it's best to keep rustfm-scrobble focused on Last.fm/Libre.fm and maintain a separate crate for ListenBrainz. I'd be happy to add you to listenbrainz-rs
if you want.
I found
isahc
which wraps Curl but otherwise looks pretty similar toureq
. It doesn't userustls
so it avoids the issuesureq
has.
isahc
is async and relatively heavyweight compared to ureq
, and requires C dependencies in the form of curl
. If you're looking for something like ureq
that allows using libraries other than rustls
, I suggest attohttpc
.
Looking at attohttpc
, it looks just what I need for my scrobbler, and for the listenbrainz-rs
crate as well. Thanks for the link 🙌🏻
@InputUsername Yep, let's do that then. We can always put a link to each other - vice versa - in the README, perhaps? Sorry for the late reply! If you could add me, that'd be great. We can then work on this enhancement there.
One thing that I did run into with my
listenbrainz
crate was thatureq
(whichrustfm-scrobble
currently uses) sometimes errors out when connecting to api.listenbrainz.org
@InputUsername could you report that issue to ureq? api.listenbrainz.org supports modern cipher suites, so there should be nothing preventing ureq and/or rustls from working with it.
It would be good to see Libre.fm support for this library. As Libre.fm aims to mimic Last.fm's scrobbler API, I think one solution would be to allow a custom API endpoint URI to be specifed when
Scrobbler::new()
is specified; this could be used to specify the Libre.fm API base URI instead, but by default, it would use Last.fm's endpoint.I'd be glad to have a crack at implementing a PR to implement the above proposed solution, if it'll work. I'd need to test it first..
Thoughts? :smile: