workflows/eval: Request reviews from changed package maintainers (#366046)

.github/workflows/codeowners-v2.yml

@@ -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:

.github/workflows/eval.yml

@@ -133,6 +133,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
@@ -194,12 +195,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
@@ -218,6 +225,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:
@@ -252,6 +267,20 @@ jobs:
               /repos/"$REPOSITORY"/issues/"$NUMBER"/labels \
               -f "labels[]=$toAdd"
           done < <(comm -13 before after)
+
+          # maintainers.json contains GitHub IDs. Look up handles to request reviews from.
+          # There appears to be no API to request reviews based on GitHub IDs
+          jq -r 'keys[]' comparison/maintainers.json \
+            | while read -r id; do gh api /user/"$id"; done \
+            | jq -s '{ reviewers: map(.login) }' \
+            > reviewers.json
+
+          # 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 reviewers.json
+
         env:
           GH_TOKEN: ${{ github.token }}
           REPOSITORY: ${{ github.repository }}

ci/eval/compare/default.nix

@@ -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.
@@ -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 (
@@ -104,10 +108,17 @@ let
           );
       }
     );
+
+  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
@@ -115,5 +126,8 @@ runCommand "compare"
     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
   ''

ci/eval/compare/maintainers.nix

@@ -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: {
+        id = maintainer.githubId;
+        packageName = pkg.name;
+        dueToFiles = pkg.filenames;
+      }) pkg.maintainers
+    ) attrsWithModifiedFiles
+  );
+
+  byMaintainer = lib.lists.foldr (
+    ping: collector:
+    collector
+    // {
+      "${toString ping.id}" = [
+        { inherit (ping) packageName dueToFiles; }
+      ] ++ (collector."${toString ping.id}" 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

ci/eval/compare/utils.nix

@@ -11,6 +11,7 @@ rec {
     into
       {
         name = "hello";
+        packagePath = [ "hello" ];
         platform = "aarch64-linux";
       }
   */
@@ -30,6 +31,9 @@ rec {
       null
     else
       {
+        # [ "python312Packages" "numpy" ]
+        inherit packagePath;
+
         # python312Packages.numpy
         inherit name;
 
@@ -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 = [ "hello" ]; }
+        { name = "hello"; platform = "x86_64-linux"; packagePath = [ "hello" ]; }
+        { name = "hello"; platform = "aarch64-darwin"; packagePath = [ "hello" ]; }
+        { name = "hello"; platform = "x86_64-darwin"; packagePath = [ "hello" ]; }
+        { name = "bye"; platform = "aarch64-darwin"; packagePath = [ "hello" ]; }
+        { name = "bye"; platform = "x86_64-darwin"; packagePath = [ "hello" ]; }
       ]
   */
   convertToPackagePlatformAttrs =
@@ -120,12 +124,12 @@ rec {
 
     Turns
       [
-        { 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"; ... }
+        { 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"; ... }
       ]
     into
       {
@@ -145,12 +149,12 @@ rec {
 
   # Turns
   # [
-  #   { 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"; ... }
+  #   { 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"; ... }
   # ]
   #
   # into