This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-december/1711/9

@infinisil I don't have access to a setup where I can test this anymore. It may be another month or so before I'm able.

I don't want to merge a PR untested. DynamicUser specifically enables some security related settings which might initially break things without the appropriate fixes.

@furrycatherder @infinisil

I'd like to have some more options. I've drafted this up and successfully tested the IMAP against my companies exchange server. Please consider my commit which is on top of the original:

master...aanderse:davmail

@aanderse I know everybody is doing this, but I'm really not a fan of making an arbitrary set of config values NixOS options. Instead see what I've done in #45470, that is, adding a single config option of a flexible type representing the configuration format. You can then set arbitrary config values comfortably through it, without the need for a NixOS option for everything.

This has the advantages:

• All configuration values get properly merged and can be inspected via the config argument
• Detached from upstream
  • No need to add/remove/change configuration values on upstream changes
  • No need to document all the values, the docs can stay localized in the place where they belong (upstream)
  • No need to copy upstream defaults which might change later (this could be solved with nullOr ... but that's kinda ugly)
• No option bloat, NixOS evaluation time after all is still linear in the number of available options, even if you only use a fraction of them.
• Simpler to implement and the module stays small and manageable
• You can always add more options later if they're needed, but you can never take options away without (probably) at least one person being annoyed over it.

There are also a couple disadvantages, namely:

• Can't type check everything as easily
• Options for the package aren't as discoverable as others in the NixOS options manual.

But I think the advantages outweigh the disadvantages. Please consider doing this instead, I'd really love to see it being used more in nixpkgs :)

@infinisil Might as well go back to cfg.configFile, then, and skip the step of translating INI to and from Nix expressions? 🤷‍♂️

@infinisil The options I added are the bare minimum required to operate the service and nothing more (except extraConfig, of course).

I've followed the conversations about reducing options which exist for no reason other than "because I can add them" and understand the problem. I've looked at some of the modules I have written and wish I understood these problems back then... but in this scenario I feel like all these options are justified.

I'll take a look at your linked PR to analyze what you did, but I think that absolutely required options for a service to run should still be listed individually, no?

@furrycatherder The problem with that is that you need to know the syntax of the file, and you can't set options in multiple NixOS modules and expect them to merge properly. Strings can only be concatenated together, whereas a Nix value can be merged nicely, which is invaluable for modularity.

@aanderse But then you have the problems of: You can't detect when the user tries to override logFilePath in extraConfig, it will just be ignored or so. Such a config option I described is very much like a semantic extraConfig type, except that the extra part falls away, because we don't need such a hardcoded config file anymore.

Note that doing it this way doesn't in any way limit what default values you may set, you can just do { config.services.davmail.config.ports.smtp = mkDefault 1025; }, etc. So these options aren't really necessary.

In my opinion, NixOS options should ideally only be used for things that interact with other parts of NixOS. E.g. take a look at my murmur module. There's options enable, config, openFirewall and acmeDomain, and each of those interact with other modules.

@aanderse But then you have the problems of: You can't detect when the user tries to override logFilePath in extraConfig, it will just be ignored or so.

To quote: #56423 (comment)

Does this need to be configurable? I don't think there's any need for it. Same for logDir tbh.

In regards to my experience with NixOS - at first I thought all the many configuration options in the module system was great. I've recently come to agree with the opinions of others that many of the options (such as stateDir, logDir, user, group, etc...) serve no real purpose or provide no value (and yes, at a cost). Debian doesn't offer any choice here and it has worked out great for them... why should we?

This "replace all options with a config option set" is definitely a larger discussion that needs to happen. You have some great points. I'd like to hear from more people like you (ie. those much smarter than myself 😃 ) on both sides of the discussion and hopefully get some sort of consensus. Have you started an RFC by chance?

As far as this PR goes... I'd really hate to see this PR get merged with the original configFile option because the beauty of the module system is that things generally "just work" with a minimal amount of configuration from the sysadmin. The configFile forces me to pkgs.writeText a bunch of stuff which is a big 👎, where the config option you suggested provides the bonus of "just works".

@infinisil thanks for all the hard work you put in, and I appreciate you trying to show us a better way.

This "replace all options with a config option set" is definitely a larger discussion that needs to happen. You have some great points. I'd like to hear from more people like you (ie. those much smarter than myself smiley ) on both sides of the discussion and hopefully get some sort of consensus. Have you started an RFC by chance?

Thanks, I haven't started an RFC yet, but I probably will at some point (soon?)

@aanderse @furrycatherder I've rewritten this module to have such a config option here: https://github.com/Infinisil/nixpkgs/blob/pr/52096/nixos/modules/services/mail/davmail.nix . What do you think?

@infinisil on irc we had discussed the idea of fewer options being better, but still having the ability to tweak if needed. The url is a required value (with no default) as noted in your code. If we saw config as that last resort a sysadmin had to take to tweak the app for some special use case then I still think url should be a NixOS option. Consider that as soon as you need to tweak config you need applicationt specific knowledge, you have to read documentation on the app you are configuring. We had discussed how NixOS is awesome because you don't need application specific knowledge, you just type services.foo.enable = true; and you're done.

The thing that bothers me here is that in order to get the app working I need to tweak the config which means I need to read the manual. In the case of davmail only the url really jumps out at me as needing to be an option and documented in NixOS.

Thoughts?

@infinisil I've based a new commit on top of yours which puts url as a nixos option, but leaves the other options as you did. I think this strikes the right balance as services.davmail.url = "blah"; (which there is no possible default value for) requires no application specific knowledge as it will be documented in https://nixos.org/nixos/options.html. This allows most users to have zero knowledge of the inner workings of davmail and simply read nixos options documentation, but provides the ability for advanced users who actually want to dig into davmail documentation to do whatever they need.

aanderse@ca57881

Does this sit well with you?

@aanderse Yeah I think that's great, I also thought maybe a NixOS option would be in order there.

@furrycatherder how does aanderse@ca57881 sit with you then? Want to add that commit on top, squash them, and get this merged? I've already tested on my commit and everything is working.

@aanderse It looks good, I have to manually cherry-pick your commit (d'oh) but I'll get it done today. Thanks for your help! And thanks @infinisil for the useful insight.

@infinisil Should be ready for approval.