Skip to content

Commit

Permalink
lib.modules.mkRenamedOption*: warn as eagerly as possible
Browse files Browse the repository at this point in the history
... 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.
  • Loading branch information
roberth committed Nov 30, 2024
1 parent d3fcf54 commit 7d0364d
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 5 deletions.
25 changes: 21 additions & 4 deletions lib/modules.nix
Original file line number Diff line number Diff line change
Expand Up @@ -1341,6 +1341,21 @@ let
toOf = attrByPath to
(abort "Renaming error: option `${showOption to}' does not exist.");
toType = let opt = attrByPath to {} options; in opt.type or (types.submodule {});
prefix = lib.take (lib.length options._module.args.loc - 2) options._module.args.loc;
# Memoized warning
# This can be triggered in two ways, and this `let` binding ensures that it
# only triggers once.
# We trigger the evaluation of this warning in two places:
# - When the new option is accessed.
# This ensures that a warning is shown also in submodules that don't have
# a `warnings` option.
# - When the `warnings` option is evaluated.
# This ensures that even if the value is not used, a warning is shown.
definitionWarning =
lib.warnIf
(warn && fromOpt.isDefined)
"The option `${showOption (prefix ++ from)}' defined in ${showFiles fromOpt.files} has been renamed to `${showOption (prefix ++ to)}'."
null;
in
{
options = setAttrByPath from (mkOption {
Expand All @@ -1352,12 +1367,14 @@ let
});
config = mkIf condition (mkMerge [
(optionalAttrs (options ? warnings) {
warnings = optional (warn && fromOpt.isDefined)
"The option `${showOption from}' defined in ${showFiles fromOpt.files} has been renamed to `${showOption to}'.";
# NOTE: We use the warning option to trigger the warning immediately, instead of adding to the list.
# Otherwise, we could show the warning twice; see `definitionWarning`.
warnings = mkIf (builtins.seq definitionWarning false) (lib.mkOverride 10000 []);
})
(if withPriority
then mkAliasAndWrapDefsWithPriority (setAttrByPath to) fromOpt
else mkAliasAndWrapDefinitions (setAttrByPath to) fromOpt)
then mkAliasAndWrapDefsWithPriority (d: setAttrByPath to (builtins.seq definitionWarning d)) fromOpt
else mkAliasAndWrapDefinitions (d: setAttrByPath to (builtins.seq definitionWarning d)) fromOpt
)
]);
};

Expand Down
38 changes: 37 additions & 1 deletion lib/tests/modules.sh
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,13 @@ logFailure() {
printf '\033[1;31mTEST FAILED\033[0m at %s\n' "$(loc 2)"
}

extraArgs=""

evalConfig() {
local attr=$1
shift
local script="import ./default.nix { modules = [ $* ];}"
nix-instantiate --timeout 1 -E "$script" -A "$attr" --eval-only --show-trace --read-write-mode --json
nix-instantiate --timeout 1 -E "$script" -A "$attr" --eval-only --show-trace --read-write-mode --json $extraArgs
}

reportFailure() {
Expand Down Expand Up @@ -107,6 +109,34 @@ checkConfigError() {
fi
}

checkConfigWarning() {
local outputContains=$1
local errContains=$2
local err=""
shift
shift
if err="$((evalConfig "$@" | grep -E --silent "$outputContains") 2>&1 >/dev/null)"; then
if echo "$err" | grep -zP --silent "$errContains" ; then
((++pass))
else
logStartFailure
echo "ACTUAL:"
reportFailure "$@"
echo "EXPECTED: log matching '$errContains'"
logFailure
logEndFailure
fi
else
logStartFailure
echo "ACTUAL:"
reportFailure "$@"
echo "EXPECTED: result matching '$outputContains'"
logFailure
logEndFailure
fi
}
# Shorthand meta attribute does not duplicate the config
checkConfigOutput '^"one two"$' config.result ./shorthand-meta.nix
Expand Down Expand Up @@ -139,6 +169,12 @@ checkConfigOutput '^true$' config.assertion ./gvariant.nix
checkConfigOutput '"ok"' config.result ./specialArgs-lib.nix
# mkRenamedOptionModule
checkConfigOutput '^true$' config.result ./mkRenamedOptionModule.nix
# Check the warning. This abuses checkConfigError to check for a warning.
extraArgs="--strict" \
checkConfigWarning '{"a":1,"b":2}' '.*The option .basic\.old. defined in .*/modules/mkRenamedOptionModule\.nix. has been renamed to .basic\.new..*' config.basic.new ./mkRenamedOptionModule.nix
# https://github.com/NixOS/nixpkgs/pull/131205
# We currently throw this error already in `config`, but throwing in `config.wrong1` would be acceptable.
checkConfigError 'It seems as if you.re trying to declare an option by placing it into .config. rather than .options.' config.wrong1 ./error-mkOption-in-config.nix
Expand Down

0 comments on commit 7d0364d

Please sign in to comment.