Page MenuHomePhabricator

[docs] Adding a Support Policy
ClosedPublic

Authored by rengolin on Nov 4 2020, 5:43 AM.

Details

Summary

As discussed in the mailing list [1-4], we need a separation of support
tiers when requiring support from the whole community versus a
sub-community. Essentially, if a sub-community is active enough and
takes maintenance into their own internal costs without affecting other
parts of the community's maintenance costs, then code that is not
immediately relevant to all parts (ie. not released, actively tested,
etc) can still find its way into the LLVM main repository without major
pain points.

The main benefit is to reduce the maintenance cost that those
sub-communities have outside of LLVM (for example, in duplicating common
code, applying the same patches on top of multiple user repositories or
downstream projects).

This document outlines the components and responsibilities of the
sub-communities with regards to maintenance costs and how they affect
the rest of the community.

It also adds an addendum on removal policies, which expand the existing
"new target removal" policy into something more generic, to encompass
any piece of code, scripts or documents in the repository.

[1] http://lists.llvm.org/pipermail/llvm-dev/2020-October/146249.html
[2] http://lists.llvm.org/pipermail/llvm-dev/2020-November/146335.html
[3] http://lists.llvm.org/pipermail/llvm-dev/2020-October/146138.html
[4] http://lists.llvm.org/pipermail/llvm-dev/2020-November/146298.html

Diff Detail

Event Timeline

rengolin created this revision.Nov 4 2020, 5:43 AM
rengolin requested review of this revision.Nov 4 2020, 5:43 AM
rengolin updated this revision to Diff 302825.Nov 4 2020, 5:48 AM
mehdi_amini added inline comments.Nov 4 2020, 8:32 AM
llvm/docs/SupportPolicy.rst
16

I wouldn't use the word "projects" here, that is a bit confusing to me (I think of projects as "clang", "lld", "mlir", ...)

Maybe: There are, however, other components (scripts/utilities for example) within the main repository that either ...

rengolin added inline comments.Nov 4 2020, 8:42 AM
llvm/docs/SupportPolicy.rst
16

Ah, yes! I use components below. Good catch.

rengolin updated this revision to Diff 302857.Nov 4 2020, 8:43 AM
rengolin updated this revision to Diff 302866.Nov 4 2020, 9:10 AM
  • Adding page link to "Getting Involved" page.
  • Fixing some formatting errors (Sphinx reports no warnings on my box).

I've suggested some changes, but this seems reasonable to me.

llvm/docs/SupportPolicy.rst
29

I think "peripheral" is fine. Informally, we want a "main things tier" and "side things tier", and I think "peripheral" is a suitably dignified word for "side things".

67

Wording here is a bit weird.

109

I'm not quite sure what the best way to express this is, but I feel like things in the peripheral tier must have at least one active subcommunity upstream or at least two disjoint active subcommunities downstream.

I.E. "somebody uses it in upstream" or "two separate downstreams use it"

111

The way I read this, I as a user of a peripheral tier component can open a bug against said component on bugs.llvm.org, and that this bug is valid and is expected to be addressed.

Is this correct? If so, maybe this should be explicitly codified here.

172

Overall LGTM, with some wording nits. One thing I think is missing is guidance for how a peripheral component gets added. Experimental targets already have their own documentation, and my impression is that editor bindings are just a matter of sending a patch (which seems reasonable to me). What about something like:

To add a new peripheral component not covered under an existing policy, send an RFC to the appropriate dev list proposing its addition and explaining how it will meet the support requirements listed here.

llvm/docs/SupportPolicy.rst
20

Nit: "nor are they"

20
29

+1 peripheral SGTM

67

Wording here is a bit weird.

74–75
108–109

Nit: s/remaining/residing/ since this could be used to consider something moving *into* the main repository as well, where "remaining" doesn't make much sense.

Also I don't really understand how one would count number of subcommunities per my comment above.

109

How are we measuring *number* of communities? Wouldn't we always have one sub-community that is "the sub-community that cares about thing X"?

114–117

"Negatively affect" seems broader than "break or invalidate", so I think it's important to have that included for core-tier as well. Added as a separate bullet point, since an immediate revert isn't necessarily the best course of action for such an issue. I'm thinking of the discussion we had about putting something under llvm/utils vs at the top-level utils/ and how that affects commit emails.

127–128

I think this note about the level of support required scaling with system complexity would be a nice thing to hold over from the original 3-tier policy proposal

130

"incursion" sounds like they're invading :-D

148–149

Wording is a bit weird here. I'm not quite sure what you're trying to say.

