rustfm-scrobble icon indicating copy to clipboard operation
rustfm-scrobble copied to clipboard

Add Libre.fm support.

Open shymega opened this issue 6 years ago • 24 comments

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:

shymega avatar Feb 25 '18 21:02 shymega

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.

dmfutcher avatar Apr 26 '19 09:04 dmfutcher

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?

shymega avatar Apr 26 '19 12:04 shymega

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.

shymega avatar Apr 26 '19 14:04 shymega

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.

dmfutcher avatar Apr 26 '19 14:04 dmfutcher

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..

shymega avatar Apr 26 '19 14:04 shymega

Right, after asking in Libre.fm's IRC channel, API key and secret doesn't matter - we can just use a 32 digit placeholder.

shymega avatar Apr 26 '19 21:04 shymega

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.

shymega avatar Sep 10 '19 12:09 shymega

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?

dmfutcher avatar Sep 26 '19 18:09 dmfutcher

@bobbo Sounds good to me. I might forget, but if you think I've forgotten, feel free to give me a prod!

Thanks.

shymega avatar Oct 02 '19 12:10 shymega

Still taking a look at this. Apologies for the delay.. will keep you updated :sweat_smile:

shymega avatar May 10 '21 13:05 shymega

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.

dmfutcher avatar May 12 '21 11:05 dmfutcher

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 avatar Jul 23 '21 12:07 shymega

@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.

InputUsername avatar Jul 23 '21 13:07 InputUsername

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 avatar Jul 23 '21 13:07 shymega

@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.

InputUsername avatar Jul 23 '21 13:07 InputUsername

@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 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 avatar Jul 23 '21 13:07 shymega

@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

InputUsername avatar Jul 27 '21 19:07 InputUsername

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.

dmfutcher avatar Jul 28 '21 15:07 dmfutcher

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.

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?

InputUsername avatar Jul 31 '21 11:07 InputUsername

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 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.

I found isahc https://crates.io/crates/isahc which wraps Curl but otherwise looks pretty similar to ureq. It doesn't use rustls so it avoids the issues ureq 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.

-- Kind regards,

Dom Rodriguez (also known as shymega)

shymega avatar Jul 31 '21 13:07 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.

InputUsername avatar Jul 31 '21 18:07 InputUsername

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.

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.

Shnatsel avatar Aug 29 '21 15:08 Shnatsel

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.

shymega avatar Aug 29 '21 15:08 shymega

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

@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.

Shnatsel avatar Aug 29 '21 15:08 Shnatsel