check-meta: wrap maintainers attribute to include team members#402991
check-meta: wrap maintainers attribute to include team members#402991winterqt merged 2 commits intoNixOS:masterfrom
Conversation
github-actions
bot
added
6.topic: python
ofborg
bot
removed
the
2.status: merge conflict
|
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. |
| maintainers = | ||
| attrs.meta.maintainers or [ ] | ||
| ++ concatMap (team: team.members or [ ]) attrs.meta.teams or [ ]; |
There was a problem hiding this comment.
Maybe add this? Unsure if it's common enough to be worth the complexity, though... probably not.
| 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 [ ]); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ((drv.meta or { }).maintainersPosition or null) | ||
| ((drv.meta or { }).teamsPosition or null) |
There was a problem hiding this comment.
Why not
| ((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).
There was a problem hiding this comment.
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.
|
Backport failed for 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 |
|
(Why remove from the stdenv board?) Also, I didn't know we backported this change to 24.11. oof. |
|
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 |
|
Makes sense. Ah, right, forgot about that. Sorry! |
|
Thanks for the work and the merge, y'all. |
The idea for the automated backport label whenever you touch Thus, I opened #405159 to do just that. |
This partially reverts 3fb04fa. As noted in a comment on repology#1497 (comment) [1] the addition of `meta.teams` to `meta.maintainers` was just a return to the status quo before the introduction of separate field. In the long run it makes sense to handle teams as single entities but while it isn't implemented let's leave them as is, since some people rely on this [2]. FWIW the largest team in nixpkgs counts 10 members so most of the team owned packages will likely still be under the limit. [1]: repology#1497 (comment) [2]: NixOS/nixpkgs#402991 (comment) Signed-off-by: Marcin Serwin <[email protected]>
This partially reverts 3fb04fa. As noted in a comment on repology#1497 [1] the addition of `meta.teams` to `meta.maintainers` was just a return to the status quo before the introduction of separate field. In the long run it makes sense to handle teams as single entities but while it isn't implemented let's leave them as is, since some people rely on this [2]. FWIW the largest team in nixpkgs counts 10 members so most of the team owned packages will likely still be under the limit. [1]: repology#1497 (comment) [2]: NixOS/nixpkgs#402991 (comment) Signed-off-by: Marcin Serwin <[email protected]>
This partially reverts 3fb04fa. As noted in a comment on repology#1497 [1] the addition of `meta.teams` to `meta.maintainers` was just a return to the status quo before the introduction of separate field. In the long run it makes sense to handle teams as single entities but while it isn't implemented let's leave them as is, since some people rely on this [2]. FWIW the largest team in nixpkgs counts 10 members so most of the team owned packages will likely still be under the limit. [1]: repology#1497 (comment) [2]: NixOS/nixpkgs#402991 (comment) Signed-off-by: Marcin Serwin <[email protected]>
This partially reverts 3fb04fa. As noted in a comment on #1497 [1] the addition of `meta.teams` to `meta.maintainers` was just a return to the status quo before the introduction of separate field. In the long run it makes sense to handle teams as single entities but while it isn't implemented let's leave them as is, since some people rely on this [2]. FWIW the largest team in nixpkgs counts 10 members so most of the team owned packages will likely still be under the limit. [1]: #1497 (comment) [2]: NixOS/nixpkgs#402991 (comment) Signed-off-by: Marcin Serwin <[email protected]>
9 participants
This is a work-around that resolves the issues that spawned from #400458 by adding the team members from teams declared in
meta.teamstometa.maintainers, effectively restoring the original behaviour of themaintainersattribute, and thus allowing downstreams to keep consumingmeta.maintainerswithout any extra changes to supportmeta.teams.Follow-up to #394797.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.