Skip to content

ci/eval: fix local full eval #442035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wolfgangwalther merged 1 commit into from
Sep 11, 2025
Merged

ci/eval: fix local full eval #442035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ci/eval/compare/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@
byName ? false,
}:
let
combined = builtins.storePath combinedDir;
# Usually we expect a derivation, but when evaluating in multiple separate steps, we pass
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@wolfgangwalther wolfgangwalther Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@philiptaron philiptaron Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"

Copy link
Copy Markdown
Contributor Author

@wolfgangwalther wolfgangwalther Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# nix store paths around. These need to be turned into (fake) derivations again to track
# dependencies properly.
# We use two steps for evaluation, because we compare results from two different checkouts.
# CI additionalls spreads evaluation across multiple workers.
combined = if lib.isDerivation combinedDir then combinedDir else lib.toDerivation combinedDir;

/*
Derivation that computes which packages are affected (added, changed or removed) between two revisions of nixpkgs.
Expand Down
9 changes: 7 additions & 2 deletions ci/eval/diff.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,13 @@
}:

let
before = builtins.storePath beforeDir;
after = builtins.storePath afterDir;
# Usually we expect a derivation, but when evaluating in multiple separate steps, we pass
# nix store paths around. These need to be turned into (fake) derivations again to track
# dependencies properly.
# We use two steps for evaluation, because we compare results from two different checkouts.
# CI additionalls spreads evaluation across multiple workers.
before = if lib.isDerivation beforeDir then beforeDir else lib.toDerivation beforeDir;
after = if lib.isDerivation afterDir then afterDir else lib.toDerivation afterDir;

/*
Computes the key difference between two attrs
Expand Down
Loading