[PATCH] D90761: [docs] Adding a Support Policy

Wed Nov 4 12:36:18 PST 2020

GMNGeoffrey added a comment.

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.

================
Comment at: llvm/docs/SupportPolicy.rst:19
+or external projects. Those parts of the main repository don't always have
+rigorous testing like the core parts, nor they are validated and shipped with
+our public upstream releases.
----------------
Nit: "nor are they"

================
Comment at: llvm/docs/SupportPolicy.rst:19
+or external projects. Those parts of the main repository don't always have
+rigorous testing like the core parts, nor they are validated and shipped with
+our public upstream releases.
----------------
GMNGeoffrey wrote:
> Nit: "nor are they"

================
Comment at: llvm/docs/SupportPolicy.rst:28
+But the maintenance costs of such diverse ecosystem is non trivial, so we divide
+the level of support in two tiers: core and peripheral (satellite?), with two
+different levels of impact and responsibilities. Those tiers refer only to the
----------------
ctetreau wrote:
> 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".
+1 peripheral SGTM

================
Comment at: llvm/docs/SupportPolicy.rst:66
+   must be reverted, as per review policy.
+ * Bit-rot will downgrade areas to the lower tier or be removed, depending on
+   new or existing efforts by a sub-community to pick up support in a timely
----------------
ctetreau wrote:
> Wording here is a bit weird.
> Wording here is a bit weird.

================
Comment at: llvm/docs/SupportPolicy.rst:73-74
+
+Peripheral tier code encompass the parts of LLVM that cater to a specific
+sub-community and which doesn't usually affect the core components directly.
+
----------------

================
Comment at: llvm/docs/SupportPolicy.rst:107-108
+Code in this tier must:
+ * Have a clear benefit for remaining in the main repository, catering to at
+   least one active sub-community (upstream or downstream).
+ * Be actively maintained by such sub-community and have its problems addressed
----------------
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.

================
Comment at: llvm/docs/SupportPolicy.rst:108
+ * Have a clear benefit for remaining in the main repository, catering to at
+   least one active sub-community (upstream or downstream).
+ * Be actively maintained by such sub-community and have its problems addressed
----------------
ctetreau wrote:
> 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"
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"?

================
Comment at: llvm/docs/SupportPolicy.rst:113-116
+ * Break or invalidate core tier code or infrastructure. If that happens
+   accidentally, reverting functionality and working on the issues offline
+   is the only acceptable course of action.
+ * Negatively affect other peripheral tier code, with the sub-communities
----------------
"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.

================
Comment at: llvm/docs/SupportPolicy.rst:126-127
+Code in this tier should:
+ * Have infrastructure to test, whenever meaningful, with either no warnings or
+   notification contained within the sub-community.
+ * Have a document making clear the status of implementation, level of support
----------------
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

================
Comment at: llvm/docs/SupportPolicy.rst:129
+ * Have a document making clear the status of implementation, level of support
+   available, who the sub-community is and, if applicable, roadmap for incursion
+   into the core tier.
----------------
"incursion" sounds like they're invading :-D

================
Comment at: llvm/docs/SupportPolicy.rst:147-148
+
+But for code to remain in the tiers above, they must not impose degradation in
+their own support but also in other parts of the code.
+
----------------
Wording is a bit weird here. I'm not quite sure what you're trying to say.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90761/new/

https://reviews.llvm.org/D90761