ci/eval: fix local full eval by wolfgangwalther · Pull Request #442035 · NixOS/nixpkgs

wolfgangwalther · 2025-09-11T13:26:58Z

The change to use builtins.storePath was good - for when the store path is already part of the nix store. In all my tests so far, that was already the case, because I was iterating on the solution and the Eval results stayed the same.

But when this is run on a entirely new commit, these the values for afterDir and combinedDir are not in the store, yet. As part of running eval.full on a new commit they will be created. eval.full is linked up, so that the values passed around there will actually be derivations, which might not be realized, yet.

Checking whether the input is a path or not fixes this for both cases.

Follow up to #441746, again.

Things done

Tested locally.
Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

ci/eval/compare/default.nix

philiptaron · 2025-09-11T16:44:22Z

I have never had good experiences with lib.toDerivation, which reliably fails for me with error: expression did not evaluate to a valid derivation (no 'drvPath' attribute).

Is the thing we want just stringification? combined = "${combinedDir}";?

wolfgangwalther · 2025-09-11T16:47:04Z

Is the thing we want just stringification? combined = "${combinedDir}";?

No, the thing we want is to register a dependency on an existing nix/store path that is passed as an argument.

wolfgangwalther · 2025-09-11T16:48:09Z

I have never had good experiences with lib.toDerivation, which reliably fails for me with error: expression did not evaluate to a valid derivation (no 'drvPath' attribute).

Yes, that would probably happen when trying to instantiate (?) that thing, I guess. It's just a dummy derivation that this function creates, not a "real" one.

MattSturgeon · 2025-09-11T16:56:12Z

I have never had good experiences with lib.toDerivation, which reliably fails for me with error: expression did not evaluate to a valid derivation (no 'drvPath' attribute).

Yes, that would probably happen when trying to instantiate (?) that thing, I guess. It's just a dummy derivation that this function creates, not a "real" one.

In this case we only use the "derivation" to get its outPath, so it should be fine that it's a fake derivation.

Although this makes me wonder (again) if we should explicitly use the stringified derivation (i.e. the in-store path) rather than a real/fake derivation attrset?

It feels like we're relying on the assumption that we'll only use the stringified derivation later in the expression. So making that explicit early on feels like a good move.

philiptaron · 2025-09-11T17:32:08Z

I have a note on my notepad to remove import-from-derivation in this code. Is this at all related?

nix-instantiate --option allow-import-from-derivation false ci -A eval.full --arg baseline ./baseline --show-trace

wolfgangwalther · 2025-09-11T17:41:54Z

I don't think we have IFD there anywhere.

philiptaron · 2025-09-11T17:44:11Z

I don't think we have IFD there anywhere.

eval.full relies on it. Did you try the command? It fails like so:

Details

