My experience of contributing to Nixpkgs

In regards to that, recently I’ve seen the CODEOWNERS features from GitLab. While it does not solve the finding a committer it might be useful?

(As much as I disliked GH this is not a forge migration suggestion, just throwing more ideas into the collective consciousness).

For item 1: Basically certain the first exists, although there are warnings that the detection isn’t always perfect. I think they just get added as requested reviewers? r-ryantm has explicit maintainer pings in its PRs also.

For item 2: Looking at https://github.com/NixOS/nixpkgs/blob/c4fa370b3419e637a73d9f5907b9752299824990/ci/github-script/bot.js#L496 suggests that something is happening with owners, although tbh I haven’t dug into what an owner is in the context of nixpkgs.

For item 3: It sounds like an interesting idea, although obviously a more complicated logic to implement. No ideas on whether or not there is anything like this in the stuff within .github/workflows, ci/eval, ci/github-script, and ci/codeowners-validator or other varioius CI-type stuff.

I want to begin to say that I can empathize with your feelings.
Onboarding on nixpkgs is hard, but at the same time, I’d argue it is much easier than on other Linux package repositories because of how open the process is.
Literally anyone can create a nixpkgs PR, and this probably also is a source of why we have so many more pending PRs than adequate reviewers.
Now I am not advocating for changing this, I actually think it’s one of the best feats of nixpkgs.
When I encounter a PR that shows all the signs of someone unfamiliar to contributing to nixpkgs, I try to get in direct contact with them to explain the process and procedure.
Doing this through a back and forth in PR review has most often turned out impersonal and counterproductive from my point of view.
As my time and resources are limited, I admittedly can’t do this direct engagement more often than a few times per year, but for those where I did, I always experienced a much faster understanding of what is required to get the PR into a mergeable state.

Then someone will come along, say “you misspelled the word ‘attribute’,” or “that doesn’t match how I would do it,“ and there it is. Your review. Nothing of substance said about the content.

There is an ongoing attempt to discourage nits.
They are not meant to be blocking, but as you rightfully noted, others will still shy away from dismissing them.
Do reach out to other committers if you encounter them. I know it seems hard, but communication is a big key here.

This is not necessarily their fault, as annoying as it is. Nixpkgs is a volunteer project and you cannot force people to do so. But there is also negative incentive for any other above to occur. Why review anything when you can just not? Why merge anything when you can just not? There’s absolutely no reason some Jane Commit should every look at anyones’ PR. In fact it sounds quite annoying to do so when she has been at work all day. Luckily to keep her commit bit all she has to do is merge one PR a year. So she merges an update for a tool she uses at work. “Great, I did my part for the year. See you next.”

I’ve been a committer for close to 1.5 years now.
Indeed, for those who just got ahold of the commit bit, merging less often and rather trivial changes will seem the saver choice.
I still see a big issue with the onboarding of new committers.
There was an attempt at bringing mentoring to new committers, but from what I heard, this was never really made use of.

I did suggest in the past to have a communication channel for committers, but unfortunately that was decided against because it might increase the perception of the privileged in-club.
I still think this would be a good thing to have, maybe even as a Discourse category instead of a Matrix room, in lack of better onboarding.
Also, a lot of committers still were “grandfathered” into the current state of nixpkgs, having been granted the commit bit in a time where stakes and activity were much lower.
Some of these actually don’t know the current criteria of reviewing and merging, some even stated they wouldn’t be able to maintain their packages if they lost it in their respective automated retirement PRs.
On the other hand, I doubt that drastically shrinking the amount of older existing committers helps with reducing the workload of reviews in any way, so this will not address your own woes unfortunately.

Taking a good look at projects that are quite nice to contribute to, what do they do? They have accountability and responsibility for PRs. Nixpkgs already has some in terms of meta.maintainers. How to make someone feel accountable or responsible for PRs from the peanut gallery? Take a good look at the Rust Project.

Nixpkgs also has topic-related responsibility in the form of teams.
In fact, these have been triaged recently to ensure they are not used to manage company-ownership of packages but actually restrict them to topics, making joining them not depend on external organization membership but on actual skill set and interest.
You can read more about this on https://github.com/NixOS/nixpkgs/issues/478780.
Also, the “CODEOWNERS” file suggested by another comment here already exists: https://github.com/NixOS/nixpkgs/blob/87524662728cd61d67c0d28abbabe3ccec182043/ci/OWNERS.

