haskellPackages: remove __attrsFailEvaluation, buildHaskellPackages, and generateOptparseApplicativeCompletions special cases

[philiptaron]

Description of changes

• Tracking issue: remove and mitigate __attrsFailEvaluation #324619

The test (nix-build pkgs/test/release/default.nix) continues to pass.

These lines were added in #269356.

There is no change in the set of names which evaluate.

Things done

• Tested, as applicable:
  • nix-build pkgs/test/release/default.nix
• Fits CONTRIBUTING.md.

[sternenseemann]

How come generateOptParseApplicativeCompletions no longer causes a failure?

[philiptaron]

How come generateOptParseApplicativeCompletions no longer causes a failure?

I can't cause it to ever generate a failure.

$ git checkout 5a0a700
HEAD is now at 5a0a700663ad Merge pull request #269356 from amjoseph-nixpkgs/pr/release-outpaths-
... editing snipped ...
$ git diff
diff --git a/pkgs/development/haskell-modules/make-package-set.nix b/pkgs/development/haskell-modules/make-package-set.nix
index 294ca295f22b..c39c934bed64 100644
--- a/pkgs/development/haskell-modules/make-package-set.nix
+++ b/pkgs/development/haskell-modules/make-package-set.nix
@@ -614,7 +614,7 @@ in package-set { inherit pkgs lib callPackage; } self // {
        Type: [str] -> drv -> drv
     */
     generateOptparseApplicativeCompletions =
-      (self.callPackage (
+      self.callPackage (
         { stdenv }:

         commands:
@@ -623,7 +623,7 @@ in package-set { inherit pkgs lib callPackage; } self // {
         if stdenv.buildPlatform.canExecute stdenv.hostPlatform
         then lib.foldr haskellLib.__generateOptparseApplicativeCompletion pkg commands
         else pkg
-      ) { }) // { __attrsFailEvaluation = true; };
+      ) { };

     /*
       Modify given Haskell package to force GHC to employ the LLVM
diff --git a/pkgs/top-level/release-attrpaths-superset.nix b/pkgs/top-level/release-attrpaths-superset.nix
index 673b63a5ac34..fdfcda68f628 100644
--- a/pkgs/top-level/release-attrpaths-superset.nix
+++ b/pkgs/top-level/release-attrpaths-superset.nix
@@ -74,7 +74,6 @@ let

     buildHaskellPackages = true;
     buildPackages = true;
-    generateOptparseApplicativeCompletions = true;

     callPackage = true;
     mkDerivation = true;
$ nix-build pkgs/test/release/default.nix
... test execution snipped ...
/nix/store/a5hs2136icqxwi7d1y6yaqx1vvvsxkyc-all-attrs-eval-under-tryEval

If @amjoseph-nixpkgs were around, they might be able to help understand how these lines ended up in the patch. My best guess is some path dependent reason that went away with a change to the implementation.

[maralorn]

This is all new to me, but I read up on it a bit. My impression is, that setting __attrsFailEvaluation is more or less equivalent to adding that attribute name to excluded-attrnames-at-any-depth. However am-joseph seemed to be mostly concerend about eval speed and adding attrnames to excluded-attrnames-at-any-depth must be O(nixpkgs).

For buildHaskellPackages it seems that putting it in excluded-attrnames-at-any-depth might be prudent, but why then keep the __attrsFailEvaluation?

For the optparse wrapper, your last snippet seems to suggest we can get away without disabling its eval at all?

[philiptaron]

Thanks for the comment @maralorn.

This is all new to me, but I read up on it a bit. My impression is, that setting __attrsFailEvaluation is more or less equivalent to adding that attribute name to excluded-attrnames-at-any-depth.

As I see it, there are currently three possible ways of making nix-instantiate --eval --strict --json pkgs/top-level/release-attrpaths-superset.nix -A names work in the presence of attrsets which are recursive (normal, not a problem) or malformed (a problem, should be worked on.)

1. __attrsFailEvaluation. This is the most targeted and least action-at-a-distance. It means that this specific attrset should not be considered any further. It's still not great, and if a solution were found that allowed detection of recursive attrsets, it could be deleted.

2. excluded-toplevel-attrs. This is the next most targeted. It's functionally identical to adding __attrsFailEvaluation to the named attribute in pkgs/top-level/all-packages.nix. Personally speaking, I would prefer this since marking specific names as being special in this one file is very action-at-a-distance.

3. excluded-attrnames-at-any-depth. This is the least targeted. If any name in any attrset ever matches something in this list, it won't be followed. It's the equivalent of marking every possible attrset with that name as having a __attrsFailEvaluation key.

After having investigated this PR and its aims, I come away with the preference stack as follows:

1. Nothing. If it's possible to leave things alone, do that. @amjoseph-nixpkgs's comments reflect that: "If you can find a way to remove any of these entries without causing CI to fail, please do so."
2. __attrsFailEvaluation. This is targeted to one specific attrset.
3. excluded-toplevel-attrs. This is targeted to the top-level attrset only.
4. excluded-attrnames-at-any-depth. This is the biggest and most indirect tool, and should be reserved for special and well-marked cases only.

However am-joseph seemed to be mostly concerned about eval speed and adding attrnames to excluded-attrnames-at-any-depth must be O(nixpkgs).

Based on the comments that he left, I think that eval speed was important, but comprehensiveness was more important; he left the remark that "If you can find a way to remove any of these entries without causing CI to fail, please do so." on every one of these exclusion mechanisms.

For buildHaskellPackages it seems that putting it in excluded-attrnames-at-any-depth might be prudent, but why then keep the __attrsFailEvaluation?

They are fairly equivalent. I think we could concoct scenarios were adding it to excluded-attrnames-at-any-depth would exclude things that the __attrsFailEvaluation addition would not, but yeah, they're mostly equivalent. I preferred to leave the pkgs/development/haskell-modules/default.nix line since it was less action-at-a-distance than the excluded-attrnames-at-any-depth change.

For the optparse wrapper, your last snippet seems to suggest we can get away without disabling its eval at all?

That's correct.

[maralorn]

Oh, man. Sorry for being dumb. I somehow read the last to deletions as additions and was very confused by this PR. :D That also led to me asking weird investigative questions …

[nixos-discourse]

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

https://discourse.nixos.org/t/discourse-via-email/29/12

[infinisil]

Tested nix-build pkgs/test/release when merging this into current master, still works, so this LGTM!