plietar / librespot

Open Source Spotify client library
MIT License
1.14k stars 185 forks source link

Update system_information_string #207

Closed sashahilton00 closed 7 years ago

sashahilton00 commented 7 years ago

After speaking with Spotify about the previous blocking of librespot, Spotify has clarified that there aren't plans to block librespot again. They have however requested that the system_information_string be changed back to librespot so that they have a metric to view and measure interest in the standalone library.

joerg-krause commented 7 years ago

It's good news they are planning not to block it again! Maybe they will even contribute :smile:

sashahilton00 commented 7 years ago

:) Don't think that'll be happening from the way the conversation went, but glad that we're not going to be playing cat and mouse for now :)

herrernst commented 7 years ago

Thanks for your communication efforts, glad that they don't plan to block it!

sashahilton00 commented 7 years ago

Indeed there was, though I am uncertain as to how to generate a random 8 character ASCII strings in rust. Furthermore the string should only be generated at build time. If you have a suggestion, I'm happy to update the PR...

sashahilton00 commented 7 years ago

@plietar the relevant changes have been made. Still unsure about how to generate a compile time random string, but one should probably the existing PR as Spotify were fairly clear that they wanted it identifying as librespot...

michaelherger commented 7 years ago

Does it need to be random? Could the git commit ID be used?

sashahilton00 commented 7 years ago

It does need to be random, for various reasons. Spotify were fairly clear about what they wanted; a string that looks like librespot[short sha][random string]. From https://github.com/plietar/librespot/issues/190 it sounds like they're now blocking the existing string, which is fair enough. Nothing to do but either wait for merge or recompile using this PR, which I've just tested to be working.

fabian-z commented 7 years ago

This works for me in order to generate a random build ID: https://github.com/fabian-z/librespot/commit/2afc7a3bdc5b9902a6d3c64a542df775c109dc1c

It extends version.rs, which is generated by vergen. The build script will only execute if there is no cache or the file changed - so generating a new ID (or version information) after the first build has to be done with cargo clean && cargo build.

Of course pushing this or a similar functionality upstream should be considered afterwards.

sashahilton00 commented 7 years ago

@fabian-z thanks for doing that :) Given that you've written a load more than my one-liner, do you want to open a PR and I'll close this one?

kingosticks commented 7 years ago

Sorry to jump in here but are you sure they don't want the random part generated at runtime? I don't see any value in it for them otherwise.

Edit: ignore me, I have just seen the previous comments where it's very clear it's meant to be build time. If that's what they want, fair enough.

mherger commented 7 years ago

I could imagine they'd want to block specific builds in case they were buggy etc. Commit ID wouldn't be good enough if eg. there was an OS specific problem. Can make sense.

kingosticks commented 7 years ago

If there was any sort of build specific issue they'd have to discover and block all the random strings corresponding to those builds. Perhaps they don't understand that anyone can build the software and use a unique agent ID. Or that anyone can use one of the distributed binaries and use a shared ID.

sashahilton00 commented 7 years ago

I would imagine that the build ID is so that they can block buggy builds. Makes sense, compared to blocking whole project. Either way, who cares.

fabian-z commented 7 years ago

@sashahilton00 You're welcome 😄 I'll open another PR then.

To throw in my two cents on the build ID requirement, in addition to what was written earlier, they may also be interested in the statistical aspect - for example how usage is distributed between multiple builds of the same commit (more individual devs or power users) and users using release builds with the same ID (indicating more interest and usage by end users).

But yeah, who cares as long as they continue to play nice 😃

balbuze commented 7 years ago