We do lack a mechanism to automatically assign a vetted reviewer to PRs that do not otherwise have someone responsible for the touched code, that much is true.
My only contribution to the Rust project did receive a review within two days, although that too just boiled down to “LGTM”.
Yet, I’d like to point out that the Rust project does not nearly experience the high amount of contributions we do.
Looking at just the past month, NixOS/nixpkgs merged 8750 pull requests while 2001 pull requests were opened that have not yet been merged.
In comparison, the rust-lang/rust repository has merged 729 pull requests in the last month, with 275 yet unmerged PRs that were opened in that time.
The ratio is quite similar, so arguably we are not doing worse than the Rust project at least.

Now what can we do to improve on this still?
There actually is some community tooling that helps with picking up PRs in dire need of review.
The best one I am aware of is developed by @FliegendeWurst: https://nixpkgs-prs.fliegendewurst.eu/.
I think this is an amazing tool, but I was not aware of it when I originally gained the commit bit.
Showing the tools available must also become part of the onboarding for new committers.

Another comment here raised the idea of approvals increasing the priority of a PR for committers.
The criticism of this becoming a “+1” is valid, but we could introduce a group of trusted reviewers as mitigation, as a pre-form of the commit bit.
There isn’t much risk coming from these, so it could be handed to any vetted person and revoked from anyone abusing it as a “+1” mechanism.
Further ideas that (as far as I remember) were considered by @emily and others were more granular privileges, partially by means of the rather successful nixpkgs merge bot.
A large amount of PRs only deal with package changes, but a committer gets merge rights for all parts of nixpkgs, be those NixOS modules, packages or the library.
Of course, blindly merging things to these without prior communication to the people responsible will rightfully get you a lecture and possibly revocation of the commit bit.

I think it makes sense to keep my reply at this length. I do have more thoughts on the topic, but writing them out takes time, and shaping them to be actionable even more.

11 Likes

I think what else could somewhat help tooling-wise is:

  • /remind-bot: I am not going to touch this for next 10 days without a reaction from maintainer, but please review-request me afterwards, and in the week afterwards I might find time to review this in the «maintainer-away» frame.
  • meta.reviewers: I acknowledge that I will not meet the canonical maintainer baseline expectations (originally established as roughly «will do best to answer any questions of another person who is trying to do something about the package»), but I am interested enough in the package that I am willing to be review-requested to merge it (as usual, after real-maintainer approval or a week with no reaction, and obvoiusly subject to the positively-merge bar being higher than no-blocking-review bar)

Both things are mostly about burnout-safely getting more reviews and merges from committers who have once tried and gave up on the firehose-triage (like me) and retreated to the smaller sectors of competence (and probably with energy expenditure limits for Nixpkgs reviews — which limits should be less stressed when the reviews requested are relevant to interests anyway).

7 Likes

To add, you can also just ignore the comment and mark it resolved (if they commented on a specific line/file). To make it clear that I’m ignoring a nit, I’ll say something like “this is not required per nixpkgs guidelines” or “let’s stick to concrete feedback” or so on, as applicable. Contributors are empowered to ignore nits, and if a reviewer (committer or otherwise) keeps pushing on a nit, that is a time to get someone else involved, as mentioned.

It’s definitely not useless; it indicates that someone tested the package and that it’s eligible for a committer to merge after they once-over the code. That is, the committer now presumably no longer needs to test it, if the code looks good as-is and the basic usecase(s) were covered in that testing.

It also indicates how many people would be interested in this PR merging :slight_smile:

3 Likes

I’ve have seen broken PRs without approvals merged within days despite failing nixos test. I’ve seen PR with approvals sit around for months. Hence my scepticism that approvals do much.

4 Likes

I’m curious how much of the review pain comes from the UX put on us by Microsoft-owned GitHub. I hear complaints about reviews with nits—but nits do matter, just not often to the one putting up the merge request. Maintainers should be able to easily fix up said nits if the rest is good enough, but the UX we are dealing with is optimized for armchair “do this thing for me, then force push, then tag me all over again for a communication roundtrip” from maintainers as leaving a comment is less ‘work’ from their side. Aside from that, often I find myself building a stack of commits that say package foo, which is required to package bar, which is required to package baz (my actual goal); layers beneath can be independently merged but are requirements to get to baz. It’s not clear whether I should open 3 PRs or 1 big PR would cover them all—but 1 PR with multiple packages is less likely to be reviewed if it has with a long title, many commits, and/or many lines changed—even if it is broken up in a easy to review. A good UX, IMO, would be one that prioritizes merging sooner where maintainers as well as being able to merge partial pull requests for commits that are in good order leaving just the ones in question for review (Patch Theory-based VCSs would also allow the stack order to be shuffled & merged to get even more approved patches thru, but I digress). Neither of these features are available in Nixpkgs’s proprietary code forge.

