Skip to content

check-meta: wrap maintainers attribute to include team members #402991

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

Merged
winterqt merged 2 commits into from
May 7, 2025
Merged

check-meta: wrap maintainers attribute to include team members #402991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d807892
check-meta: wrap maintainers attribute to include team members
SigmaSquadron May 1, 2025
92bd743
ci/eval: use correct maintainer and team positions
numinit May 6, 2025
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
7 changes: 3 additions & 4 deletions ci/eval/compare/maintainers.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ let
// {
# TODO: Refactor this so we can ping entire teams instead of the individual members.
# Note that this will require keeping track of GH team IDs in "maintainers/teams.nix".
maintainers =
meta.maintainers or [ ]
++ lib.flatten (map (team: team.members or [ ]) (meta.teams or [ ]));
maintainers = meta.maintainers or [ ];
}
) attrsWithPackages;

Expand All @@ -64,7 +62,8 @@ let
(lib.lists.unique (
builtins.map (pos: lib.strings.removePrefix (toString ../..) pos.file) (
builtins.filter (x: x != null) [
(builtins.unsafeGetAttrPos "maintainers" (drv.meta or { }))
((drv.meta or { }).maintainersPosition or null)
((drv.meta or { }).teamsPosition or null)
Comment on lines +65 to +66
Copy link
Member

@winterqt winterqt May 6, 2025

Choose a reason for hiding this comment

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

Why not

Suggested change
((drv.meta or { }).maintainersPosition or null)
((drv.meta or { }).teamsPosition or null)
(builtins.unsafeGetAttrPos "maintainers" (drv.meta or { }))
(builtins.unsafeGetAttrPos "teams" (drv.meta or { }))

? Ideally we wouldn't add (slight!) extra eval overhead just for CI scripts (which AFAIU check-meta will do).

Copy link
Contributor

@numinit numinit May 6, 2025

Choose a reason for hiding this comment

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

The new maintainers attribute was actually clobbering the original one's position, so it would return check-meta.nix every time rather than the real declaring file. I needed a way to pull the location of the original maintainers (pre-teams concatenation) out of there, and just did the same with teams. If it's written this way, it'll return a position pointing to check-meta.nix.

check-meta.nix is a little counterintuitive, but it's a bunch of // attrset updates that both prefers certain things in the meta and overrides other things in it. Maintainers is written as one of the overrides so I made sure we're holding onto the original.

(builtins.unsafeGetAttrPos "src" drv)
# broken because name is always set by stdenv:
# # A hack to make `nix-env -qa` and `nix search` ignore broken packages.
Expand Down
17 changes: 17 additions & 0 deletions pkgs/stdenv/generic/check-meta.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,10 @@ let
isFcitxEngine = bool;
isIbusEngine = bool;
isGutenprint = bool;

# Used for the original location of the maintainer and team attributes to assist with pings.
maintainersPosition = any;
teamsPosition = any;
};

checkMetaAttr =
Expand Down Expand Up @@ -589,11 +593,24 @@ let
)
] ++ optional (hasOutput "man") "man";
}
// {
# CI scripts look at these to determine pings.
maintainersPosition = builtins.unsafeGetAttrPos "maintainers" (attrs.meta or { });
teamsPosition = builtins.unsafeGetAttrPos "teams" (attrs.meta or { });
}
// attrs.meta or { }
# Fill `meta.position` to identify the source location of the package.
// optionalAttrs (pos != null) {
position = pos.file + ":" + toString pos.line;
}
// {
# Maintainers should be inclusive of teams.
# Note that there may be external consumers of this API (repology, for instance) -
# if you add a new maintainer or team attribute please ensure that this expectation is still met.
maintainers =
attrs.meta.maintainers or [ ]
++ concatMap (team: team.members or [ ]) attrs.meta.teams or [ ];
Comment on lines +610 to +612
Copy link
Member

@winterqt winterqt May 6, 2025

Choose a reason for hiding this comment

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

Maybe add this? Unsure if it's common enough to be worth the complexity, though... probably not.

Suggested change
maintainers =
attrs.meta.maintainers or [ ]
++ concatMap (team: team.members or [ ]) attrs.meta.teams or [ ];
maintainers =
lib.unique (attrs.meta.maintainers or [ ]
++ concatMap (team: team.members or [ ]) attrs.meta.teams or [ ]);

Copy link
Contributor

@numinit numinit May 6, 2025

Choose a reason for hiding this comment

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

I saw it come up in a couple of the places I tried. Another idea is a listToAttrs, but that'll probably be slightly heavier on memory since we'll just throw out the attrset anyway. May be worth a shot, though, since it's not N^2 at least?

Copy link
Member

@winterqt winterqt May 6, 2025

Choose a reason for hiding this comment

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

I mean, we could always just punt it back onto the consumer -- we already had this potential duplication pre-meta.teams, so it wouldn't be a change, just a nicety.

Copy link
Contributor

@marcin-serwin marcin-serwin Jun 25, 2025

Choose a reason for hiding this comment

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

I noticed that this change introduced duplication in the qgis package: team.members seem to be first added to qgis.unwrapped and then again in the main derivation because qgis inherits its meta.

Copy link
Contributor

@numinit numinit Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Now that Nix has tracing support, it'll be interesting to use it to test dedup while computing meta. It's what I was mostly missing here.

}
// {
# Expose the result of the checks for everyone to see.
unfree = hasUnfreeLicense attrs;
Expand Down