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: init #55936

Merged
merged 1 commit into from Mar 6, 2019
Merged

nixos/snapserver: init #55936

merged 1 commit into from Mar 6, 2019

Conversation

tobim
Copy link
Contributor

@tobim tobim commented Feb 17, 2019

A nixos module for configuring the server side of pkgs.snapcast.
The module is named "snapserver" following upstream convention.
This commit does not provide module for the corresponding client.

I've been using this module for a few months.
The module could be simplified to just expose the essential options
and an extraOptions to deal with the rest, I'm not sure what the
convention on that is.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@tobim
Copy link
Contributor Author

tobim commented Feb 17, 2019

cc @peterhoeg

nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Show resolved Hide resolved
@tobim
Copy link
Contributor Author

tobim commented Feb 19, 2019

I changed the the streams option to allow arbitrary URI query key value pairs in the query attrset.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some final nitpicks

nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/snapserver.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member

Systemd already has that condition internally, I don't see a reason to duplicate that logic in the modules.

Wait what condition do you mean? How this PR is now, I think it could potentially start mpd/mopidy after snapserver.

@tobim
Copy link
Contributor Author

tobim commented Mar 6, 2019

Systemd already has that condition internally, I don't see a reason to duplicate that logic in the modules.

Wait what condition do you mean? How this PR is now, I think it could potentially start mpd/mopidy after snapserver.

Right now mpd could be started before or aftersnapserver. There is no direct link between the two in systemds dependency graph. The problematic case is if mpd is configured to write to snapserver but snapserver is not started yet. In this case, the current version of mpd starts fine but doesn't connect, a future version might fail to start though, there are no guarantees. Adding Before = [ mpd ]; to the unit would add a connection between the two services and make sure that snapserver is up and running before mpd can start.
The same applies to any other potential sources.

@infinisil
Copy link
Member

Ah yeah, so why not include Before = [ mpd ]; in this PR?

@tobim
Copy link
Contributor Author

tobim commented Mar 6, 2019

Ah yeah, so why not include Before = [ mpd ]; in this PR?

I did that now, because the costs of it are negligible. The strongest argument against is that this list is hard to complete. A better solution would be to specify these config dependent deps outside of the units. But I'm not aware of such a feature in systemd.

@infinisil
Copy link
Member

infinisil commented Mar 6, 2019

Cool, does systemd handle non-existing units correctly? Well, it should at least not fail the unit is what I hope.

@tobim
Copy link
Contributor Author

tobim commented Mar 6, 2019

I just tested the module with mpd installed and no mopidy on the system and it works. So looks like it does.

@infinisil
Copy link
Member

Now just squash all commits together and name it "nixos/snapserver: init" and this is good to merge :)

A nixos module for configuring the server side of pkgs.snapcast.
The module is named "snapserver" following upstream convention.
This commit does not provide module for the corresponding client.

Fix handling of port and controlPort

Fix stream uri generation & address review

Remove unused streams options & add description

Add missing description & Remove default fs path

Use types.port for ports & formatting improvements

Force mpd and mopidy to wait for snapserver
@tobim
Copy link
Contributor Author

tobim commented Mar 6, 2019

Done and done.

@tobim tobim changed the title nixos/snapserver: add module nixos/snapserver: init Mar 6, 2019
@infinisil infinisil merged commit 502a426 into NixOS:master Mar 6, 2019
@infinisil
Copy link
Member

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants