misc: Add a CONTRIBUTING document
Review Request #3814 - Created Feb. 16, 2017 and submitted
Information | |
---|---|
Jason Lowe-Power | |
gem5 | |
default | |
Reviewers | |
Default | |
changesets: 11849:b5f456096b51 "misc: Add a CONTRIBUTING document This document details how to contribute to gem5 based on our new contribution flow with git and gerrit. Signed-off-by: Jason Lowe-Power <jason@lowepower.com>"
Issue Summary
Description | From | Last Updated | Status |
---|---|---|---|
This is quite defensive and negative at the moment. Could we not make 1 and 2: * Share your work ... | Andreas Hansson | Feb. 23, 2017, 3:20 a.m. | Open |
I like Andreas' comments, but actually I don't think the original is negative either. Andreas 1,2 is probably a good ... | Ali Saidi | Feb. 28, 2017, 12:15 p.m. | Open |
Can we add the Gerrit testing before the code is posted for review? Reviewers should know that the code passes ... | Brad Beckmann | March 2, 2017, 2:56 p.m. | Open |
What happens when a maintainer has not been assigned to a region of code? Is there a way even to ... | Brad Beckmann | March 2, 2017, 2:56 p.m. | Open |
Restricting maintainer to be PMC member seems too restrictive. When someone contributes large portions of the code (especially code that ... | Brad Beckmann | March 2, 2017, 2:56 p.m. | Open |
Thanks for writing formalising this!
There are some really minor nits below. You might want to consider renaming this to CONTRIBUTING.md to make sure that it renders nicely in tool GUIs (e.g., GitHub) that understand markdown.
-
CONTRIBUTING (Diff revision 1) -
Typo
-
CONTRIBUTING (Diff revision 1) -
Typo: Delete 'use'.
-
CONTRIBUTING (Diff revision 1) -
It might be worth adding a paragraph to explain the why there are both reviewers and maintainers ackign changes here.
-
CONTRIBUTING (Diff revision 1) -
Since we are switching to Gerrit, we should just state something along these lines: "You generally don't need to add these manually as they are added automatically by Gerrit."
-
CONTRIBUTING (Diff revision 1) -
"Added automatically by Gerrit"
-
CONTRIBUTING (Diff revision 1) -
We might want to specify that this is added automatically by a commit hook in git.
-
CONTRIBUTING (Diff revision 1) -
We might want to move the second half of this paragraph to the description of signed-off-by above.
-
CONTRIBUTING (Diff revision 1) -
The first slash in /refs/for/... isn't needed. I'm not sure if it breaks anything though.
Diff: |
Revision 2 (+329) |
---|
Ship It!
Ship It!
Ship It!
Thanks for getting this in shape. Some minor things worth considering.
-
CONTRIBUTING.md (Diff revision 2) -
This is quite defensive and negative at the moment.
Could we not make 1 and 2:
- Share your work with others, so that they can benefit from new functionality.
- Support the scientific principle by enabling others to evaluate your suggestions without having to guess what you did.
-
CONTRIBUTING.md (Diff revision 2) -
I would think we should drop the s at the end.
-
CONTRIBUTING.md (Diff revision 2) -
Is there a step where tests/regressions are added and/or updated, or is that perhaps too much detail for this doc?
-
CONTRIBUTING.md (Diff revision 2) -
I like Andreas' comments, but actually I don't think the original is negative either. Andreas 1,2 is probably a good reason to do it and the current 1,2 are the carrots of why you should.
-
CONTRIBUTING.md (Diff revision 2) -
Where is the "Generate Password" option? I don't see it after signing in. Is this password unique to just pushing patches to gerrit and is it independent of your google account password?
Several of us have discussed this document at AMD and we have a few more questions and comments below.
I assume reviewboard is the best place to continue this conversation since this patch has not been moved to Gerrit.
-
CONTRIBUTING.md (Diff revision 2) -
Can we add the Gerrit testing before the code is posted for review? Reviewers should know that the code passes before they review it.
-
CONTRIBUTING.md (Diff revision 2) -
What happens when a maintainer has not been assigned to a region of code? Is there a way even to guarantee that all existing portions of the code have a maintainer assigned?
It seems like we should say if there is a maintainer, then one must get their approval. However in situations where one has not been assigned, we should skip this step.
-
CONTRIBUTING.md (Diff revision 2) -
Restricting maintainer to be PMC member seems too restrictive. When someone contributes large portions of the code (especially code that adds new features) the contributor will likely be the best person to maintain the code.
Please go ahead with this and we can fine tune later. Could you also update http://gem5.org/Submitting_Contributions accordingly?
I went ahead and pushed this to the mainline this morning. Sorry I was so slow to do this. There are some outstanding issues, but we can modify the document to fix them as we go.
To follow up on Brad's comments about maintainers: We need to add a new MAINTAINERS file. For now, we'll assume that the PMC is responsible for everything, and we can add maintainers as we go. If someone has a suggestion for how to assign maintainers, let us know (probably in a new email chain).