maintainers: document expectations #250344
Conversation
Motivated by https://discourse.nixos.org/t/where-did-you-get-stuck-in-the-nix-ecosystem-tell-me-your-story Address the uncertainty around maintainers by defining what it means, what are the expectations and power you get.
|
Maybe measure the time in terms of lack of reaction to specific reports of regressions or significant upstream updates? |
ofborg
bot
added
10.rebuild-darwin: 0
|
We could write a script checking if a package is outdated for $timeframe based on repology data and if it doesn't get updated in $timeframe mark it as unmaintained. I already thought about that a bit. |
I'd not go that far. For this PR we should only try to formalize a formal understanding of the role & the responsibilities and not discuss technical solutions for (arguably) social issues. |
I'm fine with that but then we should probably make the timeframe less specific for example writing something like package updates should be performed in a timely manner. I think the issue in this case is not just social since there are other reasons for people to not update stuff like f.e. death so some kind of automation that sends a ping after $months seems like a sensible thing. |
|
Relaxed the wording a little bit so the 3 months are in relation to package-related notifications. If the package is stable it won't generate any notifications (upgrades / issues / ...) and the maintainer would stay on board. |
maintainers/README.md
Outdated
| in a functioning state, and keep up with updates. In order to do that, they | ||
| are empowered to make decisions over the packages they maintain. | ||
|
|
||
| By default, we expect committers to wait at least a week before merging |
There was a problem hiding this comment.
I would add a mention for packages that are not affected by the "one week" rule, that is: "code owned" packages which have a large reverse dependency tree, e.g. systemd, LLVM, etc.
I don't think it's easy to put an amount of time, I'd state it as:
Committers should reach out to the active code owners and discuss the timing for a potential merge, in the event where no active code owner can be reached in a reasonable amount of time (i.e. taking into account potential holidays), merge can be performed nevertheless with a provided rationale.
The idea is to:
(1) ensure that no stray committer merges a systemd change while not being in touch with active code owner of e.g. systemd which may have extra context on ongoing changes (internal information, etc.)
(2) ensure that abandoned "important dependencies" can be slowly picked up by new committers who will garden them and find a new maintenance story for them hopefully, e.g. LLVM
Tiny version bumps and security updates could not be totally exception here, I don't have a strong opinion on that, I know that systemd security updates can lead to breakage, we had e2fs tooling's tiny version bumps breaking bootloaders (something something XFS), etc. It's a very hard problem in general.
There was a problem hiding this comment.
Tiny version bumps and security updates could not be totally exception here, I don't have a strong opinion on that, I know that systemd security updates can lead to breakage, we had e2fs tooling's tiny version bumps breaking bootloaders (something something XFS), etc. It's a very hard problem in general.
Would it make sense to make an exception for security updates / fixes earlier if e.g. the security team is involved as reviewer?
Generally I think that case of e.g. important bugfixes (e.g. $bump fixes data loss on runtime) or security fixes it should be possible for a committer to decide on a case-by-case basis what's best. @RaitoBezarius has brought some great examples on why I doubt that it's possible to find a general rule for this.
Also, such a ruling would exclude all those cases where the sole motivation of a person not maintaining the package in question is e.g. "wants to have the latest version available, immediately".
There was a problem hiding this comment.
Agreed with @RaitoBezarius on specifying timing, or rather pushing that responsibility to maintainers themselves. Maybe we could establish a convention where maintainers are supposed to communicate what they can deal with.
That information could also be used to judge if someone is inactive.
There was a problem hiding this comment.
Can you look at c67b737 and tell me if that is good enough? I'm keeping the text short so people are more likely to read it. And therefor try to not overload with exceptions.
There was a problem hiding this comment.
We also need better annotations on the meta attributes for the maintainers to communicate some of these aspects to the other committers. Specifically, maintainers should list the architectures they are willing to maintain. But that's for another PR.
There was a problem hiding this comment.
Interesting point @ architectures they are willing to maintain.
There was a problem hiding this comment.
I opened #251008 for exploring the question, feel free to carry the conversation there.
There was a problem hiding this comment.
I wish we had better automation for this kind of thing, but documenting this (and adding some rough timings) is already an improvement!
Let's include the suggestion from #250344 (comment) though, I'll merge it after that.
|
|
||
| The inactivity measure is currently not strictly enforced. We would typically | ||
| look at it if we notice that the author hasn't reacted to package-related | ||
| notifications for more than 3 months. |
There was a problem hiding this comment.
I assume that "no reaction at all" is meant here, correct? I.e. this isn't done on a per-package basis (otherwise one could lose the status if a fellow maintainer is always faster and you don't have anything to add most of the time).
When I first read this paragraph, I wasn't sure, so perhaps this should be rephrased a bit to make this clearer.
Also, we'd to give people who are about to lose their status a heads-up, correct? I guess this should be mentioned here as well :)
There was a problem hiding this comment.
Well, this is a non-pressing change to a package one is a maintainer of, and I guess «wait a week or a bit more for maintainer reaction» should be treated as «maintainer in question» in this case…
There was a problem hiding this comment.
Removing a maintainer requires making a PR so that that person would be notified. But I'll add a paragraph for that.
I still think the test should be per package. Some people are active in some areas of nixpkgs, and have abandoned other parts that they don't care about anymore.
Worst case you can react on the removal PR.
There was a problem hiding this comment.
Yes, pretty sure I have completely forgotten about some packages I have maintained back when I used them…
maintainers/README.md
Outdated
| are empowered to make decisions over the packages they maintain. | ||
|
|
||
| By default, we expect committers to wait at least a week before merging | ||
| changes on packages they are not maintaining. This should leave enough time |
There was a problem hiding this comment.
Strictly speaking, this does not make an exception for a non-committer-maintainer approved changes. Maybe it would be better writing to say it?
There was a problem hiding this comment.
Can you expand on what you mean? I'm not sure to understand
There was a problem hiding this comment.
«By default, we expect committers to wait at least a week before merging changes on packages they are not maintaining.» — however, we do not currently wait to give all maintainers time to comment, if one non-committer maintainer asks to merge, this is currently supposed to be good enough when the committer also likes the change.
There was a problem hiding this comment.
I also realize we don't describe how conflict resolution happens in case multiple maintainers of the same package disagree.
|
I believe this PR to be in good shape, with the eventual typos. Please take another look. |
Co-authored-by: 7c6f434c <7c6f434c@mail.ru>
| provided enough time and priority has been given to the maintainer. | ||
|
|
||
| For most packages, we expect committers to wait at least a week before merging | ||
| changes not endorsed by a package maintainer (which may be themselves). This should leave enough time |
There was a problem hiding this comment.
Does that mean the endorsement of one maintainer is enough? What if a package has multiple maintainers? What if they disagree?
There was a problem hiding this comment.
I don't know if I'm ready to open that rabbit hole. For now, I suggest leaving that unspecified.
There was a problem hiding this comment.
IMHO, conflict resolution is 2 or 3 magnitudes harder than anything those PRs are doing so far, I would +1 not going into that rabbit hole.
There was a problem hiding this comment.
I think putting a specific duration on «wait for some maintainer reaction» is good, and everything further is reliant on judgement calls that are hard to predefine. We don't ask to definitely merge once there is a single maintainer approval, after all!
There was a problem hiding this comment.
Alright, then lets postpone this. It's just something I'm regularly asking myself when trying to decide if I should just merge something or if maybe I should wait.
There was a problem hiding this comment.
I guess for me the correct question will now be «will I be sincere or hypocritical saying ‘Sorry, after a single maintainer approval I did not understand this is going to be controversial’?».
There was a problem hiding this comment.
Can we have some tooling to get reminded of that time?
There was a problem hiding this comment.
I do not want to be mean, but calendars are pretty good for this.
There was a problem hiding this comment.
(Actually, no, they are not, for multiple reasons: they are hard to set up to get exactly the relevant notifications for this batch when you cut out some time to look at Nixpkgs, and most of them don't integrate well enough with browsing the pull request.
On the other hand, an option for CI re-requesting reviews in case there is a check you want to let finish is useful in any case, and if it exists, maybe remind-me-in-a-week would be doable on top of that.
So the need is real, but not decision changing for this PR.)
There was a problem hiding this comment.
I agree; we need some more tooling for this.
Inspired by NixOS#250344 (comment) Here, I suggest what architectures I commit to maintain for my own packages.
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
|
I'm sorry, but this last review is expanding the scope of the PR. This is exactly how PRs die; because reviewers keep asking for more stuff until the contributor gets exhausted. Before submitting reviews please ask yourself; Is this a unit of work that is consistent and that improves on the existing situation? If yes then let's merge this and start gathering real-world data from it. I'm sure more PRs will follow afterwards. |
Co-authored-by: Frederik Rietdijk <freddyrietdijk@fridh.nl>
|
Alright, this is ready. Unless anything new comes up, I'll merge it in a few days. |
Motivated by https://discourse.nixos.org/t/where-did-you-get-stuck-in-the-nix-ecosystem-tell-me-your-story
Address the uncertainty around maintainers by defining what it
means, what are the expectations and power you get.
Note to reviewers: some of the numbers are arbitrary and I expect would be tuned by experience.
Are there areas or cases that you think should be covered that I missed?
Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)