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.

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

Checked maintainers #82461

Merged
merged 7 commits into from Apr 13, 2020
Merged

Checked maintainers #82461

merged 7 commits into from Apr 13, 2020

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Mar 13, 2020

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

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

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?

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

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.

lib/options.nix Outdated Show resolved Hide resolved
lib/modules.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member Author

@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
Copy link
Member

grahamc commented Mar 13, 2020

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

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member Author

infinisil commented Mar 15, 2020

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

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 added a commit to infinisil/ofborg that referenced this pull request Mar 15, 2020
Without this, a big part of the lib tests aren't being done, which
previously lead to e.g. NixOS/nixpkgs#76861
And this will also be useful for checked maintainers in NixOS/nixpkgs#82461
@infinisil
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Contributor

Please use thorstenweber83 for the maintainers file :)

@infinisil infinisil mentioned this pull request Mar 16, 2020
2 tasks
@infinisil
Copy link
Member Author

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
Copy link
Member Author

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.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Great work!

@infinisil
Copy link
Member Author

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

@infinisil
Copy link
Member Author

infinisil commented Apr 7, 2020

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

All these IDs were carefully obtained with
- If possible, look at which commit introduced the maintainer and check
  with the GitHub API whether it was the person we think it should be.
  If yes, that's great
- If above doesn't work for any reason (which was the case for about
  half), then I went through the commit log and through GitHub PRs
  manually, to see who the person is

Sometimes this required trying to find a commit after the svn-era, but
before the committer stopped contributing. Sometimes when one person
adds two maintainers this required checking that they are known to each
other through bidirectional GitHub follows.

For tweber's github, see:
NixOS#82461 (comment)
Improves build time by about a factor of two on my system
@infinisil
Copy link
Member Author

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 infinisil merged commit 56f78c1 into NixOS:master Apr 13, 2020
@infinisil infinisil deleted the checked-maintainers branch April 13, 2020 15:55
@infinisil
Copy link
Member Author

PR that reverts that commit is now merged: #85241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet