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.

Sign up for GitHub

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

Jump to bottom

services.mpd: allow configuring playlist directory #28252

Merged
merged 1 commit into from
Aug 20, 2017
Merged

services.mpd: allow configuring playlist directory #28252

merged 1 commit into from
Aug 20, 2017

Conversation

casey
Copy link
Contributor

@casey casey commented Aug 14, 2017 •
edited

Modify the mpd service to allow configuring the playlist directory separately.

Tested on NixOS with sandboxing.

The removed line in preStart will be a no-op for existing systems. Since they won't have the new playlistDirectory, and the old playlistDirectory is underneath the old dataDir.

I removed that line since it seemed inappropriate to modify user and owner in the playlistDirectory, which might be outside of mpd's data directory.

@mention-bot
Copy link

@casey, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rickynils, @bjornfor and @shlevy to be potential reviewers.

fpletz
fpletz reviewed Aug 14, 2017
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Hmm, my issues with the path type and defaultText apply to the whole module. That's probably the reason you implemented it like this.

Do you want to fix the whole module? :)

nixos/modules/services/audio/mpd.nix
@@ -62,6 +60,14 @@ in {
'';
};

playlistDirectory = mkOption {
type = types.path;
default = "${cfg.dataDir}/playlists";
Copy link
Member

Choose a reason for hiding this comment

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

The problem here is that if the user changes dataDir, the nixos manual will get rebuilt.

You should add defaultText = ''''${dataDir}/playlists''; to fix this.

nixos/modules/services/audio/mpd.nix
@@ -62,6 +60,14 @@ in {
'';
};

playlistDirectory = mkOption {
type = types.path;
Copy link
Member

Choose a reason for hiding this comment

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

Paths in Nix refer to files on the evaluating machine that will be put in the Nix store. This is not applicable here. The path to the directory is just a string. Please use types.str here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think path is okay, it's just a string that begins with a slash: https://github.com/NixOS/nixpkgs-channels/blob/nixos-unstable/lib/types.nix#L167 It's a little unfortunate that the name is overloaded (I too tend to think of store paths in this context).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is types.path here different from a nix path literal? A path literal being /this/is/a/path/literal, as opposed to "/this/is/just/a/string".

Copy link
Contributor

Choose a reason for hiding this comment

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

As is, there should be no difference as far as types.path is concerned. The check would accept both.

nixos/modules/services/audio/mpd.nix
@@ -153,7 +159,6 @@ in {

preStart = ''
mkdir -p "${cfg.dataDir}" && chown -R ${cfg.user}:${cfg.group} "${cfg.dataDir}"
mkdir -p "${playlistDir}" && chown -R ${cfg.user}:${cfg.group} "${playlistDir}"
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if mpd creates the playlist directory if it doesn't exist yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it doesn't, I'll re-add that line.

@fpletz fpletz added this to the 17.09 milestone Aug 14, 2017
@casey
Copy link
Contributor Author

casey commented Aug 15, 2017

@fpletz I think that the path type is okay, see @joachifm's comment.

I'm not sure I understand the defaultText issue though, although I'm happy to fix it. Should every option have a defaultText value, or just the text options?

@joachifm
Copy link
Contributor

@casey default values are evaluated & the result converted to a string for rendering, the purpose of defaultText is to short.circuit that by providing the desired display string directly, avoiding unnecessary/expensive work.

@casey
Copy link
Contributor Author

casey commented Aug 15, 2017

@joachifm So for the musicDirectory option, its default is "${cfg.dataDir}/music". Which form should defaultText be in:

String literal, doesn't change if default changes:
defaultText = "/var/lib/mpd/music"

Use the default directly, not actually sure if this is possible:
defaultText = "${cfg.dataDir.default}/music"

Pull the dataDir default to somewhere else in the program, then use it:

let
  dataDirDefault = "/var/lib/${name}"
in {
...
  # in musicDirectory option
  defaultText = "${dataDirDefault}/music"

  # in dataDir option
  default = $dataDirDefault
  defaultText = $dataDirDefault
...
}

@joachifm
Copy link
Contributor

joachifm commented Aug 16, 2017 •
edited

Or you could do what @fpletz suggests and use quoting to make the default text literally render as ${dataDir}/playlist &c. The important thing is to avoid the manual becomig dependent on the user's configuration.

Doing

let
  dataDirDefault = "/var/lib/mpd";
in ...

and using dataDirDefault when constructing the default values removes the need for defaultText (it's kind of a kludge so should probably be used as little as possible).

@casey
Copy link
Contributor Author

casey commented Aug 16, 2017

@joachifm Ahh, okay, gotcha, I was confused. I didn't get that ''${ escapes ${. Pushed a new version which adds defaultText for everything in the module.

@joachifm joachifm merged commit ae02dd2 into NixOS:master Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joachifm joachifm joachifm left review comments

@fpletz fpletz fpletz left review comments

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update)
Projects
None yet
Milestone
17.09
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@casey @mention-bot @joachifm @fpletz