New subject: Document for review: coreboot Gerrit Etiquette and Guidelines

30 Oct 2015

      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
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 chips and doing code cleanup over wide areas are two examples of
this.

Don’t submit code that you know will break other platforms. If your

patch affects code that is used by other platforms, it should be
compatible with those platforms. While it would be nice to update any
other platforms, you must at least provide a path that will allow
other platforms to continue working.
Recommendations for gerrit activity:
These guidelines are less strict than the ones listed above. These are
more of the “good idea” variety. You are requested to follow the below
guidelines, but there will probably be no actual consequences if
they’re not followed. That said, following the recommendations below
will speed up review of your patches, and make the members of the
community do less work.

Test your patches before submitting them to gerrit. It's also

appreciated if you add a line to the commit message describing how the
patch was tested. This prevents people from having to ask whether and
how the patch was tested.  An example of this sort of comment would be
‘TEST=Built platform’ or ‘TEST=Built and booted platform’

Take advantage of the lint tools to make sure your patches don’t

contain trivial mistakes. By running ‘make gitconfig’, the lint-stable
tools are automatically put in place and will test your patches before
they are committed. As a violation of these tools will cause the
jenkins build test to fail, it’s to your advantage to test this before
pushing to gerrit.

Don't submit patch trains longer than around 20 patches. Long patch

trains become unmanageable and tie up the build servers for long
periods of time. Rebasing a patch train over and over as you fix
earlier patches in the train can hide comments, and make people review
the code multiple times to see if anything has changed between
revisions.

Run 'make what-jenkins-does' locally on patch trains before

submitting. This helps verify that the patch train won’t tie up the
jenkins builders for no reason if there are failing patches in the
train.

Use a topic when pushing a train of patches. This groups the commits

together so people can easily see the connection at the top level of
gerrit. Topics can be set for individual patches in gerrit by going
into the patch and clicking on the icon next to the topic line. Topics
can also be set when you push the patches into gerrit. For example, to
push a set of commits with the the i915-kernel-x60 set, use the
command:
        git push origin HEAD:refs/for/master/i915-kernel-x60

If one of your patches isn't ready to be merged, make sure it's

obvious that you don't feel it's ready for merge yet. This can be
something like giving it a -1 or -2, or marking in the commit message
that it’s not ready until X. The commit message can be updated easily
when it’s ready to be pushed.  Examples of this are "WIP: title" or
"[NEEDS_TEST]: title".  These can also be pushed as drafts as shown in
the next guideline.

When pushing patches that are not for submission, these should be

marked as such. This can be done in the title ‘[DONOTSUBMIT]’, or can
be pushed as draft commits, so that only explicitly added reviewers
will see them. These sorts of patches are frequently posted as ideas
or RFCs for the community to look at. To push a draft, use the
command:
        git push origin HEAD:refs/drafts/master

Respond to anyone who has taken the time to review your patches,

even if it's just to say that you disagree. While it may seem annoying
to address a request to fix spelling or 'trivial' issues, it’s
generally easy to handle in gerrit’s built in editor.  It's also
acceptable to add fixes for these sorts of comments to another patch,
but it's recommended that that patch be pushed to gerrit before the
initial patch gets submitted.

Consider breaking up large individual patches into smaller patches

grouped by areas. This makes the patches easier to review, but
increases the number of patches. The way you want to handle this is a
personal decision, as long as each patch is still one logical change.

If you have an interest in a particular area or mainboard, set

yourself up as a ‘maintainer’ of that area by adding yourself to the
MAINTAINERS file in the coreboot root directory. Eventually, this
should automatically add you as a reviewer when an area that you’re
listed as a maintainer is changed.

Submit mainboards that you’re working on to the board-status repo.

This helps others and shows that these mainboards are currently being
maintained.  At some point, boards that are not up to date in the
board-status repo will probably end up getting deleted from the
coreboot tree.

Abandon patches that are no longer useful, or that you don’t intend

to keep working on.

Bring attention to patches that you would like reviewed. Add

reviewers, or even just rebase it against the current codebase to
bring it to the top of the gerrit list. If you’re not sure who would
be a good reviewer, look in the MAINTAINERS file or git history of the
files that you’ve changed, and add those people.

Familiarize yourself with the commit message guidelines, and try to

follow them. This will help to keep annoying requests to fix your
commit message to a minimum.

If there have been comments or discussion on a patch, verify that

the comments have been addressed before giving a +2. If you feel that
a comment is invalid, please respond to that comment instead of just
ignoring it.

Be conscientious when reviewing patches. If you give a patch a +2

and it breaks things, you should feel as responsible as the owner of
the patch for fixing things, and could be called on by the community
to help fix any fallout of the patch. This means you shouldn’t +2 a
patch just because you trust the author of a patch - Make sure you
understand what the implications of a patch might be, or leave the
review to others. Partial reviews, reviewing code style, for example,
can be given a +1 instead of a +2. This also applies if you think the
patch looks good, but may not have the experience to know if there may
be unintended consequences.

If there is still ongoing discussion to a patch, try to wait for a

conclusion to the discussion before submitting it to the tree. If you
feel that someone is beating a dead horse, maybe just state that and
give a time that the patch will be submitted. If no new objections
have come up instead of submitting the patch immediately and ending
the discussion by force.

When working with patch trains, for minor requests it’s acceptable

to create a fix addressing a comment in another patch at the end of
the patch train. This minimizes rebases of the patch train while still
addressing the request. For major problems where the change doesn’t
work as intended or breaks other platforms, the change really needs to
go into the original patch.
Expectations contributors should have:

Don't expect that people will review your patch unless you ask them

to. Adding other people as reviewers is the easiest way. Asking for
reviews for individual patches in the IRC channel, or by sending a
direct request to an individual through your favorite messenger is
usually the best way to get a patch reviewed quickly.

Don't expect that your patch will be submitted immediately after

getting a +2. As stated previously, non-trivial patches should wait at
least 24 hours before being submitted. That said, if you feel that
your patch or series of patches has been sitting longer than needed,
you can ask for it to be submitted on IRC, or comment that it's ready
for submission in the patch. This will move it to the top of the list
where it's more likely to be noticed and acted upon.

Reviews are about the code. It's easy to take it personally when

someone is criticising your code, but the whole idea is to get better
code into our codebase. Again, this also applies in the other
direction: review code, criticize code, but don’t make it personal.
Requests for clarification and suggestions for updates to these
guidelines should be sent to the coreboot mailing list at
coreboot@coreboot.org.

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

coreboot Gerrit Etiquette and Guidelines

Summary:

More detail:

Recommendations for gerrit activity:

Expectations contributors should have: