nixos/darkhttpd: module to enable darkhttpd #48036

peterhoeg · 2018-10-08T08:55:33Z

Motivation for this change

I've had this one sitting around for quite a while.

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.

lukateras

LGTM otherwise.

lukateras · 2018-10-13T16:03:34Z

nixos/modules/services/web-servers/darkhttpd.nix

+
+    extraConfig = mkOption {
+      type = listOf str;
+      default = [ "" ];


Is that empty string necessary, would it work if I pass services.darkhttpd.extraConfig = [];?

You're right, should just be an empty list.

lukateras · 2018-10-13T16:03:51Z

nixos/modules/services/web-servers/darkhttpd.nix

+      wantedBy = [ "multi-user.target" ];
+      serviceConfig = {
+        DynamicUser = true;
+        ExecStart = "${pkgs.darkhttpd}/bin/darkhttpd ${optionsToArgs}";


Would be nice to have cfg.package.

Do you realistically see the darkhttpd package being overridden? Or is it more of a "best practice pattern"?

Probably more of a pattern.

infinisil · 2018-10-26T22:38:41Z

nixos/modules/services/web-servers/darkhttpd.nix

+
+    port = mkOption {
+      default = 80;
+      type = int;


Can use ints.u16 now

Any particular reason other than shaving off 2 bytes?

6 bytes.... 🤦‍♂️

It only allows valid port values, as opposed to int.

infinisil · 2018-10-26T22:39:31Z

nixos/modules/services/web-servers/darkhttpd.nix

+let
+  cfg = config.services.darkhttpd;
+
+  optionsToArgs = concatStringsSep " " ([


A little bikeshedding on the naming: optionsToArgs would imply it taking an argument, but really it's just the value of the arguments, so I'd go for just args instead.

infinisil · 2018-10-26T22:41:06Z

nixos/modules/services/web-servers/darkhttpd.nix

+      serviceConfig = {
+        DynamicUser = true;
+        ExecStart = "${pkgs.darkhttpd}/bin/darkhttpd ${optionsToArgs}";
+        AmbientCapabilities = lib.mkIf (cfg.port < 1024) [ "CAP_NET_BIND_SERVICE" ];


Neat, would probably be nice to have this automatically done in the systemd module at some point.

infinisil · 2018-10-26T22:42:19Z

nixos/modules/services/web-servers/darkhttpd.nix

+      '';
+    };
+
+    extraConfig = mkOption {


Should be named extraArgs

peterhoeg · 2018-10-29T08:53:28Z

@infinisil , you're good with the current state?

infinisil · 2018-10-29T09:33:59Z

nixos/modules/services/web-servers/darkhttpd.nix

+
+    package = mkOption {
+      default = pkgs.darkhttpd;
+      type = package;


I'd actually rather not have this option. In general, the package can already be overridden with overlays, whose only disadvantage is that you can't override the package only for this module, it always happens for the whole system. This can be a problem when you e.g. want to override pkgs.curl, which has lots of dependents, so it would rebuild a whole lot, which doesn't apply to darkhttpd.

Every option increases evaluation time, so I'd rather have this be used for options that allow you to do something you couldn't otherwise do.

Also see #48289 (comment).

Also #49228 (comment)

infinisil · 2018-11-10T14:16:19Z

Ping? Other than the thing mentioned above, this looks good to me

mmahut · 2019-08-10T12:57:05Z

Are there any updates on this pull request, please?

peterhoeg · 2019-08-26T06:58:53Z

@GrahamcOfBorg eval

peterhoeg · 2019-08-26T11:58:27Z

@GrahamcOfBorg eval

peterhoeg · 2019-08-26T12:38:04Z

I ripped out the package option as the jury is still out on that. The rest should be fine.

infinisil · 2019-08-27T12:39:21Z

nixos/modules/services/web-servers/darkhttpd.nix

+      wantedBy = [ "multi-user.target" ];
+      serviceConfig = {
+        DynamicUser = true;
+        ExecStart = "${cfg.package}/bin/darkhttpd ${args}";


If you remove the package option, you also need to change this :)

I'll fix this and add a test.

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

peterhoeg self-assigned this Oct 8, 2018

Mic92 changed the title ~~nixos darkhttpd: module to enable darkhttpd~~ nixos/darkhttpd: module to enable darkhttpd Oct 8, 2018

lukateras approved these changes Oct 13, 2018

View reviewed changes

infinisil requested changes Oct 26, 2018

View reviewed changes

peterhoeg force-pushed the m/darkhttp branch from 6294705 to 4a8ccc5 Compare October 29, 2018 03:08

infinisil requested changes Oct 29, 2018

View reviewed changes

infinisil mentioned this pull request Nov 16, 2018

NixOS services: When to have a package option #50476

Closed

peterhoeg force-pushed the m/darkhttp branch from 4a8ccc5 to fd5f85c Compare August 26, 2019 06:53

nixos darkhttpd: module to enable darkhttpd

c876aff

peterhoeg force-pushed the m/darkhttp branch from fd5f85c to c876aff Compare August 26, 2019 11:58

peterhoeg merged commit a859d0c into NixOS:master Aug 26, 2019

peterhoeg deleted the m/darkhttp branch August 26, 2019 12:38

peterhoeg restored the m/darkhttp branch August 26, 2019 14:26

peterhoeg deleted the m/darkhttp branch August 27, 2019 02:23

infinisil reviewed Aug 27, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/darkhttpd: module to enable darkhttpd #48036

nixos/darkhttpd: module to enable darkhttpd #48036

peterhoeg commented Oct 8, 2018

lukateras left a comment

lukateras Oct 13, 2018

peterhoeg Oct 14, 2018

lukateras Oct 13, 2018

peterhoeg Oct 14, 2018 •

edited

lukateras Oct 17, 2018

infinisil Oct 26, 2018

peterhoeg Oct 29, 2018

peterhoeg Oct 29, 2018

lukateras Oct 29, 2018

peterhoeg Oct 29, 2018

infinisil Oct 26, 2018

infinisil Oct 26, 2018

infinisil Oct 26, 2018

peterhoeg commented Oct 29, 2018

infinisil Oct 29, 2018

lukateras Oct 29, 2018 •

edited

infinisil Oct 29, 2018

infinisil commented Nov 10, 2018

mmahut commented Aug 10, 2019

peterhoeg commented Aug 26, 2019

peterhoeg commented Aug 26, 2019

peterhoeg commented Aug 26, 2019

infinisil Aug 27, 2019

peterhoeg Aug 28, 2019

nixos/darkhttpd: module to enable darkhttpd #48036

nixos/darkhttpd: module to enable darkhttpd #48036

Conversation

peterhoeg commented Oct 8, 2018

Motivation for this change

Things done

lukateras left a 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

peterhoeg Oct 14, 2018 • edited

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

peterhoeg commented Oct 29, 2018

Choose a reason for hiding this comment

lukateras Oct 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Nov 10, 2018

mmahut commented Aug 10, 2019

peterhoeg commented Aug 26, 2019

peterhoeg commented Aug 26, 2019

peterhoeg commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhoeg Oct 14, 2018 •

edited

lukateras Oct 29, 2018 •

edited