stdenv: support meta.broken as a string to explain why it's broken #109407

B4dM4n · 2021-01-14T22:21:21Z

Motivation for this change

This draft is based on the idea of @FRidh in #48663 to add a new meta.disabledBecause attribute which can describe why a Python package can't be build. Instead of adding a new attribute with a similar purpose as an existing one (meta.broken), I tried to add the wanted feature to the existing attribute.

meta.brokenBecause allows to add a custom text to the current Package ... is marked as broken ... message. Setting a message also implies broken = true to reduce the chance of only setting a message, but not actually marking the package as broken.

Instead of only booleans, meta.broken can now also be set to a string containing a descriptive message which will be included in the Package ... is marked as broken ... message.

For example the message for disabled Python would look like this:

Package ‘python2.7-sqlparse-0.4.1’ in /.../pkgs/development/python-modules/sqlparse/default.nix:29 is marked as broken because interpreter python2.7 is not supported by this version, refusing to evaluate.

a) To temporarily allow broken packages, you can use an environment variable
...

I included the buildPythonPackage change in this PR to show the potential usage of the attribute. It can be split off when needed.

Things done

veprbl · 2021-01-15T00:46:40Z

Some bikeshedding:

This would warrant adding of lib.optionalBroken = cond: because: if cond then because else false;
Also may want to just allow strings in the old attribute meta.broken = lib.optionalBroken stdenv.isFoo "because ...";

B4dM4n · 2021-01-15T11:51:35Z

Some bikeshedding:

This would warrant adding of lib.optionalBroken = cond: because: if cond then because else false;

Also may want to just allow strings in the old attribute meta.broken = lib.optionalBroken stdenv.isFoo "because ...";

I like your ideas. This makes it even simpler to specify a descriptive error message.

B4dM4n · 2021-01-15T14:28:27Z

A question I'm not sure about is, what the checkValidity function should return for the broken attribute:

https://github.com/NixOS/nixpkgs/blob/2a9dc3f48d11517be25c4817c634fd6db22d1f81/pkgs/stdenv/generic/check-meta.nix#L273-L279

Is this supposed to be a bool which is used by tools to determine the state of a package or should this propagate the original attribute / message?

As this is the value that the derivation receives, it might break some tools when changed.

holymonson · 2021-01-23T00:54:25Z

Do we really need lib.optionalBroken? Since we already have lib.optionalString, it seems easier if we test "" like false.

teto · 2021-05-19T09:04:20Z

pkgs/development/python-modules/tensorflow/bin.nix

@@ -61,7 +61,7 @@ in buildPythonPackage {
    platform = if stdenv.isDarwin then "mac" else "linux";
    unit = if cudaSupport then "gpu" else "cpu";
    key = "${platform}_py_${pyVerNoDot}_${unit}";
-  in fetchurl packages.${key};
+  in fetchurl packages.${key} or { };


is it relevant ?

I'm not sure why it's needed, but running nixpkgs-review without it will fail:

error: attribute 'linux_py_39_cpu' missing

nixpkgs-review error

$ nix-env --option system x86_64-linux -f /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs -qaP --xml --out-path --show-trace --meta error: attribute 'linux_py_39_cpu' missing at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/pkgs/development/python-modules/tensorflow/bin.nix:69:15: 68| key = "${platform}_py_${pyVerNoDot}_${unit}"; 69| in fetchurl packages.${key}; | ^ 70| … while evaluating the attribute 'src.name' at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/pkgs/development/python-modules/tensorflow/bin.nix:64:3: 63| 64| src = let | ^ 65| pyVerNoDot = lib.strings.stringAsChars (x: if x == "." then "" else x) python.pythonVersion; … while evaluating 'hasSuffix' at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/lib/strings.nix:234:5: 233| # Input string 234| content: | ^ 235| let … from call site at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:121:25: 120| pythonRemoveBinBytecodeHook 121| ] ++ lib.optionals (lib.hasSuffix "zip" (attrs.src.name or "")) [ | ^ 122| unzip … while evaluating 'optionals' at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/lib/lists.nix:270:5: 269| # List to return if condition is true 270| elems: if cond then elems else []; | ^ 271| … from call site at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:121:10: 120| pythonRemoveBinBytecodeHook 121| ] ++ lib.optionals (lib.hasSuffix "zip" (attrs.src.name or "")) [ | ^ 122| unzip … while evaluating 'chooseDevOutputs' at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/lib/attrsets.nix:497:22: 496| /* Pick the outputs of packages to place in buildInputs */ 497| chooseDevOutputs = drvs: builtins.map getDev drvs; | ^ 498| … from call site … while evaluating the attribute 'nativeBuildInputs' of the derivation 'python3.9-tensorflow-2.4.0' at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:201:11: 200| // (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) { 201| name = | ^ 202| let … while evaluating the attribute 'out.outPath' at /home/fabian/.cache/nixpkgs-review/rev-a249429bb8a506c78a39a49ff3ebbe34f8b9ba9d/nixpkgs/lib/customisation.nix:156:13: 155| drvPath = assert condition; drv.${outputName}.drvPath; 156| outPath = assert condition; drv.${outputName}.outPath; | ^ 157| }; … while querying the derivation named 'python3.9-tensorflow-2.4.0'

Running nix-instantiate --eval on the other hand fails with the correct error:

nix-instantiate --eval --strict . -A python39Packages.tensorflow-bin.outPath error: Package ‘python3.9-tensorflow-2.4.0’ in /home/fabian/Repo/nixpkgs/pkgs/development/python-modules/tensorflow/bin.nix:192 is marked as broken because interpreter python3.9 is not supported by this version, refusing to evaluate. a) To temporarily allow broken packages, you can use an environment variable for a single invocation of the nix tools. $ export NIXPKGS_ALLOW_BROKEN=1 b) For `nixos-rebuild` you can set { nixpkgs.config.allowBroken = true; } in configuration.nix to override this. c) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add { allowBroken = true; } to ~/.config/nixpkgs/config.nix. (use '--show-trace' to show detailed location information)

teto · 2021-05-19T09:05:20Z

lib/trivial.nix

+       => false
+  */
+  optionalBroken = cond: because:
+    if cond then because else false;


lets use optionalString instead

Using optionalString is possible, but I tried to avoid a second not broken value (false and "") and optionalBroken was my workaround.

But I admit, using optionalString would remove the need for a new lib function and perform the same task.

teto · 2021-05-19T09:08:39Z

lets use optionalString instead of optionalBroken. This PR would be very helpful for the haskell ecosystem where packages regularly break. I wanted to update one package and I know from experience this can be a painful but sometimes a fix is just a jailbreak/donTest away and so being able to distinguish between the cases would be nice.

Package ‘wide-word-0.1.1.2’ in /home/teto/nixpkgs2/pkgs/development/haskell-modules/hackage-packages.nix:280435 is marked as broken, refusing to evaluate.

SuperSandro2000 · 2021-05-22T15:46:39Z

pkgs/stdenv/generic/check-meta.nix

@@ -53,7 +53,7 @@ let
    hasLicense attrs &&
    isUnfree (lib.lists.toList attrs.meta.license);

-  isMarkedBroken = attrs: attrs.meta.broken or false;
+  isMarkedBroken = attrs: attrs.meta.broken or false != false;


Suggested change

isMarkedBroken = attrs: attrs.meta.broken or false != false;

isMarkedBroken = attrs: attrs.meta.broken or true;

This change would mark all packages without a meta.broken attribute as broken.

The original meta.broken or false != false is correct, because the or is binding stronger than the !=.

This allows to override the `disabled` attribute with `overridePythonAttrs`.

B4dM4n · 2021-06-01T08:56:59Z

I rebased the changes and replaced optionalBroken with optionalString as suggested, because it removes the need for a new lib function.

I'm still not sure whether the return value for broken should contain the string or stay a boolean.

A question I'm not sure about is, what the checkValidity function should return for the broken attribute:

https://github.com/NixOS/nixpkgs/blob/2a9dc3f48d11517be25c4817c634fd6db22d1f81/pkgs/stdenv/generic/check-meta.nix#L273-L279

Is this supposed to be a bool which is used by tools to determine the state of a package or should this propagate the original attribute / message?

As this is the value that the derivation receives, it might break some tools when changed.

teto · 2021-06-08T15:24:33Z

I managed to make this pass the test locally via rebasing + this diff:

diff --git a/pkgs/development/python-modules/zipfile36/default.nix b/pkgs/development/python-modules/zipfile36/default.nix
index 46dd1e173fb..d9ddf6e66a3 100644
--- a/pkgs/development/python-modules/zipfile36/default.nix
+++ b/pkgs/development/python-modules/zipfile36/default.nix
@@ -27,6 +27,6 @@ buildPythonPackage rec {
     description = "Read and write ZIP files - backport of the zipfile module from Python 3.6";
     homepage = "https://gitlab.com/takluyver/zipfile36";
     license = lib.licenses.psfl;
-    maintainers = lib.maintainers.fridh;
+    maintainers = [ lib.maintainers.fridh ];
   };
 }
diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 8c8b60c8021..53a734c9a6e 100644
--- a/pkgs/stdenv/generic/check-meta.nix
+++ b/pkgs/stdenv/generic/check-meta.nix
@@ -252,7 +252,8 @@ let
 
   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}"
+    # ${toString v}
+      if metaTypes.${k}.check v then null else "key '${k}' has a value 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 [];

fricklerhandwerk · 2021-06-11T21:02:42Z

I'm still not sure whether the return value for broken should contain the string or stay a boolean.

