From 73f32d03758a53ad1baac31795cfd99e325032f3 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 28 Oct 2013 19:48:30 +0100 Subject: [PATCH] Show precise error messages in option merge failures For instance, if time.timeZone is defined multiple times, you now get the error message: error: user-thrown exception: The unique option `time.timeZone' is defined multiple times, in `/etc/nixos/configurations/misc/eelco/x11vnc.nix' and `/etc/nixos/configuration.nix'. while previously you got: error: user-thrown exception: Multiple definitions of string. Only one is allowed for this option. and only an inspection of the stack trace gave a clue as to what option caused the problem. --- lib/modules.nix | 4 +- lib/options.nix | 21 +++--- lib/types.nix | 76 ++++++++++----------- nixos/modules/config/shells-environment.nix | 9 ++- nixos/modules/config/sysctl.nix | 2 +- nixos/modules/misc/nixpkgs.nix | 2 +- 6 files changed, 58 insertions(+), 56 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index d141838d466..d2b74733944 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -128,7 +128,7 @@ rec { opt.options ? apply && res ? apply || opt.options ? type && res ? type then - throw "The option `${showOption loc}' in `${opt.file}' is already declared in ${concatStringsSep " and " (map (d: "`${d}'") res.declarations)}." + throw "The option `${showOption loc}' in `${opt.file}' is already declared in ${showFiles res.declarations}." else opt.options // res // { declarations = [opt.file] ++ res.declarations; @@ -153,7 +153,7 @@ rec { fold (def: res: if opt.type.check def.value then res else throw "The option value `${showOption loc}' in `${def.file}' is not a ${opt.type.name}.") - (opt.type.merge' { prefix = loc; } (map (m: m.value) defsFinal)) defsFinal; + (opt.type.merge { prefix = loc; files = map (m: m.file) defsFinal; } (map (m: m.value) defsFinal)) defsFinal; # Finally, apply the ‘apply’ function to the merged # value. This allows options to yield a value computed # from the definitions. diff --git a/lib/options.nix b/lib/options.nix index d649d1160a0..66957bc7f15 100644 --- a/lib/options.nix +++ b/lib/options.nix @@ -35,7 +35,7 @@ rec { addDefaultOptionValues = defs: opts: opts // builtins.listToAttrs (map (defName: { name = defName; - value = + value = let defValue = builtins.getAttr defName defs; optValue = builtins.getAttr defName opts; @@ -49,27 +49,26 @@ rec { else # `defValue' is an attribute set containing options. # So recurse. - if hasAttr defName opts && isAttrs optValue + if hasAttr defName opts && isAttrs optValue then addDefaultOptionValues defValue optValue else addDefaultOptionValues defValue {}; } ) (attrNames defs)); - mergeDefaultOption = list: + mergeDefaultOption = args: list: if length list == 1 then head list - else if all builtins.isFunction list then x: mergeDefaultOption (map (f: f x) list) + else if all builtins.isFunction list then x: mergeDefaultOption args (map (f: f x) list) else if all isList list then concatLists list else if all isAttrs list then fold lib.mergeAttrs {} list else if all builtins.isBool list then fold lib.or false list else if all builtins.isString list then lib.concatStrings list - else if all builtins.isInt list && all (x: x == head list) list - then head list - else throw "Cannot merge values."; + else if all builtins.isInt list && all (x: x == head list) list then head list + else throw "Cannot merge definitions of `${showOption args.prefix}' given in ${showFiles args.files}."; /* Obsolete, will remove soon. Specify an option type or apply function instead. */ - mergeTypedOption = typeName: predicate: merge: list: + mergeTypedOption = typeName: predicate: merge: args: list: if all predicate list then merge list else throw "Expect a ${typeName}."; @@ -82,9 +81,10 @@ rec { (x: if builtins ? isString then builtins.isString x else x + "") lib.concatStrings; - mergeOneOption = list: + mergeOneOption = args: list: if list == [] then abort "This case should never happen." - else if length list != 1 then throw "Multiple definitions. Only one is allowed for this option." + else if length list != 1 then + throw "The unique option `${showOption args.prefix}' is defined multiple times, in ${showFiles args.files}." else head list; @@ -135,6 +135,7 @@ rec { /* Helper functions. */ showOption = concatStringsSep "."; + showFiles = files: concatStringsSep " and " (map (f: "`${f}'") files); unknownModule = ""; } diff --git a/lib/types.nix b/lib/types.nix index 3c21e34879c..34d06a9144f 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -19,27 +19,24 @@ rec { }; - # name (name of the type) - # check (check the config value) - # merge (default merge function) - # getSubOptions (returns sub-options for manual generation) isOptionType = isType "option-type"; mkOptionType = - { name - , check ? (x: true) - , merge ? mergeDefaultOption - , merge' ? args: merge - , getSubOptions ? prefix: {} + { # Human-readable representation of the type. + name + , # Function applied to each definition that should return true if + # its type-correct, false otherwise. + check ? (x: true) + , # Merge a list of definitions together into a single value. + merge ? mergeDefaultOption + , # Return a flat list of sub-options. Used to generate + # documentation. + getSubOptions ? prefix: {} }: - { _type = "option-type"; - inherit name check merge merge' getSubOptions; + inherit name check merge getSubOptions; }; - addToPrefix = args: name: args // { prefix = args.prefix ++ [name]; }; - - types = rec { unspecified = mkOptionType { @@ -49,7 +46,7 @@ rec { bool = mkOptionType { name = "boolean"; check = builtins.isBool; - merge = fold lib.or false; + merge = args: fold lib.or false; }; int = mkOptionType { @@ -60,7 +57,7 @@ rec { string = mkOptionType { name = "string"; check = builtins.isString; - merge = lib.concatStrings; + merge = args: lib.concatStrings; }; # Like ‘string’, but add newlines between every value. Useful for @@ -68,54 +65,56 @@ rec { lines = mkOptionType { name = "string"; check = builtins.isString; - merge = lib.concatStringsSep "\n"; + merge = args: lib.concatStringsSep "\n"; }; commas = mkOptionType { name = "string"; check = builtins.isString; - merge = lib.concatStringsSep ","; + merge = args: lib.concatStringsSep ","; }; envVar = mkOptionType { name = "environment variable"; inherit (string) check; - merge = lib.concatStringsSep ":"; + merge = args: lib.concatStringsSep ":"; }; attrs = mkOptionType { name = "attribute set"; check = isAttrs; - merge = fold lib.mergeAttrs {}; + merge = args: fold lib.mergeAttrs {}; }; # derivation is a reserved keyword. package = mkOptionType { name = "derivation"; check = isDerivation; + merge = mergeOneOption; }; path = mkOptionType { name = "path"; # Hacky: there is no ‘isPath’ primop. check = x: builtins.unsafeDiscardStringContext (builtins.substring 0 1 (toString x)) == "/"; + merge = mergeOneOption; }; # drop this in the future: - list = builtins.trace "types.list is deprecated; use types.listOf instead" types.listOf; + list = builtins.trace "`types.list' is deprecated; use `types.listOf' instead" types.listOf; listOf = elemType: mkOptionType { name = "list of ${elemType.name}s"; check = value: isList value && all elemType.check value; - merge' = args: defs: imap (n: def: elemType.merge' (addToPrefix args (toString n)) [def]) (concatLists defs); + merge = args: defs: imap (n: def: elemType.merge (addToPrefix args (toString n)) [def]) (concatLists defs); getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["*"]); }; attrsOf = elemType: mkOptionType { name = "attribute set of ${elemType.name}s"; check = x: isAttrs x && all elemType.check (lib.attrValues x); - merge' = args: lib.zipAttrsWith (name: - elemType.merge' (addToPrefix (args // { inherit name; }) name)); + merge = args: lib.zipAttrsWith (name: + elemType.merge (addToPrefix (args // { inherit name; }) name)); getSubOptions = prefix: elemType.getSubOptions (prefix ++ [""]); }; @@ -138,43 +137,39 @@ rec { if isList x then listOnly.check x else if isAttrs x then attrOnly.check x else false; - merge' = args: defs: attrOnly.merge' args (imap convertIfList defs); + merge = args: defs: attrOnly.merge args (imap convertIfList defs); getSubOptions = prefix: elemType.getSubOptions (prefix ++ [""]); }; uniq = elemType: mkOptionType { inherit (elemType) name check; - merge = list: - if length list == 1 then - head list - else - throw "Multiple definitions of ${elemType.name}. Only one is allowed for this option."; + merge = mergeOneOption; getSubOptions = elemType.getSubOptions; }; none = elemType: mkOptionType { inherit (elemType) name check; - merge = list: - throw "No definitions are allowed for this option."; + merge = args: list: + throw "No definitions are allowed for the option `${showOption args.prefix}'."; getSubOptions = elemType.getSubOptions; }; nullOr = elemType: mkOptionType { name = "null or ${elemType.name}"; check = x: builtins.isNull x || elemType.check x; - merge' = args: defs: + merge = args: defs: if all isNull defs then null else if any isNull defs then - throw "Some but not all values are null." - else elemType.merge' args defs; + throw "The option `${showOption args.prefix}' is defined both null and not null, in ${showFiles args.files}." + else elemType.merge args defs; getSubOptions = elemType.getSubOptions; }; functionTo = elemType: mkOptionType { name = "function that evaluates to a(n) ${elemType.name}"; check = builtins.isFunction; - merge' = args: fns: - fnArgs: elemType.merge' args (map (fn: fn fnArgs) fns); + merge = args: fns: + fnArgs: elemType.merge args (map (fn: fn fnArgs) fns); getSubOptions = elemType.getSubOptions; }; @@ -183,8 +178,7 @@ rec { mkOptionType rec { name = "submodule"; check = x: isAttrs x || builtins.isFunction x; - merge = merge' {}; - merge' = args: defs: + merge = args: defs: let coerce = def: if builtins.isFunction def then def else { config = def; }; modules = opts' ++ map coerce defs; @@ -204,4 +198,8 @@ rec { }; + + /* Helper function. */ + addToPrefix = args: name: args // { prefix = args.prefix ++ [name]; }; + } diff --git a/nixos/modules/config/shells-environment.nix b/nixos/modules/config/shells-environment.nix index 4c40f33532f..36f8549af8e 100644 --- a/nixos/modules/config/shells-environment.nix +++ b/nixos/modules/config/shells-environment.nix @@ -25,11 +25,14 @@ in ''; type = types.attrsOf (mkOptionType { name = "a string or a list of strings"; - merge = xs: + merge = args: xs: let xs' = filterOverrides xs; in if isList (head xs') then concatLists xs' - else if builtins.lessThan 1 (length xs') then abort "variable in ‘environment.variables’ has multiple values" - else if !builtins.isString (head xs') then abort "variable in ‘environment.variables’ does not have a string value" + else if builtins.lessThan 1 (length xs') then + # Don't show location info here, since it's too general. + throw "The option `${showOption args.prefix}' is defined multiple times." + else if !builtins.isString (head xs') then + throw "The option `${showOption args.prefix}' does not have a string value." else head xs'; }); apply = mapAttrs (n: v: if isList v then concatStringsSep ":" v else v); diff --git a/nixos/modules/config/sysctl.nix b/nixos/modules/config/sysctl.nix index 6b52fd38fde..a825144e466 100644 --- a/nixos/modules/config/sysctl.nix +++ b/nixos/modules/config/sysctl.nix @@ -7,7 +7,7 @@ let sysctlOption = mkOptionType { name = "sysctl option value"; check = x: builtins.isBool x || builtins.isString x || builtins.isInt x; - merge = xs: last xs; # FIXME: hacky way to allow overriding in configuration.nix. + merge = args: xs: last xs; # FIXME: hacky way to allow overriding in configuration.nix. }; in diff --git a/nixos/modules/misc/nixpkgs.nix b/nixos/modules/misc/nixpkgs.nix index 9eba728c339..dfbd98bf6ee 100644 --- a/nixos/modules/misc/nixpkgs.nix +++ b/nixos/modules/misc/nixpkgs.nix @@ -26,7 +26,7 @@ let configType = mkOptionType { name = "nixpkgs config"; check = traceValIfNot isConfig; - merge = fold mergeConfig {}; + merge = args: fold mergeConfig {}; }; in