Sorry if this exists in another thread, I tried searching for one but didn’t find anything.
nixpkgs is huge; and I’ve seen concerns from some nixpkgs committers that some PRs are touching pieces of code that they don’t have the domain knowledge of and don’t know what to do with them. Also, weblate was recently merged into nixpkgs and it seems like the Python packages missed the mark on some of the Python package conventions which no one, including me, noticed. I guess since the PR didn’t attract many Python package reviewers.
This made me think about what systems are in place for reviewers to find their PRs, so to speak.
- The
topic: label
-
CODEOWNERS, maybe?
- Multiple PRs ready for review threads (which is a chaotic solution IMO but it works somehow)
-
Nixpkgs Review Requests Matrix room
I hope some committers can give their thoughts on how much of the current system they utilise, or in what ways they navigate PRs that I haven’t listed; and then further discuss more efficient solutions.
PS: I don’t intend to say that the PRs are not getting reviewed; in fact, I’ve seen quicker reviews on PRs in the last months.
2 Likes
I’m often unaware of work in areas of interest to me (e.g. kubernetes). Sadly github doesn’t provide adequately granular “watch” functionality to just get notifications for specific files or labels (or I haven’t found it), which is usually how I keep track of things in other projects on github.
2 Likes
Well anyone who’s merging is responsible for reviewing the packages that they’re merging, missing that is certainly a problem if it’s a persistent pattern.
And if you’re asking about how committers specifically can get notified, well .CODEOWNERS exists. This won’t work for non-committers though, you need write access to the repo.
As for other maintainers they would have to rely on topic flags if they care about a specific topic.
In any case I personally just skim latest PRs when I’m bored and tend to pick out ones that catch my attention.
1 Like
How do you track things there? Those projects are also only on GitHub, right?
That is what we should uphold, yes. Though consider that we have a non-trivial PR initialising some module X, that’s touching on some concept Y, agnostic of nixpkgs but relevant. Now, the committer A who’s merging the PR thoroughly reviews X against all of the nixpkgs rules and conventions, but has limited knowledge on Y, and deeply reviewing that may not block the merge either. But say some other committer B has better knowledge on Y, but never discovered that PR, and now that it’s merged, it’s even more unlikely to be discovered.
What methods does committer A have to attract others like committer B to review the PR who might have more valuable feedback on the PR? Better yet, how do people like committer B find PRs relevant to them on their own without needing call outs?
Just yesterday, in the review requests Matrix room, I saw someone ask another committer for assistance in a review. That’s cool to see, but kind of goes to show the need for better discoverability, not just exclusive to committers either.
CODEOWNERS is a good feature, it helps with a lot of things like blocking merges et al, but it barely solves the problem here. The set of people who “own” a file or directory, and can review it the best will always be a subset of people who have interest in the general area those files cover and hence can also provide feedback on PRs, code owners or not.
I think labels are a really good solution but it doesn’t really work because I do the same thing with scrolling through the latest few. I doubt reviewers sit down with X hours dedicated to reviewing in which case, I guess, it’s unintuitive to put extra effort and figure out labels to just review a few PRs at max. There must be some middle-ground here.
Then they shouldn’t merge if they have no idea what’s going on in Y. I don’t approve haskell PRs since I know nothing about haskell, for example. And I believe natsukium is a codeowner for python, but only one person cannot review every python PR, so at least get a second opinion from someone familiar if you’re not sure (we are definitely not lacking in people who know about python).
Add them as a reviewer and/or tag them with an explanation
I just have watch enabled for them, since there isn’t anything there to filter out. Then I can just go through my github notifications like a mail inbox, and be sure I’ve seen everything that needs my attention.
1 Like
Fair enough. I suppose if all committers did this, it would be the solution in of itself
I want to circle back on this thread.
While you’re right, I’m assuming this does happen a bit, as it did in the weblate PR.
I think the bigger issue at play here is the lack of any guideline and/or “training” for nixpkgs committers once they join the force. Correct me if this already exists (I wouldn’t know since I’m not a committer), but I’d expect the nixpkgs committer delegation team to come up with something here to onboard new committers and give them necessary information to collaborate with other people with the commit bit.
cc @Mic92 from the interim team.
1 Like