Stdenv overhaul broken flag

[teto]

Motivation for this change

This is a rebased version of #109407 because I would love to have this to present why luaPackages are not available (wrong interpreters) and for haskell packages where there are many broken packages: if we could save what's wrong in the broken field, I wouldn't have to attempt building the package to have an idea of what's wrong.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[fricklerhandwerk]

This! Thank you.

[r-burns]

I really like this, but I'd like some more feedback because it seems potentially very impactful. Requesting review from lib.* codeowners, since this might be in their wheelhouse

[r-burns]

I realized another cool thing about this - consider a case where you have multiple independent causes for breakage:

{
  meta.broken = lib.optionalString enableA ''
    feature A not supported!
  '' + lib.optionalString enableB ''
    feature B not supported!
  '';
}

This will tell you all the reasons a package is broken at the same time, nice!

feature A not supported!
feature B not supported!

No need to inspect a potentially complicated boolean expression to figure out why meta.broken is evaluating to true.

[sternenseemann]

I think this is a positive change, but possibly the wrong direction. Specifically, I am a bit sceptical that this will lead to broken being used in the wrong way, for the lack of a more appropriate alternative.

In my mind, broken is meant to mark known compilation failures which should work, preventing users from wasting their time building something that's going to fail. Basically broken says: This is a bug, pending resolution. For this use case a comment in the source is sufficient: The user is doing nothing wrong and someone is going to have to touch the nix expression in order to resolve the problem.

Especially given the Python change I've commented on below, I believe that this new feature will be (ab)used in order to show users that something they are trying to do is unsupported. In this case an error message is important, since the user is doing something they can not expect to work. We need a better mechanism here, since meta.platforms and meta.badPlatforms are extremely unexpressive (you can't even mark musl as unsupported via badPlatforms because it only handles (full) platform tuples). However, I don't think that it's the best way forward to use broken for this as well, since it carries the wrong implication: If a package can't be built with musl or clang, then it is not broken in this situation, since there is no reasonable way forward for us to fix this this situation.

Another concern I have here is the API change for meta.broken, but I'm having a hard time gauging how impactful it'd be. I certainly have code that relies on broken being a boolean. At the very least this would need an entry in the changelog and I think it'd be too rushed for 21.11 — we should give ourselves at least a release cycle to figure out any problem arising from this change, if it is made.

[sternenseemann]

This is not correct, Python packages are not broken — implying fixable — for a certain Python interpreter, but fundamentally don't support it and thus should never be able to be built with the wrong interpreter which would be possible via allowBroken.

[teto]

agreed, that was carried on from the original patch, I can revert.

[fricklerhandwerk]

I think this is a positive change, but possibly the wrong direction. Specifically, I am a bit sceptical that this will lead to broken being used in the wrong way, for the lack of a more appropriate alternative.

@sternenseemann I agree with your objection. What do you propose as a resolution? I was following this PR primarily due to #48663. I think the idea to overload broken came from here and was continued in #109407. Originally we just wanted to override Python packages which were disabled. What if we just call it that: disabled - where a package on principle cannot be built in the given configuration. Setting broken can then be reserved for fixable cases, as a stopgap for actually fixing them. @teto What do you think?

[infinisil]

I think there's three main distinct ways in which a package can be non-functioning:

• Evaluation: An evaluation error caused from Nix, e.g. a missing attribute or a throw
• Buildtime: An error caused when the derivation is built, e.g. an unsupported Python interpreter version or a missing dependency
• Runtime: An error caused when the built derivation is used at runtime, e.g. it crashes with a segfault

The use case for broken, badPlatforms and co. is that they elevate an error from buildtime/runtime to evaluation time, so that we don't waste resources like build farm or people hours.

Considering this, I propose this:

• Evaluation: For actual evaluation errors (not elevated buildtime/runtime errors), we don't need anything special. These happen wherever they happen in the code
• Buildtime: Have a new generic attribute like meta.knownBuildErrors which is for manually elevating buildtime errors to evaluation errors. E.g. specifying

  mkDerivation {
    # ...
    meta.knownBuildErrors.pythonVersion =
      "Package ${pname} doesn't support Python interpreter version ${python.version}";
  }

  indicates that while you can theoretically evaluate the package, by default it won't let because it throws the knownBuildErrors (or rather, it only throws these errors when you access the .drvPath/.outPath).
• Runtime: Similarly, we can have a generic attribute meta.knownRuntimeErrors which is for manually elevating runtime errors to evaluation errors. E.g. specifying

  mkDerivation {
    # ...
    meta.knownRuntimeErrors.segfault =
      "This package is known to segfault when started for unknown reason";
  }

  indicates that while you can theoretically evaluate and build the package, by default it won't let you because it throws the knownRuntimeErrors.

Doing it this way then makes it very clear which attribute should be used for what: Build errors can be put in knownBuildErrors and runtime errors can be put in knownRuntimeErrors.

Notably I'm also suggesting for these to be an attribute set, where the key should be used to indicate the type of error (and ideally we'd have a registry of error type keys). Doing it like this makes it very flexible for the future:

• Some people may want to ignore one type of error, e.g. pythonVersion
• Errors can be merged together (e.g. for wrapped builders)
• Wrappers builders could also disable one type of error if they believe they can fix it

[Profpatsch]

@infinisil what would you suggest we do with broken then?

[infinisil]

I'd say deprecate it

[FRidh]

I like what @infinisil suggests in #140325 (comment).

[fricklerhandwerk]

I also think @infinisil's idea is good, mainly the notion of elevating build/run time errors to evaluation errors.

Main objection: It is exactly one error that makes a package unusable. I think it would be an unnecessary complication to model multiple problems. I'm all for strong types, but since build errors are fundamentally people problems (someone has to investigate and fix code), it seems that distinguishing fixable from unfixable problems and presenting a string with some explanation should suffice.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/packages-marked-as-broken-should-come-with-an-explanation/19187/8

[fricklerhandwerk]

@infinisil moving this forward sounds like a job for the Nixpkgs Architecture Team.