NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.08k stars 14.13k forks source link

Add MAINTAINERS file + associated policy / tools #13602

Closed edolstra closed 6 years ago

edolstra commented 8 years ago

Following up on http://lists.science.uu.nl/pipermail/nix-dev/2016-February/019811.html.

To prevent the Nixpkgs from descending into chaos, we need some policy about who is allowed to commit/merge what. We already have meta.maintainers for packages, but not for other parts of Nixpkgs/NixOS. In particular it's completely unclear who "owns" central parts like lib/ so it risks becoming a free-for-all.

So I would like to propose the following:

Does this sound reasonable?

7c6f434c commented 8 years ago
   HASKELL INFRASTRUCTURE
   H: peti
   F: pkgs/top-level/haskell-packages.nix
   F: pkgs/development/compilers/ghc/*
   F: pkgs/development/haskell-modules/lib.nix

where "H:" is a GitHub user name and "F:" are filename-matching regexps to help identify whether a particular change affects this subsystem (not required).

Do I understand correctly, that H: line can also be repeated?

  • Add meta.maintainers fields to NixOS modules.
  • Document the policy (e.g. changes need approval from the responsible maintainers).
  • Make a little tool that given a diff tells you who the responsible maintainers are. It would do this by 1) matching the modified files in the diff against the "F:" regexps in the MAINTAINERS file; and 2) looking for a meta.maintainers field in the modified files. Also make a GitHub bot that applies this tool to incoming PRs and pings/assigns the maintainers.

Does this sound reasonable?

What do we do with PRs effectively creating new subareas?

What do we do with tools/misc and similar?

How can I mark a package «feel free to ping me for review/ask for help/etc., but I also trust any willing NixPkgs committer to apply a change that is not obvoiusly bad and keeps the thing building» (this is meaningful for many small leaf packages, I think).

For small things the key (in my obviously wrong opinion) thing we lack is even more manpower for keeping them up-to-date and not-bit-rotted; for core and complicated things, stability matters, of course.

heydojo commented 8 years ago

:+1: 100% agree @edolstra

edolstra commented 8 years ago

@7c6f434c

jagajaga commented 8 years ago

:+1: But I thing that we should also have people who can coordinate others in PRs. Like nowadays I usually merge things myself but if I'm not sure about the patch or it's too complicated for me I call the responsible guys. (Also I have great experience with our repository and know well it's infrastructure that's why I know who can help us to solve problems or just help in a PR). Also nowadays @pSub do @joachifm do a great job with labeling pkgs. And I thing we must leave several core people with current rights as they have them, like @peti @domenkozar @vcunat @abbradar @aszlig @pSub @7c6f434c @nckx @wizeman @edwtjo @ttuegel @bjornfor @jagajaga (myself) @shlevy @lethalman if they want to.

7c6f434c commented 8 years ago
  • This would probably fall under a general "Nixpkgs scope" topic (presumably maintained by me).

Please assign some more trusted reviewers for such things, we need all of you alive.

Adding things should have a lower risk of breaking everything, hopefully…

  • Well, pkgs/tools/misc are just regular packages with meta.maintainers attributes, so no need to list them in MAINTAINERS.

Ah, so not all packages fall into such groups. Noted.

  • The kernel's MAINTAINERS file has an "R: ..." field to list designated reviewers, which could be useful for this (and maybe an analogous meta.reviewers attributes). And we could have some way to mark a package that anybody can update (maybe simply something like meta.maintainers = [ community ]).

meta.reviewers, probably — there are maintainers who can help with debugging an update, but a working update can be reviewed by whoever agrees (in contrast to some other areas where there are some people who are good at debugging such things, but wouldn't accept the responsibility of the final review).

ttuegel commented 8 years ago

:+1:

And I thing we must leave several core people with current rights as they have them, like @peti @domenkozar @vcunat @abbradar @aszlig @pSub @7c6f434c @nckx @wizeman @edwtjo @ttuegel @bjornfor and @jagajaga (myself) @shlevy @lethalman if they want to.

If I understand correctly, this is not about restricting the actual permissions of committers, but establishing policy and developing tools to notify the appropriate maintainers. At this point, I am comfortable trusting all the committers to follow an established policy; the only problem is that we don't as yet have one :smiley:

jagajaga commented 8 years ago

Oh! I misunderstood. I agree wholeheartedly with establishing policy!

abbradar commented 8 years ago

Yay! I returned again and again to a thought "how would I specify that I (or someone else) is responsible for this directory tree / feature / function?". This would scratch my itch.

7c6f434c commented 8 years ago

And I thing we must leave several core people with current rights as they have them, like @peti @domenkozar @vcunat @abbradar @aszlig @pSub @7c6f434c @nckx @wizeman @edwtjo @ttuegel @bjornfor and @jagajaga (myself) @shlevy @lethalman if they want to.

If I understand correctly, this is not about restricting the actual permissions of committers, but establishing policy and developing tools to notify the appropriate maintainers. At this point, I am comfortable trusting all the committers to follow an established policy; the only problem is that we don't as yet have one :smiley:

It could be nice to have brief recommendations for people with bigger NixPkgs experience about gotchas for updating various libraries.

For example, a few years ago GNU TLS updates loved breaking libsoup and WebKit-using browsers using libsoup… I guess there are some such things now and when updating (say) LibreOffice and needing a newer version of some dependency knowing what to check would make things go smoother.

lucabrunox commented 8 years ago

Different proposal

I think linux long flat file is nice, but maybe there's something better we can think nowadays, and different because of our nixpkgs layout. Also, consider that regexp are not much of any use in our tree because most of stuff is a single file in a directory.

I think this will also make us think about subtrees differently: more maintenance-wise than category-wise. Like, why do we have pkgs/development/compilers/ghc and pkgs/development/haskell-modules ? Maybe we should just forget about "compilers" category being useful to anybody, and put everything under pkgs/lang-support/haskell for example.

EDIT: slightly changed the header of the MAINT.md, but that's only a detail of this proposal.

maurer commented 8 years ago

@lethalman Won't the hierarchical MAINT.md method have issues with e.g. pkgs/top-level/haskell-packages.nix being delegated to the appropriate maintainers? Additionally, you end up with two MAINT.md files for a logical subtree in the case of the gnome3 subtree or similar ones which simultaneously have files in nixos/modules and in pkgs.

Re: the compilers category, it might be a useful designation for packages which are known to gate many other packages and whose build failure should be taken more seriously when cutting releases.

The overall oddity of the MAINT.md approach seems to be that it expects the directory structure of nixpkgs to match the maintenance structure. I suppose this is possible, but it's not immediately clear to me that this is ideal.

I agree that having the description of the subtree procedures sounds useful, as does not having everything in one monster file, just trying to note some potential complications to address beforehand.

P.S.: I'm mostly a user, not a bigshot maintainer, so these are just my two cents, sorry if I'm bothering you folks.

lucabrunox commented 8 years ago

@maurer I don't see any issue in creating more MAINT.md with the same maintainer. It's just your github username. About nixos and pkgs, it may or may not be a problem. You can have two different descriptions. At least for gnome, the two descriptions would be very different. Also it does not expect the tree to match the maintenance structure, it only induces to have a more maintenance-wise structure.

benley commented 8 years ago

There is a pretty decent semi-standard file format for exactly what we're trying to accomplish here: https://github.com/bkeepers/OWNERS

It's used in chromium's git repos: https://www.chromium.org/developers/owners-files https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/owners.py

Another related implementation, as a Gerrit plugin: https://gerrit.googlesource.com/plugins/owners

More related tooling: https://github.com/Nextdoor/git-change#owners-scope

Some commentary about origins and reasoning: https://github.com/bkeepers/OWNERS/issues/1#issuecomment-125322528

The net effect is comparable to the linux MAINTAINERS file, but this can be distributed throughout a codebase like @maurer's MAINT.md proposal.

dezgeg commented 8 years ago

I don't think per-subdirectory files would work well in this specific case which was about cross-cutting concerns, which kind of tend to touch files all across the directory tree. For instance I'd be thinking of something like this for myself:

ARM AND OTHER ARCHITECTURE SUPPORT
F: pkgs/misc/uboot
F: pkgs/top-level/platforms.nix
F: pkgs/stdenv/linux/make-bootstrap-tools-cross.nix
F: nixos/modules/installer/cd-dvd/sd-image*.nix
F: nixos/modules/system/boot/loader/generic-extlinux-compatible
F: nixos/modules/system/boot/loader/raspberrypi

...which is already all over the place and directories like pkgs/top-level contain files that other people maintain.

Also if we use the MAINTAINERS format from kernel, we can also steal their script (https://github.com/torvalds/linux/blob/master/scripts/get_maintainer.pl) which does other kinds of useful stuff already (like ignore files and using git blame to find additional reviewers like Mention Bot)

bendlas commented 8 years ago

Can somebody give perspective on OWNERS vs MAINTAINERS differences, with respect to encodable information vs tooling? I have seen both things but don't know the first thing about them. https://github.com/bkeepers/OWNERS/issues/1#issuecomment-125322528 seems to suggest that OWNERS is more geared towards machine-readability. Their usage seems to be about equal:


MAINTAINERS.md: 1,859 https://github.com/search?utf8=%E2%9C%93&q=filename%3AMAINTAINERS.md&type=Code&ref=searchresults

MAINTAINERS: 219,266 https://github.com/search?utf8=%E2%9C%93&q=filename%3AMAINTAINERS&type=Code&ref=searchresults


OWNERS.md: 323 https://github.com/search?utf8=%E2%9C%93&q=filename%3AOWNERS.md&type=Code&ref=searchresults

OWNERS: 224,132 https://github.com/search?utf8=%E2%9C%93&q=filename%3AOWNERS&type=Code&ref=searchresults

edolstra commented 8 years ago

@lethalman What @maurer said. Some topics don't correspond to subtrees, in fact some may not correspond to any specific files at all. (E.g., "Nixpkgs coding style", "New packages / modules", ...)

lucabrunox commented 8 years ago

@edolstra I wonder what means being maintainer of nixpkgs coding style.

edolstra commented 8 years ago

Regarding existing tooling, we'll probably have to write/adapt our own anyway because it will need to be able to handle meta.maintainers in addition to MAINTAINERS/OWNERS.

matthiasbeyer commented 8 years ago
HASKELL INFRASTRUCTURE
H: peti
F: pkgs/top-level/haskell-packages.nix
F: pkgs/development/compilers/ghc/*
F: pkgs/development/haskell-modules/lib.nix

I like the general idea, though I wouldn't bind that file to the github infrastructure so closely, but allow email entries like so:

HASKELL INFRASTRUCTURE
H: Github-User-Name
M: github_user_name@some_mail.ext
F: pkgs/top-level/haskell-packages.nix
F: pkgs/development/compilers/ghc/*
F: pkgs/development/haskell-modules/lib.nix

So we can also be reached by non-github users via email.

7c6f434c commented 8 years ago

I like the general idea, though I wouldn't bind that file to the github infrastructure so closely, but allow email entries like so:

HASKELL INFRASTRUCTURE
H: Github-User-Name
M: github_user_name@some_mail.ext
F: pkgs/top-level/haskell-packages.nix
F: pkgs/development/compilers/ghc/*
F: pkgs/development/haskell-modules/lib.nix

So we can also be reached by non-github users via email.

Add the name of maintainers.nix entry then. And display name.

domenkozar commented 8 years ago

Wouldn't it be better to add a list of paths maintained to lib/maintainers.nix or some other file so it can reference the maintainer name and email? Otherwise we have to maintain two list of maintainers.

jagajaga commented 8 years ago

@matthiasbeyer

M: github_user_name@some_mail.ext

Why github_user_name?

matthiasbeyer commented 8 years ago

@jagajaga This was just an example. Of course it would be @matthiasbeyer and mail at beyermatthias [dot] de for me.

joachifm commented 8 years ago

To my mind, the assignment of ownership is what makes this proposal valuable. That there is ownership is more important than who owns what or how it is recorded. I hope this doesn't get bogged down by trying to hash out all the details up front ... What @edolstra proposes seems perfectly workable as-is to me, though @domenkozar's idea of re-using maintainers.nix makes a lot of sense, esp. for the long term. Just my 2 NOK.

Profpatsch commented 8 years ago

Also make a GitHub bot that applies this tool to incoming PRs and pings/assigns the maintainers.

Will we keep the bot that looks up blame information and cc’s the responsible users then? I like having everyone interested in cc, but I’m afraid of creating too much noise in general. Maybe keep both active until too many people complain?

Another thing I loathe about the current system is that there is sometimes no connection between mail address, IRC user name and Github user name. That bugs me when I look up git information in my local repository and want to find out the handle of that person on Github. Maybe add that to the maintainer file?

7c6f434c commented 8 years ago

Another thing I loathe about the current system is that there is sometimes no connection between mail address, IRC user name and Github user name. That bugs me when I look up git information in my local repository and want to find out the handle of that person on Github. Maybe add that to the maintainer file?

You may want to run my vanity counter from maintainers/scripts and use its output (it does deduplication, so it tries to match names, emails and GitHub usernames even when a person varies the name spelling or changes the email while keeping the name spelling the same). The commit counts would be a useless side effect for you.

vcunat commented 8 years ago

Another thing I loathe about the current system is that there is sometimes no connection between mail address, IRC user name and Github user name.

lib/maintainers.nix explicitly states that the maintainer identifier should be equal to github username (I pushed that quite a long time ago). It even seems that new maintainers do follow it.

domenkozar commented 8 years ago

I'd like to push this forward and help with the implementation. We need to get maintainer-ship (a bit) under control.

@edolstra any objections writing the policy logic in Nix reusing maintainers.nix?

matthiasbeyer commented 8 years ago

I'd like to push my proposal up again: Lets include the email adress of the maintainer as well. Binding via the github user name binds us too closely to the github infrastructure and I really don't like that.

Benefits:

These points allow a reduction of the noise in our issue tracker. Keeping the mail address out does not even offer this possibility!

Drawbacks

vcunat commented 8 years ago

Binding via the github user name binds us too closely to the github infrastructure and I really don't like that.

I thought the suggestion was to use attribute names from lib/maintainers.nix. The fact that we suggest those to be equal to github identifiers is just a convenience for our current infrastructure.

NeQuissimus commented 8 years ago

It would only make sense to re-use maintainers.nix. And emails are already in there. Keep maintainers.nix as a flat list of "everybody" and then let's create some sort of structured "ownership" using the values defined in maintainers.nix.

Mathnerd314 commented 8 years ago

Does this sound reasonable?

I don't think this is reasonable.

  1. The goal of NixOS is to create a better packaging system (as claimed on the NixOS about page). Such a goal requires creative thinking and design, and a willingness to break from existing structures.
  2. Chaos is necessary; it is the source of creativity. Creative work doesn’t happen by plan and control.
  3. Some structure is needed too, but the existing structure of Git, Nix, Hydra, Mention-Bot, and Travis are sufficient, and can be extended if needed. Packaging software does not have much room in which to make mistakes (besides 'maintainer patches' such as the Debian OpenSSL debacle).
  4. Even if some commits are bad ideas from some perspective, it is only by looking at such commits that a real solution can be created. Software is developed upon such anti-patterns.
  5. Reverting commits or closing PRs unmerged does not progress the project; it only creates a situation of confusion and disorder. The commits were made for a reason, to fix a problem; removing them makes the problem worse, and adds a second problem of determining what will and will not be reverted, or of maintaining disparate forks.
  6. With the large collections of software that Nixpkgs deals with, chaos is guaranteed. Preventing chaos is a losing battle.

NixOS should embrace the lack of order, and be open to adding new things as they come up. With this it will keep attracting new contributors, and not lose old ones. A MAINTAINERS file will become an intractable ordeal, with all of the changes and mass-rewrites of packages. Instead, this issue should be closed, and the reverts reverted so that NixOS can be recognized as an open project.

domenkozar commented 8 years ago

@Mathnerd314 you're wrong in one assumption: implicit rules are better than explicit. Numerous studies has shown, there needs to be structure and order for people to cooperate in groups. Please read http://www.shirky.com/writings/herecomeseverybody/group_enemy.html and then we can discuss this further if needed.

That being said, rules need to be carefully chosen.

Your points have two paradoxes:

So in order to be constructive and continue this debate, please provide a feasible alternative to reverts. And no, we won't let anything land in nixpkgs.

nbp commented 8 years ago

@Mathnerd314 . The goal of having such maintainer file is exactly to prevent the reverts we all experienced. If nobody is responsible by owning parts of the code, the nobody is accountable for fixing it. Which means that things could remain broken for ages, and would never be fixed by anybody.

This is for this reasons that packages have a maintainer fields, such that we can know who to contact if there is an issue. This file is supposed to serve the same purpose, which means that we would know who to contact if there is an issue.

Now, when you are responsible of some code, you have to make sure it behaves correctly. This means that if somebody comes behind you and does some changes, you might not understand it, or you might understand it but feel that it does not go in the right direction. These are some of the reasons why people are reverting code.

This maintainer file is here to make sure we know who to ask, and that we agree and discuss these issues ahead, and not after the fact.

[…] so that NixOS can be recognized as an open project.

That's funny that you phrase it that way, because the https://www.openproject.org/ domain is the home of an open source management project.

Mathnerd314 commented 8 years ago

Please read http://www.shirky.com/writings/herecomeseverybody/group_enemy.html and then we can discuss this further if needed.

I read it, seems mostly about dealing with abuse, which Github already has tools for.

Numerous studies has shown, there needs to be structure and order for people to cooperate in groups.

Like I said, structure is needed, but it's not clear to me why the current structure is insufficient.

implicit rules are better than explicit

I'm not proposing implicit rules, but rather an explicit statement of the lack of rules. NixOS is full of undocumented small features; for example just the other day I came across makeAutostartItem. Such things can only happen when people are free to make changes without fear of review.

2) says "Chaos is necessary", but then in 5) you say "reverts create a situation of confusion and disorder." in 5) you suggest reverting is bad

Well, I am in favor of the ability to revert, precisely because it can create more chaos. But I also think reverts are learning experiences, "canaries in the coal mine"; a healthy project should not need many reverts, because coordination moves everything in the same direction. Most of the reverts now are reverts to older versions; they should instead be updates to unreleased versions where the problems are fixed. And for the auto-update systems, I think the best approach is to get 10 or 20 auto-update systems into nixpkgs, and then consolidate around the best implementation.

provide a feasible alternative to reverts

I have no real alternatives, my proposed solution is to add more reverts (in particular, reverts of reverts). So far @shlevy holds the record with a rank-4 revert (05a02c639e27ddfc8b3f9ef440a14fdccf821fc2), maybe we will see an epic rank-10 revert soon.

And no, we won't let anything land in nixpkgs.

Yes, you (https://github.com/NixOS/nixpkgs/commit/71e67797d6324afd21b6b5fb3249a3a8a64ce181) and @edolstra (https://github.com/NixOS/nixpkgs/commit/9d82f7e53e66e5594b0c8b82f6c415a0a386b580) have shown that you are unwilling to let certain types of code into NixOS. But does that mean that such code cannot enter via other means? There have been rumors of a NixOS-contrib that have failed to materialize. Instead there is only triton.

If nobody is responsible by owning parts of the code, then nobody is accountable for fixing it. Which means that things could remain broken for ages, and would never be fixed by anybody.

This isn't a problem, all open-source projects have huge open bug databases. And the unstable channel already goes weeks between updates, it would be difficult to do worse. People will fix the stuff that is used and the stuff that isn't can silently bitrot because of Nix's laziness (although Hydra needs some logic for automatically disabling builds).

This is for this reasons that packages have a maintainer fields, such that we can know who to contact if there is an issue.

I thought meta.maintainers is just for Hydra integration, so Hydra knows which people to email when a build breaks. Is it supposed to magically acquire a new and different meaning as a result of this issue?

This means that if somebody comes behind you and does some changes, you might not understand it, or you might understand it but feel that it does not go in the right direction.

Sure, but what is the right direction? I would rather have a flexible configuration system so that we can agree to disagree, but apparently @edolstra and @domenkozar think this pollutes the repository (as if it was clean in the first place).

That's funny that you phrase it that way, because the https://www.openproject.org/ domain is the home of an open source management project.

I was thinking of http://producingoss.com/ and http://www.catb.org/~esr/writings/cathedral-bazaar/cathedral-bazaar/ and its follow-on https://slashdot.org/story/98/10/13/1423253/featurecathedrals-bazaars-and-the-town-council, maybe those too should be required reading for this discussion

domenkozar commented 8 years ago

Another good consequence of this is that we can configure @mention-bot more granularly.

 "alwaysNotifyForPaths": [
    {
      "name": "ghuser", // The user's Github username
      "files": ["src/js/**/*.js"] // The array of file globs associated with the user
    }
  ], // users will always be mentioned based on file glob
