Would explicitly mentioning the option of pure-Tests PR serve as a kind of encouragement?

Thanks @Ma27 and @FRidh for the quick review.
Just to make sure you've seen the diagrams I've drawn, please have a look at https://github.com/mrVanDalo/rfcs/blob/rfcs-pull-request-workflow/rfcs/0053-pull-request-workflow.md
I thought having SVGs is better, but I will also render some PNGs so you can see the ChangesView too.

@7c6f434c what do you mean by that?

Would explicitly mentioning the option of pure-Tests PR serve as a kind of encouragement?

You mean I should create a special rule-set for pull-requests that are only adding tests?

I guess…

The main point I would put there is «don't forget to mention the maintainers of key packages and modules», as the bot doesn't handle this.

(Maybe the same is actually true for adding modules…)

Nice writeup.

One question about modules. Would it be wise to specify when they should be backported? Right now it says “SHOULD NOT”, but off the top of my head the only situation I can imagine where I would want a module backported is if it’s security-related.

@carlosdagos In #53 (comment) , I try to come up with examples when it makes sense to backport modules. If you can remember a Pull-Request please let me know.

Would it be wise to specify when they should be backported?

@Ma27 pointed out

module change that's needed due to a package backport.

I can't think of any other reason so far. But SHOULD NOT does not mean MUST NOT.

Hi. Could there be something like “bots SHOULD NOT open pull requests”? Pull requests are calling humans for reviews.

Unless there is a fully automated reviewing and merging process that could be triggered by bots, opening a pull request is the privilege of reasonable beings.

@vbgl a lot of Pull-Requests already created by bots. For example : https://github.com/r-ryantm

@vbgl I disagree. We currently have automated processes creating pull requests upgrading packages for e.g. security updates to good effect, saving the time and attention of humans. However, these updates in my opinion should still be checked manually by a human in most cases.

I'm definitely in favour of continuing to allow bots to open PRs, like @r-ryantm does. Saves lots of effort.

In an ideal world, bots would do all of the work of finding, building, testing and PRing new versions, and humans would just approve that the changelog looks good and that the upgrade is desired.

I too would like to throw in my support for bots, they should be allowed to open PRs. I also have some grammatical advice for this document.

1. Pull-Request is written as pull request, per https://help.github.com/en/articles/about-pull-requests
2. There seem to be some inconsistent casing, Backports and Document appear with a capital first letter quite often in the text where they should be written in lowercase.
3. The casing in the first letter in a list should in my opinion be capitalized, i.e. * Big letter

This RFC seems more like a documentation than policy issue. If it's describing the existing process then it can be submitted as a PR to the nixpkgs manual directly.

@zimbatm I'm ok with that. I can put it in the documentation once all the discussions are settled. I guess looking at the diagrams is a bit harder in the docbook manual.

This RFC seems to me much more about the backport workflow than the PR workflow.

@Zimmi48 What are you missing in the pull request workflow?

The contributor may have no idea. The reviewer may also have an opinion.

@Zimmi48 Of course the reviewer can also arrange a backport, but this would increase the workload of reviewers. I'm not against it, just mention it.

What are you missing in the pull request workflow?

I don't know exactly, but as a maintainer without much experience, I don't see anything really informative in this workflow. It's probably too much of a bird view to my taste.

I added some changes from the discussions.
I must say I agree with @Zimmi48 a bit. My strategy would be like @zimbatm proposed, to create a pull-request in the Nixpkgs documentation. This has the benefit, that it can be improved over time. An RFC usually is fixed.
I will put a TODO.md right next to the new documentation section, containing my current todo list about this topic.

Thanks to everybody for your help.

Just a small nitpick, the States of a pull request graph mentions Contributor has done his work, but not all of us are men 😅

Also, should there be some kind of policy about force pushing vs creating new commits during code review, or should we just use whatever works?

(looking at the SVG) I think the main part of the PR workflow that needs clarifications is depth of testing, and sometimes handling tradeoffs in the known breakages. Saying «functionality of all affected packages» doesn't help much in any interesting case (like e2fsprogs…).

Also, has I missed something or is there nothing at all about upstream Changelog in the RFC?

I guess this text could be a fine first introduction to the process if put in the documentation, but putting this as an established policy is risky

Just a small nitpick, the States of a pull request graph mentions Contributor has done his work, but not all of us are men

Very much true. This slips through some times, sorry. I'm trying.

Also, should there be some kind of policy about force pushing vs creating new commits during code review, or should we just use whatever works?

