Checked maintainers

[infinisil]

Motivation for this change

Since #66361 was merged, the maintainers list is accumulating a lot of handles without githubIds, or other problems such as misspelled githubId (partial fix in #82446) or missing name or email.

(Updated) This PR implements maintainer checks, evaluated by @GrahamcOfBorg once NixOS/ofborg#438 is merged. This guards against all of above problems. For additional convenience (and because it's possible), the error shows the correct githubId if it's missing:

lib.maintainers.infinisil: If `github` is specified, `githubId` must be too.
The GitHub ID for GitHub user infinisil is 20525370:
    githubId = 20525370;

All of the problems were also fixed in an initial commit, which was done by copy pasting directly from the error.

Ping @grahamc, ping @roberth for the module stuff

Things done

• Confirmed that ofborg can catch maintainer list errors

[grahamc]

I am not yet sure we should merge this, so Requesting Changes. In short, we can't trust that the person who owned the name in the "github" is the same person who owns that name now.

When I originally filled this data in, I wrote a somewhat complicated program to look up the commit which added an entry, find the github name, see if that github name matches what is there, if so -- add the ID automatically: https://github.com/NixOS/rfc39/blob/master/src/op_backfill.rs

I also wrote another program which verifies the author of the commit adding their maintainer entry matches the registered github ID: https://github.com/NixOS/rfc39/blob/master/src/op_blame_author.rs

On the dashboard for the RFC-39 sync I wrote:

You can't just go look up GitHub handles and assume it is right. Instead:

    Find a maintainer without a githubId
    Look in the git blame output and find that section
    Look up that commit on GitHub and see who GitHub thinks authored it.
    Find that user's ID (https://api.github.com/users/«username»)

This is because a surprising number of GitHub users change their account names!

Because adding their github ID to this file is the beginning of a good trust root, we should not add them lightly or casually. We will probably have a github name but not an ID for some users.

Finally, there is a question about having the maintainer list be a module itself. I am not sure that is a good idea, because some tools import that file directly and expect it to be a bit of an API. I like the validation the module gets us -- maybe that part can be in a different file, leaving this file plain data?

[roberth]

In addition to the comments, I'm concerned about the use of the Nix language for what appears to be data. We have perfectly usable builtins.fromJSON and builtins.toJSON, allowing anyone to write tooling that uses the data easily.

[infinisil]

@grahamc Good point, but I think it might be okay, because from now on, as soon as people add themselves with a missing githubId they will get the error. This works as long as people don't change their username in the short amount of time from writing github to writing githubId.

I think the only thing to worry about is the current PRs that don't add a githubId. But if they get merged, master will throw the eval error, at which point we can resolve it manually until none are left anymore.

[grahamc]

Note part of my concern is the IDs which are added in this PR.

[infinisil]

@grahamc As discussed on IRC, I now properly verified that all added GitHub IDs are correct, and yeah some did indeed change, namely:

