Skip to content

Commit

Permalink
workflows/eval: Request reviews from changed package maintainers
Browse files Browse the repository at this point in the history
  • Loading branch information
infinisil committed Dec 18, 2024
1 parent 1303c15 commit 26ba82d
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 12 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/codeowners-v2.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ name: Codeowners v2
#
# This split is done because checking code owners requires handling untrusted PR input,
# while requesting code owners requires PR write access, and those shouldn't be mixed.
#
# Note that the latter is also used for ./eval.yml requesting reviewers.

on:
pull_request_target:
Expand Down
26 changes: 24 additions & 2 deletions .github/workflows/eval.yml
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ jobs:
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
ref: ${{ needs.get-merge-commit.outputs.mergedSha }}
fetch-depth: 2
path: nixpkgs

- name: Install Nix
Expand Down Expand Up @@ -193,12 +194,18 @@ jobs:
- name: Compare against the base branch
if: steps.baseRunId.outputs.baseRunId
run: |
nix-build nixpkgs/ci -A eval.compare \
git -C nixpkgs worktree add ../base ${{ needs.attrs.outputs.baseSha }}
git -C nixpkgs diff --name-only ${{ needs.attrs.outputs.baseSha }} ${{ needs.attrs.outputs.mergedSha }} \
| jq --raw-input --slurp 'split("\n")[:-1]' > touched-files.json
# Use the base branch to get accurate maintainer info
nix-build base/ci -A eval.compare \
--arg beforeResultDir ./baseResult \
--arg afterResultDir ./prResult \
--arg touchedFilesJson ./touched-files.json \
-o comparison
cat comparison/step-summary.md >> "$GITHUB_STEP_SUMMARY"
# TODO: Request reviews from maintainers for packages whose files are modified in the PR
- name: Upload the combined results
if: steps.baseRunId.outputs.baseRunId
Expand All @@ -217,6 +224,14 @@ jobs:
pull-requests: write
statuses: write
steps:
# See ./codeowners-v2.yml, reuse the same App because we need the same permissions
# Can't use the token received from permissions above, because it can't get enough permissions
- uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0
id: app-token
with:
app-id: ${{ vars.OWNER_APP_ID }}
private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }}

