top-level/packages-info: pre-evaluate output paths#487944
top-level/packages-info: pre-evaluate output paths#487944techninja1008 wants to merge 1 commit intoNixOS:masterfrom
Conversation
|
@vcunat do you mind taking a look at this? Thanks. |
|
A complication is that I don't really have an idea how |
|
RAM usage will be quite high now. Roughly 60G after this PR, I'd say. (multiple of what it is) So if we do this, we certainly need that big-parallel flag. Also the total runtime increases to a multiple (of the 1 minute it was) and the |
|
Maybe some consumers of the json won't be too happy about the increase? For example, does https://search.nixos.org/packages load this file (to everyone's browser) or something else? EDIT: it doesn't seem to be this dumb, from a couple minute's glance. |
MattSturgeon
left a comment
There was a problem hiding this comment.
I don't yet have a strong opinion on whether this justifies the performance cost, but I've given the impl a quick once-over:
| pkgs ? import nixpkgs.outPath { }, | ||
| nix ? pkgs.nix, | ||
| lib-tests ? import ../../lib/tests/release.nix { inherit pkgs; }, | ||
| supportedSystems ? builtins.fromJSON (builtins.readFile ../../ci/supportedSystems.json), |
There was a problem hiding this comment.
As of #492103 this has moved to pkgs/top-level/release-supported-systems.json
| supportedSystems ? builtins.fromJSON (builtins.readFile ../../ci/supportedSystems.json), | |
| supportedSystems ? builtins.fromJSON (builtins.readFile ./release-supported-systems.json), |
| echo "generating packages.json" | ||
|
|
||
| NIX_STATE_DIR=$TMPDIR NIX_PATH= nix-instantiate --eval --raw --expr "import $src/pkgs/top-level/packages-info.nix {}" | sed "s|$src/||g" | jq -c > packages.json | ||
| echo ' '''${builtins.toJSON supportedSystems}''' ' | |
There was a problem hiding this comment.
Unsafe escaping/quoting is a bit of a pet peve of mine. Especially when going through multiple "layers" of syntax/evaluation.
Here we could use something like toVars, escapeShellArg, drv attrs, etc. But it'd be better to avoid the echo pipe entirely and instead write an actual JSON file that the nix expression can read.
E.g. using passAsFile or using __structuredAttrs and its $NIX_ATTRS_JSON_FILE file. That way the expression can do something like fromJSON (readFile $jsonFilePath).
Ideally we'd also avoid interpolating shell variables into the expression, using nix-instantiate --eval --expr '{src, json}: ...' --argstr src "$src" --argstr json "$jsonPath". Properly setting variables is much more robust than directly interpolating shell variables into the expression. That's a pre-existing issue though.
| if builtins.all (v: v != builtins.currentSystem) supportedSystems then | ||
| supportedSystems ++ [ builtins.currentSystem ] | ||
| else | ||
| supportedSystems; |
There was a problem hiding this comment.
Would this flow more readably if the if was flipped?
| if builtins.all (v: v != builtins.currentSystem) supportedSystems then | |
| supportedSystems ++ [ builtins.currentSystem ] | |
| else | |
| supportedSystems; | |
| if builtins.elem builtins.currentSystem supportedSystems then | |
| supportedSystems | |
| else | |
| supportedSystems ++ [ builtins.currentSystem ]; |
|
|
||
| pkgs = pkgsByArch.${builtins.currentSystem}; | ||
|
|
||
| inherit (pkgs) lib; |
There was a problem hiding this comment.
Off-topic: a lot of this code could be simplified if we had access to a systemless instance of lib (i.e. import ../lib instead of pkgs.lib).
E.g. the above code is re-implementing lib.genAttrs and lib.optional.
| outPaths = lib.listToAttrs ( | ||
| lib.map ( |
There was a problem hiding this comment.
This can be lib.genAttrs too.
| lib.map ( | ||
| x: |
There was a problem hiding this comment.
Same, although pre-existing
| { | ||
| name = x; | ||
| value = (builtins.tryEval outPaths).value or null; | ||
| # value = null; |
| in | ||
| { | ||
| name = x; | ||
| value = (builtins.tryEval outPaths).value or null; |
There was a problem hiding this comment.
The or null will never trigger here. value is always present on a tryEval result, it defaults to value = false when success = false.
If you want to return null for unsuccessful evals, you need to check .success:
| value = (builtins.tryEval outPaths).value or null; | |
| value = | |
| let | |
| result = builtins.tryEval outPaths; | |
| in | |
| if result.success then result.value else null; |
(You'd probably merge that let binding into your existing let block, of course.)
Note that this gets build from scratch in every GitHub CI run and every nixpkgs/nixos Hydra jobset, so this was design with performance in mind, the transformation we did on
Unrelated tangent: I also was not happy about the design how the cpe's where add also increased the size by more than it should have. |
packages.jsonis used by a variety of downstream services for a variety of purposes. Some of those services such as FloxHub and NixHub use it to enable an experience where a user can easily install packages by version, irrespective of nixpkgs versioning. A common optimization is that these services will pre-evaluate some or all of the packages in their index, to enable direct fetching fromhttps://cache.nixos.orgrather than having to locally evaluate nixpkgs.This change modifies
packages-info.nixso that the stubbedoutputssection of a package's info, which would previously look like this:{ "debug": null, "out": null }Now looks like this:
{ "debug": { "aarch64-linux": "/nix/store/bzb0vriv1a1kaz9zv6lzv62zkcvvm88r-python3-3.13.11-debug", "x86_64-linux": "/nix/store/gvhg8py0jr9v83ysqb86rr2s1yidppca-python3-3.13.11-debug" }, "out": { "aarch64-linux": "/nix/store/ygzfhw5nxrn7qqfqmz2s9p6cikcjqqkh-python3-3.13.11", "x86_64-linux": "/nix/store/slhpx9glq7vl99bwi93bgrhn3syv98s1-python3-3.13.11" } }packages-info.nix(andmake-tarball.nix) now takes insupportedSystemswhich it uses to determine which systems to evaluate.This increases the time and memory usage of
make-tarball, however in my testing it remains below 5 minutes, which is almost certainly worth it for the downstream optimization. This might necessitate re-tagging it asbig-parallelas in #229132.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.