Sickbeard/Sickgear/Sickrage: Init and module #46607

rembo10 · 2018-09-13T14:23:39Z

Motivation for this change

This is an attempt to unify #46538, #46214 and #46340. Added packages for sickbeard, sickgear and sickrage, and added a nixos module to specify which package to use.
@sterfield: I hope it's ok that I didn't merge your commit - since I made some changes to your package based on comments from @worldofpeace on #46514. I kept you as the maintainer for Sickrage though

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)
Fits CONTRIBUTING.md.

nixos/modules/services/networking/sickbeard.nix

aanderse · 2018-09-13T15:04:59Z

nixos/modules/services/networking/sickbeard.nix

+        description = "Path where to store data files.";
+      };
+      configFile = mkOption {
+        default = "/var/lib/sickbeard/config.ini";


Should be "${dataDir}/config.ini and be sure to add a rec above so you can reference dataDir properly, I believe.

nixos/modules/services/networking/sickbeard.nix

aanderse · 2018-09-13T15:06:14Z

nixos/modules/services/networking/sickbeard.nix

+
+  config = mkIf cfg.enable {
+
+    users.users.sickbeard = {


In the options you allow the user to specify username but don't act on it. If I wanted to run sickgear as my own user I could not. See the user and group creation from here as an example:

https://github.com/NixOS/nixpkgs/blob/980cbff93c20b5dcd4ac63384db84490f5e07a45/nixos/modules/services/misc/redmine.nix

aanderse · 2018-09-13T15:06:28Z

nixos/modules/services/networking/sickbeard.nix

+
+    users.users.sickbeard = {
+          uid = config.ids.uids.sickbeard;
+          group = "sickbeard";


Should be group instead of "sickgear".

aanderse · 2018-09-13T15:06:43Z

nixos/modules/services/networking/sickbeard.nix

+          uid = config.ids.uids.sickbeard;
+          group = "sickbeard";
+          description = "sickbeard user";
+          home = "/var/lib/sickbeard/";


Should be dataDir.

aanderse · 2018-09-13T15:07:12Z

nixos/modules/services/networking/sickbeard.nix

+          createHome = true;
+    };
+
+    users.groups.sickbeard = {


Same comment as user. See group in linked example.

aanderse · 2018-09-13T15:07:30Z

nixos/modules/services/networking/sickbeard.nix

+    };
+
+    systemd.services.sickbeard = {
+        description = "sickbeard server";


According to #45638:

"I don't remember where we have that documented, but service descriptions should be upper-case-first for every word."

rembo10 · 2018-09-13T16:32:31Z

@aanderse : thanks for all the comments. I don't know how I missed the user/group stuff. Anyways can you check the new commits and see if they're alright? I'll make similar changes to the headphones (#46514) module.

aanderse

Looking good to me.

sterfield · 2018-09-13T17:43:01Z

Hey all,

@rembo10 No worries, I agree with you, it's better to have all similar packages behind the same service. I suppose that, as they are all forks of sickbeard, they work the same.

However, I don't understand why we don't leverage Systemd's dynamicUser and all relative options (RuntimeDirectory, StateDirectory, etc…).

I did that in my PR (see https://github.com/NixOS/nixpkgs/pull/46340/files#diff-f653b9eaf598496872612beefd0cb752R32).

The beginning of ids.nix state that:

We only add static uids and gids for services where it is not feasible
to change uids/gids on service start, in example a service with a lot of
files.

And I don't think this is the case here.

What do you think ?

EDIT : formating

aanderse · 2018-09-13T17:50:24Z

However, I don't understand why we don't leverage Systemd's dynamicUser and all relative options (RuntimeDirectory, StateDirectory, etc…).

@sterfield Interesting. Does sickbeard not store download files under the directory specified? How would that work with DynamicUser?

I have transmission running on my system and since there is a dedicated user I can add the transmission group to my own user accounts extra groups so I have access to the downloaded files.

sterfield · 2018-09-13T17:56:31Z

Well, I looked at systemd's documentation page and found lots of interesting stuff.

Here, dynamicUser will not help, as Sickbeard/Sickrage/whatever searches for a corresponding line in /etc/passwd at boot time, but we can only declare a simple user and let the system decides the uid/gid. There's no point (IMHO) to force a specific uid/gid. I did the code in my PR (https://github.com/NixOS/nixpkgs/pull/46340/files#diff-f653b9eaf598496872612beefd0cb752R39) (actually, I declared only sickrage as a user and nogroup as a group. We could create an option to create a group if there's a specific need.)

As for the RuntimeDirectory, it's by default under /var/lib/sickrage which is the correct place. The only drawback I can see is the user cannot specify a data directory, but I don't think it's a problem, as it's only the place where the config file and the db is stored.

And obviously, RuntimeDirectory is kept between services's runs.

EDIT : formatting and some clarification.

sterfield · 2018-09-13T17:59:19Z

@sterfield Interesting. Does sickbeard not store download files under the directory specified? How would that work with DynamicUser?

As far as I can remember, no. Sickbeard is storing its db in the data path, but download folders are then added after by the user.

rembo10 · 2018-09-13T18:01:27Z

I think it's useful to give them dedicated ids since they're moving files around. For example, I add sickbeard to the 'storage' group, which has write access to a mounted drive

sterfield · 2018-09-13T18:03:09Z

I think it's useful to give them dedicated ids since they're moving files around. For example, I add sickbeard to the 'storage' group, which has write access to a mounted drive

But an non-dedicated id for sickrage / sickbeard will allow you to do that, no ?

rembo10 · 2018-09-13T18:19:01Z

Ah would it? Ok, sorry I think maybe I misunderstood. I'm happy to make the change as long as it'll work with my setup 😂😅

sterfield · 2018-09-13T18:50:04Z

It has to be tested, but from my point of view:

a sickbeard user is created along with the service
you declare sickbeard as member of your storage group
you declare your mountpoint with the storage group

and that should be it.

One additional option we could create is a list of group user sickbeard belongs to. By default, we keep nogroup but if the user add some groups, then we add sickbeard to those groups.

rembo10 · 2018-09-13T19:10:36Z

I'm still doing a bit of research but I stumbled across this comment that seems to suggest that it won't work for my use case (and I imagine many others') as these services are usually used in conjunction with transmission, sabnznd, etc. Sorry - I'm still trying to wrap my head around all of this so I might be missing something totally obvious. Reading the systemd link above, it seems like you can pass a list of groups to run the service as, but do they also have access to directories outside of /var/lib/service? Or would those directories need to be mounted?

sterfield · 2018-09-14T09:10:28Z

That's what I mentioned in one of my comment:

dynamicUser can be used, but you don't have a "real" user, only created by systemd at service boot, and deleted at service stop. I can be used here, but Sickbeard is checking user's password in /etc/passwd, which doesn't exist in that case and the application crashes (I tested it with Sickrage, so I assume that it's the same for other forks)
runtimeDirectory can be used here, but as stated in the comment you linked, the directory is created under /var/lib/<some directory> and you don't have the choice for the location.
creation of a specific user using Nixos user option creates a regular user.

So we can do three things here:

creation of a user without fixed uid/gid and use of runtimeDirectory. We don't know the uid/gid chosen by Nixos and the datadir will be handled by systemd under /var/lib/sickbeard (for example).
- Advantage: don't have to manage the datadir yourself, and let nixos decides the uid/gid. ids.nix will be less cluttered.
- Disadvantage: you don't have a fixed uid/gid, but I don't see any impact here. The datadir is also fixed, preventing the user to chose his own datadir. That's fine by me, as I don't change its content manually, but it could be an issue for some user that wants a specific folder or to use a specific config.ini
creation of a user without fixed uid/gid and creation of the datadir by Nixos install phase:
- Advantage: datadir can be customized
- Disadvantage: more things to manage at Nixos level
creation of a user with fixed uid/gid and creation of the datadir by Nixos install phase:
- Advantage: fixed uid and datadir can be customized
- Disadvantage: more things to manage at Nixos level and ids.nix will have more ids declared

So ultimately, there's no big differences.

if we want to customize datadir, then let's do it, but if there's no added value, we can let systemd handle this.
we can declare a specific uid/gid for sickbeard user, but I don't see any added value here.

rembo10 · 2018-09-14T15:13:53Z

I'm not sure I know enough about this to make the decision as I'm quite new to nix / nixos so I'll defer to your guys' judgement on this

aanderse · 2018-09-14T15:23:43Z

My opinion is that there can be a use case for specifying both username and group for services like this, so personally I would go with the third option which you currently have implemented.

I'll also add that I'm no authority though. :-)

sterfield · 2018-09-15T18:08:03Z

I still don't understand why you want to fix uid/gid in ids.nix instead of using user option and let the system decide the uid/gid itself.

But the code looks good to me, so if you want to merge it like that, then go for it !

sterfield · 2018-09-15T18:12:28Z

nixos/modules/services/networking/sickbeard.nix

@@ -0,0 +1,92 @@
+{ config, lib, pkgs, ... }:


I don't think the nixos module belongs here. Sonarr, couchpotato modules are stored under nixos/modules/services/misc so I think sickbeard should be put there as well.

I think your right. Fixed.

rembo10 · 2018-09-16T19:56:17Z

I'll leave it up to you guys to decide about the id situation. Just let me know and I can make changes

aanderse · 2018-09-16T20:29:05Z

@sterfield Just reread your options... Is the only difference between option 2 and 3 the predefined ids?

If so I think I'm indifferent between the two. I still think there would be value in allowing the user to specify the user and group the service runs as, but I don't know enough about sickbeard to say.

It sounds like @rembo10 and I are leaving the decision to you.

sterfield · 2018-09-16T21:02:04Z

So, I'm all for letting the user choose which user / group to handle the service. The code you did is really cool. The only concern I have is the use of predefined ids, where I don't see really the need here. In addition, some comments in the file mentioned to not go above uid 399, and we are getting close to that.

So here's my thoughts:

remove the predefined ids from the current code and we push the code like that.
in the meantime, I'll open a thread on discourse to understand how predefined ids are working, in which situation they are useful and in which they are not. If the content of the thread indicates that predefined ids are useful in our current situation. then I'll open up a second PR to rectify this.

What do you think ?

aanderse · 2018-09-16T21:12:11Z

Sounds good to me.

sterfield · 2018-09-16T21:16:10Z

Here's the thread on discourse : https://discourse.nixos.org/t/predefined-uids-gids-when-to-use-them-and-when-not-to-use-them/956?u=sterfield

sterfield · 2018-09-17T07:16:40Z

I thought about it yesterday, and I found one use case where fixing the uid/gid could be beneficial.
Let's say you have a server running Nixos and a separate storage disk, where you store multiple files. Now, you are using an application that writes thousand of files under id, let's say 155. If you reinstall your server, it'll recreate the user for the application, but perhaps with a different uid. Now you have the same application, same user that cannot do anything on the storage disk, as all the files are owned by uid 155 that may not even exists on the server.

Predefined uid prevents this situation. Nothing that a good find can fix, but still.

You know what ? Merge this. I'll find the answers of my questions and will stop bothering you guys with that. After all, I'm the one having questions 😄

matthewbauer · 2018-09-29T18:26:04Z

@GrahamcOfBorg build sickbeard sickgear sickrage

GrahamcOfBorg · 2018-09-29T18:26:34Z

Success on aarch64-linux (full log)

Attempted: sickbeard, sickgear, sickrage

Partial log (click to expand)

patching script interpreter paths in /nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1
/nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1/SickBeard.py: interpreter directive changed from "/usr/bin/env python2.7" to "/nix/store/bp1c65zbbw6jfjws77pbgw48mjfmk9x3-python-2.7.15/bin/python2.7"
/nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1/lib/rarfile/dumprar.py: interpreter directive changed from " /usr/bin/env python" to "/nix/store/bp1c65zbbw6jfjws77pbgw48mjfmk9x3-python-2.7.15/bin/python"
/nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1/lib/pbr/tests/testpackage/setup.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/bp1c65zbbw6jfjws77pbgw48mjfmk9x3-python-2.7.15/bin/python"
/nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1/lib/imdb/locale/rebuildmo.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/bp1c65zbbw6jfjws77pbgw48mjfmk9x3-python-2.7.15/bin/python"
/nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1/lib/imdb/locale/generatepot.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/bp1c65zbbw6jfjws77pbgw48mjfmk9x3-python-2.7.15/bin/python"
checking for references to /build in /nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1...
/nix/store/0gmsl7zgq8kckcvcnkpvhs0rsvf134i3-sickbeard-2016-03-21
/nix/store/6r7j25hqqx2425kdsgvk5p2cy2865bm3-sickgear-0.17.5
/nix/store/izrqznjjm8iylplb1d4qdbhv8l3gb8zn-sickrage-v2018.07.21-1

GrahamcOfBorg · 2018-09-29T18:26:45Z

Success on x86_64-darwin (full log)

Attempted: sickbeard, sickgear, sickrage

Partial log (click to expand)

stripping (with command strip and flags -S) in /nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1/lib  /nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1/bin
patching script interpreter paths in /nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1
/nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1/lib/imdb/locale/generatepot.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/n1l7rs42a8idw195kqsw0slsg1pbmh17-python-2.7.15/bin/python"
/nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1/lib/imdb/locale/rebuildmo.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/n1l7rs42a8idw195kqsw0slsg1pbmh17-python-2.7.15/bin/python"
/nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1/lib/pbr/tests/testpackage/setup.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/n1l7rs42a8idw195kqsw0slsg1pbmh17-python-2.7.15/bin/python"
/nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1/lib/rarfile/dumprar.py: interpreter directive changed from " /usr/bin/env python" to "/nix/store/n1l7rs42a8idw195kqsw0slsg1pbmh17-python-2.7.15/bin/python"
/nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1/SickBeard.py: interpreter directive changed from "/usr/bin/env python2.7" to "/nix/store/n1l7rs42a8idw195kqsw0slsg1pbmh17-python-2.7.15/bin/python2.7"
/nix/store/p5k8lvkiag4jmr23z03d8hhc0g7305y2-sickbeard-2016-03-21
/nix/store/1p62mr4rm5pgbaa0lvkzmn3yrqrq2w16-sickgear-0.17.5
/nix/store/jab2gic73hlllka71b37r9cqazfp47z5-sickrage-v2018.07.21-1

GrahamcOfBorg · 2018-09-29T18:27:30Z

Success on x86_64-linux (full log)

Attempted: sickbeard, sickgear, sickrage

Partial log (click to expand)

patching script interpreter paths in /nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1
/nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1/lib/pbr/tests/testpackage/setup.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/r5x2dq8ansjrlfcf37qd7x8lp6xwwvm4-python-2.7.15/bin/python"
/nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1/lib/rarfile/dumprar.py: interpreter directive changed from " /usr/bin/env python" to "/nix/store/r5x2dq8ansjrlfcf37qd7x8lp6xwwvm4-python-2.7.15/bin/python"
/nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1/lib/imdb/locale/rebuildmo.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/r5x2dq8ansjrlfcf37qd7x8lp6xwwvm4-python-2.7.15/bin/python"
/nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1/lib/imdb/locale/generatepot.py: interpreter directive changed from "/usr/bin/env python" to "/nix/store/r5x2dq8ansjrlfcf37qd7x8lp6xwwvm4-python-2.7.15/bin/python"
/nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1/SickBeard.py: interpreter directive changed from "/usr/bin/env python2.7" to "/nix/store/r5x2dq8ansjrlfcf37qd7x8lp6xwwvm4-python-2.7.15/bin/python2.7"
checking for references to /build in /nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1...
/nix/store/i7wvyniv9vp8dkvxwnrp2zz8x98fnfnm-sickbeard-2016-03-21
/nix/store/dhz6fswkyh6w9h0wdwlbr02qrv0s3nqp-sickgear-0.17.5
/nix/store/rffb9g22riqy4sm72v0c1zb4hgap4nsw-sickrage-v2018.07.21-1

rembo10 · 2018-09-29T19:26:56Z

Thanks!

rembo10 mentioned this pull request Sep 13, 2018

sickgear: init at 0.17.5 #46538

Closed

9 tasks

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Sep 13, 2018

This was referenced Sep 13, 2018

sickrage: init at v2018.07.21-1 #46214

Closed

Sickrage: nixos module #46340

Closed

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Sep 13, 2018

aanderse requested changes Sep 13, 2018

View reviewed changes

rembo10 force-pushed the sickbeard branch from 3ec962d to 2fc66c0 Compare September 13, 2018 16:31

aanderse approved these changes Sep 13, 2018

View reviewed changes

rembo10 added 3 commits September 13, 2018 19:11

sickbeard: init at 2016-03-21

437bcdf

sickgear: init at 0.17.5

4a2b028

sickrage: init at v2018.07.21-1

6fb62ac

rembo10 mentioned this pull request Sep 13, 2018

headphones: init at 0.5.19 #46514

Merged

9 tasks

sterfield suggested changes Sep 15, 2018

View reviewed changes

Add sickbeard module (with SickGear & SickRage)

f1c9d5c

rembo10 force-pushed the sickbeard branch from 2fc66c0 to f1c9d5c Compare September 16, 2018 19:54

sterfield approved these changes Sep 16, 2018

View reviewed changes

matthewbauer merged commit 21c26ca into NixOS:master Sep 29, 2018

infinisil mentioned this pull request Nov 16, 2018

NixOS services: When to have a package option #50476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sickbeard/Sickgear/Sickrage: Init and module #46607

Sickbeard/Sickgear/Sickrage: Init and module #46607

rembo10 commented Sep 13, 2018

aanderse Sep 13, 2018

aanderse Sep 13, 2018

aanderse Sep 13, 2018

aanderse Sep 13, 2018

aanderse Sep 13, 2018

aanderse Sep 13, 2018

rembo10 commented Sep 13, 2018

aanderse left a comment

sterfield commented Sep 13, 2018 •

edited

aanderse commented Sep 13, 2018

sterfield commented Sep 13, 2018 •

edited

sterfield commented Sep 13, 2018

rembo10 commented Sep 13, 2018

sterfield commented Sep 13, 2018

rembo10 commented Sep 13, 2018

sterfield commented Sep 13, 2018

rembo10 commented Sep 13, 2018

sterfield commented Sep 14, 2018

rembo10 commented Sep 14, 2018

aanderse commented Sep 14, 2018

sterfield commented Sep 15, 2018 •

edited

sterfield Sep 15, 2018

rembo10 Sep 16, 2018

rembo10 commented Sep 16, 2018

aanderse commented Sep 16, 2018

sterfield commented Sep 16, 2018 •

edited

aanderse commented Sep 16, 2018

sterfield commented Sep 16, 2018

sterfield commented Sep 17, 2018

matthewbauer commented Sep 29, 2018

GrahamcOfBorg commented Sep 29, 2018

GrahamcOfBorg commented Sep 29, 2018

GrahamcOfBorg commented Sep 29, 2018

rembo10 commented Sep 29, 2018

Sickbeard/Sickgear/Sickrage: Init and module #46607

Sickbeard/Sickgear/Sickrage: Init and module #46607

Conversation

rembo10 commented Sep 13, 2018

Motivation for this change

Things done

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

rembo10 commented Sep 13, 2018

aanderse left a comment

Choose a reason for hiding this comment

sterfield commented Sep 13, 2018 • edited

aanderse commented Sep 13, 2018

sterfield commented Sep 13, 2018 • edited

sterfield commented Sep 13, 2018

rembo10 commented Sep 13, 2018

sterfield commented Sep 13, 2018

rembo10 commented Sep 13, 2018

sterfield commented Sep 13, 2018

rembo10 commented Sep 13, 2018

sterfield commented Sep 14, 2018

rembo10 commented Sep 14, 2018

aanderse commented Sep 14, 2018

sterfield commented Sep 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rembo10 commented Sep 16, 2018

aanderse commented Sep 16, 2018

sterfield commented Sep 16, 2018 • edited

aanderse commented Sep 16, 2018

sterfield commented Sep 16, 2018

sterfield commented Sep 17, 2018

matthewbauer commented Sep 29, 2018

GrahamcOfBorg commented Sep 29, 2018

GrahamcOfBorg commented Sep 29, 2018

GrahamcOfBorg commented Sep 29, 2018

rembo10 commented Sep 29, 2018

sterfield commented Sep 13, 2018 •

edited

sterfield commented Sep 13, 2018 •

edited

sterfield commented Sep 15, 2018 •

edited

sterfield commented Sep 16, 2018 •

edited