- name: Download process result
uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8
with:
Expand Down Expand Up @@ -251,6 +266,13 @@ jobs:
/repos/"$REPOSITORY"/issues/"$NUMBER"/labels \
-f "labels[]=$toAdd"
done < <(comm -13 before after)
# Request reviewers from maintainers of changed output paths
GH_TOKEN=${{ steps.app-token.outputs.token }} gh api \
--method POST \
/repos/${{ github.repository }}/pulls/${{ github.event.number }}/requested_reviewers \
--input <(jq '{ reviewers: keys }' comparison/maintainers.json)
env:
GH_TOKEN: ${{ github.token }}
REPOSITORY: ${{ github.repository }}
Expand Down
22 changes: 18 additions & 4 deletions ci/eval/compare/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
writeText,
...
}:
{ beforeResultDir, afterResultDir }:
{
beforeResultDir,
afterResultDir,
touchedFilesJson,
}:
let
/*
Derivation that computes which packages are affected (added, changed or removed) between two revisions of nixpkgs.
Expand Down Expand Up @@ -77,11 +81,11 @@ let
# - values: lists of `packagePlatformPath`s
diffAttrs = diff beforeAttrs afterAttrs;

rebuilds = uniqueStrings (diffAttrs.added ++ diffAttrs.changed);
rebuildsPackagePlatformAttrs = convertToPackagePlatformAttrs rebuilds;

changed-paths =
let
rebuilds = uniqueStrings (diffAttrs.added ++ diffAttrs.changed);
rebuildsPackagePlatformAttrs = convertToPackagePlatformAttrs rebuilds;

rebuildsByPlatform = groupByPlatform rebuildsPackagePlatformAttrs;
rebuildsByKernel = groupByKernel rebuildsPackagePlatformAttrs;
rebuildCountByKernel = lib.mapAttrs (
Expand All @@ -99,16 +103,26 @@ let
labels = getLabels rebuildCountByKernel;
}
);

maintainers = import ./maintainers.nix {
changedattrs = lib.unique (map (a: a.packagePath) rebuildsPackagePlatformAttrs);
changedpathsjson = touchedFilesJson;
};
in
runCommand "compare"
{
nativeBuildInputs = [ jq ];
maintainers = builtins.toJSON maintainers;
passAsFile = [ "maintainers" ];
}
''
mkdir $out
cp ${changed-paths} $out/changed-paths.json
jq -r -f ${./generate-step-summary.jq} < ${changed-paths} > $out/step-summary.md
cp "$maintainersPath" "$out/maintainers.json"
# TODO: Compare eval stats
''
123 changes: 123 additions & 0 deletions ci/eval/compare/maintainers.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# Almost directly vendored from https://github.com/NixOS/ofborg/blob/5a4e743f192fb151915fcbe8789922fa401ecf48/ofborg/src/maintainers.nix
{ changedattrs, changedpathsjson }:
let
pkgs = import ../../.. {
system = "x86_64-linux";
config = { };
overlays = [ ];
};
inherit (pkgs) lib;

changedpaths = builtins.fromJSON (builtins.readFile changedpathsjson);

anyMatchingFile =
filename:
let
matching = builtins.filter (changed: lib.strings.hasSuffix changed filename) changedpaths;
in
(builtins.length matching) > 0;

anyMatchingFiles = files: (builtins.length (builtins.filter anyMatchingFile files)) > 0;

enrichedAttrs = builtins.map (path: {
path = path;
name = builtins.concatStringsSep "." path;
}) changedattrs;

validPackageAttributes = builtins.filter (
pkg:
if (lib.attrsets.hasAttrByPath pkg.path pkgs) then
(
if (builtins.tryEval (lib.attrsets.attrByPath pkg.path null pkgs)).success then
true
else
builtins.trace "Failed to access ${pkg.name} even though it exists" false
)
else
builtins.trace "Failed to locate ${pkg.name}." false
) enrichedAttrs;

attrsWithPackages = builtins.map (
pkg: pkg // { package = lib.attrsets.attrByPath pkg.path null pkgs; }
) validPackageAttributes;

attrsWithMaintainers = builtins.map (
pkg: pkg // { maintainers = (pkg.package.meta or { }).maintainers or [ ]; }
) attrsWithPackages;

attrsWeCanPing = builtins.filter (
pkg:
if (builtins.length pkg.maintainers) > 0 then
true
else
builtins.trace "Package has no maintainers: ${pkg.name}" false
) attrsWithMaintainers;

relevantFilenames =
drv:
(lib.lists.unique (
builtins.map (pos: lib.strings.removePrefix (toString ../..) pos.file) (
builtins.filter (x: x != null) [
(builtins.unsafeGetAttrPos "maintainers" (drv.meta or { }))
(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.
# # TODO(@oxij): remove this assert when something like NixOS/nix#1771 gets merged into nix.
# name = assert validity.handled; name + lib.optionalString
#(builtins.unsafeGetAttrPos "name" drv)
(builtins.unsafeGetAttrPos "pname" drv)
(builtins.unsafeGetAttrPos "version" drv)

# Use ".meta.position" for cases when most of the package is
# defined in a "common" section and the only place where
# reference to the file with a derivation the "pos"
# attribute.
#
# ".meta.position" has the following form:
# "pkgs/tools/package-management/nix/default.nix:155"
# We transform it to the following:
# { file = "pkgs/tools/package-management/nix/default.nix"; }
{ file = lib.head (lib.splitString ":" (drv.meta.position or "")); }
]
)
));

attrsWithFilenames = builtins.map (
pkg: pkg // { filenames = relevantFilenames pkg.package; }
) attrsWithMaintainers;

attrsWithModifiedFiles = builtins.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;

listToPing = lib.lists.flatten (
builtins.map (
pkg:
builtins.map (maintainer: {
handle = lib.toLower maintainer.github;
packageName = pkg.name;
dueToFiles = pkg.filenames;
}) pkg.maintainers
) attrsWithModifiedFiles
);

byMaintainer = lib.lists.foldr (
ping: collector:
collector
// {
"${ping.handle}" = [
{ inherit (ping) packageName dueToFiles; }
] ++ (collector."${ping.handle}" or [ ]);
}
) { } listToPing;

textForPackages =
packages: lib.strings.concatStringsSep ", " (builtins.map (pkg: pkg.packageName) packages);

textPerMaintainer = lib.attrsets.mapAttrs (
maintainer: packages: "- @${maintainer} for ${textForPackages packages}"
) byMaintainer;

packagesPerMaintainer = lib.attrsets.mapAttrs (
maintainer: packages: builtins.map (pkg: pkg.packageName) packages
) byMaintainer;
in
packagesPerMaintainer
16 changes: 10 additions & 6 deletions ci/eval/compare/utils.nix
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ rec {
into
{
name = "hello";
packagePath = [ "hello" ];
platform = "aarch64-linux";
}
*/
Expand All @@ -30,6 +31,9 @@ rec {
null
else
{
# [ "python312Packages" "numpy" ]
inherit packagePath;

# python312Packages.numpy
inherit name;

Expand All @@ -52,12 +56,12 @@ rec {
]
into
[
{ name = "hello"; platform = "aarch64-linux"; }
{ name = "hello"; platform = "x86_64-linux"; }
{ name = "hello"; platform = "aarch64-darwin"; }
{ name = "hello"; platform = "x86_64-darwin"; }
{ name = "bye"; platform = "aarch64-darwin"; }
{ name = "bye"; platform = "x86_64-darwin"; }
{ name = "hello"; platform = "aarch64-linux"; packagePath = [ ... ]; }
{ name = "hello"; platform = "x86_64-linux"; packagePath = [ ... ]; }
{ name = "hello"; platform = "aarch64-darwin"; packagePath = [ ... ]; }
{ name = "hello"; platform = "x86_64-darwin"; packagePath = [ ... ]; }
{ name = "bye"; platform = "aarch64-darwin"; packagePath = [ ... ]; }
{ name = "bye"; platform = "x86_64-darwin"; packagePath = [ ... ]; }
]
*/
convertToPackagePlatformAttrs =
Expand Down

0 comments on commit 26ba82d

Please sign in to comment.