Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines

[Martin Roth]

Hi Alex,
  Thanks for voicing your concerns.  I do think that most of the
issues you bring up are handled, or aren't as big issues you make them
out to be.  As always, I could be wrong, so I'd again invite others to
give their opinions.  I've tried to address your issues below.

> ... the "maintainer" is given almost absolute veto power
> in all matters relating to that code.
...
> There are also no responsibilities defined for a "maintainer", despite
> such person being given near-inter-stellar of power.
I did try to define maintainers in the document:
* Try to coordinate with platform maintainers when making changes to
  platforms. The platform maintainers are the users who initially pushed the
  code for that platform, as well as users who have made significant changes
  to a platform. To find out who maintains a piece of code, please use
  util/scripts/maintainers.go or refer to the original author of the code in
  git log.
I'm not certain how "Try to coordinate" is being read as "almost
absolute veto power" or "near-inter-stellar of power".

>  Concerns from external reviewers often get
> ignored or brushed off as "don't care".
This is absolutely addressed by these three guidelines:
* Respond to anyone who has taken the time to review your patches.
* If there have been comments or discussion on a patch, verify that the
  comments have been addressed before giving a +2.
* If there is still ongoing discussion to a patch, try to wait for a
conclusion to the discussion before submitting it to the tree.

>  I've seen plenty of cases where patches have been -2'd
> by w...@g.com until other @g.com patches were merged, only for the -2 to be
> removed once it was on the contributor to rebase his work.
I haven't seen this, but if you see a situation where you feel like
someone is being treated unfairly, please bring it to the attention of
the coreboot leadership or post something to the mailing list.

> This then creates another significant issue: the bar for contributions
> from such teams is very low, while anyone else experiences a much higher
> bar. This has many times resulted in sub-optimal code being merged, and
> great contributions getting delayed and abandoned.
Again, I feel like if you comment on these patches, the rules above
should handle these issues.  And again, if you feel like something is
unfair, bring attention to it.

> ...  Raptor Eng. was asking around $15000 to upstream ...
> All of a sudden, we've made coreboot a very expensive
> project to support.
Additionally, I specifically reached out to tpearson with these
guidelines to get his feedback before they were posted.  I have tried
to address the concerns he brought up.  If you (or anyone else) feel
like there's something specific we can add here that will help him or
future developers get code submitted, please recommend the addition.

> Then there's talk about a vague "coreboot leadership"...
Patrick addressed this in the review - here's Stefan talking about
coreboot leadership:
http://blogs.coreboot.org/blog/2015/05/11/on-coreboot-leadership/

-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines

[ron minnich]

Possibly an appendix defining coreboot leadership would help.



On Fri, Oct 30, 2015 at 9:51 AM Martin Roth  wrote:

> Hi Alex,
>   Thanks for voicing your concerns.  I do think that most of the
> issues you bring up are handled, or aren't as big issues you make them
> out to be.  As always, I could be wrong, so I'd again invite others to
> give their opinions.  I've tried to address your issues below.
>
> > ... the "maintainer" is given almost absolute veto power
> > in all matters relating to that code.
> ...
> > There are also no responsibilities defined for a "maintainer", despite
> > such person being given near-inter-stellar of power.
> I did try to define maintainers in the document:
> * Try to coordinate with platform maintainers when making changes to
>   platforms. The platform maintainers are the users who initially pushed
> the
>   code for that platform, as well as users who have made significant
> changes
>   to a platform. To find out who maintains a piece of code, please use
>   util/scripts/maintainers.go or refer to the original author of the code
> in
>   git log.
> I'm not certain how "Try to coordinate" is being read as "almost
> absolute veto power" or "near-inter-stellar of power".
>
> >  Concerns from external reviewers often get
> > ignored or brushed off as "don't care".
> This is absolutely addressed by these three guidelines:
> * Respond to anyone who has taken the time to review your patches.
> * If there have been comments or discussion on a patch, verify that the
>   comments have been addressed before giving a +2.
> * If there is still ongoing discussion to a patch, try to wait for a
> conclusion to the discussion before submitting it to the tree.
>
> >  I've seen plenty of cases where patches have been -2'd
> > by w...@g.com until other @g.com patches were merged, only for the -2 to be
> > removed once it was on the contributor to rebase his work.
> I haven't seen this, but if you see a situation where you feel like
> someone is being treated unfairly, please bring it to the attention of
> the coreboot leadership or post something to the mailing list.
>
> > This then creates another significant issue: the bar for contributions
> > from such teams is very low, while anyone else experiences a much higher
> > bar. This has many times resulted in sub-optimal code being merged, and
> > great contributions getting delayed and abandoned.
> Again, I feel like if you comment on these patches, the rules above
> should handle these issues.  And again, if you feel like something is
> unfair, bring attention to it.
>
> > ...  Raptor Eng. was asking around $15000 to upstream ...
> > All of a sudden, we've made coreboot a very expensive
> > project to support.
> Additionally, I specifically reached out to tpearson with these
> guidelines to get his feedback before they were posted.  I have tried
> to address the concerns he brought up.  If you (or anyone else) feel
> like there's something specific we can add here that will help him or
> future developers get code submitted, please recommend the addition.
>
> > Then there's talk about a vague "coreboot leadership"...
> Patrick addressed this in the review - here's Stefan talking about
> coreboot leadership:
> http://blogs.coreboot.org/blog/2015/05/11/on-coreboot-leadership/
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>
-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

[coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines

[Martin Roth]

As the community has grown, so has the need to formalize some of the
guidelines that the community lives by. When the community was small,
it was easy to communicate these things just from one person to
another.

Now, with more people joining the community every day, it seems that
it's time to write some of these things down, allowing people to
understand our policies immediately instead of making them learn our
practices as they make mistakes.

As it says in the document: The following rules are the requirements
for behavior in the coreboot codebase in gerrit. These have mainly
been unwritten rules up to this point, and should be familiar to most
users who have been active in coreboot for a period of time. Following
these rules will help reduce friction in the community.

This has been posted to gerrit for review:
http://review.coreboot.org/#/c/12256/

Please post your thoughts and comments either as a reply to this email
or in gerrit.

Thanks.
Martin



coreboot Gerrit Etiquette and Guidelines


The following rules are the requirements for behavior in the coreboot
codebase in gerrit. These have mainly been unwritten rules up to this
point, and should be familiar to most users who have been active in
coreboot for a period of time. Following these rules will help reduce
friction in the community.


Summary:

These are the expectations for committing, reviewing, and submitting
code into coreboot git and gerrit. While breaking individual rules may
not have immediate consequences, the coreboot leadership may act on
repeated or flagrant violations with or without notice.

* Don't violate the licenses.
* Let non-trivial patches sit in a review state for at least 24 hours
before submission.
* Try to coordinate with platform maintainers when making changes to platforms.
* If you give a patch a -2, you are responsible for giving concrete
recommendations for what could be changed to resolve the issue the
patch addresses.
* Don't modify other people's patches without their consent.
* Be respectful to others when commenting.
* Each patch should be kept to one logical change.
* Don’t submit patches that you know will break other platforms.


More detail:

* Don't violate the licenses. If you're submitting code that you
didn't write yourself, make sure the license is compatible with the
license of the project you're submitting the changes to. If you’re
submitting code that you wrote that might be owned by your employer,
make sure that your employer is aware and you are authorized to submit
the code. See the Developer's Certificate of Origin in the
Signed-off-by policy.
* Let non-trivial patches sit in a review state for at least 24 hours
before submission. Remember that coreboot developers are in timezones
all over the world, and everyone should have a chance to contribute.
Trivial patches would be things like whitespace changes or spelling
fixes. In general, small changes that don’t impact the final binary
output.
* Do not +2 patches that you authored or own, even for something as
trivial as whitespace fixes. When working on your own patches, it’s
easy to overlook something like accidentally updating file permissions
or git submodule commit IDs. Let someone else review the patch.
* Try to coordinate with platform maintainers when making changes to
platforms. The platform maintainers are the users who initially pushed
the code for that platform, as well as users who have made significant
changes to a platform. To find out who maintains a piece of code,
please use util/scripts/maintainers.go or refer to the original author
of the code in git log.
* If you give a patch a -2, you are responsible for giving concrete
recommendations for what could be changed to resolve the issue the
patch addresses. If you feel strongly that a patch should NEVER be
merged, you are responsible for defending your position and listening
to other points of view. Giving a -2 and walking away is not
acceptable, and may cause your -2 to be removed by the coreboot
leadership after a period of time.
* Don't modify other people's patches unless you are specifically
requested to do so by the owner of that patch. Not only is this
considered rude, but your changes could be lost unintentionally when
the owner pushes an update without realizing that you’ve changed
something.
* Be respectful to others when commenting on their patch. Comments
should be kept to the code, and should be kept in a polite tone. If
you feel attacked, resist the urge to attack back - this only
escalates the issue.
* Each patch should be kept to one logical change, which should be
described in the title of the patch. Unrelated changes should be split
out into separate patches. Fixing whitespace on a line you’re editing
is reasonable. Fixing whitespace around the code you’re working on
should be a separate ‘cleanup’ patch. Larger patches that touch
several areas are fine, so long as they are one logical change. Adding
new 

Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines

[Alex G.]

On 10/29/2015 09:28 PM, ron minnich wrote:
> It's a CL.

Ooh, I thought it was a coreboot policy change that attempted to solve
long-standing issues within coreboot. My bad.

> It would be very helpful if you want to propose specific changes.

I did. Refresh the page :)