• aminb -> @mabgnu (see alloy: add alloy5 and take maintainership #47262)
• auntie -> @auntieNeo (see Add package for Typespeed game #2876)
• thorstenweber83 -> @tw-360vier (see git-ftp: init at 1.4.0 #27177, thorstenweber83 is somebody else, seems to be a mistake in Convert maintainer file entries to attributes, add github handles #36119)

For this I used a mix of automation and manual checking. For future reference, here's a dump of my current script:

#!/usr/bin/env nix-shell
#!nix-shell -i bash -p curl jq sta

# https://help.github.com/en/github/authenticating-to-github/creating-a-personal-access-token-for-the-command-line
TOKEN=
set -o pipefail

manual() {
	xdg-open "$(echo "$object" | jq -r '.url')"
	echo -n "Enter username: " >&2
	read username
	resp=$(curl -sS -H "Authorization: token $TOKEN" "https://api.github.com/users/$username")
	id=$(echo "$resp" | jq -r '.id')
	echo "For handle $handle, 'github' should be $username, 'githubId' should be $id :"
	echo "    githubId = $id;"
}

#git -C ~/src/nixpkgs log --follow --patch maintainers/maintainer-list.nix \
#  | rg -e '^index ([0-9a-f]+)\.\.([0-9a-f]+)|^commit ([0-9a-f]+)' -or '$1-$2-$3' \
#	| while IFS="-" read before after commit; do
#	  if [ -n "$commit" ]; then
#		  currentcommit="$commit"
#			echo "$currentcommit" >&2
#		else
#			echo "builtins.attrNames (let x = $(git -C ~/src/nixpkgs show "$before"); in if builtins.isFunction x then x {} else x)" > beforefile
#			echo "builtins.attrNames (let x = $(git -C ~/src/nixpkgs show "$after"); in if builtins.isFunction x then x {} else x)" > afterfile
#		  if ! keysbefore=$(nix-instantiate beforefile --eval --json --strict | jq '.[]' -r); then
#			  continue
#			fi
#		  if ! keysafter=$(nix-instantiate afterfile --eval --json --strict | jq '.[]' -r); then
#			  continue
#			fi
#			diff \
#				--new-line-format="%L" \
#				--old-line-format="" \
#				--unchanged-line-format="" \
#				<(echo "$keysbefore") <(echo "$keysafter") | while read user; do
#				echo "Handle $user was added in $currentcommit" >&2
#				echo "$user $currentcommit"
#			done
#
#		fi
#	done
#
#exit 0

#response=$(curl -sS -H "Authorization: token $TOKEN" https://api.github.com/users/$1)
#
#echo "    githubId = $(echo "$response" | jq -r '.id');" | tee /dev/stderr |  /nix/store/5ghdbn3lmbaqdj7dba0a5m1hb2949zwy-xclip-0.13/bin/xclip -selection clipboard
#exit 0
forHandle() {
  local handle=$1
  local github=$2
	echo -n "For handle $handle the previous github login was $github. " >&2
	commit=$(rg "^$handle (.*)$" -or '$1' maintainer-map | tail -1)
	#commit=$(git -C ~/src/nixpkgs log --follow -G "$\s*$handle[^a-zA-Z0-9_'-]^" --pretty=format:%H maintainers/maintainer-list.nix | tail -1)
	echo -n "Found commit $commit . " >&2

	query=$(cat <<-EOF
{
  repository(name: "nixpkgs", owner: "NixOS") {
    object(expression: "$commit") {
      ... on Commit {
        associatedPullRequests(first: 10) {
          nodes {
            author {
              ... on User {
                databaseId
                login
              }
            }
          }
        }
        author {
          user {
            databaseId
            login
          }
        }
				url
      }
    }
  }
}
	EOF
	)
	DATA=$(jq -n --arg query "$query" '{ query : $query }')
	response=$(curl -sS -H "Authorization: token $TOKEN" -d "$DATA" https://api.github.com/graphql)
	object=$(echo "$response" | jq '.data.repository.object')
  result=$(echo "$object" | jq '[ ( .author.user , .associatedPullRequests.nodes[].author) ] | map(select(. != null)) | unique[] | "\(.login) \(.databaseId)"' -r)
	if [ -z "$result" ]; then
	  count=0
	else
		count=$(echo "$result" | wc -l)
	fi
	if [ "$count" -eq 0 ]; then
		echo -e "\e[31mCouldn't figure out who the author is!\e[0m" >&2
		manual
	elif [ "$count" -eq 1 ]; then
		read login id <<< "$result"
		echo -n "Found author with login $login and id $id. " >&2
		if [ "$github" = "$login" ]; then
		  echo -e "\e[32mThis is the same as the one in the maintainer file\e[0m" >&2
			echo "For handle $handle, 'github' should be $login, 'githubId' should be $id :"
			echo "    githubId = $id;"
		else
		  echo -e "\e[31mThis is NOT the same as the one in the maintainer file!\e[0m" >&2
			manual
		fi
	else
		echo -e "\e[31mDifferent commit author than PR author!\e[0m" >&2
		manual
	fi
}

#forHandle DerGuteMoritz DerGuteMoritz
#forHandle Fresheyeball fresheyeball
#forHandle MP2E MP2E
#forHandle Mogria mogria
#forHandle MtP MtP76
#forHandle abbradar abbradar
#forHandle acairncross acairncross
#forHandle adamt adamtulinius
#forHandle aforemny aforemny
#forHandle ak alexanderkjeldaas
#forHandle alunduil alunduil
#forHandle ambrop72 ambrop72
#forHandle aminb aminb
#forHandle andreabedini andreabedini
#forHandle andres kosmikus
#forHandle andsild andsild
#forHandle antono antono
#forHandle auntie auntie
#forHandle averelld averelld
#forHandle backuitist backuitist
#forHandle badi badi
#forHandle berdario berdario
#forHandle bluescreen303 bluescreen303
#forHandle boothead boothead
#forHandle coconnor coreyoconnor
#forHandle cransom cransom
#forHandle davidrusu davidrusu
#forHandle desiderius desiderius
#forHandle dgonyeo dgonyeo
#forHandle doublec doublec
#forHandle dxf dingxiangfei2009
#forHandle edanaher edanaher
#forHandle emmanuelrosa emmanuelrosa
#forHandle ericsagnes ericsagnes
#forHandle ertes ertes
#forHandle fare fare
#forHandle fdns fdns
#forHandle fragamus fragamus
#forHandle freezeboy freezeboy
#forHandle garbas garbas
#forHandle gavin gavinrogers
#forHandle gridaphobe gridaphobe
#forHandle hkjn hkjn
#forHandle infinisil infinisil
#forHandle j03 johannesloetzsch
#forHandle jasoncarr jasoncarr0
#forHandle jeschli jeschli
#forHandle jfb tftio
#forHandle jitwit jitwit
#forHandle joamaki joamaki
#forHandle joepie91 joepie91
#forHandle jonathanmarler marler8997
#forHandle juliendehos juliendehos
#forHandle jyp jyp
#forHandle kampfschlaefer kampfschlaefer
#forHandle knairda KnairdA
#forHandle koral k0ral
#forHandle laikq laikq
#forHandle lassulus Lassulus
#forHandle lebastr lebastr
#forHandle leonardoce leonardoce
#forHandle lovek323 lovek323
#forHandle ltavard ltavard
#forHandle lumi lumi-me-not
#forHandle matthewbauer matthewbauer
#forHandle matti-kariluoma matti-kariluoma
#forHandle melsigl melsigl
#forHandle michaelpj michaelpj
#forHandle michelk michelk
#forHandle minijackson minijackson
#forHandle mirdhyn mirdhyn
#forHandle mkf mkf
#forHandle mmlb mmlb
#forHandle mpscholten mpscholten
#forHandle mredaelli mredaelli
#forHandle nand0p nand0p
#forHandle ngerstle ngerstle
#forHandle olynch olynch
#forHandle orbitz orbitz
#forHandle pcarrier pcarrier
#forHandle plchldr plchldr
#forHandle pmeunier P-E-Meunier
#forHandle polyrod polyrod
forHandle rafaelgg rafaelgg
#forHandle rickynils rickynils
#forHandle rob rbvermaa
#forHandle robberer robberer
#forHandle rvolosatovs rvolosatovs
#forHandle ryansydnor ryansydnor
#forHandle sander svanderburg
#forHandle scalavision scalavision
#forHandle schmittlauch schmittlauch
#forHandle scubed2 scubed2
#forHandle shazow shazow
#forHandle shlevy shlevy
#forHandle shmish111 shmish111
#forHandle sjmackenzie sjmackenzie
#forHandle sprock sprock
#forHandle srghma srghma
#forHandle taha tgharib
#forHandle tckmn tckmn
#forHandle tesq0 tesq0
#forHandle teto teto
#forHandle the-kenny the-kenny
#forHandle timbertson timbertson
#forHandle timma ktrsoft
#forHandle tnias tnias
#forHandle tscholak tscholak
#forHandle tvestelind tvestelind
#forHandle tweber thorstenweber83
#forHandle twey twey
#forHandle uwap uwap
#forHandle valeriangalliat valeriangalliat
#forHandle vcanadi vcanadi
#forHandle viric viric
#forHandle vizanto vizanto
#forHandle vmchale vmchale
#forHandle wscott wscott
#forHandle xnaveira xnaveira
#forHandle y0no y0no
#forHandle zalakain umazalakain

The tests are now also run as part of lib/tests/release.nix and are separate of the maintainer file. They are now implemented as a fixed-output derivation.

[infinisil]

@grahamc Looks like ofborg doesn't run/build lib tests (only instantiate them via nixpkgs-tarball), which is needed for this to work now. See NixOS/ofborg#438 which should add this.

[thorstenweber83]

thorstenweber83 and tw-360vier are indeed the same person. I have a separate work account :/
Please use thorstenweber83 for the maintainers file.
I'll add a comment with tw-360vier in a moment.

[tw-360vier]

Please use thorstenweber83 for the maintainers file :)

[infinisil]

Note that there's also some maintainer checks in https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/misc/meta.nix . Ideally that should be combined with this

[infinisil]

Checks are working nicely now that NixOS/ofborg#438 is merged :D. See https://gist.github.com/GrahamcOfBorg/c999ed1b03e7061cdd4f213cac6e394d for how such failures look. I'll now update the PR to fix those two instances of people not setting githubIds. Once merged, we'll probably have to fix those a bunch more times in the near future as old PRs get merged.

[Ma27]

Great work!

[infinisil]

Rebased on top of master to fix another case of a missing githubId

[infinisil]

This evening I'll fix up any new potential missing githubIds and then merge this myself if there aren't any complaints.

[infinisil]

Turns out I forgot about doing it that evening. Done it now though, only one githubId had to be fixed since then. One other change I made is that I removed the descriptions from the options, because they're useless and would be duplicated (once in lib/tests/maintainers.nix and once in maintainers/maintainer-list.nix).

I'll merge once @GrahamcOfBorg is happy

[infinisil]

PR that reverts that commit is now merged: #85241