Merge pull request #204840 from ncfavier/check-meta-deep

9 files changed, 51 insertions, 38 deletions

diff --git a/doc/using/overrides.chapter.md b/doc/using/overrides.chapter.md
index 1c51d27eced..198b4504197 100644
--- a/doc/using/overrides.chapter.md
+++ b/doc/using/overrides.chapter.md
@@ -63,7 +63,7 @@ You should prefer `overrideAttrs` in almost all cases, see its documentation for
 :::
 
 ::: {.warning}
-Do not use this function in Nixpkgs as it evaluates a Derivation before modifying it, which breaks package abstraction and removes error-checking of function arguments. In addition, this evaluation-per-function application incurs a performance penalty, which can become a problem if many overrides are used. It is only intended for ad-hoc customisation, such as in `~/.config/nixpkgs/config.nix`.
+Do not use this function in Nixpkgs as it evaluates a derivation before modifying it, which breaks package abstraction. In addition, this evaluation-per-function application incurs a performance penalty, which can become a problem if many overrides are used. It is only intended for ad-hoc customisation, such as in `~/.config/nixpkgs/config.nix`.
 :::
 
 The function `overrideDerivation` creates a new derivation based on an existing one by overriding the original's attributes with the attribute set produced by the specified function. This function is available on all derivations defined using the `makeOverridable` function. Most standard derivation-producing functions, such as `stdenv.mkDerivation`, are defined using this function, which means most packages in the nixpkgs expression, `pkgs`, have this function.
