Page MenuHomePhabricator

[Docs] Add Contributing page.
ClosedPublic

Authored by fhahn on Jan 2 2018, 6:11 AM.

Details

Summary

This new page acts as an entry point for (new) contributors to LLVM and
provides information about

  • What to contribute
  • How to submit a patch
  • Where to start to learn more about LLVM's architecture and internals.

This version of the page duplicates some information from the
DeveloperPolicy and Phabricator pages. Subsequent changes should work
towards moving information for new developers to this page, where it
makes sense.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jan 2 2018, 6:11 AM

Add rendered version of the page (CSS & co missing)

fhahn added a reviewer: asb.Jan 2 2018, 6:20 AM
JonasToth added inline comments.
docs/Contributing.rst
69 ↗(On Diff #128414)

at then looks incorrect to me. Only the?

asb added a comment.Jan 2 2018, 6:40 AM

This is a great initiative Florian - many thanks for kicking this off.

I wonder if it would be helpful to try to change include changes to docs/DeveloperPolicy.rst and docs/Phabricator.rst in this patch. I can see the argument for trying to keep it self-contained, but you could also see this patch as being a partial "refactoring" of the current guidance for contributors, which is split across multiple files. e.g. here and guidance on picking reviewers here.

I think the way the document is currently structured works and has some advantages, but it might be worth considering an alternative. The way I see it, there are two likely readers of this documentation:

  1. People new to LLVM who are interested in getting involved in both compiler development and upstream LLVM
  2. People with some experience hacking LLVM or other compilers but are looking for information on how to work with the upstream LLVM community (or do you disagree, and think this case is adequately covered by DeveloperPolicy and the Phabricator code review docs?)

With that in mind, perhaps it makes sense to split the document in to one section covering how to find something to work on and a second section which details the practicalities of preparing your changes for submission? I don't feel particularly strongly about this, but figured I'd share the thought.

fhahn added a comment.Jan 2 2018, 7:16 AM
In D41665#965809, @asb wrote:

This is a great initiative Florian - many thanks for kicking this off.

I wonder if it would be helpful to try to change include changes to docs/DeveloperPolicy.rst and docs/Phabricator.rst in this patch. I can see the argument for trying to keep it self-contained, but you could also see this patch as being a partial "refactoring" of the current guidance for contributors, which is split across multiple files. e.g. here and guidance on picking reviewers here.

I am all in favor of partially refactoring the docs on that topic. But I am afraid I am not sure I understand entirely what you suggest here. Do you mean moving important info from the developer policy to docs/Contributing.rst? Or the other way around?

IMO, I think there is value in a self contained contributing document. For example, I think Rust's Contributing info is quite good: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md
Although putting too much in a single document would be counterproductive as well.

I think the way the document is currently structured works and has some advantages, but it might be worth considering an alternative. The way I see it, there are two likely readers of this documentation:

  1. People new to LLVM who are interested in getting involved in both compiler development and upstream LLVM
  2. People with some experience hacking LLVM or other compilers but are looking for information on how to work with the upstream LLVM community (or do you disagree, and think this case is adequately covered by DeveloperPolicy and the Phabricator code review docs?)

    With that in mind, perhaps it makes sense to split the document in to one section covering how to find something to work on and a second section which details the practicalities of preparing your changes for submission? I don't feel particularly strongly about this, but figured I'd share the thought.

Thanks Alex. Yes I think it makes sense to split that up!

I think it's really cool that you've started on this and I think this is a great start. Thanks.

However, what I commonly see coming through the dev list in terms of questions from beginners are questions about how to familiarize oneself with the LLVM code base and infrastructure. I think a beginners page should include all this info you added here about what to work on and how to contribute, but also where to learn. I think this information is kind of strewn all over the place and a nice summary would be useful. I think "User Guides", "Programming Documentation" and "Subsystem Documentation" sections contain a wealth of useful information, but I think it can be overwhelming to a new contributor.
What do I need to read to fix this beginner bug? Which of these are generally useful and which are useful to some specific subset of developers? etc...
Perhaps this page can be a general entry point for new contributors. It can include the information you included here and then it can include a section on "Familiarizing Yourself With LLVM" where we would initially link documents that are useful for new Front-End/Back-End/Middle-End developers. Then I think new contributors should be encouraged to contribute to this page and improve it with their experience.

I wonder if we can start with something kind of small and limited that will grow into a useful, comprehensive, easy-to-navigate entry point for new contributors.

docs/Contributing.rst
7 ↗(On Diff #128414)

This sentence seems a bit weird. It sounds like "we appreciate the different ways of contributing" rather than "we appreciate all contributions".

asb added a comment.Jan 2 2018, 7:29 AM
In D41665#965809, @asb wrote:

This is a great initiative Florian - many thanks for kicking this off.

I wonder if it would be helpful to try to change include changes to docs/DeveloperPolicy.rst and docs/Phabricator.rst in this patch. I can see the argument for trying to keep it self-contained, but you could also see this patch as being a partial "refactoring" of the current guidance for contributors, which is split across multiple files. e.g. here and guidance on picking reviewers here.

I am all in favor of partially refactoring the docs on that topic. But I am afraid I am not sure I understand entirely what you suggest here. Do you mean moving important info from the developer policy to docs/Contributing.rst? Or the other way around?

IMO, I think there is value in a self contained contributing document. For example, I think Rust's Contributing info is quite good: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md
Although putting too much in a single document would be counterproductive as well.

I meant the first. Right now we have info about contributing spread across several places. This patch consolidates that information (great!) but ends up adding a little more duplication because it doesn't replace the existing info with references to docs/Contributing.rst. It could make sense to get docs/Contributing.rst committed then consider whether other docs can be updated to reference it, or instead to try to incorporate those changes in this patch. I don't feel particularly strongly either way.

fhahn added a comment.Jan 2 2018, 7:30 AM

I think it's really cool that you've started on this and I think this is a great start. Thanks.

However, what I commonly see coming through the dev list in terms of questions from beginners are questions about how to familiarize oneself with the LLVM code base and infrastructure. I think a beginners page should include all this info you added here about what to work on and how to contribute, but also where to learn. I think this information is kind of strewn all over the place and a nice summary would be useful. I think "User Guides", "Programming Documentation" and "Subsystem Documentation" sections contain a wealth of useful information, but I think it can be overwhelming to a new contributor.
What do I need to read to fix this beginner bug? Which of these are generally useful and which are useful to some specific subset of developers? etc...
Perhaps this page can be a general entry point for new contributors. It can include the information you included here and then it can include a section on "Familiarizing Yourself With LLVM" where we would initially link documents that are useful for new Front-End/Back-End/Middle-End developers. Then I think new contributors should be encouraged to contribute to this page and improve it with their experience.

Thanks! The "Familiarizing Yourself With LLVM" is an excellent suggestion and should be part of this document.

I wonder if we can start with something kind of small and limited that will grow into a useful, comprehensive, easy-to-navigate entry point for new contributors.

That was my hope. I think a sensible way to go about this would be to add a first version of the document and go from there. And hopefully it will get better with feedback from new contributors.

fhahn added a comment.Jan 2 2018, 1:41 PM
In D41665#965834, @asb wrote:

I meant the first. Right now we have info about contributing spread across several places. This patch consolidates that information (great!) but ends up adding a little more duplication because it doesn't replace the existing info with references to docs/Contributing.rst. It could make sense to get docs/Contributing.rst committed then consider whether other docs can be updated to reference it, or instead to try to incorporate those changes in this patch. I don't feel particularly strongly either way.

Ok great! I agree that in the end, we should get rid of the duplication and I am slightly worried about the additional duplication added by this patch. If people are happy with consolidating some documentation under the Contributing page, I am happy to link to the Contributing page DeveloperPolicy & co.

I hope this patch can get things started, we can agree on what the end goal should be and start to gradually move some documentation over to the Contributing page, if it makes sense. I expect this to be an incremental process, as we hopefully get some helpful feedback from new contributors. Also, I am not confident enough in my writing skills to single-handedly take on a substantial rework of the docs in a single big patch ;)

sdardis added a subscriber: sdardis.Jan 2 2018, 2:24 PM

Thanks for doing this, @fhahn.

A few small comments.

docs/Contributing.rst
29 ↗(On Diff #128414)

"assign it to you" -> "assign it to yourself"

39 ↗(On Diff #128414)

Colon after should.

55 ↗(On Diff #128414)

I suggest adding a short paragraph stating that reviewers may request changes, and that if you are uncertain on how provide test cases/documentation to ask for guidance.

The previous and following paragraphs IMHO jump from "how to submit" to "when you patch is accepted". A brief description of our process of having patches go through a cycle of changes required / repost with fixes should be included.

57 ↗(On Diff #128414)

"In case you do not yet have commit access" -> "If you do not have commit access"

This matches the Phabricator documentation we have, and I think it's a little more straight forward.

62 ↗(On Diff #128414)

"In that case you can ping the patch. The common courtesy ping rate is one week."

->

"If you have received no comments on your patch for a week, you can request a review by 'ping'ing a patch by responding to the email thread containing the patch, or the Phabricator review with "Ping." The common courtesy 'ping' rate is once a week."

My suggestion here is based off new contributors not necessarily knowing the fairly common terminology.

63 ↗(On Diff #128414)

"Remember" -> "Please remember"

fhahn updated this revision to Diff 128522.Jan 3 2018, 7:43 AM
fhahn marked 7 inline comments as done.

Thanks for all the feedback. I've included the wording improvements. I also restructured the document in 3 main sections

  • Ways to Contribute
  • How to Submit a Patch
  • Helpful Information About LLVM

The last section contains a couple of links to pages giving an overview of LLVM's structure and internals.

I think this is a great start. Thanks Florian.

fhahn added a comment.Jan 3 2018, 9:33 AM

Rendered version with latest changes

silvas accepted this revision.Jan 3 2018, 8:27 PM

This is awesome, thanks for doing this Florian!

I agree with others that we have various newbie information spread across various pages, but refactoring that is a bigger task and this is a great start (besides being an awesome independent improvement by itself).

docs/Contributing.rst
64 ↗(On Diff #128522)

Can you somehow mention that adding a reviewer for an email patch means adding them to the CC of the email and that for Phab that means to fill in the field in the web UI?

This revision is now accepted and ready to land.Jan 3 2018, 8:27 PM
fhahn updated this revision to Diff 128620.Jan 4 2018, 9:11 AM
fhahn edited the summary of this revision. (Show Details)

Add info about how to add reviewers in phabricator & email reviews. Also updated the description.

Closed by commit rL321804: [Docs] Add Contributing page. (authored by fhahn, committed by ). · Explain WhyJan 4 2018, 9:13 AM
This revision was automatically updated to reflect the committed changes.