modernize MAINTAINERS

[markjdb]

We need a high-level API which maps a set of file paths to a set of email addresses of maintainers who could plausibly review changes to those paths. The same idea as CODEOWNERS and MAINTAINERS, except:

• it should not be tied to any particular system like github or phabricator
• it should not assume that one has an account on any particular system, i.e., identifiers are email addresses

Such a database should then be used to generate things like github CODEOWNERS or phabricator herald rules. This will be useful when we eventually switch to forgejo.

The task is to design a database format which enables mapping a git diff to a set of candidate reviewers. The database should:

• live in the FreeBSD src repo,
• be machine-parseable,
• support globbing and be somewhat normalized, e.g., if reviewers foo@, bar@, baz@ want to review all changes to "bhyve", they can add their names in one place and thus cover all of usr.sbin/bhyve/, lib/libvmmapi/, sys/dev/vmm/, sys/amd64/vmm/, ...
• come with a script which 1) takes as input a set of paths and emits matching reviewers, and 2) takes a reviewer name and emits the set of paths that they are nominally responsible for,
• ...?

[emaste]

We could just copy the Linux MAINTAINERS format, it's a simple line-based file where each line starts with a letter e.g. R for a reviewer, F for a path to include in that component, etc.

[markjdb]

I think that's fine. I'd be a bit more inclined to use a json or yaml-based format just to make it easier to write tools around it, but even then we could just steal the scheme from Linux's MAINTAINERS.

[bsdimp]

Yea, using the MAINTAINER format will let us just use the parse-maintianer.pl file from Linux w/o needing to re-implement all that stuff. There's opt-in, opt-out, etc eg, I'm interested in sys/dev/pci, except I don't care about isa_pci.c can be articulated.

Going full json means we have to reimplement.

That's why I was suggesting we just use this format during the meeting.

[markjdb]

Going full json means we have to reimplement.

That's why I was suggesting we just use this format during the meeting.

Well sure, but going full json also means that I can write something like

import json

json = json.load("MAINTAINERS")
entries = [entry.paths for entry in json.get('entries', []) if "markj" in entry.reviewers]

in about 30s, and downstreams can write their own tools or extensions easily. There's nothing to parse, and you can steal and extend the schema from Linux easily.

[bsdimp]

True, but that doesn't give all the opt-in / opt-out specification that linux MAINTAINERS file gives you. So you have the entries and now are re-implementing that code. The Linux format is ordered, while json tends not to be, so you can nest those specifications. How would you handle that?

It's more the logic to take path -> reviewer that we're not having to reimplement... Or path to upstream repo... or path to other things the file allows.

[markjdb]

True, but that doesn't give all the opt-in / opt-out specification that linux MAINTAINERS file gives you. So you have the entries and now are re-implementing that code. The Linux format is ordered, while json tends not to be, so you can nest those specifications. How would you handle that?

Which features are you talking about specifically? JSON arrays are ordered and we can nest things.

It's more the logic to take path -> reviewer that we're not having to reimplement... Or path to upstream repo... or path to other things the file allows.

That's simple though. Here's a straw example/proposal: markjdb/freebsd@46740af

The schema could be extended easily.

[markjdb]

That's simple though. Here's a straw example/proposal: markjdb/freebsd@46740af

Example output:

$ ./maintainers.py ./MAINTAINERS.json usr.sbin/bhyve/pci_passthru.c
['bnovkov', 'corvink', 'jhb', 'markj']
$ ./maintainers.py ./MAINTAINERS.json bin/ls/ls.c
No reviewers found

[bsdimp]

That's simple though. Here's a straw example/proposal: markjdb/freebsd@46740af

Example output:

$ ./maintainers.py ./MAINTAINERS.json usr.sbin/bhyve/pci_passthru.c
['bnovkov', 'corvink', 'jhb', 'markj']
$ ./maintainers.py ./MAINTAINERS.json bin/ls/ls.c
No reviewers found

We need to add regular expressions and exceptions to the paths too at the very least. And we'll need to handle the union of all the reviewers for lists of paths.. the basics are easy, but it gets uglier as we layer in the features we know we'll need...

I'm guessing the maintainers.json is going to get kinda ugly if we lean into this very hard at all...

[markjdb]

That's simple though. Here's a straw example/proposal: markjdb/freebsd@46740af

Example output:

$ ./maintainers.py ./MAINTAINERS.json usr.sbin/bhyve/pci_passthru.c
['bnovkov', 'corvink', 'jhb', 'markj']
$ ./maintainers.py ./MAINTAINERS.json bin/ls/ls.c
No reviewers found

We need to add regular expressions and exceptions to the paths too at the very least. And we'll need to handle the union of all the reviewers for lists of paths.. the basics are easy, but it gets uglier as we layer in the features we know we'll need...

Globbing is trivial. Done here: markjdb/freebsd@2538c93

Now it matches things like usr.sbin/bhyvectl/bhyvectl.c against usr.sbin/bhyve*/*. What exceptions are you referring to?

I'm guessing the maintainers.json is going to get kinda ugly if we lean into this very hard at all...

So far it's just a small subset of Linux's maintainers file, using json instead of a bespoke format. Note that get_maintainer.pl and parse-maintainers.pl together are about 3KLOC. A lot of that code does stuff that we will never ever need because we don't use a email-based workflow. My script is 70 lines and easy to hack on.

I think it's better to enumerate our requirements (which might include things like Linux's format doesn't cover, like contrib/vendor code) and build a simpler format around them.

[bsdimp]

I'm interested in share/*, except share/mk is the concept we need

[markjdb]

I'm interested in share/*, except share/mk is the concept we need

Done: markjdb/freebsd@63dc802

[bsdimp]

I'm interested in share/*, except share/mk is the concept we need

Done: markjdb/freebsd@63dc802

Nice. The implementation simplicity makes up for having to write json...

[markjdb]

@bsdimp what do you need in order to get PRs to automatically add relevant maintainers? We need the github<->FreeBSD email mappings I guess, and some affordances for maintainers who don't have a commit bit.

[markjdb]

I'm interested in share/*, except share/mk is the concept we need

Done: markjdb/freebsd@63dc802

Nice. The implementation simplicity makes up for having to write json...

I mean, a lot of editors will flag invalid json, plus it's not like this file would get frequent updates...

[bsdimp]

@bsdimp what do you need in order to get PRs to automatically add relevant maintainers? We need the github<->FreeBSD email mappings I guess, and some affordances for maintainers who don't have a commit bit.

Checklist right now does that. We'd need to generate this file from codeowners/maintainers (both have had recent changes). I'll generate a github mapping file and we can tie that in. I'll try to do that this weekend. Checklist is in javascript. If you want to run your python, you'll need to do something similar to what the perl style checker: setup the env, install python, run the script.