Skip to content
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

lib.modules.mkRenamedOption*: warn as eagerly as possible #360443

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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