error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'compare'
         whose name attribute is located at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:480:13

       … while evaluating attribute 'AFTER_DIR' of derivation 'compare'

       … while calling anonymous lambda
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:798:14:
          797|         mapAttrs (
          798|           n: v:
             |              ^
          799|           assert assertMsg (isString v || isBool v || isInt v || isDerivation v)

       … in the condition of the assert statement
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:799:11:
          798|           n: v:
          799|           assert assertMsg (isString v || isBool v || isInt v || isDerivation v)
             |           ^
          800|             "The `env` attribute set can only contain derivation, string, boolean or integer attributes. The `${n}` attribute is of type ${builtins.typeOf v}.";

       … from call site
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:799:18:
          798|           n: v:
          799|           assert assertMsg (isString v || isBool v || isInt v || isDerivation v)
             |                  ^
          800|             "The `env` attribute set can only contain derivation, string, boolean or integer attributes. The `${n}` attribute is of type ${builtins.typeOf v}.";

       … while calling 'assertMsg'
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/lib/asserts.nix:50:21:
           49|   # TODO(Profpatsch): add tests that check stderr
           50|   assertMsg = pred: msg: pred || builtins.throw msg;
             |                     ^
           51|

       … in the left operand of the OR (||) operator
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/lib/asserts.nix:50:31:
           49|   # TODO(Profpatsch): add tests that check stderr
           50|   assertMsg = pred: msg: pred || builtins.throw msg;
             |                               ^
           51|

       … in the left operand of the OR (||) operator
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:799:63:
          798|           n: v:
          799|           assert assertMsg (isString v || isBool v || isInt v || isDerivation v)
             |                                                               ^
          800|             "The `env` attribute set can only contain derivation, string, boolean or integer attributes. The `${n}` attribute is of type ${builtins.typeOf v}.";

       … in the left operand of the OR (||) operator
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:799:52:
          798|           n: v:
          799|           assert assertMsg (isString v || isBool v || isInt v || isDerivation v)
             |                                                    ^
          800|             "The `env` attribute set can only contain derivation, string, boolean or integer attributes. The `${n}` attribute is of type ${builtins.typeOf v}.";

       … in the left operand of the OR (||) operator
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:799:40:
          798|           n: v:
          799|           assert assertMsg (isString v || isBool v || isInt v || isDerivation v)
             |                                        ^
          800|             "The `env` attribute set can only contain derivation, string, boolean or integer attributes. The `${n}` attribute is of type ${builtins.typeOf v}.";

       … while calling the 'isString' builtin
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:799:29:
          798|           n: v:
          799|           assert assertMsg (isString v || isBool v || isInt v || isDerivation v)
             |                             ^
          800|             "The `env` attribute set can only contain derivation, string, boolean or integer attributes. The `${n}` attribute is of type ${builtins.typeOf v}.";

       … while calling the 'storePath' builtin
         at /home/philip/Code/github.com/philiptaron/nixpkgs/working/ci/eval/compare/default.nix:16:14:
           15| let
           16|   combined = builtins.storePath combinedDir;
             |              ^
           17|

       … while calling the 'getAttr' builtin
         at <nix/derivation-internal.nix>:50:17:
           49|     value = commonAttrs // {
           50|       outPath = builtins.getAttr outputName strict;
             |                 ^
           51|       drvPath = strict.drvPath;

       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'combined-eval'
         whose name attribute is located at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:480:13

       … while evaluating attribute 'buildCommand' of derivation 'combined-eval'
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/build-support/trivial-builders/default.nix:80:17:
           79|         enableParallelBuilding = true;
           80|         inherit buildCommand name;
             |                 ^
           81|         passAsFile = [ "buildCommand" ] ++ (derivationArgs.passAsFile or [ ]);

       … while calling the 'getAttr' builtin
         at <nix/derivation-internal.nix>:50:17:
           49|     value = commonAttrs // {
           50|       outPath = builtins.getAttr outputName strict;
             |                 ^
           51|       drvPath = strict.drvPath;

       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'nixpkgs-eval-diffs'
         whose name attribute is located at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:480:13

       … while evaluating attribute 'paths' of derivation 'nixpkgs-eval-diffs'
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/build-support/trivial-builders/default.nix:618:11:
          617|           inherit preferLocalBuild allowSubstitutes;
          618|           paths = mapPaths (path: "${path}${stripPrefix}") paths;
             |           ^
          619|           passAsFile = [ "paths" ];

       … while calling anonymous lambda
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/build-support/trivial-builders/default.nix:600:11:
          599|         map (
          600|           path:
             |           ^
          601|           if path == null then

       … from call site
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/build-support/trivial-builders/default.nix:606:13:
          605|           else
          606|             f path
             |             ^
          607|         ) paths;

       … while calling anonymous lambda
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/build-support/trivial-builders/default.nix:618:29:
          617|           inherit preferLocalBuild allowSubstitutes;
          618|           paths = mapPaths (path: "${path}${stripPrefix}") paths;
             |                             ^
          619|           passAsFile = [ "paths" ];

       … while calling the 'getAttr' builtin
         at <nix/derivation-internal.nix>:50:17:
           49|     value = commonAttrs // {
           50|       outPath = builtins.getAttr outputName strict;
             |                 ^
           51|       drvPath = strict.drvPath;

       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'diff'
         whose name attribute is located at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/stdenv/generic/make-derivation.nix:480:13

       … while evaluating attribute 'buildCommand' of derivation 'diff'
         at /nix/store/qdpngwc9f0rr4ph265nwwz4hmfq8xjz9-source/pkgs/build-support/trivial-builders/default.nix:80:17:
           79|         enableParallelBuilding = true;
           80|         inherit buildCommand name;
             |                 ^
           81|         passAsFile = [ "buildCommand" ] ++ (derivationArgs.passAsFile or [ ]);

       … while calling the 'storePath' builtin
         at /home/philip/Code/github.com/philiptaron/nixpkgs/working/ci/eval/diff.nix:15:11:
           14|   before = builtins.storePath beforeDir;
           15|   after = builtins.storePath afterDir;
             |           ^
           16|

       error: path '/nix/store/l97f2vbxnydlixgk6lkky2pmbd43w68g-nixpkgs-eval-aarch64-linux' does not exist and cannot be created

wolfgangwalther · 2025-09-11T17:51:43Z

I did try this command on a different branch, but got the same error that we're trying to fix here:

error: path '/nix/store/l97f2vbxnydlixgk6lkky2pmbd43w68g-nixpkgs-eval-aarch64-linux' does not exist and cannot be created

But it will be there with or without --option allow-import-from-derivation false.

But indeed, with this PR*, it does make a difference - because builtins.readFile then throws:

error: cannot build '/nix/store/qzpdaf0aac1z3hhbcsrr6173wask108s-nixpkgs-eval-aarch64-linux.drv^out' during evaluation because the option 'allow-import-from-derivation' is disabled

I don't think we can change that, ever?

Edit:
* to clarify: It does with this PR, because the bug before is fixed. This PR does not introduce IFD, though.

wolfgangwalther · 2025-09-11T18:13:55Z

Let's put it this way: We could pretend to not do IFD in the CI case, because we serialize the steps there anyway (not much difference to what happens in the IFD case, actually, because we have multiple eval / build steps alternating, too). But it's hard, without rewriting the diff + compare parts to not rely on nix code, to avoid IFD for the local evaluation.

I don't think it's worthy or relevant goal either. IFD is perfectly fine here.

philiptaron · 2025-09-11T18:16:31Z

This PR does not introduce IFD, though.

Ah, yes, absolutely true. My wonder was if the use of IFD made the directories here more likely to encounter strange not-found errors. No evidence for it.

philiptaron

I tested it out locally and this does indeed work.

The change to use `builtins.storePath` was good - for when the store path *is* already part of the nix store. In all my tests so far, that was already the case, because I was iterating on the solution and the Eval results stayed the same. But when this is run on a entirely new commit, these the values for `afterDir` and `combinedDir` are *not* in the store, yet. As part of running `eval.full` on a new commit they will be created. `eval.full` is linked up, so that the values passed around there will actually be derivations, which might not be realized, yet. Checking whether the input is a path or not fixes this for both cases.

MattSturgeon · 2025-09-11T18:23:13Z

ci/eval/compare/default.nix

 }:
 let
