From 2c83122c1877b20728c8ae97faddbf2166f105cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Na=C3=AFm=20Favier?= Date: Tue, 6 Dec 2022 18:15:20 +0100 Subject: [PATCH 1/3] treewide: fix broken `meta` attributes --- pkgs/applications/misc/keepass-plugins/otpkeyprov/default.nix | 2 +- pkgs/games/quake3/quake3e/default.nix | 2 +- pkgs/os-specific/linux/kernel/linux-rpi.nix | 4 ++-- pkgs/tools/security/onlykey-agent/default.nix | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkgs/applications/misc/keepass-plugins/otpkeyprov/default.nix b/pkgs/applications/misc/keepass-plugins/otpkeyprov/default.nix index 780de142c5fd..abed55d91593 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 5303d679be17..0c57c97bbe90 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 ef742be0de84..cd0db1f1eff2 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/tools/security/onlykey-agent/default.nix b/pkgs/tools/security/onlykey-agent/default.nix index 8be0971f964e..86095191973f 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 From 4df10debe79feba975631347b25f8699b7cd3554 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Na=C3=AFm=20Favier?= Date: Tue, 6 Dec 2022 18:51:29 +0100 Subject: [PATCH 2/3] lib/customisation.overrideDerivation: propagate evaluation condition The new derivation should evaluate only if the old derivation does. Sadly this means that the old derivation cannot depend on the new one any more, which was used by xorgserver on Darwin. But this is not a problem as `overrideAttrs` can (and should) usually be used instead. This change allowed catching an invalid `meta.platforms` in the linux_rpi kernels, which use `overrideDerivation`. --- doc/using/overrides.chapter.md | 2 +- lib/customisation.nix | 10 +++++++++- pkgs/servers/x11/xorg/overrides.nix | 3 +-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/doc/using/overrides.chapter.md b/doc/using/overrides.chapter.md index 1c51d27ecedc..198b4504197d 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 bd7ee3c83b8c..7ba8ed06180b 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/pkgs/servers/x11/xorg/overrides.nix b/pkgs/servers/x11/xorg/overrides.nix index ddd5acad7a1a..8b3129f5ae21 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" From 4af22aab8e239b1ca28da851755c6da1a35fc91b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Na=C3=AFm=20Favier?= Date: Sun, 18 Dec 2022 12:42:43 +0100 Subject: [PATCH 3/3] stdenv/check-meta: do deep type checks Use a wrapper around `mergeDefinitions` to type-check values deeply, so that e.g. `maintainers = [ 42 ];` is an error. --- lib/generators.nix | 6 ++-- pkgs/stdenv/generic/check-meta.nix | 56 ++++++++++++++++-------------- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/lib/generators.nix b/lib/generators.nix index 4c9c2d1e9869..968331a0ebde 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/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix index 56390dc3627b..83557f995af3 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; + # 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; - # 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. + # 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 ---