After searching around the code it does not look like it would immediately break anything if we allowed strings in meta.broken. There are a few callers of the attribute in a bunch of packages, and only changing those values in the package definition to strings will throw errors where conditionals expect a boolean. I'm in favor of keeping it consistent and boolean. Dynamic union types confuse me often enough. I'd like to keep them at a minimum, and only if necessary for backwards compatibility hacks.

Other than that - great pull request, looks ready for un-draft-ing to me.

teto · 2021-10-02T22:21:29Z

I've opened a rebased version #140325

ofborg bot added 6.topic: python 6.topic: stdenv Standard environment 8.has: documentation 8.has: package (new) 10.rebuild-darwin: 501+ 10.rebuild-darwin: 501-1000 10.rebuild-linux: 501+ 10.rebuild-linux: 501-1000 labels Jan 14, 2021

ofborg bot requested review from rbvermaa, marsam, jluttine, costrouc and joachifm January 15, 2021 12:00

B4dM4n mentioned this pull request Jan 15, 2021

check-meta: should we explicitly state broken reason? #109424

Closed

FRidh added this to the 21.05 milestone Jan 20, 2021

primeos mentioned this pull request Feb 10, 2021

fscryptctl: init at 1.0.0 #112651

Merged

10 tasks

teto reviewed May 19, 2021

View reviewed changes

SuperSandro2000 reviewed May 22, 2021

View reviewed changes

B4dM4n added 3 commits June 1, 2021 09:49

stdenv: allow meta.broken to be a string

7e88ece

buildPythonPackage: use meta.broken for disabled packages

4734e19

This allows to override the `disabled` attribute with `overridePythonAttrs`.

pythonPackages.tensorflow-bin: fix error for unsupported python versions

e4d7bae

B4dM4n force-pushed the stdenv-broken-because branch from 2a9dc3f to e4d7bae Compare June 1, 2021 08:50

ofborg bot added the 10.rebuild-darwin: 101-500 label Jun 1, 2021

ofborg bot added 10.rebuild-linux: 101-500 and removed 10.rebuild-darwin: 501+ 10.rebuild-darwin: 501-1000 10.rebuild-linux: 501+ 10.rebuild-linux: 501-1000 labels Jun 1, 2021

teto changed the title ~~stdenv: add meta.brokenBecause attribute~~ stdenv: support meta.broken as a string to explain why it's broken Jun 1, 2021

teto mentioned this pull request Jul 24, 2021

hackage2nix: Correctly flag broken Haskell packages NixOS/cabal2nix#554

Open

teto mentioned this pull request Oct 2, 2021

Stdenv overhaul broken flag #140325

Draft

12 tasks

B4dM4n closed this Jan 22, 2022

B4dM4n deleted the stdenv-broken-because branch January 24, 2022 15:00

FRidh mentioned this pull request Mar 30, 2022

Unable to override "disabled" attribute with overridePythonAttrs #48663

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv: support meta.broken as a string to explain why it's broken #109407

stdenv: support meta.broken as a string to explain why it's broken #109407

B4dM4n commented Jan 14, 2021 •

edited

veprbl commented Jan 15, 2021

B4dM4n commented Jan 15, 2021

B4dM4n commented Jan 15, 2021

holymonson commented Jan 23, 2021

teto May 19, 2021

B4dM4n Jun 1, 2021

teto May 19, 2021

B4dM4n Jun 1, 2021

teto commented May 19, 2021

SuperSandro2000 May 22, 2021

B4dM4n Jun 1, 2021

B4dM4n commented Jun 1, 2021

teto commented Jun 8, 2021

fricklerhandwerk commented Jun 11, 2021 •

edited

teto commented Oct 2, 2021

	isMarkedBroken = attrs: attrs.meta.broken or false != false;
	isMarkedBroken = attrs: attrs.meta.broken or true;

stdenv: support meta.broken as a string to explain why it's broken #109407

stdenv: support meta.broken as a string to explain why it's broken #109407

Conversation

B4dM4n commented Jan 14, 2021 • edited

Motivation for this change

Things done

veprbl commented Jan 15, 2021

B4dM4n commented Jan 15, 2021

B4dM4n commented Jan 15, 2021

holymonson commented Jan 23, 2021

teto May 19, 2021

Choose a reason for hiding this comment

B4dM4n Jun 1, 2021

Choose a reason for hiding this comment

teto May 19, 2021

Choose a reason for hiding this comment

B4dM4n Jun 1, 2021

Choose a reason for hiding this comment

teto commented May 19, 2021

SuperSandro2000 May 22, 2021

Choose a reason for hiding this comment

B4dM4n Jun 1, 2021

Choose a reason for hiding this comment

B4dM4n commented Jun 1, 2021

teto commented Jun 8, 2021

fricklerhandwerk commented Jun 11, 2021 • edited

teto commented Oct 2, 2021

B4dM4n commented Jan 14, 2021 •

edited

fricklerhandwerk commented Jun 11, 2021 •

edited