after applying pr it connect well but fails when starts playing : [./librespot -n test INFO:librespot: librespot eacf465 (2017-07-10). Built on 2017-07-13. Build ID: dviBsr35 WARN:mdns: Failed to register IPv6 receiver: Error { repr: Os { code: 19, message: "No such device" } } INFO:librespot::session: Connecting to AP "lon6-accesspoint-a27.ap.spotify.com:4070" INFO:librespot::session: Authenticated as "myname" ! INFO:librespot::session: Country: "FR" INFO:librespot::audio_backend::alsa: Using alsa sink INFO:librespot::player: Loading track "Morts les enfants" ERROR:librespot::player: Vorbis error: InvalidSetup INFO:librespot::player: Track "Morts les enfants" loaded INFO:librespot::player: Loading track "Si t'es mon pote" INFO:librespot::player: Track "Si t'es mon pote" loaded thread '' panicked at 'called Option::unwrap() on a None value', /checkout/src/libcore/option.rs:329 stack backtrace: 0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49 1: std::sys_common::backtrace::_print at /checkout/src/libstd/sys_common/backtrace.rs:71 2: std::panicking::default_hook::{{closure}} at /checkout/src/libstd/sys_common/backtrace.rs:60 at /checkout/src/libstd/panicking.rs:355 3: std::panicking::default_hook at /checkout/src/libstd/panicking.rs:371 4: std::panicking::rust_panic_with_hook at /checkout/src/libstd/panicking.rs:549 5: std::panicking::begin_panic at /checkout/src/libstd/panicking.rs:511 6: std::panicking::begin_panic_fmt at /checkout/src/libstd/panicking.rs:495 7: rust_begin_unwind at /checkout/src/libstd/panicking.rs:471 8: core::panicking::panic_fmt at /checkout/src/libcore/panicking.rs:69 9: core::panicking::panic at /checkout/src/libcore/panicking.rs:49 10: ::start 11: librespot::player::PlayerInternal::run 12: std::panicking::try::do_call 13: rust_maybe_catch_panic at /checkout/src/libpanic_unwind/lib.rs:98 14: <F as alloc::boxed::FnBox>::call_box 15: std::sys::imp::thread::Thread::new::thread_start at /checkout/src/liballoc/boxed.rs:650 at /checkout/src/libstd/sys_common/thread.rs:21 at /checkout/src/libstd/sys/unix/thread.rs:84 thread 'main' panicked at 'called Result::unwrap() on an Err value: "SendError(..)"', /checkout/src/libcore/result.rs:859 stack backtrace: 0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49 1: std::sys_common::backtrace::_print at /checkout/src/libstd/sys_common/backtrace.rs:71 2: std::panicking::default_hook::{{closure}} at /checkout/src/libstd/sys_common/backtrace.rs:60 at /checkout/src/libstd/panicking.rs:355 3: std::panicking::default_hook at /checkout/src/libstd/panicking.rs:371 4: std::panicking::rust_panic_with_hook at /checkout/src/libstd/panicking.rs:549 5: std::panicking::begin_panic at /checkout/src/libstd/panicking.rs:511 6: std::panicking::begin_panic_fmt at /checkout/src/libstd/panicking.rs:495 7: rust_begin_unwind at /checkout/src/libstd/panicking.rs:471 8: core::panicking::panic_fmt at /checkout/src/libcore/panicking.rs:69 9: core::result::unwrap_failed 10: librespot::player::Player::command 11: librespot::player::Player::load 12: librespot::spirc::SpircTask::load_track 13: librespot::spirc::SpircTask::handle_next 14: ::poll 15: librespot::main 16: __rust_maybe_catch_panic at /checkout/src/libpanic_unwind/lib.rs:98 17: std::rt::lang_start at /checkout/src/libstd/panicking.rs:433 at /checkout/src/libstd/panic.rs:361 at /checkout/src/libstd/rt.rs:57 18: libc_start_main ]

fabian-z commented 7 years ago

@balbuze This may be an unrelated problem, possibly even specific to your setup or environment - one hint is ERROR:librespot::player: Vorbis error: InvalidSetup. Can you try to reproduce with a clean build on a different machine?

On a side note, you can format this log block in Markdown and make it readable like this:

``` [....] ```

sashahilton00 commented 7 years ago

This PR has been superseded by https://github.com/plietar/librespot/pull/218, please continue discussions there if necessary.