If the change is anything more than changing the version number and sha256, having access to the changelog can help provide context for the other changes being made. It also provides an easy way to learn about what the package itself is if you’re not already familiar with it.
I agree that this is very important. If I hadn’t read any of the changelog entries for package updates for software that I knew virtually nothing about, I wouldn’t have reviewed (sometimes merged) so confidently.
Most projects had some sort of “release notes” too if their wasn’t a changelog. And at a last resort I would use the “compare changes” feature that’s present in many places.
Changelogs are also a good help in figuring out whether the PR can or cannot be backported.
Indeed a reviewer should be determining whether a change needs a backport or not.
I was mostly thinking about r-ryantm PRs, otherwise the author should already include the link to changelog as @lilyball says above.
As mentioned by @worldofpeace, changelogs are essential to properly review PRs. I have reviewed dozens of PRs that did not link the changelog and upon finding them and reading them, I discovered that the author forgot to add/remove some dependency.
If there is no changelog link, I want to encourage reviewers to find it and share it. It will save other reviewers time too.
1 Like
My apologies if this is not the appropriate place to ask this, but what is the general policy in nixpkgs for library update PRs that break packages that depend on them, e.g. the libtorrent 1.2.0 update? Do you recommend that the library should be split to have multiple versions so the broken packages can use the old version? Or do you have some tag added or just close the PR? Or does it depend on the situation?
1 Like
Of course 
Still, in general I’d go through these solutions in order of preference:
- look if there are new upstream versions of the broken packages that work with the new library; apply the updates if there is
- look if there are new upstream patches for the broken packages to make them compatible; apply the patches if there is
- create an upstream issue about it if there is not (not really a short term solution, but good open source etiquette)
- ask myself whether there is any urgency to the update or it may be better to just wait for the broken packages to add support for the new library. If there is no real urgency, you can ask in the “PRs ready for review” tags to apply a “wait for upstream” tag and ideally also revisit the issue after some time.
- create two versions of the library in nixpkgs and configure the broken packages to keep using the old one
2 Likes
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.
1 Like
Can we add to contributing.md something like:
- added changelog for package update?
1 Like
Any thoughts about including other repo’s in this effort? Things like nix or NixOps would also benefit from more reviews and reduced PR count.
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.
I like the idea for information about changelogs to be at https://github.com/NixOS/nixpkgs/blob/180aa21259b666c6b7850aee00c5871c89c0d939/.github/CONTRIBUTING.md#submitting-changes and in the nixpkgs manual too.
Reviewing an update pr entails reviewing the changes to the package.
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.
1 Like
Of you’re doing more than a couple drive by commits here and there you should open a PR asking for access to the bot: GitHub - NixOS/ofborg: @ofborg tooling automation https://monitoring.ofborg.org/dashboard/db/ofborg
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?
1 Like
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?
There have been several of them, those actually gave me the idea for “already reviewed”: PRs ready for review
I agree that those two are supplemental.
Below 1200 open PRs. Bravo!
3 Likes