check-meta.nix: Add package warnings#338267
Closed
AkechiShiro wants to merge 2 commits intoNixOS:masterfrom
Closed
check-meta.nix: Add package warnings#338267AkechiShiro wants to merge 2 commits intoNixOS:masterfrom
AkechiShiro wants to merge 2 commits intoNixOS:masterfrom
Conversation
github-actions
bot
added
8.has: documentation
Contributor
Author
|
I will rebase then work on solving conflicts, marking this PR as draft for now. |
ofborg
bot
added
the
2.status: merge conflict
adisbladis
requested changes
Aug 29, 2024
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
3 times, most recently
from
03ea8fe to
f56564b
Compare
AkechiShiro
removed
the
2.status: merge conflict
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
from
f56564b to
e9eff04
Compare
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
from
e9eff04 to
1589696
Compare
ofborg
bot
added
8.has: clean-up
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
3 times, most recently
from
a1269fe to
69b5584
Compare
adisbladis
reviewed
Sep 5, 2024
pkgs/stdenv/generic/check-meta.nix
Outdated
| ++ lib.optionals (meta ? problems) ( | ||
|
|
||
| # Check problem kinds are correct | ||
| lib.pipe meta.problems [ |
Member
There was a problem hiding this comment.
This all looks very allocation/iteration heavy:
I'd completely avoid using lib.pipe in any hot paths for one.
For a normal code review I wouldn't be so pedantic about this, but check-meta is a performance special case.
Changes here needs to be carefully measured, and the Ofborg performance report doesn't include checkMeta, so it's entirely useless in this context.
Measuring nixpkgs eval with checkMeta = true before and after this PR (env NIX_SHOW_STATS=1 nix-env -qa -f ./outpaths.nix --arg checkMeta true > /dev/null):
- Before
{
"cpuTime": 271.2768249511719,
"envs": {
"bytes": 7850426888,
"elements": 413454253,
"number": 283924554
},
"gc": {
"heapSize": 22804639744,
"totalBytes": 47139879824
},
"list": {
"bytes": 987373624,
"concats": 21648163,
"elements": 123421703
},
"nrAvoided": 329081776,
"nrFunctionCalls": 261979419,
"nrLookups": 131336801,
"nrOpUpdateValuesCopied": 675865734,
"nrOpUpdates": 32046977,
"nrPrimOpCalls": 146549876,
"nrThunks": 399878601,
"sets": {
"bytes": 15552994336,
"elements": 911935337,
"number": 60126809
},
"sizes": {
"Attr": 16,
"Bindings": 16,
"Env": 16,
"Value": 24
},
"symbols": {
"bytes": 2887399,
"number": 178950
},
"values": {
"bytes": 12350285592,
"number": 514595233
}
}- After
{
"cpuTime": 296.9483947753906,
"envs": {
"bytes": 8506454064,
"elements": 438912624,
"number": 312197067
},
"gc": {
"heapSize": 22737526784,
"totalBytes": 48902439376
},
"list": {
"bytes": 1000276576,
"concats": 22264431,
"elements": 125034572
},
"nrAvoided": 349063893,
"nrFunctionCalls": 289412278,
"nrLookups": 149742381,
"nrOpUpdateValuesCopied": 694111348,
"nrOpUpdates": 35108342,
"nrPrimOpCalls": 168992563,
"nrThunks": 417292743,
"sets": {
"bytes": 15871244064,
"elements": 927562812,
"number": 64389942
},
"sizes": {
"Attr": 16,
"Bindings": 16,
"Env": 16,
"Value": 24
},
"symbols": {
"bytes": 4092168,
"number": 234982
},
"values": {
"bytes": 12774278736,
"number": 532261614
}
}
Contributor
Author
There was a problem hiding this comment.
What could be used to have better performance whilst keeping the same or similar functionality ?
Member
There was a problem hiding this comment.
-
Don't keep calling mapAttrsToList
mapAttrsToList isattrs: map fn (attrNames attrs).
You should store attrNames in a variable and keep reusing that -
lib.remove null -> concatMap
Nix has an optimisation for empty lists
The code below is a good simple example:
# Check that problem has a message (required field)
++ lib.pipe meta.problems [
(lib.mapAttrsToList ( name: problem:
if problem ? message then
null
else
"key 'meta.problems.${name}.message' is missing"
))
(lib.remove null)
]->
let
# Stick this in the outermost scope where it makes sense
problemNames = attrNames meta.problems;
in
# Nix has an optimisation for returning empty lists
# And another optimisation for lists with <= 2 elements
concatMap (name: if meta.problems.${name} ? message then [] else [ "key 'meta.problems.${name}.message' is missing" ])
Member
There was a problem hiding this comment.
And just in general: Read the lib implementations you're using here and understand that Nix is not very optimised.
Sadly we don't have good guidance on how to write performant Nix code.
NIX_SHOW_STATS=1 is your friend.
adisbladis
reviewed
Sep 7, 2024
pkgs/stdenv/generic/check-meta.nix
Outdated
| # Check that some problem kinds are unqiue | ||
| ++ lib.pipe meta.problems [ | ||
| (lib.mapAttrs (name: problem: { kind = name; } // problem)) | ||
| # Count the number of instances of each problem kind, |
Member
There was a problem hiding this comment.
I think the manual counting & checking is better implemented by combining groupBy & concatMap over problemKindsUnique'.
13 tasks
13 tasks
wegank
added
the
2.status: merge conflict
nixpkgs-ci
bot
added
the
2.status: stale
nixpkgs-ci
bot
added
10.rebuild-linux: 11-100
1 task
infinisil
added a commit
to tweag/nixpkgs
that referenced
this pull request
Dec 2, 2025
- Makes the code more clean and obvious - Allows remediations to depend on values computed during the check (useful for NixOS#338267)
infinisil
force-pushed
the
rfc127-implementation-test
branch
3 times, most recently
from
bbcf3e3 to
dcc4587
Compare
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
infinisil
force-pushed
the
rfc127-implementation-test
branch
from
af94fab to
4a1fdd9
Compare
infinisil
added a commit
to tweag/nixpkgs
that referenced
this pull request
Dec 15, 2025
- Makes the code more clean and obvious - Allows remediations to depend on values computed during the check (useful for NixOS#338267) Reapplying 42cfcdf after reversal in 0b90ed8 Compared to the original commit, this includes the fix for the problem that required the revert.
barsikus007
pushed a commit
to barsikus007/nixpkgs
that referenced
this pull request
Dec 16, 2025
- Makes the code more clean and obvious - Allows remediations to depend on values computed during the check (useful for NixOS#338267) Reapplying 42cfcdf after reversal in 0b90ed8 Compared to the original commit, this includes the fix for the problem that required the revert.
infinisil
force-pushed
the
rfc127-implementation-test
branch
2 times, most recently
from
d5de905 to
d007542
Compare
Member
|
I've now optimised this as far as I think is reasonably possible. Performance is still a bit worse than before, but much better now. Check the automatic performance report for details. I've also adjusted the error messages slightly, while making sure that the tests still pass. Now the only thing that's left imo are:
|
Run with `nix-build -A tests.problems` RFC 127: Add implementation tests * Also fix merge conflicts * Make sure that performance PR is conserved * Use RunCommand instead of alias RunCommandNoCC * Add enum types to meta-types.nix and use them for problemTypes * Nixfmt (excepted check-meta.nix) * "import" elem * Remove dead code Co-Authored-By: piegames <[email protected]> Co-Authored-By: AkechiShiro <[email protected]> Co-Authored-By: Silvan Mosberger <[email protected]> [WIP] Fast problem handler lookup switch Disable problem check for testing Better perf
infinisil
force-pushed
the
rfc127-implementation-test
branch
from
d007542 to
1af96fe
Compare
infinisil
force-pushed
the
rfc127-implementation-test
branch
from
5cd767e to
fecb526
Compare
Member
|
At this point I think it makes sense to open a new PR. Will do so shortly with the latest changes and advertise it for people to try out and give feedback :) Edit: #478539 |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
Implements RFC 127 NixOS/rfcs#127
Adds testing for multiple cases
Should supersede : #177272
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.