Skip to content

Commit

Permalink
workflows/eval: Request reviews from changed package maintainers
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
infinisil committed Dec 18, 2024
1 parent 1303c15 commit b9d800d
Show file tree
Hide file tree
Showing 5 changed files with 189 additions and 24 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
40 changes: 22 additions & 18 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 = [ "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 =
Expand Down Expand Up @@ -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
{
Expand All @@ -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
Expand Down

0 comments on commit b9d800d

Please sign in to comment.