From 44e4c1ba9f7521ab1ffcd04691677c3a306897ef Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 16 Oct 2025 12:56:44 +0200 Subject: [PATCH 1/5] maintainers/scripts/get-maintainer-pings-between: drop This has been broken for months, since all the interface to `ci/eval` changed. Nobody noticed, so nobody is using it. --- ci/eval/compare/default.nix | 2 - ci/eval/compare/maintainers.nix | 3 +- .../scripts/get-maintainer-pings-between.sh | 78 ------------------- 3 files changed, 1 insertion(+), 82 deletions(-) delete mode 100755 maintainers/scripts/get-maintainer-pings-between.sh diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index 3a025a0238f6..d0820618a0a0 100644 --- a/ci/eval/compare/default.nix +++ b/ci/eval/compare/default.nix @@ -49,7 +49,6 @@ in combinedDir, touchedFilesJson, githubAuthorId, - byName ? false, }: let # Usually we expect a derivation, but when evaluating in multiple separate steps, we pass @@ -171,7 +170,6 @@ let changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs); changedpathsjson = touchedFilesJson; removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs); - inherit byName; }; in runCommand "compare" diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index c3caf7403784..4a33c47962df 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -5,7 +5,6 @@ changedattrs, changedpathsjson, removedattrs, - byName ? false, }: let pkgs = import ../../.. { @@ -89,7 +88,7 @@ let }) pkg.maintainers ) attrsWithModifiedFiles; - byMaintainer = lib.groupBy (ping: toString ping.${if byName then "github" else "id"}) listToPing; + byMaintainer = lib.groupBy (ping: toString ping.id) listToPing; packagesPerMaintainer = lib.attrsets.mapAttrs ( maintainer: packages: map (pkg: pkg.packageName) packages diff --git a/maintainers/scripts/get-maintainer-pings-between.sh b/maintainers/scripts/get-maintainer-pings-between.sh deleted file mode 100755 index 4b6d7ff78052..000000000000 --- a/maintainers/scripts/get-maintainer-pings-between.sh +++ /dev/null @@ -1,78 +0,0 @@ -#!/usr/bin/env nix-shell -#!nix-shell -i bash -p git jq - -# Outputs a list of maintainers that would be pinged across two nixpkgs revisions. -# Authors: -# Morgan Jones (@numinit) -# Tristan Ross (@RossComputerGuy) - -set -euo pipefail - -if [ $# -lt 2 ]; then - echo "Usage: $0 " >&2 - exit 1 -fi - -repo="$(git rev-parse --show-toplevel)" -system="$(nix-instantiate --eval --expr builtins.currentSystem)" -rev1="$(git -C "$repo" rev-parse "$1")" -rev2="$(git -C "$repo" rev-parse "$2")" - -echo "Touched files:" >&2 -git -C "$repo" diff --name-only "$rev1" "$rev2" \ - | jq --raw-input --slurp 'split("\n")[:-1]' | tee "$TMPDIR/touched-files.json" >&2 - -# Runs an eval in the given worktree, outputting the path to $TMPDIR/$1.path. -# $1: The revision SHA. -eval_in_worktree() ( - mkdir -p .worktree - local rev="$1" - local tree=".worktree/$rev" - if [ ! -d "$tree" ]; then - git -C "$repo" worktree add -f -d "$tree" "$rev" >&2 - fi - cd "$tree" - - local workdir="$TMPDIR/$rev" - rm -rf "$workdir" - mkdir -p "$workdir" - - nix-build ci -A eval.attrpathsSuperset -o "$workdir/paths" >&2 - mkdir -p "$workdir/intermediates" - nix-build ci -A eval.singleSystem \ - --arg evalSystem "$system" \ - --arg attrpathFile "$workdir/paths/paths.json" \ - --arg chunkSize ${CHUNK_SIZE:-10000} \ - -o "$workdir/intermediates/.intermediate-1" >&2 - - # eval.combine nix-build needs a directory, not a symlink - cp -RL "$workdir/intermediates/.intermediate-1" "$workdir/intermediates/intermediate-1" - chmod -R +w "$workdir/intermediates/intermediate-1" - rm -rf "$workdir/intermediates/.intermediate-1" - - nix-build ci -A eval.combine \ - --arg resultsDir "$workdir/intermediates" \ - -o "$workdir/result" >&2 -) - -eval_in_worktree "$rev1" & -pid1=$! -eval_in_worktree "$rev2" & -pid2=$! - -wait $pid1 -wait $pid2 - -path1="$TMPDIR/$rev1" -path2="$TMPDIR/$rev2" - -# Use the repo this script was executed in to get accurate maintainer info -nix-build "$repo/ci" -A eval.compare \ - --arg beforeResultDir "$path1/result" \ - --arg afterResultDir "$path2/result" \ - --arg touchedFilesJson "$TMPDIR/touched-files.json" \ - --arg byName true \ - -o comparison - -echo "Pinged maintainers (check $repo/comparison for more details)" >&2 -jq < comparison/maintainers.json From 79029e39a4178037c4d76458fdfcf0e970971ff2 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 16 Oct 2025 12:59:05 +0200 Subject: [PATCH 2/5] ci/eval/compare/maintainers: refactor using lib Removes unused code and comments, uses shorter lib over builtins and switches to `lib.importJSON`. Readability is key. --- ci/eval/compare/maintainers.nix | 40 +++++++++++++-------------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index 4a33c47962df..31cadedcde7f 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -7,18 +7,13 @@ removedattrs, }: let - pkgs = import ../../.. { - system = "x86_64-linux"; - config = { }; - overlays = [ ]; - }; + pkgs = import ../../.. { system = "x86_64-linux"; }; - changedpaths = builtins.fromJSON (builtins.readFile changedpathsjson); + changedpaths = lib.importJSON changedpathsjson; - anyMatchingFile = - filename: builtins.any (changed: lib.strings.hasSuffix changed filename) changedpaths; + anyMatchingFile = filename: lib.any (changed: lib.hasSuffix changed filename) changedpaths; - anyMatchingFiles = files: builtins.any anyMatchingFile files; + anyMatchingFiles = files: lib.any anyMatchingFile files; attrsWithMaintainers = lib.pipe (changedattrs ++ removedattrs) [ (map ( @@ -39,24 +34,19 @@ let # No need to match up packages without maintainers with their files. # This also filters out attributes where `packge = null`, which is the # case for libintl, for example. - (builtins.filter (pkg: pkg.maintainers != [ ])) + (lib.filter (pkg: pkg.maintainers != [ ])) ]; relevantFilenames = drv: - (lib.lists.unique ( - map (pos: lib.strings.removePrefix (toString ../..) pos.file) ( - builtins.filter (x: x != null) [ - ((drv.meta or { }).maintainersPosition or null) - ((drv.meta or { }).teamsPosition or null) - (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) + (lib.unique ( + map (pos: lib.removePrefix (toString ../..) pos.file) ( + lib.filter (x: x != null) [ + (drv.meta.maintainersPosition or null) + (drv.meta.teamsPosition or null) + (lib.unsafeGetAttrPos "src" drv) + (lib.unsafeGetAttrPos "pname" drv) + (lib.unsafeGetAttrPos "version" drv) # Use ".meta.position" for cases when most of the package is # defined in a "common" section and the only place where @@ -76,7 +66,7 @@ let pkg: pkg // { filenames = relevantFilenames pkg.package; } ) attrsWithMaintainers; - attrsWithModifiedFiles = builtins.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames; + attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames; listToPing = lib.concatMap ( pkg: @@ -90,7 +80,7 @@ let byMaintainer = lib.groupBy (ping: toString ping.id) listToPing; - packagesPerMaintainer = lib.attrsets.mapAttrs ( + packagesPerMaintainer = lib.mapAttrs ( maintainer: packages: map (pkg: pkg.packageName) packages ) byMaintainer; in From 7c865ddd46df7fe8ad6c4d79d1a8373faeb04081 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 16 Oct 2025 13:07:18 +0200 Subject: [PATCH 3/5] ci/eval/compare/maintainers: refactor path matching The only reason for the additional `lib.hasSuffix` check was, that the `lib.removePrefix` was broken - it was never adjusted when porting this from ofborg, so the relative path was wrong and no prefix ever removed, since no packages are in `ci/`. This additionally strips the leading `/`, so that `relevantFilenames` will then have paths starting with `pkgs/...`, similar to how git reports those paths in the `changedpathsjson` file. This allows simple equality comparison. --- ci/eval/compare/maintainers.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index 31cadedcde7f..8aacf2f6d644 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -11,7 +11,7 @@ let changedpaths = lib.importJSON changedpathsjson; - anyMatchingFile = filename: lib.any (changed: lib.hasSuffix changed filename) changedpaths; + anyMatchingFile = filename: lib.any (changed: changed == filename) changedpaths; anyMatchingFiles = files: lib.any anyMatchingFile files; @@ -40,7 +40,7 @@ let relevantFilenames = drv: (lib.unique ( - map (pos: lib.removePrefix (toString ../..) pos.file) ( + map (pos: lib.removePrefix "${toString ../../..}/" pos.file) ( lib.filter (x: x != null) [ (drv.meta.maintainersPosition or null) (drv.meta.teamsPosition or null) From 0a5ffcdf3182ed7b6556fb3ec756634a037cbdc4 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 16 Oct 2025 13:29:00 +0200 Subject: [PATCH 4/5] ci/eval/compare/maintainers: ping by-name reviewers for all rebuilds This adds support to ping maintainers when arbitrary files in by-name are changed, as long as they still cause a rebuild. For example, this is the case when changing .json files with version metadata. These were previously not detected as belonging to the package, and didn't cause maintainer pings. --- ci/eval/compare/maintainers.nix | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index 8aacf2f6d644..1ddad23da909 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -11,21 +11,29 @@ let changedpaths = lib.importJSON changedpathsjson; - anyMatchingFile = filename: lib.any (changed: changed == filename) changedpaths; + anyMatchingFile = filename: lib.any (lib.hasPrefix filename) changedpaths; anyMatchingFiles = files: lib.any anyMatchingFile files; + sharded = name: "${lib.substring 0 2 name}/${name}"; + attrsWithMaintainers = lib.pipe (changedattrs ++ removedattrs) [ (map ( name: let + path = lib.splitString "." name; # Some packages might be reported as changed on a different platform, but # not even have an attribute on the platform the maintainers are requested on. # Fallback to `null` for these to filter them out below. - package = lib.attrByPath (lib.splitString "." name) null pkgs; + package = lib.attrByPath path null pkgs; in { inherit name package; + # Adds all files in by-name to each package, no matter whether they are discoverable + # via meta attributes below. For example, this allows pinging maintainers for + # updates to .json files. + # TODO: Support by-name package sets. + filenames = lib.optional (lib.length path == 1) "pkgs/by-name/${sharded (lib.head path)}/"; # TODO: Refactor this so we can ping entire teams instead of the individual members. # Note that this will require keeping track of GH team IDs in "maintainers/teams.nix". maintainers = package.meta.maintainers or [ ]; @@ -63,7 +71,7 @@ let )); attrsWithFilenames = map ( - pkg: pkg // { filenames = relevantFilenames pkg.package; } + pkg: pkg // { filenames = pkg.filenames ++ relevantFilenames pkg.package; } ) attrsWithMaintainers; attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames; From f33892fcc9846ca2f6b6a922cdd245a631c2729c Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Thu, 16 Oct 2025 13:34:02 +0200 Subject: [PATCH 5/5] ci/eval/compare/maintainers: ping by-name reviewers for pure refactors This allows requesting reviewers for pure refactor PRs, which don't cause a rebuild of the package. This is only possible for by-name, because only here the package names can be inferred from the filenames. --- ci/eval/compare/maintainers.nix | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index 1ddad23da909..fef5ecb8d9b9 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -11,13 +11,24 @@ let changedpaths = lib.importJSON changedpathsjson; + # Extract attributes that changed from by-name paths. + # This allows pinging reviewers for pure refactors. + touchedattrs = lib.pipe changedpaths [ + (lib.filter (changed: lib.hasPrefix "pkgs/by-name/" changed)) + (map (lib.splitString "/")) + (map (path: lib.elemAt path 3)) + lib.unique + ]; + anyMatchingFile = filename: lib.any (lib.hasPrefix filename) changedpaths; anyMatchingFiles = files: lib.any anyMatchingFile files; sharded = name: "${lib.substring 0 2 name}/${name}"; - attrsWithMaintainers = lib.pipe (changedattrs ++ removedattrs) [ + attrsWithMaintainers = lib.pipe (changedattrs ++ removedattrs ++ touchedattrs) [ + # An attribute can appear in changed/removed *and* touched + lib.unique (map ( name: let