You force pushes to you own branch. Or more what belongs to a commit and what should be split in multiple commits?

You force pushes to you own branch. Or more what belongs to a commit and what should be split in multiple commits?
No, this was a question about iterating the fixes in additional commits before squashing and force-pushing

I typically ask contributors to please use additional commits for the points I review.
I feel like in general that's a good code review practice, otherwise I have to start completely over with my review which can be tiresome.

I typically ask contributors to please use additional commits for the points I review.
I feel like in general that's a good code review practice, otherwise I have to start completely over with my review which can be tiresome.

For PR flow I've recently settled on pushing a bunch of commits called "fixup xxx Description," then rebasing, then pushing again, which leads to both a clean history and a nice review flow.

@sauyon Yep that's exactly what I suggest. Git also has the fixup commit feature which is very handy for interactive rebases.

I typically ask contributors to please use additional commits for the points I review.
I feel like in general that's a good code review practice, otherwise I have to start completely over with my review which can be tiresome.

We might also consider adding https://reviewable.io integration to our PRs.

I found it an excellent review tool for my open-source projects, because it allows to:

• review across force-pushes (you can see what changed since your last push)
• review commit-by-commit instead of whole-PR if desired (good to ensure no bad commits make it into the history)

Comments made there reflect back into Github, and the other way around, so any reviewer may choose to use it or not. Github review emails work as usual.

Like Github, it's free for open-source projects.

For an example, here's a screenshot of me and @qrilka reviewing a PR iterated with force-pushes on my project nh2/hatrace#49:

Here you can see how it tracks how far the reviewers have read across the 7 different force-pushed versions of the PR. As a reviewer, you can display either only the changes since you last looked, or between any force-pushed revisions you manually choose.

I found that this approach saves time because it saves a review roundtrip: People can directly force-push their respective best versions, instead of first pushing fixup commits and then post-review squashing them.

I've used Reviewable in the past and it's an excellent tool, so I second that recommendation.

I think that this RFC, as it stands, is far too intimidating to be useful for new contributors — an extremely long list of rules and complicated diagrams and SHOULD/MUST-style language is going to put people off contributing at all, and the ease of contributing to Nixpkgs compared to other package repositories is one of its core strengths. I think a description of how things usually work for new contributors in the manual would be good. This feels like more like imposing how things should work, and I don’t think we need rules like that.

The RFC also claims to be describing how things currently work rather than proposing something new, but the flow it describes doesn’t match with my experience as a committer. For one thing, it talks at length about backport PRs, but in my experience at least backport PRs are much rarer than the committee who merged just cherry-picking directly to the appropriate branches. I think this works well for most cases. Backport are usually uncontroversial, but they can already be annoying to do, so making it more effort by making them go through PRs would, I think, result in fewer backports.

I think you should take some time to figure out what it is you want to do here — if it’s just to describe things as they are, make a pull request to the manual and we’ll help make sure it’s accurate. If you actually want to change how things work, then it should be done as an RFC that doesn’t suggest that it’s entirely descriptive.

We might also consider adding https://reviewable.io integration to our PRs

What does such an integration consist of? From what you’ve described, I don’t see anything that would even be visible to anybody but the person using it for review?

@alyssais Agree with you completely here, was waiting for you to state it 😄 . I also don't see what is a workflow here, it's just a definition of how changes get integrated which isn't going to be useful to contributors. Unless they're interested in researching how things get integrated into an opensource project like this. A proposal that could benefit from an RFC would be something like an onboarding process.

Though I don't disagree with the inspiration, which I'm seeing is that documentation on how to submit a pull request to nixpkgs and who's going to review it and such would all be helpful (github teams, ofborg). All things I've gone out of my way to explain to people individually. In it's current form it's not designed for that kind of consumer of information though.

But all in all, I'm glad these discussions happened because an RFC was opened.
(in this accessible way on GitHub)

✨ @mrVanDalo @Lassulus

What does such an integration consist of? From what you’ve described, I don’t see anything that would even be visible to anybody but the person using it for review?

@alyssais See what I mentioned above:

Comments made there reflect back into Github, and the other way around, so any reviewer may choose to use it or not. Github review emails work as usual.

Concrete example: nh2/hatrace#41 (review)

When you post review comments in Reviewable, they appear on the PR as comments, and you can click to jump directly into reviewable.

That means they will be visible to anybody subscribed to the PR or reading through its updates, as usual.

Ok, I would close this pull request than, as everybody agrees (including me) this pull request should never be merged.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-many-people-are-paid-to-work-on-nix-nixpkgs/8307/88