Additionally, a majority of the open pull requests are also from bots which makes it hard to find which ones are frem humans vs. version bumps by a machine (personally I use a userStyle to make it clearer if I go thru the PR list). There also ends up being a bias to make the bot update the package as you get access to @NixOS/nixpkgs-merge-bot merge, but the bot is slow to pick up new version when often I can manually bump something I care about in 2 minutes—it’s just that now I need to go back to begging for reviews which ends up being slower in aggregate.

All of these say I’m now usually end up trying to game my pull request for a what-might-actually-get-reviewed target instead of what would actually make the ‘best’ pull request. I don’t think this is a healthy sign.

7 Likes

Are people actually complaining about people leaving not nitpicky comments, the comments getting addressed and the PR getting merged? Because that just feels like the review process working a inteded. I think it’s more likely that people are complaining about nits when they solve the nits but their PR still doesn’t get merged. Which is frustrating for sure but the PR not getting merged likely has little to do with the nits.

I don’t think that’s bad though. The bot makes changes in a predictable fashion and can be trusted not to make malicious changes. Maybe the nixpkgs-merge-bot could be a bit smarter and analyze the changes to check whether they are version bumps instead of looking at who made the change but that’s sort of risky.

Having yet another bot that can do nixpkgs-update of a specific package by maintainer’s request, and make the update mergebot-eligible, might be a good thing, though. (I don’t say anything about effort of getting there, though)

3 Likes

25 posts were split to a new topic: Domen and the SC discuss commit bits

Technically, they have this:

Unfortunately, they don’t seem to be prioritizing FOSS projects, or even any private project that uses personal forks. :frowning:

We should fork the forge’s source then & add support… oh.

3 Likes

It’s a signal, it’s not the only signal :wink:
For example, least committers can filter by approvals: 3+ and find popular PRs that way.
Not everyone will operate that way, but some absolutely do (I would know, I used to be a committer, until I quit from not really wanting to invest my energy into nixpkgs due to unrelated issues).

Ehhhhhh see guidelines. Though obviously there are some caveats (e.g., contributors should not be writing code that is vastly “unique” or violating of established nixpkgs patterns), stuff like “change this formatting” or “alphabetically sort the arg list” is nonsensical and ignorable:

Unless there are standards forbidding the patterns used in your code or there are serious technical, maintainability or readability issues with your code, you can disregard these requests.

https://github.com/NixOS/nixpkgs/blob/c59305bab2065cfecc4944690d9eedbb56f3a9fa/CONTRIBUTING.md#a-reviewer-requested-a-bunch-of-insubstantial-changes

And I’m saying this from experience, since I’ve managed to have others merge nearly 300 of my PRs with basically minimal back-and-forth (unless I asked for help) and minimal effort from me to “advertise” the PR (while some PRs absolutely can take weeks or months before someone looks at it, in which case I post it in the discourse thread as a last resort, and that’s about it). I semi-regularly get nits and I semi-regularly tell them that I will ignore said nits, unless it’s a one-character change or something. PRs still get merged. The world continues spinning. etc.

2 Likes

In case anyone is reading this in the future feeling the same way- if you’re making meaningful changes to more than one package, (imo) you should probably split the PRs up. Every time I consider but don’t do this, I end up regretting it. Just this week, I opened #568020. The idea was simple: clean up the build for a package that I maintain. I split it into a few commits for my own sanity, but it probably could’ve all been done in three. Part of this cleanup involved breaking off the vim/emacs plugins pulled with the source repo into usable Nix packages (mostly so I could remove emacs from the build closure).

I naively thought this was simple enough to all be one PR- it wasn’t. If I’d been smarter, it would’ve been 3- one adding a vim package, one adding the emacs package, and one cleaning up the original package that depends on the first two getting merged first. Two pinged teams, a small documentation change, and 40+ comments later, I realized that splitting it would’ve made things easier on everyone.

To be clear, I don’t imply that this is an example of dysfunction- things were functioning great actually! I don’t really do editor plugins, so of course my original commits did need work! It just would’ve saved lots of my own time, as well as some time from the pinged teams to do three PRs instead of one. Getting three merges is trickier than one, but as I said a long time ago:

TL;DR: Just split the PRs! (also a better UX could fix this)

2 Likes

On the contrary, I was recently told not to break up a standalone library away from its downstream consuming library as an individual PR recently: ocamlPackages.typegist: init at 0.0.0 by toastal · Pull Request #559346 · NixOS/nixpkgs · GitHub so I can’t say the feedback has been consistent (not that it necessarily needs to be consistent as different cases may prefer broken out vs not).

2 Likes