search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.38k stars 14.33k forks source link

NixOS services: When to have a package option #50476

Closed infinisil closed 4 years ago

infinisil commented 6 years ago

Issue description

It has become somewhat of a pattern to add a package option to NixOS service modules for selecting a package to use for the service. Doing a quick count, of the ~600 service modules in nixpkgs about 120 of them have a package option. For comparison, the total amount of options in nixpkgs is about 7000. Every new NixOS option increases evaluation time for all NixOS system evaluations, whether you use it or not. Adding an option just because other services do it doesn't sound sensible. I'd rather add this option only once a need for it arises.

This is an issue to track opinions about this.

Alternative

In general, all packages can already be overridden via overlays. To override the package foo with foo2:

{
  nixpkgs.overlays = [(self: super: {
    foo = self.foo2;
  })];
}

The need for such an option

The 3 uses I see for a package option are:

Related

For reference, I used this to find these:

while true; do firefox $(git blame $(fzf) | rg '([^ ]+) .*package = mkOption' -or 'https://github.com/NixOS/nixpkgs/commit/$1'); done

Related discussions

Justified addings of the option

Commits/PRs that add the option without justification

Mostly these are probably just people not knowing they can use overlays to do it. I'm pinging all of you in the hopes that you see that overlays could have been used instead or to potentially find other reasons to introduce such an option.

domenkozar commented 6 years ago

I'd be happy to discuss this once we see how much impact do these 600 options have on evaluation time :)

infinisil commented 6 years ago

A benchmark sure would be nice, but I'm mainly concerned about option bloat in general, having lots of options that can be achieved another way and aren't used by anybody.

zimbatm commented 6 years ago

The package option is quite convenient, it allows to select a different version without reading the module source or triggering too many rebuilds.

copumpkin commented 6 years ago

My vote is to put it in for all services that have a relevant notion of packages backing them. The main reason for this is that it's not always clear which packages are being used by a service (e.g., we have services.httpd which uses pkgs.apacheHttpd; yes the module lives in services/apache-httpd but in general nothing requires them to line up) so unless someone pokes at the nixpkgs source code, they don't necessarily know which module to override and there's no real feedback to figure out which was overridden other than hoping to see the .service unit get rebuilt if you got it right.

The reasons you give in favor of the field also factor into my preference. I'm already frustrated at how "singleton"-flavored our services all are, so forcing all of them to go through a single registry of versions seems annoying.

infinisil commented 6 years ago

@copumpkin That's a good point, I haven't thought of this.

Eventually I hope that we can have a generic service module creator function that contains stuff like enable, user, group, package, extraArgs, configFile, etc. (with some configurability)

matthewbauer commented 6 years ago

I favor the idea of using overlays here. You should be able to accomplish the same thing as the package option with a simple overlay of pkgs. This works without the extra code of implementing and documenting extra options.

jtojnar commented 6 years ago

Because there are multiple upower versions now: df95a8c

Not any more https://github.com/NixOS/nixpkgs/commit/2a2cb8354ecfada313dcdb36154000c7f7f74964#diff-0bcefda72ac48cd4aa6372226ec3f6a7

7c6f434c commented 6 years ago

A bit more general question: so enable-guarding still pays the cost for the options inside the module? Can this problem be fixed globally? Maybe via some kind of conditional require-management, I am not sure.

Ekleog commented 6 years ago

@7c6f434c See also https://github.com/NixOS/rfcs/pull/22

LnL7 commented 6 years ago

The package option is also useful for overrides or even using a different channel. Having to override the default attribute means there's no choice to do this in a contained way anymore. Even when using overlays I always do this instead.

{
  services.foo.package = pkgs.namespace.foo;
  nixpkgs.overlays = [(self: super: {
     namespace.foo = super.foo.override { withBar = true; };
  })];
}
Ekleog commented 6 years ago

I've tried benchmarking with this commit.

The results are, for a removal of 19 package = mkOption arguments (got bored after that), for my configuration on my computer, all tested with time nixos-rebuild -I nixpkgs=../prog/nixpkgs build && time nixos-rebuild -I nixpkgs=../prog/nixpkgs build after the actual build was already complete, to warm up the disk cache:

I also removed package lines that might be useful, given I didn't care about having an actually useful nixpkgs in the end, just to benchmark removal of a few options. Feel free to remove more options on top of that commit and to bench again on your side :)

Special mention for hadoop, that has a package argument that's never used (at least in its file, and whose removal did not to cause an evaluation error).

joachifm commented 6 years ago

To me, the problem is mechanically adding options "just because" we can. Sure, there are cases where it makes sense to expose the underlying package, but it seems preferable to default to treating the package(s) as an implementation detail to be exposed only when absolutely necessary. My view is that fewer options is better and no options is best (regardless of eval time). EDIT: I realized my original wording was a little bit uncharitable, sorry

7c6f434c commented 6 years ago

@joachifm I guess a nontrivial subset of Nixpkgs/NixOS developers have experience that unchangeable defaults end up in a state not fitting our needs. This leads to the ideal of having a lot of knobs that can be ignored if everything acceptable, but provide an escape hatch if something exotic/complicated is desired. Then it turns out there are evaluation time issues…

I might be wrong, as I don't use NixOS proper anymore, and I use only a reasonably small subset of NixOS module code. (A few unchangeable defaults ended p annoying me too much).

coretemp commented 6 years ago

Essentially echoing @7c6f434c with some details:

The overlay system provides a single global overlay. There is no way to say that you want one service to use an overridden version and for another you don't, which is actually something I use currently.

There are workarounds for that too, but your proposed solution is inferior.

AFAIK, I need to fork the module in order to modify it to my needs in that case. Having a package option means I need to fork less often, which is what most people probably want.

P.S. Kind of curious as to what @7c6f434c is using.

joachifm commented 6 years ago

@7c6f434c that's fair, but I take it you don't think we should add options willy nilly without any regard for whether they make sense? All I really ask is for some justification. It doesn't have to make sense for everybody, but it has to make sense (realistically) for somebody.

I see several general claims that overriding a specific package only for a specific module is useful but few concrete examples. For it to be a general policy I'd hope we could agree that it has to make sense more often than not.

I think there are real costs associated with greater variability implied by adding options beyond eval time, and so that there is a benefit to being selective about what is added.

7c6f434c commented 6 years ago

@joachifm About overlays — maybe in general, given discussions like #43266 not everyone fully trusts overlays for all use cases. And we usually ship a single client+server package and people might want to override the two parts independently.

You are not alone in thinking there is a cost — different people get different levels of annoyance from having to deal with an interface with many optional knobs, and also from not having access to obvious knobs. I guess we never had a Nix* survery to know the local shape of the distribution (there is a weird enough selection criterion that I don't expect general population studies of option paralysis, cost of choice etc. to apply)

Of course, reminding people about disabledModules to replace a single module (like in https://github.com/NixOS/nixpkgs/issues/46464 discussion) is useful.

@coretemp I guess explicitly saying what service you are talking about might help to discuss specifics.

Re: P.S. — feel free to email me or open an issue about lack of documentation against my lang-os repo or something; by the way, there you can find an example of grabbing things out of a separate NixOS import — maybe this can be combined with overriding Nixpkgs in a reimport of NixOS for some intents.

coretemp commented 6 years ago

@7c6f434c For $CENSORED_REASONS I was vague on this topic. It was a fair question. Perhaps I can answer the question in a fairly long time.

Shados commented 5 years ago

@Ekleog 

* Before, `9.47s user 0.68s system 133% cpu 7.599 total`

* After, `9.11s user 0.56s system 135% cpu 7.118 total`

This is interesting, but for the results to be useful you'd really need to take some steps to exclude other factors from influencing the benchmark (e.g. by running each case many times over and looking at the distribution of results). 0.48s is a lot more of a change than I would expect for only 19 less options; it seems likely the difference has more to do with other things happening on the system during the tests.

Some thoughts:

zimbatm commented 5 years ago

I propose to close this issue and go toward adding package attribute to all the modules that are backed by a single package.

Without a proper benchmark it's not worth talking about hypothetical performance improvements. NixOS evaluation is highly sensitive to the disk cache and needs to be repeated many times.

On the other hand, having a consistent interface is useful on many levels. Like @copumpkin said, the user can rely on the option being there and not have to dig into the nixos source. Not everybody has write access to nixpkgs to add the package option "when needed".

infinisil commented 5 years ago

I think the ideal way to solve this would be to add a generic nixos.services.* option (or tasks.* or jobs.* or whatever), defined somewhat like this:

{
  options.nixos.services = mkOption {
    type = types.attrsOf (types.submodule {
      options = {
        package = mkOption {
          type = types.package;
        };

        user = mkOption {
          type = types.str;
        };

        systemd = mkOption {
          type = /* ... */;
          description = "systemd.services.* link";
        };
      };
    });
  };
}

This is then like a layer on top of systemd.services which defines all the additional options NixOS services ought to have, like package, user, group, maybe even dataDir, config and/or configFile.

In this new abstraction layer we could define all these options without having to clutter all service definitions with duplicate definitions.

This would then be customizable like this:

{ pkgs, ... }: {
  services.foo.service.package = pkgs.foo_1;
  services.foo.someFooSpecificOption = 10;
  services.foo.service.systemd.preStart = "echo hi";
}

Where services.foo.service is a link to the corresponding nixos.services.foo option set.

stale[bot] commented 4 years ago

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.
infinisil commented 4 years ago

I guess we can close this, most people seem to be for always having a package option. Previously I was against this, but I've since changed my opinion and think that it might very well be alright to always have it, even if it's not immediately useful.