From 7d0364d59c1f0ad4571bcfeae46bb39689c977b4 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 30 Nov 2024 15:33:19 +0100 Subject: [PATCH] lib.modules.mkRenamedOption*: warn as eagerly as possible ... i.e. without adding _any_ new strictness This adds support for warnings in submodules. Fixes https://github.com/NixOS/nixpkgs/issues/96006 To recap, https://github.com/NixOS/nixpkgs/pull/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. - #97023 created data dependencies at submodule roots, not just `toplevel` While something like #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. --- lib/modules.nix | 25 +++++++++++++++++++++---- lib/tests/modules.sh | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 855ffaf25ed81..fded37c1660dd 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -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 { @@ -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 + ) ]); }; diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh index 2884c00d0768c..14027bc2ccfe1 100755 --- a/lib/tests/modules.sh +++ b/lib/tests/modules.sh @@ -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() { @@ -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 @@ -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