aome510 / spotify-player

A Spotify player in the terminal with full feature parity
MIT License
3.46k stars 149 forks source link

Comprehensive CI suite #350

Open LucasFA opened 8 months ago

LucasFA commented 8 months ago

Well, we just had #349, an issue about breaking with rust versions up to and including 1.73. That was a case from the bump in MSVR in souvlaki: Sinono3/souvlaki#46, breaking pulseaudio. Let's test this doesn't happen.

There's quite a few features, so we can't check each combination, but we could at least check each backend and feature once and use more sparse testing with the rest of the feature space. Another with as many features as possible, too.

There's also some interaction between target features and target OS, at least around the daemon code, so there's even more combinations.

I'm going to list all possible features (if I miss any please warn)

Audio backends

Each requires their own dependencies to compile.

Features

A: Media control is compiled in by default but only actually active on Linux by default because of what seems described as a bug or something. B: requires: 1. not(Windows) (README), 2. streaming (code), 3. not((W || macOS) && media-control) (code, C), 4. C: I don't know if what I found was intended. As written it's that way, I think. Lines 285 of spotify-player/main.rs. Also, I think some of these can be turned into compile time errors.

LucasFA commented 7 months ago

There's #385, but I am still puzzles by this tangent: the Daemon feature requires

One way or the other this can be turned into a compile time error - but to confirm, compiling on Windows with the daemon feature just doesn't work, right? I don't want to write a compile_error! only to find out it does work

aome510 commented 7 months ago

but to confirm, compiling on Windows with the daemon feature just doesn't work, right?

I'm not sure if it will fail to compile. The readme was added based on https://github.com/aome510/spotify-player/pull/263#issuecomment-1742623193.

One way or the other this can be turned into a compile time error

Sounds good to me but maybe only for "not to have the media-control feature enabled, if on Windows or Mac". I haven't confirmed the first one.

LucasFA commented 7 months ago

I'm not sure if it will fail to compile. The readme was added based on https://github.com/aome510/spotify-player/pull/263#issuecomment-1742623193.

FWIW, I just cargo b --features daemon --target x86_64-pc-windows-gnu and the daemonize crate does fail to compile, starting with

error[E0433]: failed to resolve: could not find `unix` in `os`
  --> /home/lucasfa/.local/share/cargo/registry/src/index.crates.io-6f17d22bba15001f/daemonize-0.5.0/src/lib.rs:55:14
   |
55 | use std::os::unix::ffi::OsStringExt;
   |              ^^^^ could not find `unix` in `os`

Which I'd take to mean that the crate only supports unix-likes.

maybe only for "not to have the media-control feature enabled, if on Windows or Mac".

Sounds good

aome510 commented 7 months ago

FWIW, I just cargo b --features daemon --target x86_64-pc-windows-gnu and the daemonize crate does fail to compile, starting with

Got it. Thanks for the confirmation.