Minidlna: Allow more configuration options #57036

ardumont · 2019-03-07T18:49:32Z

Motivation for this change

"As a minidlna user", i'd like to have more setup option choices.

At the moment, we have the choice between:

having a subset of options
override completely the full configuration ("losing" the existing minimal subset)

This PR allows to:

configure some other options
configure the ones not yet disclosed in nix (extending the
existing minimal subset). This matches what is done for example in the sshd service.

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 (debian with nix)
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)

Added test on that service.
I followed the doc about tests [1] though which i think is a better link
https://nixos.org/nixos/manual/index.html#sec-nixos-tests

Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"

I'm still not sure what that does but it seems to have worked:

$ git reset --soft origin/master  # now there is wip
$ nix-shell -p nox --run "nox-review wip"
...
Result in /run/user/1000/nox-review-1xmyktwu
total 0
lrwxrwxrwx 1 tony tony 67 Mar  8 18:36 result -> /nix/store/l7lzfp2i0yizm7grylcrmkikm96hmx82-nixos-system-nixos-test

Determined the impact on package closure size (by running nix path-info -S before and after)

Not sure i did this correctly:

$ git checkout master
$ nix-build -A minidlna
$ nix path-info -S ./result
/nix/store/1283pa3i7yyjpz7aana2rncq41qjwkxd-minidlna-1.2.1        141423736
$ git checkout minidlna-update-configuration
$ nix-build -A minidlna
$ nix path-info -S ./result
/nix/store/1283pa3i7yyjpz7aana2rncq41qjwkxd-minidlna-1.2.1        141423736

Assured whether relevant documentation is up to date
~> every new option opened has the docstring updated as well
Fits CONTRIBUTING.md.
as far as i could tell ;)
Tested execution of all binary files (usually in ./result/bin/)

It's a service, i don't know how to test that without actually
installing it... Any pointer on that is very welcome, thanks. (I
searched quite some time at nixpkgs, nixos, and nix manuals without
quite the answer i hoped for)

So, i settled on testing it with my rpi3 (aarch64), so:

disabled the mainstream service
import this patched version
define what i wanted as setup

Excerpt:

  ...
  disabledModules = [ "services/networking/minidlna.nix" ];
  imports = [
    ./override-minidlna.nix  # <- the pr's patch
  ];
  services.minidlna = {
    enable = true;
    mediaDirs = [
      "PV,${path}/family"
      "V,${path}/judo"
      "V,${path}/courses"
    ];
    friendlyName = "rpi3";
    rootContainer = "B";
    extraConfig = ''
      album_art_names=Cover.jpg/cover.jpg/AlbumArtSmall.jpg/albumartsmall.jpg
      album_art_names=AlbumArt.jpg/albumart.jpg/Album.jpg/album.jpg
      album_art_names=Folder.jpg/folder.jpg/Thumb.jpg/thumb.jpg
      notify_interval=60
    '';
  };

built
installed
runs fine with the new configuration \m/

tony@kids> cat $(systemctl status minidlna.service  | grep minidlna.conf | awk '{print $7}')                                                                                                 ~
port=8200
friendly_name=rpi3
db_dir=/var/cache/minidlna
log_level=warn
inotify=yes
root_container=B
media_dir=PV,/family
media_dir=V,/judo
media_dir=V,/courses

album_art_names=Cover.jpg/cover.jpg/AlbumArtSmall.jpg/albumartsmall.jpg
album_art_names=AlbumArt.jpg/albumart.jpg/Album.jpg/album.jpg
album_art_names=Folder.jpg/folder.jpg/Thumb.jpg/thumb.jpg
notify_interval=60

Cheers,

ardumont · 2019-03-07T20:41:43Z

Fixed mixed tab/spaces characters.

aanderse · 2019-03-09T05:16:46Z

@infinisil would be real nice to be able to point someone to an RFC regarding module options and config files right now... 😃

nixos/tests/minidlna.nix

aanderse · 2019-03-09T15:30:54Z

nixos/tests/minidlna.nix

+album_art_names=Cover.jpg/cover.jpg/AlbumArtSmall.jpg/albumartsmall.jpg
+album_art_names=AlbumArt.jpg/albumart.jpg/Album.jpg/album.jpg
+album_art_names=Folder.jpg/folder.jpg/Thumb.jpg/thumb.jpg
+notify_interval=60


Can you please correct the invento indentation?

