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.
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
lib.callPackages(With): guard against a repeated mistake #81713
Conversation
One other option could be to merge these functions and automatically choose the right of the two variants (single package or a set of them). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't test, but I see no harm in such guard
in | ||
if lib.isDerivation pkgs then throw | ||
"function callPackages called on a *single* derivation `${pkgs.name or "<unknown-name>"}`." | ||
else lib.mapAttrs mkAttrOverridable pkgs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parenthetical to this PR but doing a mapAttrs
over all of pkgs
seems like a really bad idea for performance. Hadn't noticed that before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically callPackages
is used on tiny sets, fortunately.
Is there a faster way? Even if it's very hacky, we can at least use it to measure potential performance benefit – estimate how much human work is worth putting into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick nix-env -qa ...
trace showed only a few larger sets, all under 100
trace: 28
trace: 34
trace: 43
trace: 43
trace: 88
trace: 93
(caveat is that this quick test recurses into a bit fewer sets than Hydra)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll assume it's not bad enough to invest more time (of mine).
For example see the parent commit.
For example see the parent commit.
I tried some 8-second evaluation tasks on nixpkgs on this commit and its parent, and the performance impact didn't seem measurable (only a tiny bit more memory).