Let’s keep the meta discussion for PRs already reviewed here and keep the original post to reviews.
CC @ryantm @vcunat @Ekleog @aanderse @samueldr @Infinisil @matthewbauer @manveru
2 Likes
Good idea! I made some small grammatical edits and pinned this to help with visibility. I think we should make this into a wiki post so the community can improve the top post, and when someone posts a PR here, we should delete the post after it has been merged. If you don’t have power to delete a post, post that you took care of it, and I’ll delete it.
It might be enough to choose some keyword used in comments, so that people with push access can filter those reviewed PRs. I don’t know about GitHub offering any better way, as non-members can’t do much more than post comments.
Thanks!
I agree, any improvements to the top post are welcome. We can improve the process over time if it is well received by the community.
This is basically a lite-version of my [RFC 0030] Formalize review workflow by timokau · Pull Request #30 · NixOS/rfcs · GitHub proposal, which assumes ofBorg support for adding labels. Let’s see if there is interest and how discourse works out. There is also a realistic chance github may improve this in the future.
Now, I’m wondering whether discourse is a proper UI, as if multiple committers monitor the thread there is no good way of marking a comment over there as “done” so only one needs to click the link I think, but…
If discourse works for this is to be seen, but if we really stick to the “shouldn’t take more than a minute” rule race conditions are probably not too problematic.
1 Like
If the touched package or one of its dependencies has always failed on a platform, it should be explicitly marked as broken (by adding broken = True to its meta attributes), so that ofBorg reports “No Attempt”.
Are things like stdenv.isDarwin documented anywhere in the manual?
We’re talking about platforms here and broken = true; will make it broken for everything.
I don’t believe so. There’s a bunch of stuff that’s undocumented and requires reading the source in the lib/ dir, including things like stdenv.optional and stdenv.optionals. There’s also things like stdenv.ifEnable which lives in lib/deprecations.nix with no explanation as to why it’s deprecated (trawling through history right now, this file used to be called misc.nix and was renamed with no explanation).
In fact, I don’t see any function-level documentation for nixpkgs at all. A few things are described in the manual, but as part of the general guide and not an explicit reference.
2 Likes
I’d like to suggest that the PR template include a comment under “motivation for change” saying something to the effect for package updates that if there’s a GitHub release, or a changelog entry somewhere, it should be linked there.
Can we add that for updates, the reviewer should find the changelog(s) and add a link (if OP does not already do that), and check that there is nothing missing.
1 Like
What’s the best way to correct a nixpkgs-update automatic PR? Submit a new PR? Recommend the changes in review?
Just “fork the PR” and create a new one. The bot won’t be offended 
Just mention the replaced PR in the description. I’m not sure if githubs “fixes #xyz” keyword works with PRs, but you might try that.
1 Like
Just mention the replaced PR in the description. I’m not sure if githubs “fixes #xyz” keyword works with PRs, but you might try that.
We have recently seen it work for «Closes #NNNNN» in an update-collecting PR, I think.
Right, I think GitHub will only pay attention to the closes #NNNN phrases in the body a PR’s first comment/message.
This is a great way to handle r-ryantm’s PRs. If you close its PRs before merging the other one, it might open a duplicate PR on the next run.
Fair point, there is documentation for lib.attrsets, but it seems for nothing else.
I take it the “rebuild count” is the number in the label? What does that actually indicate?
Edit: Does that mean the number of packages that will get rebuilt as a consequence of updating this one?
Yes, it is the total number of packages that will be rebuild due to the change.
1 Like
I have edited the OP to clear up broken vs platforms, mention how to fix bot PRs and explain more clearly that reviews should be on GitHub.
1 Like
What is the value in having a changelog link in the PR? I could see the value if it was somehow accessible to the user when updating, but without that I’m not convinced its worth it to ask reviewers to hunt down changelogs for every single package.