vcunat commented 8 years ago

Links to closely related stuff: https://github.com/kragniz/nixbot and its announcement.

kamilchm commented 8 years ago

found something related on hacker news https://lgtm.co/

and a comparison of available tools by Zappr https://zappr.readthedocs.io/en/latest/competitors/

domenkozar commented 8 years ago

I'm reassigning this to next NixOS release, I'll do my best to find some time to advance the effort.

FRidh commented 7 years ago

This has been dormant for a while. Looks like we seem to agree to the need of the file or structure that is proposed. It's just the details.

I'm fine with any of the solutions proposed here. The MAINTAINERS file is very clear, but I can imagine paths in it could get outdated. Nested OWNERS files likely stay more up to date because they're there and you simply move them around (or remove them when they become obsolete). But then again, as soon as you dive into the subfolders you get package expressions which typically have lib.maintainers in them.

In any case, I'm of the opinion we should make a decision now because there's a strong need for this. We can always change it again if the need arises. Therefore, I say we go ahead with the MAINTAINERS file.

FRidh commented 7 years ago

@grahamc just mentioned a new feature, the CODEOWNERS file. https://github.com/blog/2392-introducing-code-owners

Profpatsch commented 7 years ago

the CODEOWNERS file.

Since mention-bot has been dead for a while, maybe we should migrate directly?

