Page MenuHomePhabricator

[docs] Adding target status definition to dev policy
AbandonedPublic

Authored by rengolin on Aug 4 2016, 6:36 AM.

Details

Reviewers
rengolin
lattner
Summary

Making explicit our current policy to accept new targets as experimental, official and maintained.

Diff Detail

Event Timeline

rengolin updated this revision to Diff 66798.Aug 4 2016, 6:36 AM
rengolin retitled this revision from to [docs] Adding target status definition to dev policy.
rengolin updated this object.
rengolin added a reviewer: lattner.
rengolin set the repository for this revision to rL LLVM.
rengolin added subscribers: chandlerc, mehdi_amini, arsenm and 5 others.

Fine with me.

(And thanks Renato for taking the time to lead and write this)

Hi Renato,

Thank you for working on this!

My comments (purely subjective ones, really, not hard objections at all) are below.

Yours,
Andrey

docs/DeveloperPolicy.rst
565

Should we stress *always*? In the discussion you present an example of a target (BPF) that was added as an official right away (http://lists.llvm.org/pipermail/llvm-dev/2016-July/102505.html). Maybe it's better to say *practically always* or something like this?

572–573

I don't understand this (most likely my bad) -- how keeping a target downstream helps with openness and quality concerns?

625–628

I'm not entirely happy with the wording, which makes target maintenance look like a daily walk through a minefield, with each single mistake evaluated and penalties taken. I guess your idea is that adhering to the rules and policies shouldn't be forgotten once a target becomes experimental / official. IMHO, just saying that "continuous violations of aforementioned rules and policies could lead to complete removal of the target from the code base" would be sufficient.

rengolin added inline comments.Aug 5 2016, 8:34 AM
docs/DeveloperPolicy.rst
565

The BPF back-end started experimental, then moved to non-experimental.

http://lists.llvm.org/pipermail/llvm-dev/2015-June/086492.html

So, I think making it a requirement is a strong suggestion that we don't want to fast-track targets, since they won't have proven that they can "cope with upstream development pace for the required time".

572–573

No, that's a horrible paragraph, but I didn't know how to say it better. :)

My point here is that, in the same way the target's community (a company, maybe) may want to keep the targets downstream (secrecy, product roadmap, etc), the LLVM community may also refuse to accept a target upstream just because the company wants to. Ie. this is a two-way street.

For example, when the Lanai back-end was proposed, there were concerns that the lack of documents, emulators and targets available would make it unusable for everyone *but* the Lanai community, so a cost to everyone else for the sake of saving time to the Lanai community.

But later they have shown interest in publishing ISA docs and will make an emulator open source, so that has shown interest, usefulness and also reduces the tension in the LLVM community.

I'd love if someone could convey that idea into a concise and meaningful paragraph. :)

625–628

I agree, it is a bit harsh. Wasn't meant to be. :)

I'll re-word.

rengolin updated this revision to Diff 66963.Aug 5 2016, 8:40 AM
rengolin removed rL LLVM as the repository for this revision.

Changes:

  • Removed weird paragraph. The intention was good, but the wording was horrible. We can add it later, since the policy itself already covers most of what I wanted to say anyway.
  • Re-worded the continuation clause to make it sound less harsh. We're really lenient to bit-rotting targets... (see CppBackend :)
lattner requested changes to this revision.Aug 15 2016, 9:49 PM
lattner edited edge metadata.

Overall, looks great. I added some specific detailed comments above. Once you addressed those, I think this is good to merge, thank you for driving this forward Renato!

docs/DeveloperPolicy.rst
572

I think that the very first bullet point is that the target must have a code owner. The active community is the second bullet. This allows you to divide the responsibilities a bit: the code owner makes sure that changes to the target get reviewed and steers the overall effort, but the community ensures that bots and other larger issues are handled.

576

Nit-pick: Please use American spelling: "behavior".

584

Link to the IR compatibility section of the policy?

586

License compatibility is not enough, it must be the same license. Please say something like:

"The code conforms to all of the policies laid out in this developer policy document, including license, patent, and coding standards."

598

If we're going to pick an arbitrary timeframe, it might as well be specific. Just say "3 months" instead of "2-3 months".

611

please expand "ex." to "for example". This is more comprehensible to non-english-as-first-language readers.

This revision now requires changes to proceed.Aug 15 2016, 9:49 PM
rengolin accepted this revision.Aug 17 2016, 1:47 PM
rengolin added a reviewer: rengolin.

Thanks Chris!

Addressed your concerns and committed on r278971.

Thanks Renato!

rengolin abandoned this revision.Oct 3 2016, 3:09 AM