Introduction
In the spirit of PRs ready for review I’d like to start another experiment. I know (or hope) that many people would like to get more involved and help resolve nixpkgs’s “PR crisis”, with 1,327 currently open pull requests (1,041 without any review). We recently started encouraging people to review PRs in the pull request template. Reviewing can be a bit frustrating for people without commit access however, since they do not have the means to actually act on their review or attract the attention of someone who can.
As an attempt to bridge this gap, I’d like people to review PRs and post their results here. The intention is to reduce the amount of time a committer (like me, maybe others will help) has to spend on a reviewed PR to ideally less than a minute. They should only need to read the review and confirm the results at a glance. To achieve that, I’d like people to follow the following review procedure (also see the manual):
Select an open pull request that is not your own
It should be a PR that you think should be merged. For example package updates are always safe bets. Having a PR you think should be merged that is not your own ensures that at least two people think this is a good idea, reducing the time a committer has to invest to make sure the PR is worthwhile.
A helpful filter to start:
PR’s that have no review, are mergeable and are not WIP. Oldest first.
If you’re looking for low hanging fruit, you may append one or more of the following to the end of the search string:
-
author:r-ryantm– package updates by the friendly package update bot, generally low controversy -
label:"8.has: package (update)"– package updates in general -
label:"10.rebuild-linux: 1-10"– PRs that impact only a small number of packages, making reviews quicker and the impact of a mistake lower -
comments:<2– filter out PRs with discussions, which may indicate that something is unresolved / controversial
Here is the combination of all of those (except the r-ryantm part, because I don’t want to encourage people too much to only look at bot PRs): low hanging fruit
Look at the commit(s) to be merged
Make sure the changes look reasonable to you. Some things to pay attention to are
- all new patches should have
- an explanatory comment or at least a good name
- a link to an upstream discussion about the change or an explanation why the change can’t be contributed upstream
- in general, everything non-obvious should be commented. If you don’t know why a change was made, leave a review and ask the PR creator to clarify in a comment (in the nix file, not just a comment on the PR)
Assure that all ofBorg checks pass
The list of checks at the bottom of the page should look like here. The bot should only report successes or “No Attempt”. Make sure that ofBorg has actually built the package. This needs to be triggered explicitly if somebody without ofBorg permissions created the pull request. If you do not have those permissions yourself, ask here or on the #nixos-dev IRC for someone to trigger the ofBorg builds. If you have been involved in the community for a while, consider asking for access. See the repo for details.
If one of the builds fails, leave a review (click “Files changed” → “Review changes”) asking the PR author to fix it.
If the touched package or one of its dependencies has always failed on a platform, it should be explicitly marked as not available on that platform (by setting meta.platforms to something more restrictive, like [ linux ] or if you want to disable aarch64 as well [ "i686-linux" "x86_64-linux" ]), so that ofBorg reports “No Attempt”.
Ensure all reverse dependencies still build
Reverse dependencies are packages that depend on the packages being touched by the PR. For example, a major update in a library could break packages that depend on that library. To make sure nothing is broken, install the wonderful nix-review tool (it is packaged as a nix package of course). Then, from a local checkout of nixpkgs execute nix-review pr <pr number>. This will check out the pull request and build the touched packages and all reverse dependencies. Depending on the amount of packages affected, this may take a while. If the amount of rebuilds is unreasonable, still let nix-review run for a while (half an hour). Realistically, breakage is most likely in a packages immediate dependencies which will be built first.
If any build failures are reported, this does not have to mean that the PR caused them. Unfortunately, there are currently some packages that are just broken but not marked as such. You should try to reproduce any failures on nixpkgs master (with a plain nix build -f. <package name>).
If the breakage is already present on master, create a pull request marking the package as broken. Ping its maintainers if there are any. See this as an example. Link that in your review comment.
If the breakage is not already present on master, leave a review and ask the PR author to fix it. If there is no PR author to request changes on (as is the case with r-ryantm PRs), feel free to replace it with your own improved PR. Mention the keyword Closes #NNNN in the PR description to replace PR NNNN.
Link your reviewed PR here
Leave a message with a link to the PR and your suggested action. The action may be
- merge
- close
- add some tag (like “work in progress”)
- whatever you can think of that would need commit access to the repository
Remember that you should provide enough information so that committers can be comfortable actually doing that action without investing much time!
This is an experiment. If anything about the process is unclear, please help improve it by asking in this discussion thread: [Discussion] PR reviews - #15 by vcunat
Checklist
Remember, the goal of this post is to call attention to actionable reviews that require minimal time commitment from people with the necessary access rights. If something is unclear about the process, please help improve it by posting in the discussion thread. If you can’t reach a definitive conclusion on a particular PR, still leave a review and explain why the decision is difficult. Consider asking for help/opinions in the PR, in a separate discourse thread or on IRC.
If you have finished your review, leave a review on the Github PR explaining what you have done and what action you suggest. Link it here if you have
[ ] reviewed the diff and commit messages
[ ] made sure ofBorg build succeeded for all applicable platforms
[ ] run nix-review for a reasonable amount of time without any failures (or marked preexisting failures as broken)
Posts will be removed after they have been acted on.
