Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comprehensive CI suite #350

Open
LucasFA opened this issue Jan 26, 2024 · 4 comments
Open

Comprehensive CI suite #350

LucasFA opened this issue Jan 26, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@LucasFA
Copy link
Contributor

LucasFA commented Jan 26, 2024

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

  • Rodio (default)
  • rodiojack (rodio with a jackaudio feature in librespot I think)
  • jackaudio
  • alsa
  • pulseaudio
  • portaudio
  • sdl
  • gstreamer

Each requires their own dependencies to compile.

Features

  • Streaming (enabled by default, requires Audio backend)
  • Lyrics (disabled)
  • Clipboard (enabled)
  • Media control (enabled by default, A:)
  • Image (disabled)
  • Sixel (disabled, implies Image)
  • Daemon (disabled, requires B)

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 LucasFA added the enhancement New feature or request label Jan 26, 2024
@LucasFA
Copy link
Contributor Author

LucasFA commented Mar 2, 2024

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

  • not be on Windows (according to the README)
  • not to have the media-control feature enabled, if on Windows or Mac (according to the main.rs, which leads to a sys exit of 1)

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
Copy link
Owner

aome510 commented Mar 4, 2024

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 #263 (comment).

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
Copy link
Contributor Author

LucasFA commented Mar 4, 2024

I'm not sure if it will fail to compile. The readme was added based on #263 (comment).

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
Copy link
Owner

aome510 commented Mar 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants