lib.modules.mkRenamedOption*: warn as eagerly as possible #360443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
... i.e. without adding _any_ new strictness This adds support for warnings in submodules. Fixes NixOS#96006 To recap, NixOS#97023 didn't make it because of unexpected infinite recursions. Context: - Attaching a warning to an expression can create an otherwise unnecessary data dependency, which can lead to infinite mutual recursion when a (typically very necessary) dependency exists in the other direction. - The `warnings` option is largely independent of the evaluation of the configuration, because it only reads from it. Nothing except `system.build.toplevel` reads from `warnings`. Not cyclical, everyone happy. - NixOS#97023 created data dependencies at submodule roots, not just `toplevel` While something like NixOS#97023 is a very general solution that would solve this `mkRenamedOptionModule` with ease, we don't need it for this particular problem. Specifically, in this case, we already have a data dependency where the new option has a definition based on the old option. So, we can attach the warning to the `mkIf` condition of that definition and make it trigger when the old option is about to be read anyway. As a side effect of this change, these particular warnings don't appear in the `warnings` option anymore. Maybe someone used those for testing, but `mkRenamed`* usages aren't really worth testing, so this is a small price to pay for support for renames in submodules.
github-actions
bot
added
6.topic: module system
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
... i.e. without adding any new strictness
This adds support for warnings in submodules.
Fixes #96006
To recap, #97023 didn't make it because of unexpected infinite recursions. Context:
warningsoption is largely independent of the evaluation of the configuration, because it only reads from it. Nothing exceptsystem.build.toplevelreads fromwarnings. Not cyclical, everyone happy.toplevelWhile something like #97023 is a very general solution that would solve this
mkRenamedOptionModulewith ease, we don't need it for this particular problem.Specifically, in this case, we already have a data dependency where the new option has a definition based on the old option. So, we can attach the
warning to the
mkIfcondition of that definition and make it trigger whenthe old option is about to be read anyway.
As a side effect of this change, these particular warnings don't appear in the
warningsoption anymore. Maybe someone used those for testing, butmkRenamed* usages aren't really worth testing, so this is a small price to pay for support for renames in submodules.TODO
lib/tests/modules.sh)Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.