Document Nixpkgs committer rules

[rhendric]

CONTRIBUTING.md covers the case of being a non-committer trying to get code into Nixpkgs. I don't think there's an authoritative place to look for committers who want to know, e.g., how long they should wait before self-merging a PR (on possible penalty of losing bits), or whose reviews are needed before that's okay, or whether it's okay to be a committer and both a Nixpkgs maintainer and the upstream author of a package.

Probably this is because we, as a community, have never really hammered out what those rules are. But if they're going to have teeth, and especially if the teeth get used without trying to work things out ahead of time, I, uh, really want to know where the lines are, for obvious reasons.

[pbsds]

It's very much possible to be both nefarious and compliant with any set of rigid rules we may come up with. I also believe too many such rules will in effect mainly serve to make nixpkgs less agile, as we risk no one becoming willing to for example bypass a "wait for CI" rule to unbreak a channel in fear of penalty. (Here is one example where I personally felt paralyzed.). We risk making all the small pragmatic choices you have to make on a regular basis being in violation of some some minor detail of some rule, which if compiled and enumerated years later by an adversary becomes a compelling argument to eject you. That is not an environment I want to operate in.

I propose we instead discuss and decide upon a shared a set of values for committers to align and adhere to (with maybe a very restricted set of rigid don'ts). One such value relevant to recent events that I myself strive but sometimes fail to stick to is one of insight, which I put in practice by preferring to wait about a week if I get no reviews or comments before self-merging a bump. If a bump is needed rather quickly I prefer to either ping more people or somehow document why I opted to merge early.

EDIT: "insight" may be a mis-translation from Norwegian, perhaps "transparency" is a better term describing the value

[rhendric]

It's very much possible to be both nefarious and compliant with any set of rigid rules we may come up with.

Yeah, totally. Similar to codes of conduct, the idea isn't to have a set of rigid rules that then constrain us from stopping bad behavior. The idea instead is to (a) make the mindset of the moderators/commit-bit-distributors more legible to people who aren't them and (b) call out a non-exhaustive list of things that we definitely don't want here, so that people can get a sense of the vibe before they commit too deeply to a system that isn't for them (or bounce off because they don't know that it is for them).

Doing things by consensus and vibes is fine if there's leniency and forewarning of disciplinary actions. Being quick with the axe is fine if everyone knows what brings the axe upon one's neck. Being a big group full of people with different perspectives is fine if there's some way of getting everyone on the same page for the important decisions, and it gets used. But all three together—no rules, any one person on a three-person team can swing the axe with no warning, and a big community with different senses of what is ‘obviously’ a problem—is untenable. I figure the first of the three is the one we want to change. That doesn't mean it has to go all the way to the other extreme. But some guidelines would be good, and they have to be signed off on by the commit-bit triumvirate, so we can have some confidence that we're coloring inside the lines as far as they're concerned if we stick to the guidelines.

[numinit]

I'd like to recommend a variant of the Collective Code Construction Contract, with changes tailored to nixpkgs.

The goal of the C4 is to allow people to merge correct changes rapidly with minimal gatekeeping, under the assumption that most people will produce patches that are good rather than bad, and that bad patches, including from someone who usually does good work, are a mistake. This is a completely reasonable assumption, and consistent with the Nix values:

We are here, first and foremost, as individuals working together.

Note that in the following, I've basically run s/maintainer/committer, since that's the naming we use.

To accept or reject a patch, a Committer SHALL use the Platform interface.

Definitely keep this as is.

Committers SHALL NOT accept their own patches.

Definitely reword based on the necessity for this in nixpkgs, perhaps something like this, which is also imperfect:

Committers MAY accept their own patches if they are approved by others beforehand, and go without merge for over a day afterwards.

Is a day the correct amount of time? Good question. I'm not sure.

Committers SHALL NOT make value judgments on correct patches.

Committers SHALL merge correct patches rapidly.

These two forbid different types of gatekeeping that cause problems like this one., and offer an alternative:

Any Contributor who has value judgments on a patch SHOULD express these via their own patches.

If you don't like it, put in the work, don't just block PRs.

Committers MAY merge incorrect patches from other Contributors with the goals of (a) ending fruitless discussions, (b) capturing toxic patches in the historical record, (c) engaging with the Contributor on improving their patch quality.

Don't allow bikeshedding, be able to fall back to reverts when needed rather than just blocking changes, and teach people who are newer and would like to learn.

---

I am newer to the commit process, and perhaps offering an alternative is a step too far. However, if the vibe is that people who want to contribute get blocked, and that committers are subject to arbitrary sets of rules, there will be less contribution and willingness to merge.

I strongly believe that the vast majority of contributors and committers want to help nixpkgs. If we create an environment of positive incentives surrounding the contribution process where people can be mentored into succeeding, we will get the community we want to foster, which is (in my opinion) that of a top-tier project that completely changes the game for global deployment of software.

[wolfgangwalther]

don't just block PRs.

I think we should have a very clear rule of: "If you intend to block a PR for any reason, use GitHub's 'Request Changes' feature." This would immediately make a lot of comments that appear to be blocking (for various reasons).. non-blocking.

It happened to me a few times: When I intend to block something I use that feature. But sometimes others understand my regular comments as blocking even though they are not meant that way.

[numinit]

I could easily get behind that. Having that as the standard would reduce communication headaches I've noticed.

[wolfgangwalther]

Committers MAY accept their own patches if they are approved by others beforehand, and go without merge for over a day afterwards.

Is a day the correct amount of time? Good question. I'm not sure.

I wrote up something about the "1 day or more" here... but then realized this is about "if they are approved". Do you mean I am not allowed to self-merge my PR, when somebody else approved it already?

I think 1 day would be too much in that case. I open a PR for a trivial change, a non-committer maintainer approves it, I should be allowed to merge immediately.

On the other side, for PRs where I don't receive any feedback at all, self-merging after 1 day is more often than not way too quick, imho.

[philiptaron]

I'm strongly in favor of responsible self-merge, occasionally without explicit review on GitHub. I see paragons of the community like @K900 and @mweinelt doing so. Their bias for action gets me new Linux kernels and Firefox security updates and break-fix PRs. I appreciate that. Let's draw some words around that responsible behavior (after all, unwritten rules should be written.)

Strongly co-sign a norm of using "Request changes" to block PRs, and stating the blocking reason when doing so.

After having been cautioned against using my quick trigger finger to merge spicy approved PRs, I have a norm of stating in a comment that I will merge this PR in a hour, day, week or whatever is appropriate for the scope of the PR (usually day) unless blocking feedback is received. I think this is a great way to gather trailing feedback while maintaining good order.

[numinit]

Do you mean I am not allowed to self-merge my PR, when somebody else approved it already?

Yeah, but to be clear, I think you should be able to self merge in that case, so maybe time is just the entirely wrong way to go about it.

Could hedge like this, assuming that we want to reduce change latency while still having approvals go through:

Committers MAY accept their own patches if they are approved by others beforehand, including those without a commit bit.

[wolfgangwalther]

Could hedge like this, assuming that we want to reduce change latency while still having approvals go through:

Committers MAY accept their own patches if they are approved by others beforehand, including those without a commit bit.

This is certainly better. It allows to do so, but "MAY" indicates that the best case is to have someone else merge it.

However, this would not cover the latter part of this, yet:

I'm strongly in favor of responsible self-merge, occasionally without explicit review on GitHub.

I agree with this. Sometimes, it's necessary to self-merge without review.

(If the general opinion is otherwise, then we should encode it as a branch protection rule to "require any approvals")

[pbsds]

That "MAY" is quite load bearing though. There is big difference between self-merging a leaf package bump and for example a patch to CONTRIBUTING.md deserving the label 9.needs: community feedback. How long should one expect to wait for specifically any ci/OWNERS or meta.maintainers to respond? Establishing or at least demystifying where that line likely is will greatly help distributing decision-making.

[wolfgangwalther]

Establishing or at least demystifying where that line likely is will greatly help distributing decision-making.

Here's my own take / approach, when I open a PR and wait for feedback:

• After a certain amount of time X without feedback, I switch from "need feedback / need approval -> OK to self-merge" to "no objection -> OK to self-merge" mode.
• This time X starts after the last time I received and addressed feedback and is reset when I receive feedback again.
• When in "no objection" mode, of course I need to be even more careful. It helps that there is a delay here, because I'll effectively self-review my own PR after time has passed.
• Most often, X is between 1 and 2 weeks for me.
• X could also be much lower. The extreme case would be a revert of something that I just merged and turns out to be broken. Or maybe a treefmt fix, because master is currently broken. Especially the latter, I might not even wait for CI to finish, because there is no point in running Eval, when I literally just ran treefmt to create the commit I'm about to merge.
• X could also be infinite. Some changes, I would never merge without feedback from others.
• X can depend on the type of change. For simpler, less controversial changes it's lower and vice-versa.
• X can depend on the "urgency" of the change. If CI is broken for others, I will be quicker to merge a fix. If nobody is affected, not so much.
• X can depend on whether there are any maintainers or code owners pinged. If there are, I will certainly allow time to respond. If there are none, then I might not wait as long for a drive-by reviewer to appear.
• X can depend on my own future work. If a change is trivial enough, I might merge it earlier to avoid merge conflicts with other things I am currently working on and not delaying those unnecessarily - especially when those are the kind of changes I would really like to have feedback on...
• ... which is the next point: X can depend on how much I want to have feedback, aka on how sure I am about my change myself.

Assuming that all of this is OK, I find it hard to "establish a line" to reflect that. Of course, if some of that is not OK, that might become the line you are looking for ;)

[rhendric]

Assuming that all of this is OK, I find it hard to "establish a line" to reflect that.

We don't have to exactly demarcate the zone of acceptable behavior. We don't even have to approximate it. Something as permissive as this would still be useful in adjudicating the most recent case:

• Committers must either get an approving review from another committer or wait at least one hour before merging their own PRs. No exceptions. If a security issue or eval-breaking issue or similar is important enough that it needs to be merged sooner than in one hour, that importance should make it straightforward to muster another committer to give an approval.

The specifics aren't important—add in waiting for CI or letting a non-committer maintainer give the approval or what have you. The point is, it's a clear example of no-no behavior, and not one so outlandish that we haven't seen it in practice.

On the other side could be a line like this:

• Committers are permitted to self-merge PRs that touch or add only leaf packages if the maintainers have been pinged, all CI checks pass, and they have received no feedback from anyone in one month.

Again, the specifics aren't important. The point is it's a clear example of ‘you will not lose your bit for this’ behavior.

There can be a wide gray zone in between, with lots of nuance and disputed heuristics for when X should be one week versus two! That's fine! Right now the entire domain is gray, and that's not fine.

[wolfgangwalther]

If a security issue or eval-breaking issue or similar is important enough that it needs to be merged sooner than in one hour, that importance should make it straightforward to muster another committer to give an approval.

You're right, this renders my "I literally just ran treefmt" argument void. If somebody's CI is failing, I will surely find somebody else to merge my PR, maybe even before CI finishes to run through. I don't need to self-merge without feedback for that. If you can't convince anyone to look at it, then it can't be urgent enough.

If we want to make this a hard rule, we should try to force it by GitHub / CI - and not "just" write it down. If we enable required status checks, maybe we can find a way to create a status check that will be marked as successful after the first approval - or automatically after, for example, 1 hour, whichever comes first. So one of the two would need to happen to even be able to hit the merge button...

Taking this a step further, the best case would be:

• To make everything that is forbidden actually impossible. Committers shouldn't be able to break hard no-no rules.
• To document only things that are 100% OK to do. Aka only positive rules.

Between those, there is a gray area. But we can try to make it smaller from both sides.

[rhendric]

If we want to make this a hard rule, we should try to force it by GitHub / CI - and not "just" write it down.

Code is a very acceptable form of writing it down! But in case that turns out to be difficult to implement, let's not let perfect block good. Documenting positive and negative rules, and then making a best effort to transcribe the latter in code, sounds like a good plan to me.

[philiptaron]

Between those, there is a gray area. But we can try to make it smaller from both sides.

Yes, which is why I orient around norms rather than rules. Norms are a standard or pattern, especially of social behavior, that is typical or expected of a group. That's what we ought to be interested in making: guidelines that form expectations of social behavior. They ought to be abetted on the one hand by code -- CI and rulesets set by @NixOS/org that govern what is technically possible -- and on the other by you and I, where "we don't do that here" and "thanks for doing that, I really appreciate it".

Norms can include things like "be vocal about appreciation" and "diligence is rewarded with trust" and "benefit of the doubt" where they would be either silly or monstrously oppressive if set down as rules.

Norms are about what this virtual community space feels like. They matter enormously. In many ways, they matter more than rules, since most rules are actually trying and failing to point to norms.

I disagree with almost all of Ryan @rhendric's rule-crafting in his earlier reply on these grounds. It's not fun living in a space with no exceptions to a rule, followed with heavy consequences! Even if I think I can live up to that most of the time, it's neither fun nor comfortable to do so.

Instead, let's have a norm of diligence... and if there's a pattern of lack of diligence, a process to encourage correction of those behavior from the committer team AND from us, the body that forms the committer group.

I love what Wolfgang @wolfgangwalther sketched out as his norms of behavior. They closely match my own. If everyone followed those -- and dare I say, most do! -- the need for rules would vanish because we would have no need. But I would hate being in a space where those norms were enforced like rules.

OK, fine, Philip, you hate rules. If you had to be rules czar, because this is the rules issue, what would YOU make, knowing there do have to be rules?

Fine question, unnamed interlocutor. These are still norms, probably, since I still don't like rules. Call them "Committer standards" or something.

1. You are responsible for what you merge.
   1. So is the author.
   2. Since you have the power to correct it, you are often more responsible.
2. You are responsible for how you behave in reviewing code.
   1. You are entrusted with upholding Nixpkgs-wide quality
   2. You are entrusted with making Nixpkgs a welcoming and inclusive space.
   3. This means you cannot make up a standard and personally enforce it.
   4. This means you defer to other reviewers if they understand the area better than you.
3. You are responsible for learning.
   1. If you are given feedback by others, take it to heart, or fight it fairly.
   2. If you are given feedback by the committer team in particular, they will note whether your privilege is at stake.
4. You are responsible for teaching.
   1. Don't just correct, explain.
   2. Don't just ❌, encourage what to do next.
5. Merging your own PRs is acceptable.
   1. Don't do so to avoid community discussion.
   2. Don't do so and break things. Again, you are responsible for what you merge.
6. You have limits. Please honor them and allow for other's efforts to come in, without gating whole areas on yourself and your review.
7. When you do break things, acknowledge your mistake and apologize.
   1. Try to make it better either by changing your behavior in future or by undoing what has been broken.