yes, on it.

Done btw ;)

nixos/tests/minidlna.nix

aanderse · 2019-03-24T13:56:49Z

@ardumont how do you feel about something like this? master...aanderse:minidlna
See https://github.com/Infinisil/rfcs/blob/config-option/rfcs/0042-config-option.md by @infinisil for reference.

ardumont · 2019-03-24T15:47:28Z

@ardumont how do you feel about something like this?
master...aanderse:minidlna See
https://github.com/Infinisil/rfcs/blob/config-option/rfcs/0042-config-option.md
by @infinisil for reference.

That sounds about right and way more sustainable in the long run indeed.

Thanks for the pointer, I understand the current limitations. I'm not
sure i understood all the points described in the document referred to
though (i'd need another pass ;).

In any case, one remark in the current context, the mediaDirs option
could be dropped altogether. That'd simplify a lot the derivation
(well the let configFile definition at least).

To demonstrate that this specific option can be repeated, i suppose
adding 2 media dirs in the literal examples would help.

I imagine you kept it for backward compabibility ;)

aanderse · 2019-03-24T15:58:36Z

I specifically kept mediaDirs because the service won't effectively operate without specifying media directories. Generally speaking any option for which there cannot be a default value and the user must specify for the service to run probably deserves a NixOS option with documentation.

I like to see the new config option as something which usually shouldn't be required to get a service up and running in a usable state.

The loglevel was kept for backwards compatibility, though. In a release or two it should probably be removed.

ardumont · 2019-03-24T16:23:50Z

Right, that sounds like sane choices indeed.

aanderse · 2019-03-25T00:51:48Z

@infinisil do you feel like reviewing master...aanderse:minidlna as an alternative to the code in this PR? I'd like to hear your thoughts on the proposed code because the goal of this code is to spread the word of your new RFC 😄

Bonus points if you write something really cool to replace my horrible toKeyValue implementation 😉

rycee · 2019-04-07T21:24:30Z

nixos/modules/services/networking/minidlna.nix

+    };
+
+    services.minidlna.extraConfig = mkOption {
+      type = types.str;


types.lines?

This seems to be fine with types.lines as well, fixing.

rycee · 2019-04-07T21:26:53Z

nixos/modules/services/networking/minidlna.nix

@@ -36,6 +36,36 @@ in
        '';
    };

+    services.minidlna.friendlyName = mkOption {
+      type = types.str;
+      default = "${config.networking.hostName} MiniDLNA";


I think you'll need a defaultText field as well containing something like

defaultText = "$HOSTNAME MiniDLNA"

otherwise the manual would have to be rebuilt for each host.

rycee · 2019-04-07T21:27:29Z

nixos/modules/services/networking/minidlna.nix

+        ''
+            Use a different container as the root of the directory tree presented
+            to clients. The possible values are:
+            ,* "." - standard container


Will this list be formatted OK?

No idea, i pasted as is from minidlna's documentation.
Will simplify this.

matthewbauer · 2019-06-11T01:21:51Z

@GrahamcOfBorg test minidlna

aanderse · 2019-08-05T02:15:24Z

ping (triage)

ardumont · 2019-08-05T14:18:40Z

ping (triage)

pong?

;)

bjornfor · 2019-09-15T16:48:08Z

@ardumont: Can you squash fixup commit(s?) and prefix commits with "nixos/minidlna:"?

ardumont · 2019-09-15T18:46:29Z

@ardumont: Can you squash fixup commit(s?) and prefix commits with "nixos/minidlna:"?

done

bjornfor · 2019-09-16T06:01:56Z

nixos/modules/services/networking/minidlna.nix

+          - "P" - "Pictures"
+          - "V" - "Video"
+          - Or, you can specify the ObjectID of your desired root container
+            (eg. 1$F for Music/Playlists)ss


Is the trailing 'ss' supposed to be here?

Looks like typos indeed.

bjornfor · 2019-09-16T06:05:31Z

nixos/tests/minidlna.nix

+      { ... }:
+      {
+        imports = [ ../modules/profiles/minimal.nix ];
+        networking.firewall.allowedTCPPorts = [ 8200 ];


Perhaps the module could use an "openFirewall" option?

According to https://help.ubuntu.com/community/MiniDLNA, we might want to open UDP port 1900 too.

Personally, i use it solely with the tcp port 8200 and it run fine so far ;)
Also, I checked my configuration, and indeed i opened that port myself.