ctetreau added inline comments.Nov 5 2020, 9:33 AM
llvm/docs/SupportPolicy.rst
109

How are we measuring *number* of communities? Wouldn't we always have one sub-community that is "the sub-community that cares about thing X"?

This would be largely honor-system. In the RFC, part of the justification would be spelling out who the multiple subcommunities are. With the bazel example, the original RFC states "we use it internally at google, but tensorflow also has a parallel version of this". In this case "llvm devs at google" and "llvm devs for the tensorflow project" are the 2 subcommunities. I would think that a representative from each subcommunity should respond to the RFC stating that they agree to be maintainers (or at least that they care about it). For the bazel example, Google has clearly stated interest. A Tensorflow maintainer should respond to the RFC stating that they do in fact care (I don't know if this has happened).

The idea here being: "If only one org is using this thing in their downstream, and nobody else wants it, why would the community accept it?"

I guess perhaps the "one subcommunity upstream" requirement is kind of pointless. If it's not being used by a core-tier component, then by definition it's only used by downstreams or out of tree users of llvm. And if it were being used by a core-tier component, then I would think it should also be core-tier.

rengolin updated this revision to Diff 303148.Nov 5 2020, 9:51 AM

Updating simple suggestions / fixups.

rengolin marked 12 inline comments as done.Nov 5 2020, 9:52 AM
rengolin added inline comments.
llvm/docs/SupportPolicy.rst
130

:D

rengolin marked an inline comment as done.Nov 5 2020, 10:06 AM
rengolin added inline comments.
llvm/docs/SupportPolicy.rst
109

I think I consider "sub-community" as a set, so the union of two sub-communities is still a "sub-community". I also don't see how we could "count" them, as there is a lot of overlap.

Worst case we'd be forcing people to create bogus splits to "increase the number of sub-communities" to pass a threshold.

I'm simplifying the second bullet to avoid confusion. In the end, the points I wanted to make are already there on the second (must not) bullet section.

111

Not just a user for component X, but developers on core tier stuff as well as developers of component Y.

It is the responsibility of the sub-communities to maintain their code so that their quality is at the very least "invisible" to other users, but optimally it should behave well not "negatively affect" others.

148–149

I'm trying to say that we're not all looking hard to destroy other people's work, and this document is mostly to allow us to do so when it's affecting the rest of the community. I have removed the weird paragraph and have simplified the last one. I hope that's clearer.

rengolin updated this revision to Diff 303157.Nov 5 2020, 10:07 AM

Rewriting some confusing paragraphs.

GMNGeoffrey added inline comments.Nov 5 2020, 2:01 PM
llvm/docs/SupportPolicy.rst
109

I think I consider "sub-community" as a set, so the union of two sub-communities is still a "sub-community". I also don't see how we could "count" them, as there is a lot of overlap.

Yes this was my reasoning as well

148–149

I think you kept the paragraph I find confusing. Maybe my issue is with the phrasing "impose degradation" which sounds odd to me. It seems like mostly this is covered by the "must nots" above, but maybe you think it still is useful to include in this section as well? I took a swing at rewording

For code to remain in the repository, its presence must not impose an undue burden on maintaining other components (core or peripheral).

rengolin marked 2 inline comments as done.Nov 6 2020, 2:41 AM

Overall LGTM, with some wording nits. One thing I think is missing is guidance for how a peripheral component gets added. Experimental targets already have their own documentation, and my impression is that editor bindings are just a matter of sending a patch (which seems reasonable to me). What about something like:

To add a new peripheral component not covered under an existing policy, send an RFC to the appropriate dev list proposing its addition and explaining how it will meet the support requirements listed here.

Sorry I missed this. I have now added a whole new "Inclusion Policy" section, which starts with your phrase, but adds a bit more of the mechanics to be clear what to expect.

llvm/docs/SupportPolicy.rst
109

The idea here being: "If only one org is using this thing in their downstream, and nobody else wants it, why would the community accept it?"

I think the main idea of the policy is to say that it's ok to have side-projects in the main repo if they follow the guidelines and don't break other people's stuff. Basically "be excellent to each other".

If only one downstream user is using it and that user always fixes stuff whenever it breaks and keeps it up-to-date, then this follows the spirit of the policy in two ways:

  1. It is being "excellent" to all others.
  2. It allows people that haven't used that component to experiment with it without having to pay the cost of downstream maintenance (and learning), which is often prohibitive.

Point 2 above encourages sub-community growth, and if/when the sub-community is large enough, we may even make it core.

Case 1: Autoconf vs. CMake.

When I started working on LLVM, the default was autoconf and CMake support was patchy. I had rarely used CMake until then, but when I did I wonder why the heck were we still using autoconf. After enough people gathered behind CMake, we moved official support to CMake (after fixing all its bugs).

This would be a typical tier-inversion, with autoconf moving down to peripheral tier, while CMake went up to core.

Later on, when I was building Arm buildbots, I realised we were still supporting autoconf. Back then, most Arm developers were still using autoconf to build because CMake support on Arm was still patchy. So I fixed what I could and took the conscious decision to not create any autoconf Arm bot ever. Other bot maintainers have moved their existing autoconf bots to CMake and in due time, there were none.

That conscious decision by the community is what drove a lot of people to fix most CMake bugs and improve it considerably. This is when we decided to drop support for autoconf.

Case 2: GN

I had no idea it existed until George proposed to include Bazel. I still have no idea what it does or why it exists, but I still literally don't care. I don't know if the sub-community is one person or half of the developers, and that doesn't matter because I have never been affected by it.

It can continue to exist for as long as it does and I probably won't even notice. This means to me that either those files are so stable that no one ever changes and it never breaks (similar to most editor configuration) or the sub-community is extremely responsive. Either way, they're already following the policy we're trying to create.

rengolin updated this revision to Diff 303385.Nov 6 2020, 2:41 AM
  • Adding inclusion policy section
  • Reword phrase in deprecation policy

Sorry, s/George/Geoffrey/. :S

GMNGeoffrey accepted this revision.Nov 6 2020, 11:52 AM

LGTM, but please wait for others' review, obviously :-)

llvm/docs/SupportPolicy.rst
145

I think it's worth saying that simple things like editor configs don't need to go through this process. I believe they haven't in the past and it seems like a lot of overhead to add.

156
This revision is now accepted and ready to land.Nov 6 2020, 11:52 AM
ctetreau added inline comments.Nov 6 2020, 4:00 PM
llvm/docs/SupportPolicy.rst
109

I guess I have a personal aversion to an excess of cruft.

I suppose the proposal for inclusion will justify the "who cares", and as long as "someone does", it meets the criteria for inclusion.

mehdi_amini accepted this revision.Nov 6 2020, 4:29 PM

LGTM from me on the principle, I didn't get into detailed wordsmith. Overall I would have been more concise and lay down the general principle rather than trying to prescribe this level of details (like the "Reinstatement" section for example), I believe that's also more in line with how we've been operating / documenting things in the past, but I don't really mind it either.
I suspect we can incrementally fix/improve anything as we go as well.

rengolin marked 9 inline comments as done.Nov 7 2020, 2:49 AM
rengolin added inline comments.
llvm/docs/SupportPolicy.rst
109

I guess I have a personal aversion to an excess of cruft.

I suppose the proposal for inclusion will justify the "who cares", and as long as "someone does", it meets the criteria for inclusion.

That's what I hope, yes.

rengolin updated this revision to Diff 303628.Nov 7 2020, 2:50 AM
rengolin marked an inline comment as done.
  • Include a short phrase on inclusion policy, as requested.
  • Fixed some typos

LGTM from me on the principle, I didn't get into detailed wordsmith. Overall I would have been more concise and lay down the general principle rather than trying to prescribe this level of details (like the "Reinstatement" section for example), I believe that's also more in line with how we've been operating / documenting things in the past, but I don't really mind it either.
I suspect we can incrementally fix/improve anything as we go as well.

I hoped for something leaner, too. But this policy received some opposing views and multiple requests for clarification, which led to some bloat on the text.

I tried to only write things that I know we have been doing all along, avoiding creating new rules for anything, so hopefully, it will still convey the same meaning.

I'd also be happy if someone could simplify it later. I just wanted to get something out there so that Geoffrey can continue his proposal.

I'll wait for more people to give their approvals, to make sure the document reflect the community.

@lattner @echristo @reames what do you guys think?

rengolin updated this revision to Diff 303632.Nov 7 2020, 3:25 AM
lattner accepted this revision.Nov 7 2020, 10:04 AM

This look really really great Renato. After this lands, please also prepare a patch (~paragraph length) for the main DeveloperPolicy doc that gives the high level idea of what is going on, and links to this doc for more information.

Thank you so much for driving this -- this is a huge step forward for LLVM!

-Chris

Thanks Chris! Will do.

rengolin closed this revision.Nov 7 2020, 1:17 PM

Sorry, forgot the review text on the commit message:
Closed by 25ba6b2bcd1

This looks fine to me for now. I really think it's too wordy and too elaborate and could be about 1/10th as long.

-eric