Skip to content

Commit b9d800d

Browse files
committed
workflows/eval: Request reviews from changed package maintainers
Currently we need to rely on ofborg requesting reviews from package maintainers, which takes a while with ofborg's eval queue. Since recently we're doing faster evaluations with GitHub Actions, which contain all necessary information to determine reviewers of changed packages the same way ofborg does. This PR takes advantage of that.
1 parent 1303c15 commit b9d800d

5 files changed

Lines changed: 189 additions & 24 deletions

File tree

.github/workflows/codeowners-v2.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ name: Codeowners v2
1919
#
2020
# This split is done because checking code owners requires handling untrusted PR input,
2121
# while requesting code owners requires PR write access, and those shouldn't be mixed.
22+
#
23+
# Note that the latter is also used for ./eval.yml requesting reviewers.
2224

2325
on:
2426
pull_request_target:

.github/workflows/eval.yml

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ jobs:
132132
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
133133
with:
134134
ref: ${{ needs.get-merge-commit.outputs.mergedSha }}
135+
fetch-depth: 2
135136
path: nixpkgs
136137

137138
- name: Install Nix
@@ -193,12 +194,18 @@ jobs:
193194
- name: Compare against the base branch
194195
if: steps.baseRunId.outputs.baseRunId
195196
run: |
196-
nix-build nixpkgs/ci -A eval.compare \
197+
git -C nixpkgs worktree add ../base ${{ needs.attrs.outputs.baseSha }}
198+
git -C nixpkgs diff --name-only ${{ needs.attrs.outputs.baseSha }} ${{ needs.attrs.outputs.mergedSha }} \
199+
| jq --raw-input --slurp 'split("\n")[:-1]' > touched-files.json
200+
201+
# Use the base branch to get accurate maintainer info
202+
nix-build base/ci -A eval.compare \
197203
--arg beforeResultDir ./baseResult \
198204
--arg afterResultDir ./prResult \
205+
--arg touchedFilesJson ./touched-files.json \
199206
-o comparison
207+
200208
cat comparison/step-summary.md >> "$GITHUB_STEP_SUMMARY"
201-
# TODO: Request reviews from maintainers for packages whose files are modified in the PR
202209
203210
- name: Upload the combined results
204211
if: steps.baseRunId.outputs.baseRunId
@@ -217,6 +224,14 @@ jobs:
217224
pull-requests: write
218225
statuses: write
219226
steps:
227+
# See ./codeowners-v2.yml, reuse the same App because we need the same permissions
228+
# Can't use the token received from permissions above, because it can't get enough permissions
229+
- uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0
230+
id: app-token
231+
with:
232+
app-id: ${{ vars.OWNER_APP_ID }}
233+
private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }}
234+
220235
- name: Download process result
221236
uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8
222237
with:
@@ -251,6 +266,13 @@ jobs:
251266
/repos/"$REPOSITORY"/issues/"$NUMBER"/labels \
252267
-f "labels[]=$toAdd"
253268
done < <(comm -13 before after)
269+
270+
# Request reviewers from maintainers of changed output paths
271+
GH_TOKEN=${{ steps.app-token.outputs.token }} gh api \
272+
--method POST \
273+
/repos/${{ github.repository }}/pulls/${{ github.event.number }}/requested_reviewers \
274+
--input <(jq '{ reviewers: keys }' comparison/maintainers.json)
275+
254276
env:
255277
GH_TOKEN: ${{ github.token }}
256278
REPOSITORY: ${{ github.repository }}