Then again, that was the current behavior so i kept it.
The proposed patch is about allowing more configuration, not behavior change.

Also, i don't really know what's preventing this to be merged already so i'd rather not change the scope just yet ;)

Ok, fair enough. (Although it wouldn't be behavioural change since openFirewall options (should) default to false in nixpgks.)

This commits allows the user to configure: - more minidlna options - the ones not yet disclosed in nix (extending the existing minimal subset)

ardumont requested a review from infinisil as a code owner March 7, 2019 18:49

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Mar 7, 2019

ardumont force-pushed the minidlna-update-configuration branch from a8fca0e to 3ed89ae Compare March 7, 2019 20:40

ardumont mentioned this pull request Mar 8, 2019

pull-request-template: Add the link on tests documentation? #57075

Closed

aanderse reviewed Mar 9, 2019

View reviewed changes

nixos/tests/minidlna.nix Outdated Show resolved Hide resolved

ardumont force-pushed the minidlna-update-configuration branch from 47878af to 97fede2 Compare March 9, 2019 08:12

aanderse reviewed Mar 9, 2019

View reviewed changes

ardumont commented Mar 9, 2019

View reviewed changes

nixos/tests/minidlna.nix Show resolved Hide resolved

ardumont force-pushed the minidlna-update-configuration branch from 97fede2 to fb51d22 Compare March 9, 2019 16:06

rycee reviewed Apr 7, 2019

View reviewed changes

matthewbauer approved these changes Jun 11, 2019

View reviewed changes

ardumont force-pushed the minidlna-update-configuration branch from df06b8f to 202bfeb Compare September 15, 2019 18:45

ofborg bot added the 10.rebuild-linux: 0 label Sep 15, 2019

ofborg bot removed the 10.rebuild-linux: 1-10 label Sep 15, 2019

bjornfor reviewed Sep 16, 2019

View reviewed changes

nixos/minidlna: Allow more configuration options

d228a2d

This commits allows the user to configure: - more minidlna options - the ones not yet disclosed in nix (extending the existing minimal subset)

ardumont force-pushed the minidlna-update-configuration branch from 202bfeb to d228a2d Compare September 16, 2019 07:50

bjornfor approved these changes Sep 17, 2019

View reviewed changes

bjornfor merged commit 35fe503 into NixOS:master Sep 17, 2019

ardumont deleted the minidlna-update-configuration branch September 18, 2019 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minidlna: Allow more configuration options #57036

Minidlna: Allow more configuration options #57036

ardumont commented Mar 7, 2019 •

edited

ardumont commented Mar 7, 2019

aanderse commented Mar 9, 2019

aanderse Mar 9, 2019

ardumont Mar 9, 2019

ardumont Mar 9, 2019

aanderse commented Mar 24, 2019

ardumont commented Mar 24, 2019

aanderse commented Mar 24, 2019

ardumont commented Mar 24, 2019

aanderse commented Mar 25, 2019

rycee Apr 7, 2019

ardumont Apr 8, 2019

rycee Apr 7, 2019

ardumont Apr 8, 2019

rycee Apr 7, 2019 •

edited

ardumont Apr 8, 2019

matthewbauer commented Jun 11, 2019

aanderse commented Aug 5, 2019

ardumont commented Aug 5, 2019

bjornfor commented Sep 15, 2019

ardumont commented Sep 15, 2019

bjornfor Sep 16, 2019

ardumont Sep 16, 2019

ardumont Sep 16, 2019

bjornfor Sep 16, 2019

ardumont Sep 16, 2019

bjornfor Sep 17, 2019

Minidlna: Allow more configuration options #57036

Minidlna: Allow more configuration options #57036

Conversation

ardumont commented Mar 7, 2019 • edited

Motivation for this change

Things done

ardumont commented Mar 7, 2019

aanderse commented Mar 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanderse commented Mar 24, 2019

ardumont commented Mar 24, 2019

aanderse commented Mar 24, 2019

ardumont commented Mar 24, 2019

aanderse commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rycee Apr 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewbauer commented Jun 11, 2019

aanderse commented Aug 5, 2019

ardumont commented Aug 5, 2019

bjornfor commented Sep 15, 2019

ardumont commented Sep 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardumont commented Mar 7, 2019 •

edited

rycee Apr 7, 2019 •

edited