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

nixos/snapserver: update module to work with snapcast 0.20 #94071

Merged
merged 2 commits into from Aug 2, 2020

Conversation

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Jul 28, 2020

Motivation for this change

With the update of snapcast from 0.15 to 0.20 in #90294 I inadvertently broke the module, since the CLI parameters changed quite a bit.

This should rectify and even improve the situation.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@mweinelt mweinelt requested a review from tobim Jul 28, 2020
@mweinelt mweinelt marked this pull request as draft Jul 28, 2020
@mweinelt mweinelt force-pushed the mweinelt:snapserver branch from aca4688 to 6b7c861 Jul 28, 2020
@mweinelt mweinelt marked this pull request as ready for review Jul 28, 2020
@mweinelt
Copy link
Member Author

@mweinelt mweinelt commented Jul 28, 2020

For some reason I'm always gettingthe renamed trace, even though I'm not using the controlPort variable any more. This is also visible in the tests.

trace: Obsolete option `services.snapserver.controlPort' is used. It was renamed to `services.snapserver.tcp.port'.
@tobim
Copy link
Contributor

@tobim tobim commented Jul 28, 2020

I'll try to squeeze a review in later today.

@mweinelt
Copy link
Member Author

@mweinelt mweinelt commented Jul 29, 2020

@tobim I also believe we need to add a static group for snapcast, since we need to allow other users like mpd to write to the pipe at /run/snapserver/mpd.

Copy link
Contributor

@tobim tobim left a comment

I also believe we need to add a static group for snapcast, since we need to allow other users like mpd to write to the pipe at /run/snapserver/mpd.

The server always creates the pipe with write access for all:
https://github.com/badaix/snapcast/blob/d44f3be4edee76494cecb1c28caf46969f3ba15c/server/streamreader/pipe_stream.cpp#L50

If we need access control for that it should be implemented upsteam.


in {
imports = [
(mkRenamedOptionModule [ "services" "snapserver" "controlPort"] [ "services" "snapserver" "tcp" "port" ])

This comment has been minimized.

@tobim

tobim Aug 2, 2020
Contributor

Adjust line 282 to fix the deprecation warning.

This comment has been minimized.

@mweinelt

mweinelt Aug 2, 2020
Author Member

Fixed. Good catch.

@mweinelt mweinelt force-pushed the mweinelt:snapserver branch from c57b495 to abd332c Aug 2, 2020
@mweinelt
Copy link
Member Author

@mweinelt mweinelt commented Aug 2, 2020

Indeed, thanks for the pointer to the snapcast source code.

@mweinelt
Copy link
Member Author

@mweinelt mweinelt commented Aug 2, 2020

@GrahamcOfBorg test snapcast

Checks
- if all configured ports are listened on
- if all pipes for multiple streams get set up
- if rpc interaction is possible
@mweinelt mweinelt force-pushed the mweinelt:snapserver branch from abd332c to 0a9dd49 Aug 2, 2020
@tobim
tobim approved these changes Aug 2, 2020
Copy link
Contributor

@tobim tobim left a comment

Tested locally, works with mpd.

@mweinelt mweinelt merged commit bd9ea65 into NixOS:master Aug 2, 2020
16 checks passed
16 checks passed
tests tests
Details
tests
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0a9dd49"; rev="0a9dd49634be5a8da8ec76110ea1f0696e68d22d"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0a9dd49"; rev="0a9dd49634be5a8da8ec76110ea1f0696e68d22d"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0a9dd49"; rev="0a9dd49634be5a8da8ec76110ea1f0696e68d22d"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0a9dd49"; rev="0a9dd49634be5a8da8ec76110ea1f0696e68d22d"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0a9dd49"; rev="0a9dd49634be5a8da8ec76110ea1f0696e68d22d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0a9dd49"; rev="0a9dd49634be5a8da8ec76110ea1f0696e68d22d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0a9dd49"; rev="0a9dd49634be5a8da8ec76110ea1f0696e68d22d"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@mweinelt
Copy link
Member Author

@mweinelt mweinelt commented Aug 2, 2020

Thanks for reviewing this!

@mweinelt mweinelt deleted the mweinelt:snapserver branch Aug 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.