Skip to content

top-level/packages-info: pre-evaluate output paths #487944

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

Open
techninja1008 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

top-level/packages-info: pre-evaluate output paths #487944

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
1 change: 1 addition & 0 deletions ci/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,5 +200,6 @@ rec {
officialRelease = false;
inherit pkgs lib-tests;
nix = pkgs.nixVersions.latest;
supportedSystems = [ system ];
};
}
8 changes: 7 additions & 1 deletion pkgs/top-level/make-tarball.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
pkgs ? import nixpkgs.outPath { },
nix ? pkgs.nix,
lib-tests ? import ../../lib/tests/release.nix { inherit pkgs; },
supportedSystems ? builtins.fromJSON (builtins.readFile ../../ci/supportedSystems.json),
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

As of #492103 this has moved to pkgs/top-level/release-supported-systems.json

Suggested change
supportedSystems ? builtins.fromJSON (builtins.readFile ../../ci/supportedSystems.json),
supportedSystems ? builtins.fromJSON (builtins.readFile ./release-supported-systems.json),

}:

pkgs.releaseTools.sourceTarball {
Expand Down Expand Up @@ -44,7 +45,12 @@ pkgs.releaseTools.sourceTarball {
checkPhase = ''
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}''' ' |
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

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.

NIX_STATE_DIR=$TMPDIR NIX_PATH= nix-instantiate \
--eval --raw --arg-from-stdin supportedSystems \
--expr "{ supportedSystems }: import $src/pkgs/top-level/packages-info.nix { supportedSystems = builtins.fromJSON supportedSystems; }" |
sed "s|$src/||g" |
jq -c > packages.json

# Arbitrary number. The index has ~115k packages as of April 2024.
if [ $(jq -r '.packages | length' < packages.json) -lt 100000 ]; then
Expand Down
61 changes: 56 additions & 5 deletions pkgs/top-level/packages-info.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,29 @@
{
trace ? false,
supportedSystems,
}:

let
pkgs = import ../.. { config = import ./packages-config.nix; };
pkgsSystems =
if builtins.all (v: v != builtins.currentSystem) supportedSystems then
supportedSystems ++ [ builtins.currentSystem ]
else
supportedSystems;
Comment on lines +8 to +11
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

Would this flow more readably if the if was flipped?

Suggested change
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 ];


pkgsByArch = builtins.listToAttrs (
map (system: {
name = system;
value = import ../.. {
inherit system;
config = import ./packages-config.nix;
};
}) pkgsSystems
);

pkgs = pkgsByArch.${builtins.currentSystem};

inherit (pkgs) lib;
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

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.


generateInfo =
path: value:
let
Expand All @@ -23,10 +42,42 @@ let
system
;
${if value ? "outputs" then "outputs" else null} = lib.listToAttrs (
lib.map (x: {
name = x;
value = null;
}) value.outputs
lib.map (
x:
Comment on lines +45 to +46
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

Same, although pre-existing

let
platforms = lib.intersectLists (value.meta.hydraPlatforms
or (lib.subtractLists (value.meta.badPlatforms or [ ]) (value.meta.platforms or supportedSystems))
) supportedSystems;

outPaths = lib.listToAttrs (
lib.map (
Comment on lines +52 to +53
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

This can be lib.genAttrs too.

platform:
let
drvForPlatform =
if lib.hasAttrByPath path pkgsByArch.${platform} then
lib.getAttrFromPath path pkgsByArch.${platform}
else
null;
outPath =
if builtins.isAttrs drvForPlatform && drvForPlatform ? ${x}.outPath then
builtins.unsafeDiscardStringContext drvForPlatform.${x}.outPath
else
null;
eval = builtins.tryEval outPath;
in
{
name = platform;
value = if eval.success then eval.value else null;
}
) platforms
);
in
{
name = x;
value = (builtins.tryEval outPaths).value or null;
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

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:

Suggested change
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.)

# value = null;
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon Feb 28, 2026

Choose a reason for hiding this comment

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

Why the comment?

}
) value.outputs
);
# TODO: Remove the following two fallbacks when all packages have been fixed.
# Note: pname and version are *required* by repology, so do not change to
Expand Down
2 changes: 1 addition & 1 deletion pkgs/top-level/release-small.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ in
{

tarball = import ./make-tarball.nix {
inherit nixpkgs;
inherit nixpkgs supportedSystems;
officialRelease = false;
};

Expand Down
1 change: 1 addition & 0 deletions pkgs/top-level/release.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ let
pkgs
nixpkgs
officialRelease
supportedSystems
;
};

Expand Down
Loading