Alex

-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines

[ron minnich]

On Thu, Oct 29, 2015 at 9:49 PM Alex G.  wrote:

> My bad.
>
>
>
Apology accepted.

ron
-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines

[Alex G.]

Hi Martin,

It seems that the first part of your document ("Summary", and "More
detail") is written in direct response to the removal of FSP 1.0 a month
or so ago. As such, it's very situation-centric, and fails to address
some more fundamental issues.

The concept of a "maintainer" is central to a large number of the
definitions and guidelines posted, but that concept is very ill-defined.
According to your definition of "maintainer", someone who touched that
code, even with a simple refactoring patch, can now be construed to be a
"maintainer". Then, the "maintainer" is given almost absolute veto power
in all matters relating to that code.

There are also no responsibilities defined for a "maintainer", despite
such person being given near-inter-stellar of power. This only
exacerbates some of the long-standing problems we've had in coreboot.

One such problem is code parking, whereas a contributor merges a piece
of code, but then doesn't do his due diligence in taking care of that
code. If the code causes issues, or prevents other refactorings from
happening, the original contributor disappears, and it's up to the
reacaftoring contributor to fix that code as well.

To that, you now add the requirement that the refactoring contribution
leaves the broken platform in a functioning state. That places the
entire burden on the new contributor, while the original submitter can
come in at any time and say "veto".

This creates a counter-constructive disproportion of powers and
responsibilities. You want to make it such that both original submitter
and new contributor have balanced amounts of power and responsibility
over the piece of code in question.

We might want to learn from what other projects have done here, most
notably linux distributions. Take a look at Fedora's nonresponsive
maintainer policy [1] as an example. Give "maintainers"
responsibilities, not just plain stupid absolute power. We're not
solving any issues otherwise.

Another issue that's not addressed here is the problem of people in the
same team submitting and merging patches. This has been a recurring
theme with google contributions. x...@g.com submits the patch, y...@g.com
approves and merges it. Concerns from external reviewers often get
ignored or brushed off as "don't care". @g.com patches that create merge
conflicts for other contributors are sometimes rushed so @g.com doesn't
have to rebase. I've seen plenty of cases where patches have been -2'd
by w...@g.com until other @g.com patches were merged, only for the -2 to be
removed once it was on the contributor to rebase his work.

This then creates another significant issue: the bar for contributions
from such teams is very low, while anyone else experiences a much higher
bar. This has many times resulted in sub-optimal code being merged, and
great contributions getting delayed and abandoned.

This is actually a huge issue that we're dealing with. I don't remember
the exact numbers, but Raptor Eng. was asking around $15000 to upstream
their fam15 work. That's because it's painfully difficult for anyone
outside those teams to merge their contributions. Considering that
there's one guy paying Raptor for all this work, one guy who believes
in, and wants to support coreboot. We've cost him $15000 dollars (of his
own money!). All of a sudden, we've made coreboot a very expensive
project to support.

Then there's talk about a vague "coreboot leadership" who can make
arbitrary decisions about X and Y. There's no concise guideline really,
but it does allow a mysterious group to make any decisions about
contributions and contributors, and leave too much open to the whim of
someone's personal thoughts or the goodness or badness of his day.

I don't think that's acceptable. This needs more work.

Alex

[1]
https://fedoraproject.org/wiki/Policy_for_nonresponsive_package_maintainers

On 10/29/2015 11:55 AM, Martin Roth wrote:
> As the community has grown, so has the need to formalize some of the
> guidelines that the community lives by. When the community was small,
> it was easy to communicate these things just from one person to
> another.
> 
> Now, with more people joining the community every day, it seems that
> it's time to write some of these things down, allowing people to
> understand our policies immediately instead of making them learn our
> practices as they make mistakes.
> 
> As it says in the document: The following rules are the requirements
> for behavior in the coreboot codebase in gerrit. These have mainly
> been unwritten rules up to this point, and should be familiar to most
> users who have been active in coreboot for a period of time. Following
> these rules will help reduce friction in the community.
> 
> This has been posted to gerrit for review:
> http://review.coreboot.org/#/c/12256/
> 
> Please post your thoughts and comments either as a reply to this email
> or in gerrit.
> 
> Thanks.
> Martin
> 
> 
> 
> coreboot Gerrit Etiquette and Guidelines
>