matthewbauer commented 7 years ago

:+1: Probably someone could write a script to generate CODEOWNERS data from our "maintainers" attribute.

FRidh commented 7 years ago

We could have two parts, the first generated, and then some identifier for the update script after which we could add manual overrides/extra's.

aszlig commented 7 years ago

Hm, what about running our own mention bot instead (we could even patch it to incorporate maintainers meta info), because CODEOWNERS looks a bit redundant to me.

edolstra commented 7 years ago

Yeah, a patched mentionbot that uses meta.maintainers would be ideal.

vcunat commented 7 years ago

Nitpick: then it would be a rather important requirement to enforce matching maintainer IDs with github account names or add some extra mapping for that. (For instance, Eelco's names don't match :)

7c6f434c commented 7 years ago

Maybe add optional fields like Nix-related GitHub username, Nix-related GitLab user name, etc.?

(Yes, my maintainers.nix name is my Nix SVN username back from when it made sense, and my GitHub username is the same user-part as the email I use in connection to Nix stuff. These two are different)

domenkozar commented 7 years ago

Very relevant thoughts: http://blog.ffwll.ch/2017/08/github-why-cant-host-the-kernel.html

dezgeg commented 7 years ago

So what's the next actionable thing to do given that the CODEOWNERS file was merged in https://github.com/NixOS/nixpkgs/pull/27499?