Reviewing PRs in Nixpkgs

Development
1

Sometimes, when I have some spare time to kill, I look at the list of open PRs and want to review some of them. But I often find myself unsure about what that actually encompasses and leads me to kill the time with more useless activities. So I am wondering, what is the actual expectation on a reviewer.

I read the reviewing section in nixpkgs/CONTRIBUTING.md but that seems to focus on how to write a good review and not annoy the PR-authors. And while very important, I found it a bit lacking in terms of a general approach to reviewing. Of course I am aware that this also depends on the type of PR, someone changing the stdenv would probably get a lot more scrutiny, I want to aim this at the (perceived) majority of PRs that are either new packages or version updates without impacting thousands of downstream dependencies.

Some of the things that I tend to ask myself:

  • For simple updates, is just looking at the changes to see that there’s nothing fishy going on sufficient?
  • Do you actually build the package and check that it works? To what degree?
  • If you build and check, any recommendations for an environment that minimizes potential impact on your own system.
  • Do we need to to check hashes (of sources, but also cargo or other build-system specific hashes), or will this be handled by the CI?
  • What do we NOT have to review because it’s being done automatically? Formatting seems to be one of them, what else?
  • Do “drive-by” reviews by non-maintainers actually carry any value?

Maybe some reviewers can write a bit about their approach to this topic. Who knows, maybe we can distill this into an actual document.

2 Likes
2

Yes, and verifying that it builds. If CI didn’t build it then I may try to build it locally.
If there aren’t any tests then I might run some ad-hoc tests.

If I’m reviewing from the phone, no, but otherwise I do make sure it builds (see above). If you want to be safer you can run it in a vm as well. I personally don’t run closed-source stuff, I do a quick once-over of open-source code, and use @max’s GitHub - max-privatevoid/hover: Hover - Temporary home directories to prevent programs from taking a dump in my $HOME.

Avoid stupid nits, or if you must, at least say nit: <feedback> or something.
If CI spits up an error then just point to CI rather than treating it as your own feedback, that seems to avoid some (but not all) frustration for contributors.

When you do give some feedback, try to point to something in the manual or a written guideline, or provide a technical justification, otherwise it may come off like a nit.

Maintainer or not is mostly irrelevant, just give the review and approve it once feedback is addressed. I’ve given about 700 reviews and I don’t maintain anything in nixpkgs.

3 Likes