Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RFC 0042] NixOS settings options #42
[RFC 0042] NixOS settings options #42
Changes from 5 commits
4c436f1
68f1b44
feb2e40
3d072be
3e60aab
cde38bc
9a263ef
88b2997
9986adf
d59771b
1231eaa
9e37c0e
166d8d4
1e0bdd6
7dcc00b
dd9e628
e9082a9
0e832f3
d1a8974
ada0658
10d8b15
18b3b4e
39a4f80
0fb7758
b50d1a3
3630c2c
abd729f
49a2898
e1d248c
28a3111
4f98924
15a6c5d
64aeeda
65842f1
13d4fe8
aef7f5f
a48b55b
7d6353d
2c63f95
5ab880f
e719102
3361309
9ef0221
d46bafd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens because specifying defaults has the effect of
lib.mkOptionDefault
(likelib.mkDefault
) on the whole attribute set instead of individual settings.Likewise, you could make the same mistake in the
config.x.y.z.settings
definition, assuming{ a = lib.mkDefault 1; }
would be equivalent tolib.mkDefault { a = 1; }
, which it is not. Only the prior preservesa
when another setting (nota
) is defined by the user.For a real life fix, see NixOS/nixpkgs#110614
@infinisil, perhaps we could make
default = {}
implied wheneverfreeformType
is set? It'd be less tempting.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberth That reads to me like an important use case to think through thoroughly in
nickel
(in case it's not already a in depth first tree merge)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blaggacao then please take responsibility and post on that repo instead. Let's keep this PR on-topic, respecting the 40+ participants' time and attention. Thank you.
Update *: I regret the tone of my initial comment. I should have phrased this as advice instead. You may have read this as an accusation, which was not my intent.
Your comments are appreciated, even this one. However, I felt this case was exceptional for another reason, because of the large number of people subscribed to this thread and my judgement that the overlap between audiences was small, based on the fact that
nickel
is a separate, experimental configuration language whereas participants in this discussion are mostly interested in improving a proven configuration system they already use.By all means, keep commenting. Just be aware of your audience when it's not a small thread.
*: I'm sending a PM about this edit; no worries.
For posterity, this comment had three +1s at the time of the update.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been planning to make a PR for allowing something like
Which would be equivalent to
This would also allow rendering the default in the manual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought a little bit about this and I personally think that one would like to see some kind of mechanism that prints all default values into the summary of
config
. But IIRC only config options are evaluated when generating the manual, so I'm not even sure if that's possible without too much effort.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would certainly be nice in the future, I feel like it should be possible, by just determining the default by actually evaluating the options value with an empty
configuration.nix
, instead of just looking at the optionsdefault
attribute.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea and would be something I'd probably help to implement. How about adding this to "Future work" for now? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is pretty important that the defaults in use should show up in the documentation. Perhaps the
mkOption
function could be adjusted to allow setting a priority of the default option? Then you could have something likeTo have user config merged into the option default value unless the user applies
mkForce
.I haven't thought this through very carefully, though, so I might have missed some point causing this not to work.
But this may be an easier way to solve this documentation issue instead of attempting to evaluate an "empty" configuration. A difficulty, for example, is that we have to figure out how to actually get the config option to be populated. In the foo module, for example, you would have to know that the enable option should be set to
true
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like
The manual could generate
But then there's also the problem with defaults that depend on other options, these can't be in the manual. And then the default declarations would have to be split up in the module itself.
And there's also the problem with serializing nix values from nix itself. We can't properly escape stuff, and multi-line strings won't work, and interpolated values, derivations.. I feel like we almost need some Nix language change to make this work properly.
And then there's also the discussion on which defaults should be seen by the user at all. Does it make sense that
logType = "syslog"
should be in the manual for the user to see? Probably not. See the Defaults section of this RFC as well, the first type of default specifically shouldn't be seen by the user.I guess the simplest solution is to just say "A reasonable default base configuration is set in order for the module to work, see for details". We already link to the module files already anyways.
Not entirely sure what to do about this. But then again, having this problem is nothing inherent to this RFC, lots of modules already set defaults in this way invisible to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any advantage in using
mkDefault
to override some configuration keys over usingapply = recursiveUpdate default
inmkOption
like here?This has the advantage of already showing the default values in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this
apply
way doesn't seem too bad, I don't think it's a good idea because it completely sidesteps the module system, which leads to some unexpected behavior. E.g. in your example if a user setsconfig.ircService = mkForce {}
, this doesn't do anything, becauserecursiveUpdate
doesn't know not to recurse over that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add
addDefaultAnnotation = annotation: content: { _type = "annotation:default"; inherit content; };
and let the documentation builder use that for describing the value in the manual?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this example behind a
cfg.enable
is a very bad idea IMHO, because of NixOS/nixpkgs#19504 (comment).