PRs already reviewed

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

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.

11 Likes

3 posts were merged into an existing topic: [Discussion] PR reviews

@tomberek thank you for starting with the reviews :slight_smile:

Please do remember to do your review as a GitHub review (Files changed -> Review changes). That has several advantages:

  • the PR will no longer show up in the filtered view I link from the top post
  • it is easier to understand the reasoning later
  • you will get subscribed to the PR in case there will be future discussion
  • as a little bonus there is a gamification aspect: it will be possible to generate statistics who reviewed how many PRs

This post is only to get someone to act on the review. It should be enough to just post “merge: <link>” or you could copy your review here. But it should not only be here, especially if these posts are deleted.

Regarding your second review: Please only post clear-cut reviews here. Your review is of course helpful and you could open a discussion in the PR and maybe ask for advice on IRC or a separate discourse post. But this post should be reserved for cases where the committer only has to “review a review”, not make a decision themselves or be particularly familiar with the particular packages.

Please don’t read the remarks in any negative way, I’m glad you started with the reviews :slight_smile: This is just the first step in refining the process.

1 Like
Finished ~~merge: https://github.com/NixOS/nixpkgs/pull/58681~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58703~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58762~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58862~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57478~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58008~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58005~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57335~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58656~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58316~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58673~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58299~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58883~~ ~~re-run ofborg: https://github.com/NixOS/nixpkgs/pull/58909~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/49868~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/52589~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/52948~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/56809~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57937~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59077 (approved by @matthewbauer ,awaiting merge, minimal impact)~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59307~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/56418~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59502~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59474~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59493~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59476~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59447~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59383~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59436~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59536~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59634~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59649~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60051~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60205~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60190~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60156~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59643~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58774~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60338~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60160~~ ~~needs more reviewers: https://github.com/NixOS/nixpkgs/pull/60102~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60484~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60486~~ merge: https://github.com/NixOS/nixpkgs/pull/60494 merge: https://github.com/NixOS/nixpkgs/pull/60531 merge: https://github.com/NixOS/nixpkgs/pull/60532 merge: https://github.com/NixOS/nixpkgs/pull/60547 merge: https://github.com/NixOS/nixpkgs/pull/60550 merge: https://github.com/NixOS/nixpkgs/pull/60307 merge: https://github.com/NixOS/nixpkgs/pull/60281 merge: https://github.com/NixOS/nixpkgs/pull/60315 merge: https://github.com/NixOS/nixpkgs/pull/60277

needs more testing/reviewers: https://github.com/NixOS/nixpkgs/pull/59503
merge or deny: https://github.com/NixOS/nixpkgs/pull/47794
merge: https://github.com/NixOS/nixpkgs/pull/59967
fix darwin: https://github.com/NixOS/nixpkgs/pull/60474
merge: https://github.com/NixOS/nixpkgs/pull/60480

5 Likes

I’ll start easy :wink:

merge: https://github.com/NixOS/nixpkgs/pull/58643
merge: https://github.com/NixOS/nixpkgs/pull/56464
merge: https://github.com/NixOS/nixpkgs/pull/58442

I don’t know enough about node to say this is good, but I have reviewed the module and everything there looks good. If someone would like to review the package that would be appreciated:

1 Like

A post was merged into an existing topic: [Discussion] PR reviews

These are simple updates. I tested both with nix-review but since both have higher rebuild count I would like to have a second opinion:
review: https://github.com/NixOS/nixpkgs/pull/58803
review: https://github.com/NixOS/nixpkgs/pull/53460

2 posts were merged into an existing topic: [Discussion] PR reviews

merge: https://github.com/NixOS/nixpkgs/pull/57359
merge: https://github.com/NixOS/nixpkgs/pull/58615
add tag “status: wait-for-upstream”: https://github.com/NixOS/nixpkgs/pull/54179

let’s check this one
merge: https://github.com/NixOS/nixpkgs/pull/53565

@NRHelmi please review the list of requirements for PRs to post here. I understand that you’re probably frustrated, especially since your first contribution has been sitting there for 3 months. You’re encouraged to

  • ping someone you think might be qualified/interested to review in the PR. Infinisil has already reviewed it once and may be willing to give it another look if you remind him
  • ask for a review on IRC or on discourse. Maybe start another iteration of “PRs ready for review”

Unfortunately I’m not very qualified to review nixos modules myself and there is one outstanding comment by Infinisil (https://github.com/NixOS/nixpkgs/pull/53565#discussion_r253309876) who has much more experience with that than I do.

2 Likes

Please check/merge:


Those PRs both have merge conflicts, so are not ready to merge. They are also from nixpkgs members, so posting them here is not necessary (they can merge themselves after you’ve given a positive review).

merge - https://github.com/NixOS/nixpkgs/pull/59942
merge - https://github.com/NixOS/nixpkgs/pull/59971
merge - https://github.com/NixOS/nixpkgs/pull/59899
merge - https://github.com/NixOS/nixpkgs/pull/59950
merge - https://github.com/NixOS/nixpkgs/pull/60194
merge - https://github.com/NixOS/nixpkgs/pull/60148
merge - https://github.com/NixOS/nixpkgs/pull/59872

If someone can validate or propose a comment for this pr?

It fixes a bug that prevents to use diskImageFuns.debian9x86_64 and thus generate debian9 debs for example. The workaround is to use the debian8 image for the moment.

Regards

close https://github.com/NixOS/nixpkgs/issues/24250

(hope it’s ok to link issues as well)

Done :slight_smile:

davidak via Nix community nixos1@discoursemail.com writes:

I left a review for this one:

Merge: https://github.com/NixOS/nixpkgs/pull/58649

These two have both been reviewed by someone else, but could be acted on quickly by a committer:

Close: https://github.com/NixOS/nixpkgs/pull/37353
Tag with WIP: https://github.com/NixOS/nixpkgs/pull/52424

Reasoning: The description of #52424 mentions that it obsoletes #37353, and the most recent comment in #52424 says the contributor is currently testing an additional proposed change.

1 Like

Merge: https://github.com/NixOS/nixpkgs/pull/55361
Close: https://github.com/NixOS/nixpkgs/pull/63476 because this change was included in #63876 which has been merged.