-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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: reusable assertions #207187
base: master
Are you sure you want to change the base?
nixos: reusable assertions #207187
Conversation
Nice. Could we call |
This is actually hard, because assertions and warnings tend to add strictness.
For context, the discussion of such an attempt: |
@@ -29,6 +43,17 @@ with lib; | |||
''; | |||
}; | |||
|
|||
_module.assertAndWarn = mkOption { |
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.
_module.assertAndWarn = mkOption { | |
assertAndWarn = mkOption { |
The _module
namespace should be reserved for module-internal options
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.
Quoting you:
The main insight I had was that _module doesn't have a _ at the start because it's for module-internal things, but rather to avoid clashing with option names. While @roberth pointed out that we don't have a checks option at the root, these options are added to submodules as well. So introducing sugar for this could cause problems for any module or submodule which has a checks option. While there is no such option in nixpkgs, we can't be sure that users don't have one on their own.
This is module-internal in spirit anyway.
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 PR was a module-internal implementation of assertions and warnings though, it added support for them to any module (including submodules) since _module.checks
is added by default. This is in contrast to this PR here, which is opt-in for modules.
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.
Perhaps a middle ground would be for _module.assertAndWarn
to be a module-system-standardized interface rather than a full-featured implementation of the concept?
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'd think it would be better then to make it a full-featured implementation. Essentially resurrect #97023 but with opt-in triggering of checks using _module.triggerChecks
(like _module.assertAndWarn
).
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.
Shorthand strikes again. Fair enough.
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 suppose users such as the NixOS toplevel could alias the checks
as an internal config.checks
option, at which point we're basically full circle, but at least the intent around the _module.*
attributes will be clear?
Seems like a lot of bookkeeping just to create an illusion of a formally approved interface, but ok.
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.
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.
Oh no, I forgot to submit my huge response to this. We'll have to make do with a less detailed comment for now.
Laziness is crucial for modularity without infinite recursions. Adding strictness to a module is inherently risky, as we can not know the dependencies that user logic adds. A supposedly helpful warning must never cause evaluations to fail with an infinite recursion. Not even config
would be a good enough trigger because the user may need a mutually recursive dependency with another submodule. For the main logic to keep working it must not depend on any checks, because the checks themselves must depend on the main logic. Anything that makes creating such dependencies on checks easy would be a mistake. Instead, we could standardize on specific _module
attributes, and leave config
itself unencumbered by checks.
This way we can just add a couple manual fixes on top of the rare breakages.
Try to explain this design choice to someone who's confronted with a 120-item eval trace that appears to have no relation to their code, except for two items that are obviously not in a mutually recursive relationship according to any sensible logic. That's if they can even make sense of the data flow in their modules, because yes, that's a plural.
- just - no, this requires a specific skill
- manual fixes - so that will have to be us then
- rare - feel free to play with the crocodile; it rarely bites someone's head off.
Triggering must only happen "automatically" at the ultimate of top level. Anything lower than that is susceptible to infinite recursions when users indirectly create mutual dependencies between, for example, NixOS configurations in a deployment.
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.
We're going to need something outside NixOS. It doesn't have to be perfect, and when the time comes, we deprecate the old opt-in solution.
Encountered it again today
This came up again in nix-community/home-manager#3793 (comment) |
Can we please just merge this |
@infinisil The built-in idea is valid but hard and isn't going anywhere anytime soon, unless you commit time (I wish I could). Until then, this is the solution that people want use, and they'll be happy to migrate to something better when it comes along. TODO
|
Hit this again today, this time we wanted to Context: https://github.com/nix-community/home-manager/pull/4086/files#r1235087054 |
I guess another problem with |
Removing them from As for the reason why, here's a simple example,
Now a user defines { config, ... }: {
settings.foo = config.settings.bar * 2;
} So now While you could argue that this situation is avoidable in one way or another, the problem is that the situation will occur, and infinite recursions are very hard to work with for users. So if you want to have it in |
What do you mean? |
What if instead of returning the usual config = mkIf cfg.enable {
_module.checks = [ services.foo.settings.checks ];
systemd.services.foo = fooService services.foo.settings.config;
} My main objection with this is that it normalizes the in-band handling of checks, whereas I think handling them outside of Should that stop us from implementing
I have to leave it at this for now, though I feel like I'm going to have to elaborate some things. |
It's more of a |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/the-module-system-is-dead-how-to-do-input-validation/30297/1 |
Why is the type for assertions |
d5047fa updated the type but didn't model the item type. A submodule for the individual assertion type would be nice. |
Description of changes
Cleaning up some tech debt that we bumped into in #207095 (comment)
Manually tested by editing
simple.nix
And of course, the classic:
cc @infinisil for a humble improvement in an area where we had more ambitious ideas.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes