check-meta: wrap maintainers attribute to include team members by SigmaSquadron · Pull Request #402991 · NixOS/nixpkgs

SigmaSquadron · 2025-04-30T07:47:47Z

This is a work-around that resolves the issues that spawned from #400458 by adding the team members from teams declared in meta.teams to meta.maintainers, effectively restoring the original behaviour of the maintainers attribute, and thus allowing downstreams to keep consuming meta.maintainers without any extra changes to support meta.teams.

Follow-up to #394797.

Things done

Add a 👍 reaction to pull requests you find important.

numinit · 2025-05-01T22:13:57Z

We should remove this consumer that was added in #394797: https://github.com/NixOS/nixpkgs/blob/master/ci/eval/compare/maintainers.nix#L58

Apart from that, we can close the Repology and s.n.o. PRs, since the behavior is now compatible with both of them.

numinit

Is the hasNoMaintainers predicate in check-meta now redundant, or do we still need to check both maintainers and teams?

RossComputerGuy · 2025-05-01T22:30:24Z

I think leave it, I don't think it passes through in this state.

SigmaSquadron · 2025-05-01T22:32:51Z

We should remove this consumer that was added in #394797: https://github.com/NixOS/nixpkgs/blob/master/ci/eval/compare/maintainers.nix#L58

Apart from that, we can close the Repology and s.n.o. PRs, since the behavior is now compatible with both of them.

I was hoping this workaround would be temporary, until all consumers of meta.maintainers adopt the new teams attribute.

RossComputerGuy · 2025-05-01T22:41:29Z

That's what I was thinking as well.

numinit · 2025-05-01T22:52:28Z

OK. Regardless we should just make sure that we don't duplicate pings, because it will (in the current state).

Long term I definitely agree with s.n.o listing teams. However, I don't think Repology much cares. The maintainers attribute is an external API contract that we have effectively created with them to display maintainers, so "maintainers are just inclusive of members of all teams" is the way to avoid breaking it. And we should generally try to avoid breaking external API contracts.

Speaking of which, we may want to also deduplicate maintainers in the eval in case someone is on more than one team maintaining something.

SigmaSquadron · 2025-05-01T22:54:55Z

We could wrap everything with lib.unique. It may slow down eval though; I'm not sure how efficient that function is.

numinit · 2025-05-01T23:00:37Z

Fair point. It is likely not necessary since that is a N^2. (I think what we can easily dedup is more important anyway, like the eval script, since currently I think it's adding teams to maintainers which already take the teams into account?)

numinit · 2025-05-04T17:17:33Z

We should remove this consumer that was added in #394797: https://github.com/NixOS/nixpkgs/blob/master/ci/eval/compare/maintainers.nix#L58

I think this still applies. At present it'd be concatenating maintainers (which are inclusive of teams) with all the teams. meta.maintainers or [ ] is likely sufficient.

mweinelt · 2025-05-06T04:09:21Z

It's been a week. I'm not getting repology notifications for a number of packages any more. This is breaking my workflow.

I think I've been patient, when you decided against a revert and for inheriting team members into maintainer. Frankly, this is how it should've been from the start.

But can we please get on with this?

This is a work-around that resolves the issues that spawned from NixOS#400458 by adding the team members from teams declared in `meta.teams` to `meta.maintainers`, effectively restoring the original behaviour of the `maintainers` attribute, and thus allowing downstreams to keep consuming `meta.maintainers` without any extra changes to support `meta.teams`. Follow-up to NixOS#394797. Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>

We need to pass through the maintainers and teams positions from the original meta so pings work correctly, since check-meta clobbers the original attribute positions in them. Tested with `maintainers/scripts/get-maintainer-pings-between.sh` on a handful of major packages maintained by both individuals and teams.

numinit · 2025-05-06T06:53:56Z

Did some prodding at it tonight to make sure this does not break anything that depends on it (notably, maintainer pings). Also added a comment asking to please not break this again since it is an external API and Repology depends on it.

winterqt · 2025-05-06T07:03:33Z

pkgs/stdenv/generic/check-meta.nix

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


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 [ ]);

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?

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.

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.

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.

winterqt · 2025-05-06T07:18:56Z

ci/eval/compare/maintainers.nix

+          ((drv.meta or { }).maintainersPosition or null)
+          ((drv.meta or { }).teamsPosition or null)


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

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.

winterqt

Thanks! :)

nixpkgs-ci · 2025-05-07T21:11:49Z

Backport failed for release-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.11
git worktree add -d .worktree/backport-402991-to-release-24.11 origin/release-24.11
cd .worktree/backport-402991-to-release-24.11
git switch --create backport-402991-to-release-24.11
git cherry-pick -x d807892d1f42210129f9fc18f81e5f828150cf4d 92bd743239823dc7c80572539528398f722a9278

winterqt · 2025-05-07T21:15:12Z

(Why remove from the stdenv board?)

Also, I didn't know we backported this change to 24.11. oof.

SigmaSquadron · 2025-05-07T21:17:37Z

I removed it from Stdenv and CUDA because this isn't part of their projects. The projects are added automatically if we touch certain files during treewides.

We also didn't backport this; it was another mishap caused by changing files in the ci/ directory.

winterqt · 2025-05-07T21:20:45Z

Makes sense.

Ah, right, forgot about that. Sorry!

numinit · 2025-05-07T22:17:15Z

Thanks for the work and the merge, y'all.

wolfgangwalther · 2025-05-08T08:03:18Z

We also didn't backport this; it was another mishap caused by changing files in the ci/ directory.

The idea for the automated backport label whenever you touch ci/ (except OWNERS) is, that we don't want a diff between stable and unstable for the workflows/ and ci/ folders. This will allow us to easily backport other changes related to CI whenever they come up, without hitting conflicts.

Thus, I opened #405159 to do just that.

numinit reviewed May 1, 2025

View reviewed changes

SigmaSquadron and others added 2 commits May 5, 2025 23:40

winterqt reviewed May 6, 2025

View reviewed changes

maxbol mentioned this pull request May 6, 2025

error: attribute 'maintainers' missing nix-community/neovim-nightly-overlay#885

Closed

winterqt approved these changes May 7, 2025

View reviewed changes

This was referenced May 8, 2025

Fix packages with nix teams not showing up repology/repology-updater#1492

Closed

check-meta: add a teams attribute #394797

Merged

wolfgangwalther mentioned this pull request May 8, 2025

[24.11]: ci/eval/compare: support optional byName argument #405159

Merged

1 task

AMDmi3 mentioned this pull request May 14, 2025

Nix maintainers limit repology/repology-updater#1497

Closed

marcin-serwin mentioned this pull request Jun 19, 2025

nix: Don't exclude team members repology/repology-updater#1512

Merged

wolfgangwalther mentioned this pull request Sep 4, 2025

ci/eval/compare: handle missing packages #439198

Merged

2 tasks

infinisil mentioned this pull request Sep 30, 2025

First-class GitHub team support for review requests #447514

Closed

		((drv.meta or { }).maintainersPosition or null)
		((drv.meta or { }).teamsPosition or null)

Uh oh!

Conversation

SigmaSquadron commented Apr 30, 2025 • edited by numinit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

numinit commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

numinit left a comment

Choose a reason for hiding this comment

Uh oh!

RossComputerGuy commented May 1, 2025

Uh oh!

SigmaSquadron commented May 1, 2025

Uh oh!

RossComputerGuy commented May 1, 2025

Uh oh!

numinit commented May 1, 2025

Uh oh!

SigmaSquadron commented May 1, 2025

Uh oh!

numinit commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

numinit commented May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweinelt commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

numinit commented May 6, 2025

Uh oh!

winterqt May 6, 2025

Choose a reason for hiding this comment

Uh oh!

numinit May 6, 2025

Choose a reason for hiding this comment

Uh oh!

winterqt May 6, 2025

Choose a reason for hiding this comment

Uh oh!

marcin-serwin Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

numinit Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winterqt May 6, 2025

Choose a reason for hiding this comment

Uh oh!

numinit May 6, 2025

Choose a reason for hiding this comment

Uh oh!

winterqt left a comment

Choose a reason for hiding this comment

Uh oh!

nixpkgs-ci bot commented May 7, 2025

Uh oh!

winterqt commented May 7, 2025

Uh oh!

SigmaSquadron commented May 7, 2025

Uh oh!

winterqt commented May 7, 2025

Uh oh!

numinit commented May 7, 2025

Uh oh!

wolfgangwalther commented May 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

SigmaSquadron commented Apr 30, 2025 •

edited by numinit

Loading

numinit commented May 1, 2025 •

edited

Loading

numinit commented May 1, 2025 •

edited

Loading

numinit commented May 4, 2025 •

edited

Loading

mweinelt commented May 6, 2025 •

edited

Loading

numinit Jun 25, 2025 •

edited

Loading