I agree that changelogs are good, I’m just not entirely convinced if they are worth the work required to make them a hard requirement for every update PR. I think if we want to prioritize changelogs more (which is a good idea), we should
add a meta attribute for them (that points to a listing of changelogs or something)
shift the burden from the reviewers to the PR authors to link the changelog for the particular version
Instead of requiring reviewers (which we already do not have enough of) to do this highly repetitive work.
They require a different set of permissions (I can’t merge those PRs), a different skillset and a different review workflow though. They also don’t have as many low hanging fruit that most people can review. So it should probably be a separate project.
Tweaking the PR template to just have a comment under motivation requesting a link to the changelog should be sufficient I think. I don’t see any need to have it be a meta attribute, or even a checklist item, unless we want to actually require it instead of merely encouraging it.
One thing that might help is doing rollups similar to how they are done with Rust: Rollup of 5 pull requests by Centril · Pull Request #59760 · rust-lang/rust · GitHub. I don’t know how Centril does this, the commitment he has for this is nearly superhuman but I think the idea is certainly something that would be cool to try. A lot of the changes come are for cosmetic changes, minor version bumps, documentation clarifications and so on, and often they linger in the queue waiting to be merged.
I don’t know how you’d solve it for NixOS, Bors works very similar to GrahamcOfBorg, though it is a bit more automated. I think it’d be cool of approved maintainers could roll up a bunch of minor PRs, let the robots do their job and then automatically merge them.
I thought the rollups were primarily because otherwise you end up with a huge backlog of stuff waiting on bors to build, as rebuilding Rust is rather slow. Meanwhile, my experience submitting packages to Nixpkgs is there’s a long wait before a committer asks ofBorg to build it, it gets built relatively quickly, and then another long wait before a committer merges it.
It’s not like we require all boxes in the checklist, though.
And defining a meta attribute is sometimes useful for packages that put changelog in a predictable place; without defining such an attribute, the checks will complain about a typo in meta.
Also, as a supplement to this new PRs that have been reviewed thread I think a “Please review my PR thread because no one has yet” thread would be excellent. Clearly there are people who wish to review PRs and clearly there are PRs which have fallen into the waiting game… shall we connect the two?
You’re probably correct on that account, and I agree. I’ve seen that there’s an issue for ofBorg to add a build-and-then-merge command which will build it, and if successful wait 24 hours before automatically merging it to master. Might be a good compromise?
Could Nixpkgs adopt a policy to ease the update burden? Perhaps a set of tests that would signal to ofborg that the community would accept minor version bumps for specific packages without manual review? This has the cost of increased breakages for people on bleeding edge. But those breakages focus the effort to where it is most critical.
Increase the number of people who can commit? Assign areas of responsibility?
Encourage reviewers/commuters to modify PRs directly instead of relying on the originator review-recommend-edit cycle.
Question to reviewers: what slows you down the most? What gives you the most reason the doubt a PR?
Is the broken/not broken system too binary?
Any other ways we could improve the nix-review process? Is there a danger of over-reliance on nix-review? (Perhaps a list of problems that are often missed by nix-review).
I suspect nix-fmt could have a strong positive influence on the PR effort, reducing manual syntactic changes.
Remove old/stale PRs, or backlog them?
Any other policies that people apply during the review process that could be automated?
Even if we wanted to do that, how would we automatically differentiate between minor version bumps and potentially malicious PRs?
That has been part of the solution for a while, but its not working very well. I feel like new committers often tend to lose interest / burn out. Its not infinitely scalable either: each committer is a potential security risk for nixpkgs users.
I think that is the exact opposite of what we need for two reasons:
we have plenty of people who want to get changes in, not many people who review. Shifting burdon from the first group to the second is not going to help
the people proposing changes are not going to learn that way
Lack of clear commenting of intentions is one thing. Missing upstream reports another. GitHubs UX yet another.
I still think a process of
contributor proposes PR, it is tagged with needs:review
reviewer reviews, tags with needs:changes / merges / tags with needs:merge
contributor makes necessary changes, re-tags with needs:review.
Then, we can just automatically close all PRs that have been in needs:changes for too long and reviewers have a clear place to look.
Even if we wanted to do that, how would we automatically differentiate between minor version bumps and potentially malicious PRs?
Because a minor version bump changes exactly two lines in the code: version and sha256 in a predictable way.
That has been part of the solution for a while, but its not working very well. I feel like new committers often tend to lose interest / burn out. Its not infinitely scalable either: each committer is a potential security risk for nixpkgs users.
Changing version to something that is not an upstream version may have security implications though. I’m not sure if it is really that “predictable”.
Now just trusting r-ryantm would be a different matter. I think more or less the only thing blocking that is more thorough automated reverse-dependency testing.
Of course, I didn’t mean to imply that the spoiled youth has become lazy Just that making more people committers does not have as strong an impact as one would hope.
Changing version to something that is not an upstream version may have security implications though.
Well, if the source is fetched by a version-based URL from a known-good upstream mirror, we do get some version from upstream. Checking that version actually increases should not be that hard. Whether the version is the latest is a better question, of course. (On the other hand, the more open PRs we have, the easier it is to make a human miss this question, too)
Of course, I didn’t mean to imply that the spoiled youth has become lazy Just that making more people committers does not have as strong an impact as one would hope.
Well, if we do not add committers and do not add a way for non-committer maintainers to merge or something like that, the situation is getting worse on its own; if we add many committers, the situation might be getting slightly better, even if slower than desired…
What about adding committers for specific packages? Or rather, meta.maintainers could approve a PR, and a passing ofborg run could be enough to commit. It would provide more meaning to be listed as a maintainer.
As a maintainer and user of a package, I am in a much better position to evaluate PRs related to it. When trying to review other packages, I often cannot do a proper evaluation due unfamiliarity with the package/ecosystem/etc.
I also find it “frustrating” sometimes there are small commits that we could validate that would save time for other more “experienced” committers who would be more beneficial in their time to validate new packages or complex modifications.
Just that making more people committers does not have as strong an impact as one would hope.