Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Jason Lowe-Power]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9504
---


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).

- Jason Lowe-Power


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Andreas Hansson]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9503
---

Ship it!


Please go ahead with this and we can fine tune later. Could you also update 
http://gem5.org/Submitting_Contributions accordingly?

- Andreas Hansson


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Andreas Sandberg]

> On March 2, 2017, 10:56 p.m., Brad Beckmann wrote:
> > CONTRIBUTING.md, line 55
> > 
> >
> > 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.

I think we need some way of tracking maintainers on a per file/directory basis. 
Linux has a similar system in place (see the MAINTAINERS file). In addition to 
component/architecture maintainers, I expect we'll end up with a set of core 
maintainers that are responsible for shared functionality and ACKs unmaintained 
bits.


> On March 2, 2017, 10:56 p.m., Brad Beckmann wrote:
> > CONTRIBUTING.md, line 34
> > 
> >
> > Can we add the Gerrit testing before the code is posted for review?  
> > Reviewers should know that the code passes before they review it.

AFAIK, there is no easy way to get gerrit to run the regressions before 
publishing a review. One possibility would be to submit a draft review and wait 
for Jenkins to run and then publish the review. The problem with that approach 
is that (automatically) assigned reviewers typically get notifications for 
draft reviews. In my experience, this isn't a large problem.


> On March 2, 2017, 10:56 p.m., Brad Beckmann wrote:
> > CONTRIBUTING.md, line 293
> > 
> >
> > 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.

Good point. I think we might need to move in this direction to avoid 
scalability issues going forwards.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9500
---


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Andreas Sandberg]

> On Feb. 28, 2017, 9:48 p.m., Brad Beckmann wrote:
> > CONTRIBUTING.md, line 205
> > 
> >
> > 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?

It's in the top bar when you navigate to https://gem5.googlesource.com/. The 
password is used to avoid storing and sending your normal account password 
(it's effectively a replacement for SSH keys) across when pushing code for 
review.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9495
---


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Brad Beckmann]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9500
---


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 (line 34)


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 (line 55)


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 (line 293)


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.


- Brad Beckmann


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Brad Beckmann]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9495
---



CONTRIBUTING.md (line 205)


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?


- Brad Beckmann


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Ali Saidi]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9494
---



CONTRIBUTING.md (line 7)


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.


- Ali Saidi


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Andreas Sandberg]

> On Feb. 23, 2017, 11:20 a.m., Andreas Hansson wrote:
> > CONTRIBUTING.md, line 69
> > 
> >
> > Is there a step where tests/regressions are added and/or updated, or is 
> > that perhaps too much detail for this doc?

We probably shouldn't include that in the general contribution guide since 
users in general can't run the regressions (binary restrictions etc.). The code 
review system will integrate with a hosted Jenkins instance to automatically 
test that code is functionally correct, but there is little we can do 
automatically for stat diffs.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9457
---


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Andreas Hansson]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9457
---


Thanks for getting this in shape. Some minor things worth considering.


CONTRIBUTING.md (line 7)


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 (line 10)


I would think we should drop the s at the end.



CONTRIBUTING.md (line 69)


Is there a step where tests/regressions are added and/or updated, or is 
that perhaps too much detail for this doc?


- Andreas Hansson


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Pierre-Yves Péneau]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9456
---

Ship it!


Ship It!

- Pierre-Yves Péneau


On Feb. 21, 2017, 6:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 6:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Tony Gutierrez]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9441
---

Ship it!


Ship It!

- Tony Gutierrez


On Feb. 21, 2017, 9:34 a.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 9:34 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Andreas Sandberg]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9440
---

Ship it!


Ship It!

- Andreas Sandberg


On Feb. 21, 2017, 5:34 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 21, 2017, 5:34 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Jason Lowe-Power]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/
---

(Updated Feb. 21, 2017, 5:34 p.m.)


Review request for Default.


Repository: gem5


Description
---

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 "


Diffs (updated)
-

  CONTRIBUTING.md PRE-CREATION 

Diff: http://reviews.gem5.org/r/3814/diff/


Testing
---


Thanks,

Jason Lowe-Power

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3814: misc: Add a CONTRIBUTING document

[Andreas Sandberg]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3814/#review9437
---


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 (line 10)


Typo



CONTRIBUTING (line 19)


Typo: Delete 'use'.



CONTRIBUTING (line 29)


It might be worth adding a paragraph to explain the why there are both 
reviewers and maintainers ackign changes here.



CONTRIBUTING (line 157)


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 (line 160)


"Added automatically by Gerrit"



CONTRIBUTING (line 162)


We might want to specify that this is added automatically by a commit hook 
in git.



CONTRIBUTING (line 166)


We might want to move the second half of this paragraph to the description 
of signed-off-by above.



CONTRIBUTING (line 213)


The first slash in /refs/for/... isn't needed. I'm not sure if it breaks 
anything though.


- Andreas Sandberg


On Feb. 16, 2017, 4:54 p.m., Jason Lowe-Power wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3814/
> ---
> 
> (Updated Feb. 16, 2017, 4:54 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> 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 "
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING PRE-CREATION 
> 
> Diff: http://reviews.gem5.org/r/3814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lowe-Power
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev