nixos/zeronet: init #44842

Chiiruno · 2018-08-09T20:59:03Z

Motivation for this change

Simple service for zeronet, adjusted the package itself to make things easier.

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.

Chiiruno · 2018-08-09T20:59:13Z

cc @fgaz

nixos/modules/services/networking/zeronet.nix

Mic92 · 2018-08-10T12:41:22Z

nixos/modules/services/networking/zeronet.nix

+        # Ensure folder exists or create it and permissions are correct
+        mkdir -p ${escapeShellArg cfg.dataDir} ${escapeShellArg cfg.logDir}
+        chmod 750 ${escapeShellArg cfg.dataDir} ${escapeShellArg cfg.logDir}
+        chown -R zeronet:zeronet ${escapeShellArg cfg.dataDir} ${escapeShellArg cfg.logDir}


If on startup all files are chowned, static uid/gids in nixos/modules/misc/ids.nix are not necessary.

I'm confused, this was fine on my hydron and meguca nix expressions?
Is there something that zeronet does differently?

Not every reviewer knows about everything. I think ids.nix should be avoided if possible, and apparently it's indeed not needed with this.

It can now be removed in meguca as well. Back then when the static numbers where allocated it did not chown everything as this was added later: 36ab899
As meguca might have a large number of image files this could potentially slow down the startup, which why having a static uid/gid would be fine to avoid having to adapt ownership on service start.

Wait, so then we do need static uid/gids for zeronet?
If I remember right, zeronet should, at least over time, store at least a few gigabytes, over a lot of files(?).

Oh, I see what you mean now.

pkgs/applications/networking/p2p/zeronet/default.nix

fgaz · 2018-08-14T07:37:25Z

pkgs/applications/networking/p2p/zeronet/default.nix

+
+    makeWrapper "$out/share/zeronet.py" "$out/bin/zeronet_service" \
+      --set PYTHONPATH "$PYTHONPATH" \
+      --set PATH ${python2Packages.python}/bin


I think you can just use the first wrapper. If you pass a second set of --log_dir and --data_dir, it will override the first.

That's already defined in the conf this service generates.
Better for ease-of-use and less possible problems.

Chiiruno · 2018-08-24T00:09:47Z

@Mic92 Does anything need to be changed still?

infinisil

https://github.com/NixOS/nixpkgs/pull/44842/files#r212492686

Chiiruno · 2018-08-24T00:50:39Z

@infinisil All good?

infinisil · 2018-08-24T14:25:04Z

nixos/modules/services/networking/zeronet.nix

+      type = types.path;
+      default = "/var/log/zeronet";
+      example = "/home/okina/zeronet/log";
+      description = "Path to the zeronet log directory.";


Is it not possible to make it use the systemd journal?

We can do this with zeronet?
Is it just a matter of moving the default value to $whatever/systemd/log?

stdout/stderr should work automatically. If it's not possible to tell it to output like that, /proc/self/fd/{1,2} might work

https://github.com/HelloZeroNet/ZeroNet/blob/d677eb1698d55f878801bc2751bb6fa93473e08c/zeronet.py#L53

Does not seems like it can at the moment. It also does not do log rotation.

Is there anything we can do here, or should I mark this as resolved?

infinisil · 2018-08-24T23:21:33Z

pkgs/applications/networking/p2p/zeronet/default.nix

+      --set PYTHONPATH "$PYTHONPATH" \
+      --set PATH ${python2Packages.python}/bin \
+      --add-flags "--log_dir \$HOME/.local/share/zeronet/logs" \
+      --add-flags "--data_dir \$HOME/.local/share/zeronet"


I don't think this binary should have been wrapped like this in the first place. Nix packages shouldn't pass convenience arguments, that's up to the user. If anything I'd argue to remove these arguments for the main binary and not have the zeronet_service wrapper, makes much more sense.

Unfortunately, zeronet will not start without log_dir and data_dir set, so I think I understand why @fgaz made this a wrapper. Not sure about PYTHONPATH or PATH, though, but I remember having problems calling the .py directly without the wrapper.

Nix packages don't need to provide a binary that runs without any arguments, they just need to provide the binary that gets distributed, optionally wrapped with some env vars to make it work under Nix. Passing the right arguments will be the job of either the user or some wrapper higher up.

Okay, I'll see if I can't get rid of the wrappers altogether, but I may still have to wrap the PYTHONPATH and maaaaaybe the PATH. If log_dir and data_dir aren't a problem, then I can easily get rid of the zeronet_service wrapper too.

Chiiruno · 2018-08-25T00:26:00Z

@infinisil Okay, that should do it.
I'll need to do the same for my hydron and meguca package whenever I update them.

nixos/modules/services/networking/zeronet.nix

Chiiruno · 2018-08-30T04:30:58Z

@infinisil Are we all good?
Thoughts on my question above?

Chiiruno · 2018-08-30T07:32:26Z

Okay, ready for review.

infinisil · 2018-08-30T13:25:01Z

nixos/modules/services/networking/zeronet.nix

+      description = "zeronet";
+      after = [ "network.target" (optionalString cfg.tor "tor.service") ];
+      wantedBy = [ "multi-user.target" ];
+      restartTriggers = [ zConfFile ];


Ah, this isn't needed when you reference it in ExecStart already

Chiiruno · 2018-08-30T21:10:17Z

@infinisil All good?

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 9, 2018

Mic92 reviewed Aug 10, 2018

View reviewed changes

nixos/modules/services/networking/zeronet.nix Show resolved Hide resolved

Mic92 reviewed Aug 10, 2018

View reviewed changes

nixos/modules/services/networking/zeronet.nix Show resolved Hide resolved

Mic92 reviewed Aug 10, 2018

View reviewed changes

nixos/modules/services/networking/zeronet.nix Show resolved Hide resolved

Mic92 reviewed Aug 10, 2018

View reviewed changes

Chiiruno force-pushed the dev/zeronet branch from b6e86c8 to 6ee0307 Compare August 10, 2018 23:40

fgaz reviewed Aug 14, 2018

View reviewed changes

infinisil requested changes Aug 24, 2018

View reviewed changes

Chiiruno force-pushed the dev/zeronet branch from 6ee0307 to c8249d0 Compare August 24, 2018 00:50

infinisil reviewed Aug 24, 2018

View reviewed changes

Chiiruno force-pushed the dev/zeronet branch from c8249d0 to f159b27 Compare August 25, 2018 00:25

Chiiruno commented Aug 25, 2018

View reviewed changes

nixos/modules/services/networking/zeronet.nix Show resolved Hide resolved

Chiiruno force-pushed the dev/zeronet branch from f159b27 to 52e1f44 Compare August 30, 2018 07:30

infinisil reviewed Aug 30, 2018

View reviewed changes

Madouura added 2 commits August 30, 2018 16:09

nixos/zeronet: init

9151f69

zeronet: Adjust for service

41f46f7

Chiiruno force-pushed the dev/zeronet branch from 52e1f44 to 41f46f7 Compare August 30, 2018 21:09

Mic92 merged commit 17564e0 into NixOS:master Aug 31, 2018

Chiiruno deleted the dev/zeronet branch August 31, 2018 20:12

Chiiruno mentioned this pull request Oct 21, 2018

Package zeronet as a service #31566

Closed

aanderse mentioned this pull request Apr 22, 2019

Add Jellyfin module #59465

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/zeronet: init #44842

nixos/zeronet: init #44842

Chiiruno commented Aug 9, 2018 •

edited

Chiiruno commented Aug 9, 2018

Mic92 Aug 10, 2018

Chiiruno Aug 10, 2018

infinisil Aug 24, 2018

Mic92 Aug 24, 2018

Chiiruno Aug 24, 2018

Chiiruno Aug 24, 2018

fgaz Aug 14, 2018

Chiiruno Aug 14, 2018

Chiiruno commented Aug 24, 2018

infinisil left a comment

Chiiruno commented Aug 24, 2018

infinisil Aug 24, 2018

Chiiruno Aug 24, 2018

infinisil Aug 24, 2018

Mic92 Aug 24, 2018

Mic92 Aug 24, 2018

Chiiruno Aug 30, 2018

infinisil Aug 24, 2018

Chiiruno Aug 24, 2018

infinisil Aug 24, 2018

Chiiruno Aug 24, 2018

Chiiruno commented Aug 25, 2018

Chiiruno commented Aug 30, 2018

Chiiruno commented Aug 30, 2018

infinisil Aug 30, 2018

Chiiruno commented Aug 30, 2018

nixos/zeronet: init #44842

nixos/zeronet: init #44842

Conversation

Chiiruno commented Aug 9, 2018 • edited

Motivation for this change

Things done

Chiiruno commented Aug 9, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chiiruno commented Aug 24, 2018

infinisil left a comment

Choose a reason for hiding this comment

Chiiruno commented Aug 24, 2018

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

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

Chiiruno commented Aug 25, 2018

Chiiruno commented Aug 30, 2018

Chiiruno commented Aug 30, 2018

Choose a reason for hiding this comment

Chiiruno commented Aug 30, 2018

Chiiruno commented Aug 9, 2018 •

edited