check-meta.nix: type checking changes

- Enable metadata checking by default, see https://github.com/NixOS/nixpkgs/pull/25304#issuecomment-298385426
- Check metadata before any other package issues, see https://github.com/NixOS/nixpkgs/issues/191124#issuecomment-1246523976
- Document that type checks only apply to the top level of nested values.

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

nixos/doc/manual/from_md/release-notes/rl-2211.section.xml

@@ -474,6 +474,14 @@
           maintainer to update the package.
         </para>
       </listitem>
+      <listitem>
+        <para>
+          The (previously undocumented) Nixpkgs configuration option
+          <literal>checkMeta</literal> now defaults to
+          <literal>true</literal>. This may cause evaluation failures
+          for packages with incorrect <literal>meta</literal> attribute.
+        </para>
+      </listitem>
       <listitem>
         <para>
           xow package removed along with the

nixos/doc/manual/release-notes/rl-2211.section.md

@@ -164,6 +164,9 @@ Available as [services.patroni](options.html#opt-services.patroni.enable).
 
 - riak package removed along with `services.riak` module, due to lack of maintainer to update the package.
 
+- The (previously undocumented) Nixpkgs configuration option `checkMeta` now defaults to `true`. This may cause evaluation
+  failures for packages with incorrect `meta` attribute.
+
 - xow package removed along with the `hardware.xow` module, due to the project being deprecated in favor of `xone`,  which is available via the `hardware.xone` module.
 
 - dd-agent package removed along with the `services.dd-agent` module, due to the project being deprecated in favor of `datadog-agent`,  which is available via the `services.datadog-agent` module.

pkgs/stdenv/generic/check-meta.nix

@@ -13,10 +13,6 @@ let
 
   getName = attrs: attrs.name or ("${attrs.pname or "«name-missing»"}-${attrs.version or "«version-missing»"}");
 
-  # See discussion at https://github.com/NixOS/nixpkgs/pull/25304#issuecomment-298385426
-  # for why this defaults to false, but I (@copumpkin) want to default it to true soon.
-  shouldCheckMeta = config.checkMeta or false;
-
   allowUnfree = config.allowUnfree
     || builtins.getEnv "NIXPKGS_ALLOW_UNFREE" == "1";
 
@@ -248,6 +244,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.
   metaTypes = with lib.types; rec {
     # These keys are documented
     description = str;
@@ -297,17 +303,23 @@ 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 null else "key '${k}' has a value ${toString v} of an invalid type ${builtins.typeOf v}; expected ${metaTypes.${k}.description}"
-    else "key '${k}' is unrecognized; expected one of: \n\t      [${lib.concatMapStringsSep ", " (x: "'${x}'") (lib.attrNames metaTypes)}]";
-  checkMeta = meta: if shouldCheckMeta then lib.remove null (lib.mapAttrsToList checkMetaAttr meta) else [];
+      if metaTypes.${k}.check v then
+        null
+      else
+        "key 'meta.${k}' has a value of invalid type ${builtins.typeOf v}; expected ${metaTypes.${k}.description}"
+    else
+      "key 'meta.${k}' is unrecognized; expected one of: \n\t      [${lib.concatMapStringsSep ", " (x: "'${x}'") (lib.attrNames metaTypes)}]";
+  checkMeta = meta: if config.checkMeta then lib.remove null (lib.mapAttrsToList checkMetaAttr meta) else [];
 
   checkOutputsToInstall = attrs: let
       expectedOutputs = attrs.meta.outputsToInstall or [];
       actualOutputs = attrs.outputs or [ "out" ];
       missingOutputs = builtins.filter (output: ! builtins.elem output actualOutputs) expectedOutputs;
-    in if shouldCheckMeta
+    in if config.checkMeta
        then builtins.length missingOutputs > 0
        else false;
 
@@ -326,7 +338,17 @@ let
       unsupported = hasUnsupportedPlatform attrs;
       insecure = isMarkedInsecure attrs;
     }
-    // (if hasDeniedUnfreeLicense attrs && !(hasAllowlistedLicense attrs) then
+    // (
+    # 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
+      { valid = "no"; reason = "broken-outputs"; errormsg = "has invalid meta.outputsToInstall"; }
+
+    # --- Put checks that can be ignored here ---
+    else if hasDeniedUnfreeLicense attrs && !(hasAllowlistedLicense attrs) then
       { valid = "no"; reason = "unfree"; errormsg = "has an unfree license (‘${showLicense attrs.meta.license}’)"; }
     else if hasBlocklistedLicense attrs then
       { valid = "no"; reason = "blocklisted"; errormsg = "has a blocklisted license (‘${showLicense attrs.meta.license}’)"; }
@@ -338,10 +360,7 @@ let
       { valid = "no"; reason = "unsupported"; errormsg = "is not supported on ‘${hostPlatform.system}’"; }
     else if !(hasAllowedInsecure attrs) then
       { valid = "no"; reason = "insecure"; errormsg = "is marked as insecure"; }
-    else if checkOutputsToInstall attrs then
-      { valid = "no"; reason = "broken-outputs"; errormsg = "has invalid meta.outputsToInstall"; }
-    else 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}"; }
+
     # --- warnings ---
     # Please also update the type in /pkgs/top-level/config.nix alongside this.
     else if hasNoMaintainers attrs then

pkgs/top-level/config.nix

@@ -128,6 +128,13 @@ let
       '';
     };
 
+    checkMeta = mkOption {
+      type = types.bool;
+      default = true;
+      description = ''
+        Whether to check that the `meta` attribute of derivations are correct during evaluation time.
+      '';
+    };
   };
 
 in {