Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 2, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9d800d
workflows/eval: Request reviews from changed package maintainers
infinisil Nov 28, 2024
b844cba
workflows/eval: Use maintainer GitHub IDs for review requests of chan…
infinisil Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/codeowners-v2.yml
View file
Open in desktop
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
33 changes: 31 additions & 2 deletions .github/workflows/eval.yml
View file
Open in desktop
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
wolfgangwalther marked this conversation as resolved.
Show resolved Hide resolved
- uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0
Mic92 marked this conversation as resolved.
Show resolved Hide resolved
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,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
Comment on lines +272 to +275
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
jq -r 'keys[]' comparison/maintainers.json \
| while read -r id; do gh api /user/"$id"; done \
| jq -s '{ reviewers: map(.login) }' \
> reviewers.json
jq -r 'keys[]' comparison/maintainers.json \
| head -n15 \
| while read -r id; do gh api /user/"$id"; done \
| jq -s '{ reviewers: map(.login) }' \
> reviewers.json

GitHub apps are limited to 15 reviewer requests anyway, and this way treewide changes are handled better. Adding a head -n15 here should work, but I did not test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea. Should be the list sorted, so it's always the same 15 reviewers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two other options to handle treewides / PRs with more than 15 reviewers:

  • Don't request reviewers in this case.
  • Instead of requesting review, post a comment with all maintainers mentioned for this case.

I actually prefer the first option: When you make a treewide change and you get some random 15 reviewers, chances are high that those reviewers:

  • are not responding anyway
  • don't have much to say about that treewide change

So why notify many random people?

(Note that for a treewide, we very likely have already requested reviews from a bunch of people via OWNERS!)

For treewide changes it makes much more sense to pick your reviewers individually, specifically for that change.

Copy link
Member

@Mic92 Mic92 Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is also a limit with posting maintainers in comments. But we probably should avoid mass-pinging so many people and leave it to the pr authors/reviewers to select the right set of people.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we have now implemented requesting reviewers 1-by-1 to work around failures for some of them.

See #370749 for a run with that. Clearly the limit seems to apply for a single API request - and going 1-by-1 requested reviews from 30 reviewers there.

ofborg had limited itself to 10 per run manually. We'll need to do something about this as well.


# 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 }}
Expand Down
22 changes: 18 additions & 4 deletions ci/eval/compare/default.nix
View file
Open in desktop
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
View file
Open in desktop
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 "")); }
]
)
));
Comment on lines +56 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is an extension on top of the current ofborg behavior, but maybe a good time to discuss it:

Can we extend the "relevant file names" for a derivation to include some dependent files like patches or setup hooks?

I remember having to look up maintainers from the derivation manually when changing the setup-hook.sh.

I think some basic rules could be:

  • if the path ends in /default.nix or the path ends in /package.nix and is in /by-name/, then
  • add all files in the same directory (recursively) to the list of relevant files.

A random example that would benefit from it:

pkgs/by-name/me/meson/package.nix would include changes to all the patch and .sh files in the same folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infinisil

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we can first start with a minimal feature set and make it more complex later. Now since ofborg has been shutdown and will be re-instantiated with only a smaller feature set.


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
40 changes: 22 additions & 18 deletions ci/eval/compare/utils.nix
View file
Open in desktop
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
Loading