packages-config.nix: don't index stuff thats not getting evaled#434501
packages-config.nix: don't index stuff thats not getting evaled#434501jopejoe1 wants to merge 6 commits intoNixOS:stagingfrom
Conversation
0d434b0 to
2040d66
Compare
wolfgangwalther
left a comment
There was a problem hiding this comment.
While I like the general direction, this gives us 24924 new packages to build for hydra. I don't think we can do it like this.
pkgs/top-level/all-packages.nix
Outdated
There was a problem hiding this comment.
Commenting here, but that's for the whole commit: This also causes all these packages to additionally be built by hydra, right? I think that's a massive number of packages added.
I don't think we should or even can do this for all of these.
I see that you want to include all of these in Eval, and that makes sense. At the same time, we'd have to ensure that they're not built in Hydra, though.
There was a problem hiding this comment.
It seems like most of these come from rPackages so it's probably fine to just leave that one out for now and see what can be done about that.
There was a problem hiding this comment.
rPackages was disabled in ccd1029, like the others, too. The discussion for this was in #12203. It was also noted:
I suppose we could disable the builds for R since those packages tend to compile very quickly
So maybe, build-wise, adding R packages wouldn't be that much of a problem. Not sure. Eval times would certainly go up, though. For GHA, this is an increase of 15 seconds, from 2:30 to 2:45 right now. That should work.
I can't estimate what the consequences for Hydra would be, however.
@vcunat you were involved in that issue above back then as well. WDYT? Are we ready to enable rPackages again or not?
There was a problem hiding this comment.
rPackageswas disabled in ccd1029, like the others, too. The discussion for this was in #12203. It was also noted:
Note that some packages that have recurseIntoAttrs (e.g. ocamlPackages) were disabled this way, too. So it is also worth checking (though it may be hard with the time that has passed) why dontRecurseIntoAttrs was added for these package sets. I suspect that eval time on a simple nix-env (which is the stable way to list packages) was a factor, too.
There was a problem hiding this comment.
rPackageswas disabled in ccd1029, like the others, too. The discussion for this was in #12203. It was also noted:I suppose we could disable the builds for R since those packages tend to compile very quickly
So maybe, build-wise, adding R packages wouldn't be that much of a problem. Not sure. Eval times would certainly go up, though. For GHA, this is an increase of 15 seconds, from 2:30 to 2:45 right now. That should work.
I can't estimate what the consequences for Hydra would be, however.
@vcunat you were involved in that issue above back then as well. WDYT? Are we ready to enable
rPackagesagain or not?
I would very much like to see rPackages re-enabled if there's sufficient capacity. With it disabled, we have very little visibility of the state of the tree as the r-updates jobset has also been disabled in hydra since last year. Currently I'm running a hydra instance on my desktop to help with bumping rPackges while maintining a good overall state.
I've attached a plot using data I took from my hydra instance (buildoutputs table in postgres) showing the build times for packages. It shows that the vast majority of builds in the tree are quite small, with 99% of jobs finishing in under 60s. Evaluation seems to take ~ 108s.
There was a problem hiding this comment.
Let's split the commit that adds all the recurseIntoAttrs into separate commits in separate PRs - one for each package set that is touched. This will allow us to see rebuild numbers for each of these changes. Then we can judge these case-by-case.
There was a problem hiding this comment.
I suppose we could disable the builds for R since those packages tend to compile very quickly
So maybe, build-wise, adding R packages wouldn't be that much of a problem.
For hydra.nixos.org, it seems build time of each packages affects much less the throughput than the number of packages to build. In other words, many small packages are much harder than a few large packages for hydra.nixos.org.
This is our experience for emacsPackages. emacsPackages are 6k (for one platform) small packages. Building them takes 30 minutes on my desktop. However, due to its large number, we were kindly asked by the infra team to target staging. (I totally support infra team's request and I am sorry we added some extra work for them. I mention these only to help the discussion here: estimating the potential influence of this PR.)
pkgs/top-level/packages-config.nix
Outdated
There was a problem hiding this comment.
Not sure why this is removed. Isn't this required to get emacsPackages to show up on the search?
There was a problem hiding this comment.
emacsPackages is behind config.allowAliases, which does not seem like something that should appear in search.
There was a problem hiding this comment.
I don't understand the conclusion, tbh. Maybe your sentence is missing a "not"? I still wouldn't agree with it, but it would at least make more sense.
This file (packages-config.nix) sets allowAliases = false. This means that aliases will by default not show up. If emacsPackages is an alias - it won't show up. But emacs.pkgs is an attribute on a derivation, so won't be recursed into either. That means the emacs packages are not showing up anywhere this way.
Also consider:
nix-repl> emacsPackages.recurseForDerivations
error: attribute 'recurseForDerivations' missing
at «string»:1:1:
1| emacsPackages.recurseForDerivations
| ^
(even if the alias was considered, or emacsPackages was not an alias to begin with, it still wouldn't recurse into that attrset)
There was a problem hiding this comment.
The correct solution would probably be to move it out of aliases.nix so that it exists when allowAliases is false, as I see everything behind that as being deprecated, and if that is not the case for emacsPackages it should not be there.
nixpkgs/pkgs/top-level/aliases.nix
Line 639 in cefe143
There was a problem hiding this comment.
It has been intentionally moved there. emacsPackages in search is a workaround so that the package set gets indexed in search at all since it is impossible to get search to index emacs.pkgs. We do need this unless nix-env/search.nixos.org is sufficiently adapted.
There was a problem hiding this comment.
That sounds like emacsPackages should not be in aliases.nix but in all-packages.nix
There was a problem hiding this comment.
But
emacs.pkgsis an attribute on a derivation, so won't be recursed into either. That means the emacs packages are not showing up anywhere this way.Also consider:
nix-repl> emacsPackages.recurseForDerivations error: attribute 'recurseForDerivations' missing at «string»:1:1: 1| emacsPackages.recurseForDerivations | ^
First, we want to keep emacsPackages as a deprecated alias of emacs.pkgs and remove emacsPackages in the future.
Second, we want emacsPackages (emacs.pkgs actually) to be shown on search. And they actually are shown on search now.
nix-repl> emacs.pkgs.recurseForDerivations
true
pkgs/top-level/packages-config.nix
Outdated
There was a problem hiding this comment.
I don't think we can ultimately do that last change. For example, I'd like to add postgresqlPackages here, too, eventually (just didn't get to it, yet). You can currently search only for postgresql17Packages (and all other version numbers, specifically), but I argue it should be the other way around: No need to expose all the packages 5 times for different PG versions, it would be better if we only exposed postgresqlPackages in the search.
There was a problem hiding this comment.
So adding the numbered ones here, like I left minimal-bootstrap and tests for, should work for those too.
There was a problem hiding this comment.
Adding these here would only remove them, but not add postgresqlPackages itself, which is not showing up right now.
There was a problem hiding this comment.
In this case it actually would, as postgresqlPackages is only not added to search because it is just a repeat of postgresql17Packages, and if those no longer exist, it would be found in search.
There was a problem hiding this comment.
Oh, that would certainly explain my confusion about why postgresqlPackages is not showing up :)
There was a problem hiding this comment.
Also, seems like lix is way faster with NIX_STATE_DIR=$TMPDIR NIX_PATH= time nix-env -f ../.. -qa --meta --json --show-trace --arg config 'import ./pkgs/top-level/packages-config.nix' > packages.json which gets used to generate the index for search.nixos.org.
On nix (Nix) 2.28.4: 370.68user 45.55system 7:00.56elapsed 98%CPU (0avgtext+0avgdata 8870956maxresident)k 0inputs+792480outputs (0major+2326175minor)pagefaults 0swaps
On nix (Lix, like Nix) 2.91.3: 28.24user 4.69system 0:33.22elapsed 99%CPU (0avgtext+0avgdata 8969504maxresident)k 0inputs+803856outputs (0major+2353707minor)pagefaults 0swaps
There was a problem hiding this comment.
Yes, that regression is fixed with Nix 2.30.
There was a problem hiding this comment.
I wonder whether the deduplication you mentioned is related to that performance difference... and maybe postgresqlPackages would show up automatically, if the Nix version used for building this tarball was changed?
We use Nix 2.30 in GHA for performance, but I think Hydra does not use it. It probably uses 2.28 or 2.29... and that's where the results for search are coming from.
There was a problem hiding this comment.
I tested it with 2.30.2 and that still does the deduping.
There was a problem hiding this comment.
This actually does not use the nix defined by Hydra, but its independently declared one:
nixpkgs/pkgs/top-level/make-tarball.nix
Line 23 in e4f2852
2040d66 to
3ecc9e2
Compare
3ecc9e2 to
343193f
Compare
wolfgangwalther
left a comment
There was a problem hiding this comment.
Maybe we should split the PR for the moment and put the first 3 commits in a separate one? These should be uncontroversial, so we could do them already.
these package set where already recoursed into
343193f to
04948a4
Compare
|
This got split into seprate prs so no need for this anymore |
Eval or remove all the package sets that had special treatment in packages-config.nix and only use it to filter stuff out from search like minimal-bootstrap and tests.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.