diff --git a/lib/customisation.nix b/lib/customisation.nix
index bd7ee3c83b8..7ba8ed06180 100644
--- a/lib/customisation.nix
+++ b/lib/customisation.nix
@@ -27,11 +27,19 @@ rec {
      For another application, see build-support/vm, where this
      function is used to build arbitrary derivations inside a QEMU
      virtual machine.
+
+     Note that in order to preserve evaluation errors, the new derivation's
+     outPath depends on the old one's, which means that this function cannot
+     be used in circular situations when the old derivation also depends on the
+     new one.
+
+     You should in general prefer `drv.overrideAttrs` over this function;
+     see the nixpkgs manual for more information on overriding.
   */
   overrideDerivation = drv: f:
     let
       newDrv = derivation (drv.drvAttrs // (f drv));
-    in lib.flip (extendDerivation true) newDrv (
+    in lib.flip (extendDerivation (builtins.seq drv.drvPath true)) newDrv (
       { meta = drv.meta or {};
         passthru = if drv ? passthru then drv.passthru else {};
       }
diff --git a/lib/generators.nix b/lib/generators.nix
index 4c9c2d1e986..968331a0ebd 100644
--- a/lib/generators.nix
+++ b/lib/generators.nix
@@ -289,7 +289,9 @@ rec {
        (This means fn is type Val -> String.) */
     allowPrettyValues ? false,
     /* If this option is true, the output is indented with newlines for attribute sets and lists */
-    multiline ? true
+    multiline ? true,
+    /* Initial indentation level */
+    indent ? ""
   }:
     let
     go = indent: v: with builtins;
@@ -348,7 +350,7 @@ rec {
                 };") v)
         + outroSpace + "}"
     else abort "generators.toPretty: should never happen (v = ${v})";
-  in go "";
+  in go indent;
 
   # PLIST handling
   toPlist = {}: v: let
diff --git a/pkgs/applications/misc/keepass-plugins/otpkeyprov/default.nix b/pkgs/applications/misc/keepass-plugins/otpkeyprov/default.nix
index 780de142c5f..abed55d9159 100644
--- a/pkgs/applications/misc/keepass-plugins/otpkeyprov/default.nix
+++ b/pkgs/applications/misc/keepass-plugins/otpkeyprov/default.nix
@@ -17,7 +17,7 @@ let
       homepage    = "https://keepass.info/plugins.html#otpkeyprov";
       platforms   = with lib.platforms; linux;
       license     = lib.licenses.gpl2;
-      maintainers = [ lib.maintainers.ente ];
+      maintainers = [ lib.maintainers.Enteee ];
     };
 
     pluginFilename = "OtpKeyProv.plgx";
diff --git a/pkgs/games/quake3/quake3e/default.nix b/pkgs/games/quake3/quake3e/default.nix
index 5303d679be1..0c57c97bbe9 100644
--- a/pkgs/games/quake3/quake3e/default.nix
+++ b/pkgs/games/quake3/quake3e/default.nix
@@ -47,7 +47,7 @@ stdenv.mkDerivation rec {
     license = licenses.gpl2;
     platforms = platforms.linux;
     maintainers = with maintainers; [ pmiddend ];
-    badPlatforms = [ platforms.aarch64 ];
+    badPlatforms = platforms.aarch64;
     # never built on aarch64-linux since first introduction in nixpkgs
     broken = stdenv.isLinux && stdenv.isAarch64;
   };
diff --git a/pkgs/os-specific/linux/kernel/linux-rpi.nix b/pkgs/os-specific/linux/kernel/linux-rpi.nix
index ef742be0de8..cd0db1f1eff 100644
--- a/pkgs/os-specific/linux/kernel/linux-rpi.nix
+++ b/pkgs/os-specific/linux/kernel/linux-rpi.nix
@@ -41,10 +41,10 @@ lib.overrideDerivation (buildLinux (args // {
   '';
 
   extraMeta = if (rpiVersion < 3) then {
-    platforms = with lib.platforms; [ arm ];
+    platforms = with lib.platforms; arm;
     hydraPlatforms = [];
   } else {
-    platforms = with lib.platforms; [ arm aarch64 ];
+    platforms = with lib.platforms; arm ++ aarch64;
     hydraPlatforms = [ "aarch64-linux" ];
   };
 } // (args.argsOverride or {}))) (oldAttrs: {
diff --git a/pkgs/servers/x11/xorg/overrides.nix b/pkgs/servers/x11/xorg/overrides.nix
index ddd5acad7a1..8b3129f5ae2 100644
--- a/pkgs/servers/x11/xorg/overrides.nix
+++ b/pkgs/servers/x11/xorg/overrides.nix
@@ -11,7 +11,6 @@
 
 let
   inherit (stdenv) isDarwin;
-  inherit (lib) overrideDerivation;
 
   malloc0ReturnsNullCrossFlag = lib.optional
     (stdenv.hostPlatform != stdenv.buildPlatform)
@@ -761,7 +760,7 @@ self: super:
       ];
       # XQuartz requires two compilations: the first to get X / XQuartz,
       # and the second to get Xvfb, Xnest, etc.
-      darwinOtherX = overrideDerivation xorgserver (oldAttrs: {
+      darwinOtherX = xorgserver.overrideAttrs (oldAttrs: {
         configureFlags = oldAttrs.configureFlags ++ [
           "--disable-xquartz"
           "--enable-xorg"
diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 56390dc3627..83557f995af 100644
--- a/pkgs/stdenv/generic/check-meta.nix
+++ b/pkgs/stdenv/generic/check-meta.nix
@@ -247,16 +247,16 @@ let
       isEnabled = lib.findFirst (x: x == reason) null showWarnings;
     in if isEnabled != null then builtins.trace msg true else true;
 
-
-  # A shallow type check. We are using NixOS'
-  # option types here, which however have the major drawback
-  # of not providing full type checking (part of the type check is
-  # done by the module evaluation itself). Therefore, the checks
-  # will not recurse into attributes.
-  # We still provide the full type for documentation
-  # purposes and in the hope that they will be used eventually.
-  # See https://github.com/NixOS/nixpkgs/pull/191171 for an attempt
-  # to fix this, or mkOptionType in lib/types.nix for more information.
+  # Deep type-checking. Note that calling `type.check` is not enough: see `lib.mkOptionType`'s documentation.
+  # We don't include this in lib for now because this function is flawed: it accepts things like `mkIf true 42`.
+  typeCheck = type: value: let
+    merged = lib.mergeDefinitions [ ] type [
+      { file = lib.unknownModule; inherit value; }
+    ];
+    eval = builtins.tryEval (builtins.deepSeq merged.mergedValue null);
+  in eval.success;
+
+  # TODO make this into a proper module and use the generic option documentation generation?
   metaTypes = with lib.types; rec {
     # These keys are documented
     description = str;
@@ -266,9 +266,11 @@ let
     homepage = either (listOf str) str;
     downloadPage = str;
     changelog = either (listOf str) str;
-    license = either (listOf lib.types.attrs) (either lib.types.attrs str);
-    sourceProvenance = either (listOf lib.types.attrs) lib.types.attrs;
-    maintainers = listOf (attrsOf str);
+    license = let
+      licenseType = either (attrsOf anything) str; # TODO disallow `str` licenses, use a module
+    in either licenseType (listOf licenseType);
+    sourceProvenance = either (listOf (attrsOf anything)) (attrsOf anything);
+    maintainers = listOf (attrsOf anything); # TODO use the maintainer type from lib/tests/maintainer-module.nix
     priority = int;
     platforms = listOf str;
     hydraPlatforms = listOf str;
@@ -310,16 +312,16 @@ let
     badPlatforms = platforms;
   };
 
-  # WARNING: this does not check inner values of the attribute, like list elements or nested attributes.
-  # See metaTypes above and mkOptionType in lib/types.nix for more information
   checkMetaAttr = k: v:
     if metaTypes?${k} then
-      if metaTypes.${k}.check v then
+      if typeCheck metaTypes.${k} v then
         null
       else
-        "key 'meta.${k}' has a value of invalid type ${builtins.typeOf v}; expected ${metaTypes.${k}.description}"
+        "key 'meta.${k}' has invalid value; expected ${metaTypes.${k}.description}, got\n    ${
+          lib.generators.toPretty { indent = "    "; } v
+        }"
     else
-      "key 'meta.${k}' is unrecognized; expected one of: \n\t      [${lib.concatMapStringsSep ", " (x: "'${x}'") (lib.attrNames metaTypes)}]";
+      "key 'meta.${k}' is unrecognized; expected one of: \n  [${lib.concatMapStringsSep ", " (x: "'${x}'") (lib.attrNames metaTypes)}]";
   checkMeta = meta: if config.checkMeta then lib.remove null (lib.mapAttrsToList checkMetaAttr meta) else [];
 
   checkOutputsToInstall = attrs: let
@@ -333,25 +335,27 @@ let
   # Check if a derivation is valid, that is whether it passes checks for
   # e.g brokenness or license.
   #
-  # Return { valid: Bool } and additionally
+  # Return { valid: "yes", "warn" or "no" } and additionally
   # { reason: String; errormsg: String } if it is not valid, where
   # reason is one of "unfree", "blocklisted", "broken", "insecure", ...
+  # !!! reason strings are hardcoded into OfBorg, make sure to keep them in sync
   # Along with a boolean flag for each reason
   checkValidity = attrs:
-    {
+    # Check meta attribute types first, to make sure it is always called even when there are other issues
+    # Note that this is not a full type check and functions below still need to by careful about their inputs!
+    let res = checkMeta (attrs.meta or {}); in if res != [] then
+      { valid = "no"; reason = "unknown-meta"; errormsg = "has an invalid meta attrset:${lib.concatMapStrings (x: "\n  - " + x) res}\n";
+        unfree = false; nonSource = false; broken = false; unsupported = false; insecure = false;
+      }
+    else {
       unfree = hasUnfreeLicense attrs;
       nonSource = hasNonSourceProvenance attrs;
       broken = isMarkedBroken attrs;
       unsupported = hasUnsupportedPlatform attrs;
       insecure = isMarkedInsecure attrs;
-    }
-    // (
-    # Check meta attribute types first, to make sure it is always called even when there are other issues
-    # Note that this is not a full type check and functions below still need to by careful about their inputs!
-    let res = checkMeta (attrs.meta or {}); in if res != [] then
-      { valid = "no"; reason = "unknown-meta"; errormsg = "has an invalid meta attrset:${lib.concatMapStrings (x: "\n\t - " + x) res}"; }
+    } // (
     # --- Put checks that cannot be ignored here ---
-    else if checkOutputsToInstall attrs then
+    if checkOutputsToInstall attrs then
       { valid = "no"; reason = "broken-outputs"; errormsg = "has invalid meta.outputsToInstall"; }
 
     # --- Put checks that can be ignored here ---
diff --git a/pkgs/tools/security/onlykey-agent/default.nix b/pkgs/tools/security/onlykey-agent/default.nix
index 8be0971f964..86095191973 100644
--- a/pkgs/tools/security/onlykey-agent/default.nix
+++ b/pkgs/tools/security/onlykey-agent/default.nix
@@ -38,7 +38,7 @@ let
     meta = oa.meta // {
       description = "Using OnlyKey as hardware SSH and GPG agent";
       homepage = "https://github.com/trustcrypto/onlykey-agent/tree/ledger";
-      maintainers = with maintainers; [ kalbasit ];
+      maintainers = with lib.maintainers; [ kalbasit ];
     };
   });
 in