-  combined = builtins.storePath combinedDir;
+  # Usually we expect a derivation, but when evaluating in multiple separate steps, we pass


If what @philiptaron is saying about toDerivation being unreliable when you try and use its result as an actual derivation is accurate, then my preference shifts back to something like:

Suggested change

# Usually we expect a derivation, but when evaluating in multiple separate steps, we pass

combined = "${if lib.isDerivation combinedDir then combinedDir else lib.toDerivation combinedDir}";

Or

Suggested change

# Usually we expect a derivation, but when evaluating in multiple separate steps, we pass

combined = if lib.isDerivation combinedDir then "${combinedDir}" else builtins.storePath combinedDir;

As these are explicit about returning a store path rather than a maybe-real/maybe-fake derivation-attrset.

I'm open to counter arguments or a demonstration that toDerivation doesn't cause the issues Philip alludes to, though.

(Edit: my comment ended up on the wrong line, so my suggestions are messed up)

I'm having a hard time to come up with a case where the fake derivation would be a problem. Would we need to.. somehow expose this derivation and then try to do something with nix-env on it or so? Like explicitly querying its drvPath?

This seems so unlikely to me that I wouldn't bother the complication.

To see the "it's not a real derivation" error message, do this in the repl (nix repl -f. in the nixpkgs root):

