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.

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

nixos/darkhttpd: module to enable darkhttpd #48036

Merged
merged 1 commit into from Aug 26, 2019

Conversation

@peterhoeg
Copy link
Member

commented Oct 8, 2018

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.

@peterhoeg peterhoeg self-assigned this Oct 8, 2018
@Mic92 Mic92 changed the title nixos darkhttpd: module to enable darkhttpd nixos/darkhttpd: module to enable darkhttpd Oct 8, 2018
Copy link
Member

left a comment

LGTM otherwise.

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

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

This comment has been minimized.

Copy link
@yegortimoshenko

yegortimoshenko Oct 13, 2018

Member

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

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Oct 14, 2018

Author Member

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

nixos/modules/services/web-servers/darkhttpd.nix Outdated
wantedBy = [ "multi-user.target" ];
serviceConfig = {
DynamicUser = true;
ExecStart = "${pkgs.darkhttpd}/bin/darkhttpd ${optionsToArgs}";

This comment has been minimized.

Copy link
@yegortimoshenko

yegortimoshenko Oct 13, 2018

Member

Would be nice to have cfg.package.

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Oct 14, 2018

Author Member

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

This comment has been minimized.

Copy link
@yegortimoshenko

yegortimoshenko Oct 17, 2018

Member

Probably more of a pattern.

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

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

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 26, 2018

Member

Can use ints.u16 now

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Oct 29, 2018

Author Member

Any particular reason other than shaving off 2 bytes?

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Oct 29, 2018

Author Member

6 bytes.... 🤦‍♂

This comment has been minimized.

Copy link
@yegortimoshenko

yegortimoshenko Oct 29, 2018

Member

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

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Oct 29, 2018

Author Member

👍

nixos/modules/services/web-servers/darkhttpd.nix Outdated
let
cfg = config.services.darkhttpd;

optionsToArgs = concatStringsSep " " ([

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 26, 2018

Member

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.

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

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 26, 2018

Member

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

nixos/modules/services/web-servers/darkhttpd.nix Outdated
'';
};

extraConfig = mkOption {

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 26, 2018

Member

Should be named extraArgs

@peterhoeg peterhoeg force-pushed the peterhoeg:m/darkhttp branch to 4a8ccc5 Oct 29, 2018
@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

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


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

This comment has been minimized.

Copy link
@Infinisil

Infinisil Oct 29, 2018

Member

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.

This comment has been minimized.

Copy link
@yegortimoshenko

yegortimoshenko Oct 29, 2018

Member

Also see #48289 (comment).

This comment has been minimized.

Copy link
@Infinisil
@Infinisil

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

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

@mmahut

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Are there any updates on this pull request, please?

@peterhoeg peterhoeg force-pushed the peterhoeg:m/darkhttp branch from 4a8ccc5 to fd5f85c Aug 26, 2019
@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@peterhoeg peterhoeg force-pushed the peterhoeg:m/darkhttp branch from fd5f85c to c876aff Aug 26, 2019
@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

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

@peterhoeg peterhoeg merged commit a859d0c into NixOS:master Aug 26, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@peterhoeg peterhoeg deleted the peterhoeg:m/darkhttp branch Aug 26, 2019
@peterhoeg peterhoeg restored the peterhoeg:m/darkhttp branch Aug 26, 2019
@peterhoeg peterhoeg deleted the peterhoeg:m/darkhttp branch Aug 27, 2019
wantedBy = [ "multi-user.target" ];
serviceConfig = {
DynamicUser = true;
ExecStart = "${cfg.package}/bin/darkhttpd ${args}";

This comment has been minimized.

Copy link
@Infinisil

Infinisil Aug 27, 2019

Member

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

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Aug 28, 2019

Author Member

I'll fix this and add a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.