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

Make Daemon feature imply streaming feature #376

Merged
merged 2 commits into from
Feb 25, 2024

Conversation

LucasFA
Copy link
Contributor

@LucasFA LucasFA commented Feb 23, 2024

Right now launching spotify_player with the daemon CLI flag results in a runtime error if the binary was compiled without the streaming feature. As far as I understand it, I do not see why not just do this. The README says it still indicates that it needs some audio backend but AFAIK there's no way to encode that in the feature system.

Note: stumbled into this for the CI suite. There's tooling (aka cargo hack) that uses feature dependencies to build many options. This would stop compilation of binaries with the daemon feature without the streaming feature, reducing number of possible checks. Also just general less possible binaries.

@LucasFA
Copy link
Contributor Author

LucasFA commented Feb 24, 2024

I suppose that a possible reason for the current state of affairs is to notify the user in the case that they are missing the audio backend (rather than just the streaming feature)

For that we can write a compile_error! (not(any(rodio_backend, jackaudio_backend, etc))) instead of the runtime error.

If that sounds good, I'll push it

@aome510
Copy link
Owner

aome510 commented Feb 24, 2024

I suppose that a possible reason for the current state of affairs is to notify the user in the case that they are missing the audio backend (rather than just the streaming feature)

For that we can write a compile_error! (not(any(rodio_backend, jackaudio_backend, etc))) instead of the runtime error.

If that sounds good, I'll push it

Sounds good. When I try to run the app with cargo run --no-default-features --features streaming, which was built successfully, I got a bunch of cryptic errors:

�1������������b�>�2��������u��=����g���Q���@��0���k��K��.������������������������������m��K�z�$�Y��1�����������������������~��j��X��E��6��,��%��%��)���.���2���6���2��*�%�
�����#��1��>�&�L�2�X�A�f�P�i�\�e�c�]�e�O�k�A�m�1�s�%�~���	��������������������~��r��e�8�`�X�T�t�H��8��'������������������������������-�
�J��a�,�u�0���-��x��e���N��5�����s���d���_���_���_���b���m���}�������������������#��>��T��g�p��{���������������������������(���?���Q���`���o���x������⏎

Improve the error message (with compiler error) when user runs streaming w/o audio backend is a nice addition.

@LucasFA
Copy link
Contributor Author

LucasFA commented Feb 24, 2024

Just force pushed a commit for that. For reference, cargo check --no-default-features --features streaming now returns:

error: Streaming feature is enabled but no audio backend has been selected. Consider adding one of the following features:
           rodio-backend,
           alsa-backend,
           pulseaudio-backend,
           portaudio-backend,
           jackaudio-backend,
           rodiojack-backend,
           sdl-backend,
           gstreamer-backend
       For more information, visit https://github.com/aome510/spotify-player?tab=readme-ov-file#streaming
  --> spotify_player/src/streaming.rs:28:1
   |
28 | / compile_error!("Streaming feature is enabled but no audio backend has been selected. Consider adding one of the following features:
29 | |     rodio-backend,
30 | |     alsa-backend,
31 | |     pulseaudio-backend,
...  |
37 | | For more information, visit https://github.com/aome510/spotify-player?tab=readme-ov-file#streaming
38 | | ");
   | |__^

error: could not compile `spotify_player` (bin "spotify_player") due to 1 previous error

Forbids compilation of one non functional feature combination

Now, `cargo check --no-default-features --features streaming` (lacking
an audio backend) returns a compilation error instead of silently
producing a failing binary.
Remove runtime error, favouring compile time verification
@aome510 aome510 merged commit 67e9851 into aome510:master Feb 25, 2024
4 checks passed
@LucasFA LucasFA deleted the rm-runtime-errors branch February 25, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants