Nixpkgs self-merge etiquette

(I see that there’s been a small disagreement brewing around some recent self-merges and this topic isn’t directly about that.)

Ideally, every Nixpkgs PR gets another pair of eyes looking at it before it’s merged. As a practical matter, reviews can be hard to get. Are there circumstances under which it’s acceptable to merge your own PR without a review? Say, if:

  • the author is the only maintainer
  • the PR has gone unreviewed for X period of time (two weeks? a month?)
  • the package touched has no dependencies and is not likely to be considered ‘infrastructure’ by anyone
4 Likes

Strict review requirement would clearly be either destructive or unenforceable and gamed.

We have it documented in writing that «maintainer reaction cannot be obtained quickly» is normally around a week (assuming it is a normal week with no clear reasons to not assume there has been five working days and two non-far-away days off). This also affects merges by non-maintainer non-submitter committers.

Impact of course increases the expectations about efforts to get feedback.

There are some expectations that affecting very recent work by others under non-pressing circumstances should give them time to reply.

Reducing objectvely measurable breakage via a plausibly minimal fix gets a pass on a lot of norms in practice. Self-merged stuff increasing breakage gets significant annoyance, of course. Objectively measurable breakage not within the scope of CI is kind of in a limbo.

I am not sure how uniform is this, but there is probably some feeling that self-merges increase the expectations around post-merge responsiveness.

5 Likes

This may be a bit of a special case, but I’ve been merging a lot of my own PRs related to RFC 140. I didn’t get reviews for many of these, even after mentioning them in channels. waiting for accepting reviews would slow down development considerably.

Or even worse: requiring reviews would incentivise creating larger PRs (since you can get many changes accepted together) instead of splitting the work up into smaller PRs.

While I do think such non-trivial changes should really be reviewed better, I think it’s okay when:

  • The PR quality is high:
    • writing tests and documentation when applicable
    • Keeping an explanatory Git history
    • writing a good PR description with context and links
  • The author is responsive and addresses issues as they come up, both before and after the PR is merged (as mentioned by @7c6f434c)
7 Likes

I’m regularly self-merging PRs for

  • Homeautomation related packages (e.g. home-assistant, zigbee2mqtt)
  • Various python packages
  • Firefox

and probably more things, that I just forgot.

I don’t think there is anything wrong with that. I rarely get reviews for most of these things, as they either appear too frequently (home-assistant makes like five releases per month), or too expensive to build (firefox takes at least 60–90 minutes to build). Furthermore, I do a lot of due diligence before going for the merge.

We are just not equipped to review everything in a reasonable time, so I regularly use self-merges to unblock myself.

10 Likes

One thing to keep in mind is the intentions behind the self-merge.

When done in a reactionary manner, as a retaliatory measure, it is simply unacceptable.

All of the criteria being discussed are not binary, but really are scales. It’s important to keep them balanced, and to err with caution as maintainers.

And also important to keep in mind that changes should be/have been done in good faith. Discuss with eachother when changes are problematic!

6 Likes

And speaking of personal practices, I have quick-self-merged (without waiting any meaningful amount of time, at least after CI eval) some niche updates, and quite a few build fixes.

5 Likes