ci/eval/compare/default.nix

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@
55
writeText,
66
...
77
}:
8-
{ beforeResultDir, afterResultDir }:
8+
{
9+
beforeResultDir,
10+
afterResultDir,
11+
touchedFilesJson,
12+
}:
913
let
1014
/*
1115
Derivation that computes which packages are affected (added, changed or removed) between two revisions of nixpkgs.
@@ -77,11 +81,11 @@ let
7781
# - values: lists of `packagePlatformPath`s
7882
diffAttrs = diff beforeAttrs afterAttrs;
7983

84+
rebuilds = uniqueStrings (diffAttrs.added ++ diffAttrs.changed);
85+
rebuildsPackagePlatformAttrs = convertToPackagePlatformAttrs rebuilds;
86+
8087
changed-paths =
8188
let
82-
rebuilds = uniqueStrings (diffAttrs.added ++ diffAttrs.changed);
83-
rebuildsPackagePlatformAttrs = convertToPackagePlatformAttrs rebuilds;
84-
8589
rebuildsByPlatform = groupByPlatform rebuildsPackagePlatformAttrs;
8690
rebuildsByKernel = groupByKernel rebuildsPackagePlatformAttrs;
8791
rebuildCountByKernel = lib.mapAttrs (
@@ -99,16 +103,26 @@ let
99103
labels = getLabels rebuildCountByKernel;
100104
}
101105
);
106+
107+
maintainers = import ./maintainers.nix {
108+
changedattrs = lib.unique (map (a: a.packagePath) rebuildsPackagePlatformAttrs);
109+
changedpathsjson = touchedFilesJson;
110+
};
102111
in
103112
runCommand "compare"
104113
{
105114
nativeBuildInputs = [ jq ];
115+
maintainers = builtins.toJSON maintainers;
116+
passAsFile = [ "maintainers" ];
106117
}
107118
''
108119
mkdir $out
109120
110121
cp ${changed-paths} $out/changed-paths.json
111122
112123
jq -r -f ${./generate-step-summary.jq} < ${changed-paths} > $out/step-summary.md
124+
125+
cp "$maintainersPath" "$out/maintainers.json"
126+
113127
# TODO: Compare eval stats
114128
''

ci/eval/compare/maintainers.nix

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
# Almost directly vendored from https://github.com/NixOS/ofborg/blob/5a4e743f192fb151915fcbe8789922fa401ecf48/ofborg/src/maintainers.nix
2+
{ changedattrs, changedpathsjson }:
3+
let
4+
pkgs = import ../../.. {
5+
system = "x86_64-linux";
6+
config = { };
7+
overlays = [ ];
8+
};
9+
inherit (pkgs) lib;
10+
11+
changedpaths = builtins.fromJSON (builtins.readFile changedpathsjson);
12+
13+
anyMatchingFile =
14+
filename:
15+
let
16+
matching = builtins.filter (changed: lib.strings.hasSuffix changed filename) changedpaths;
17+
in
18+
(builtins.length matching) > 0;
19+
20+
anyMatchingFiles = files: (builtins.length (builtins.filter anyMatchingFile files)) > 0;
21+
22+
enrichedAttrs = builtins.map (path: {
23+
path = path;
24+
name = builtins.concatStringsSep "." path;
25+
}) changedattrs;
26+
27+
validPackageAttributes = builtins.filter (
28+
pkg:
29+
if (lib.attrsets.hasAttrByPath pkg.path pkgs) then
30+
(
31+
if (builtins.tryEval (lib.attrsets.attrByPath pkg.path null pkgs)).success then
32+
true
33+
else
34+
builtins.trace "Failed to access ${pkg.name} even though it exists" false
35+
)
36+
else
37+
builtins.trace "Failed to locate ${pkg.name}." false
38+
) enrichedAttrs;
39+
40+
attrsWithPackages = builtins.map (
41+
pkg: pkg // { package = lib.attrsets.attrByPath pkg.path null pkgs; }
42+
) validPackageAttributes;
43+
44+
attrsWithMaintainers = builtins.map (
45+
pkg: pkg // { maintainers = (pkg.package.meta or { }).maintainers or [ ]; }
46+
) attrsWithPackages;
47+
48+
attrsWeCanPing = builtins.filter (
49+
pkg:
50+
if (builtins.length pkg.maintainers) > 0 then
51+
true
52+
else
53+
builtins.trace "Package has no maintainers: ${pkg.name}" false
54+
) attrsWithMaintainers;
55+
56+
relevantFilenames =
57+
drv:
58+
(lib.lists.unique (
59+
builtins.map (pos: lib.strings.removePrefix (toString ../..) pos.file) (
60+
builtins.filter (x: x != null) [
61+
(builtins.unsafeGetAttrPos "maintainers" (drv.meta or { }))
62+
(builtins.unsafeGetAttrPos "src" drv)
63+
# broken because name is always set by stdenv:
64+
# # A hack to make `nix-env -qa` and `nix search` ignore broken packages.
65+
# # TODO(@oxij): remove this assert when something like NixOS/nix#1771 gets merged into nix.
66+
# name = assert validity.handled; name + lib.optionalString
67+
#(builtins.unsafeGetAttrPos "name" drv)
68+
(builtins.unsafeGetAttrPos "pname" drv)
69+
(builtins.unsafeGetAttrPos "version" drv)
70+
71+
# Use ".meta.position" for cases when most of the package is
72+
# defined in a "common" section and the only place where
73+
# reference to the file with a derivation the "pos"
74+
# attribute.
75+
#
76+
# ".meta.position" has the following form:
77+
# "pkgs/tools/package-management/nix/default.nix:155"
78+
# We transform it to the following:
79+
# { file = "pkgs/tools/package-management/nix/default.nix"; }
80+
{ file = lib.head (lib.splitString ":" (drv.meta.position or "")); }
81+
]
82+
)
83+
));
84+
85+
attrsWithFilenames = builtins.map (
86+
pkg: pkg // { filenames = relevantFilenames pkg.package; }
87+
) attrsWithMaintainers;
88+
89+
attrsWithModifiedFiles = builtins.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;
90+
91+
listToPing = lib.lists.flatten (
92+
builtins.map (
93+
pkg:
94+
builtins.map (maintainer: {
95+
handle = lib.toLower maintainer.github;
96+
packageName = pkg.name;
97+
dueToFiles = pkg.filenames;
98+
}) pkg.maintainers
99+
) attrsWithModifiedFiles
100+
);
101+
102+
byMaintainer = lib.lists.foldr (
103+
ping: collector:
104+
collector
105+
// {
106+
"${ping.handle}" = [
107+
{ inherit (ping) packageName dueToFiles; }
108+
] ++ (collector."${ping.handle}" or [ ]);
109+
}
110+
) { } listToPing;
111+
112+
textForPackages =
113+
packages: lib.strings.concatStringsSep ", " (builtins.map (pkg: pkg.packageName) packages);
114+
115+
textPerMaintainer = lib.attrsets.mapAttrs (
116+
maintainer: packages: "- @${maintainer} for ${textForPackages packages}"
117+
) byMaintainer;
118+
119+
packagesPerMaintainer = lib.attrsets.mapAttrs (
120+
maintainer: packages: builtins.map (pkg: pkg.packageName) packages
121+
) byMaintainer;
122+
in
123+
packagesPerMaintainer

ci/eval/compare/utils.nix

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ rec {
1111
into
1212
{
1313
name = "hello";
14+
packagePath = [ "hello" ];
1415
platform = "aarch64-linux";
1516
}
1617
*/
@@ -30,6 +31,9 @@ rec {
3031
null
3132
else
3233
{
34+
# [ "python312Packages" "numpy" ]
35+
inherit packagePath;
36+
3337
# python312Packages.numpy
3438
inherit name;
3539

@@ -52,12 +56,12 @@ rec {
5256
]
5357
into
5458
[
55-
{ name = "hello"; platform = "aarch64-linux"; }
56-
{ name = "hello"; platform = "x86_64-linux"; }
57-
{ name = "hello"; platform = "aarch64-darwin"; }
58-
{ name = "hello"; platform = "x86_64-darwin"; }
59-
{ name = "bye"; platform = "aarch64-darwin"; }
60-
{ name = "bye"; platform = "x86_64-darwin"; }
59+
{ name = "hello"; platform = "aarch64-linux"; packagePath = [ "hello" ]; }
60+
{ name = "hello"; platform = "x86_64-linux"; packagePath = [ "hello" ]; }
61+
{ name = "hello"; platform = "aarch64-darwin"; packagePath = [ "hello" ]; }
62+
{ name = "hello"; platform = "x86_64-darwin"; packagePath = [ "hello" ]; }
63+
{ name = "bye"; platform = "aarch64-darwin"; packagePath = [ "hello" ]; }
64+
{ name = "bye"; platform = "x86_64-darwin"; packagePath = [ "hello" ]; }
6165
]
6266
*/
6367
convertToPackagePlatformAttrs =
@@ -120,12 +124,12 @@ rec {
120124
121125
Turns
122126
[
123-
{ name = "hello"; platform = "aarch64-linux"; }
124-
{ name = "hello"; platform = "x86_64-linux"; }
125-
{ name = "hello"; platform = "aarch64-darwin"; }
126-
{ name = "hello"; platform = "x86_64-darwin"; }
127-
{ name = "bye"; platform = "aarch64-darwin"; }
128-
{ name = "bye"; platform = "x86_64-darwin"; }
127+
{ name = "hello"; platform = "aarch64-linux"; ... }
128+
{ name = "hello"; platform = "x86_64-linux"; ... }
129+
{ name = "hello"; platform = "aarch64-darwin"; ... }
130+
{ name = "hello"; platform = "x86_64-darwin"; ... }
131+
{ name = "bye"; platform = "aarch64-darwin"; ... }
132+
{ name = "bye"; platform = "x86_64-darwin"; ... }
129133
]
130134
into
131135
{
@@ -145,12 +149,12 @@ rec {
145149

146150
# Turns
147151
# [
148-
# { name = "hello"; platform = "aarch64-linux"; }
149-
# { name = "hello"; platform = "x86_64-linux"; }
150-
# { name = "hello"; platform = "aarch64-darwin"; }
151-
# { name = "hello"; platform = "x86_64-darwin"; }
152-
# { name = "bye"; platform = "aarch64-darwin"; }
153-
# { name = "bye"; platform = "x86_64-darwin"; }
152+
# { name = "hello"; platform = "aarch64-linux"; ... }
153+
# { name = "hello"; platform = "x86_64-linux"; ... }
154+
# { name = "hello"; platform = "aarch64-darwin"; ... }
155+
# { name = "hello"; platform = "x86_64-darwin"; ... }
156+
# { name = "bye"; platform = "aarch64-darwin"; ... }
157+
# { name = "bye"; platform = "x86_64-darwin"; ... }
154158
# ]
155159
#
156160
# into

0 commit comments

Comments
 (0)