:b lib.toDerivation "${./default.nix}"

Yeah - to hit that, these fake derivations would need to be somehow exposed as attributes. I don't know how to get to them, though.

wolfgangwalther · 2025-09-11T18:23:51Z

Ah, yes, absolutely true. My wonder was if the use of IFD made the directories here more likely to encounter strange not-found errors. No evidence for it.

My experiene so far was the opposite: The IFD parts worked even before I made the switch to storePath - it was the parts inside the sandbox that didn't work. That makes sense, because not creating that dependency didn't make these store paths available to the sandbox. However, the evaluator runs on the host and would always find these in the store, no matter whether dependency or not.

MattSturgeon

LGTM, other than my nitpick that we could bikeshed all day 🫣

nixpkgs-ci · 2025-09-11T19:22:23Z

Successfully created backport PR for release-25.05:

[Backport release-25.05] ci/eval: fix local full eval #442157

wolfgangwalther marked this pull request as ready for review September 11, 2025 13:30

nix-owners bot requested review from MattSturgeon, Mic92, philiptaron and zowoq September 11, 2025 13:31

MattSturgeon reviewed Sep 11, 2025

View reviewed changes

ci/eval/compare/default.nix Outdated Show resolved Hide resolved

philiptaron approved these changes Sep 11, 2025

View reviewed changes

wolfgangwalther force-pushed the ci-eval-baseline-again branch from 5dc6dde to b9d4098 Compare September 11, 2025 18:18

nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 11, 2025

MattSturgeon reviewed Sep 11, 2025

View reviewed changes

MattSturgeon approved these changes Sep 11, 2025

View reviewed changes

wolfgangwalther added this pull request to the merge queue Sep 11, 2025

Merged via the queue into NixOS:master with commit 970b801 Sep 11, 2025
30 of 32 checks passed

wolfgangwalther deleted the ci-eval-baseline-again branch September 11, 2025 19:21

github-actions bot added the 8.has: port to stable This PR already has a backport to the stable release. label Sep 11, 2025

nixpkgs-ci bot mentioned this pull request Sep 11, 2025

[Backport release-25.05] ci/eval: fix local full eval #442157

Merged

1 task

nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Sep 11, 2025

	# Usually we expect a derivation, but when evaluating in multiple separate steps, we pass
	combined = "${if lib.isDerivation combinedDir then combinedDir else lib.toDerivation combinedDir}";

	# Usually we expect a derivation, but when evaluating in multiple separate steps, we pass
	combined = if lib.isDerivation combinedDir then "${combinedDir}" else builtins.storePath combinedDir;

Uh oh!

Conversation

wolfgangwalther commented Sep 11, 2025

Things done

Uh oh!

Uh oh!

philiptaron commented Sep 11, 2025

Uh oh!

wolfgangwalther commented Sep 11, 2025

Uh oh!

wolfgangwalther commented Sep 11, 2025

Uh oh!

MattSturgeon commented Sep 11, 2025

Uh oh!

philiptaron commented Sep 11, 2025

Uh oh!

wolfgangwalther commented Sep 11, 2025

Uh oh!

philiptaron commented Sep 11, 2025

Uh oh!

wolfgangwalther commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Sep 11, 2025

Uh oh!

philiptaron commented Sep 11, 2025

Uh oh!

philiptaron left a comment

Choose a reason for hiding this comment

Uh oh!

MattSturgeon Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

philiptaron Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther commented Sep 11, 2025

Uh oh!

MattSturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nixpkgs-ci bot commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wolfgangwalther commented Sep 11, 2025 •

edited

Loading

MattSturgeon Sep 11, 2025 •

edited

Loading