From b9d800d46814e44db4d0a0104141aeb54e0c3c9a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 28 Nov 2024 22:57:12 +0100 Subject: [PATCH] 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. --- .github/workflows/codeowners-v2.yml | 2 + .github/workflows/eval.yml | 26 +++++- ci/eval/compare/default.nix | 22 ++++- ci/eval/compare/maintainers.nix | 123 ++++++++++++++++++++++++++++ ci/eval/compare/utils.nix | 40 +++++---- 5 files changed, 189 insertions(+), 24 deletions(-) create mode 100644 ci/eval/compare/maintainers.nix diff --git a/.github/workflows/codeowners-v2.yml b/.github/workflows/codeowners-v2.yml index 5cfeafa8489e2..8c1782437a80f 100644 --- a/.github/workflows/codeowners-v2.yml +++ b/.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: diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index bac9394500ac6..a0604bbc8fcd4 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -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 @@ -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 @@ -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: @@ -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 }} diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index 4f3b943a5a135..712a656d11add 100644 --- a/ci/eval/compare/default.nix +++ b/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 ( @@ -99,10 +103,17 @@ 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 @@ -110,5 +121,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 '' diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix new file mode 100644 index 0000000000000..0c08f85cec43b --- /dev/null +++ b/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: { + 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 diff --git a/ci/eval/compare/utils.nix b/ci/eval/compare/utils.nix index 82ba64e06a173..04ac4f6e61629 100644 --- a/ci/eval/compare/utils.nix +++ b/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