Patrick Georgi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Documentation: Revise "24 hours wait period" rules
They're more or less the same but reworked for hopefully some more clarity. There have been some best practices around documenting the reason for expedited processing so let's make them official, too.
Change-Id: I620e48016a1ceda2ac43f26624ed21af01f6a0a5 Signed-off-by: Patrick Georgi pgeorgi@google.com --- M Documentation/getting_started/gerrit_guidelines.md 1 file changed, 19 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43484/1
diff --git a/Documentation/getting_started/gerrit_guidelines.md b/Documentation/getting_started/gerrit_guidelines.md index 59f675a..5824b17 100644 --- a/Documentation/getting_started/gerrit_guidelines.md +++ b/Documentation/getting_started/gerrit_guidelines.md @@ -43,15 +43,25 @@ clarification, see the Developer's Certificate of Origin in the coreboot [Signed-off-by policy](https://www.coreboot.org/Development_Guidelines#Sign-off_Procedure).
-* Let non-trivial patches sit in a review state for at least 24 hours -before submission. Remember that there are coreboot developers 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 those that don’t impact the final binary output. The -24-hour period would start at submission, and would be restarted at any -update which significantly changes any part of the patch. Patches can be -'Fast-tracked' and submitted in under 24 hours with the agreement of at -least 3 +2 votes. +* In general, patches should remain open for review for at least 24 hours +since the last significant modification to the change. The purpose is to +let coreboot developers around the world have a chance to review. Complex +reworks, even if they don't change the purpose of the patch but the way +it's implemented, should restart the wait period. + +* A change can go in without the wait period if its purpose is to fix a +recently introduced build or boot issue. Its commit message should explain +what change introduced the problem and the nature of the problem. The +change itself should be as limited in scope and impact as possible +to make it simple to assess the impact. Such a change can be merged +early with 3 Code-Review+2, ideally not all from the same organization, +although that's not a hard requirement. + +* Trivial changes that deal with minor issues like inconsistencies in +whitespace or spelling fixes that don't impact the final binary output +also don't need to wait. Such changes should point out in their commit +messages that they're trivial and also how they verified that the binary +output is identical (e.g. a TIMELESS build for a given configuration).
* 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
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 53: build or boot issue Is this just a build issue at coreboot.org, or maybe something that isn't tested at coreboot.org but is used somewhere else? This seems to be the heart of the unwritten rule that I got caught up in.
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 66: * Do not +2 patches that you authored or own Unrelated to this patch, but I think this section could be removed now. The author and committer's +2s are now ignored for merge purposes, so this no longer matters.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 64: TIMELESS I've seen a lot of those patches recently. Would it be possible for jenkins to confirm that for all defconfigs?
Hello build bot (Jenkins), Marshall Dawson, Jay Talbott, Jeremy Soller, Matt DeVillier, Stefan Reinauer, Angel Pons, Eric Peers, Kyösti Mälkki, Patrick Rudolph, Martin Roth, David Hendricks, Werner Zeh, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43484
to look at the new patch set (#2).
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Documentation: Revise "24 hours wait period" rules
They're more or less the same but reworked for hopefully some more clarity. There have been some best practices around documenting the reason for expedited processing so let's make them official, too.
Change-Id: I620e48016a1ceda2ac43f26624ed21af01f6a0a5 Signed-off-by: Patrick Georgi pgeorgi@google.com --- M Documentation/getting_started/gerrit_guidelines.md 1 file changed, 20 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43484/2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 53: build or boot issue
Is this just a build issue at coreboot.org, or maybe something that isn't tested at coreboot. […]
qa.coreboot.org also doesn't really cover boot issues (the lava system is still ramping up), but you're right that this can be clarified. Done.
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 64: TIMELESS
I've seen a lot of those patches recently. […]
Possible, yes. Lots of work though. Not in scope anytime soon.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 64: TIMELESS
Possible, yes. Lots of work though. Not in scope anytime soon.
One could store the artifacts' SHA256 hashes for each commit somewhere, then compare. The problem is that the amount of stored hashes could grow up really fast.
As we build-test each submitted change now, maybe we could start off by assessing whether each submitted commit is reproducible? More or less like the 9elements automated testing bot, we could print out which boards are reproducible and which are not.
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 66: * Do not +2 patches that you authored or own
Unrelated to this patch, but I think this section could be removed now. […]
I'd still keep it: it's the reason why self-approval is ignored by gerrit.
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 53: recently introduced works as an adjective: recently-introduced
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 56: The change itself should be as limited in scope : and impact as possible to make it simple to assess the impact. I just thought about it for a while, and I noticed a certain pattern: fixup commits work best for build breakage, whereas reverts are better-suited for boot issues. I think it's worth mentioning this idea here. Sorry for the wall of text.
# Build failures
Since we build-test each patchset only once before submitting it, build breakage mainly happens when patch trains are submitted out-of-order, or when change A renames something and change B uses it on a completely different file, but aren't in the same patch train.
When building the tree breaks, pretty much *all* changes will fail to build. This ends up stalling everyone's workflow, which is really bad. However, it's easy to check if a change works to solve the problem: if it builds, it's good enough. Furthermore, the commits which introduced the problem may be large, so reverting them is often regarded as too invasive of a solution. So, making a small fixup change is the usual course of action.
# Boot failures
On the other hand, boot failures are (as of now) meaningless on Gerrit. That is, if e.g. Pineview boards stop booting for some reason, there's no way one can tell unless they build an affected coreboot version, flash it onto hardware and end up with a brick. Moreover, if the breakage happened because of a change to common code, it's nearly impossible to know if other platforms are affected or not, and if so to which extent.
That boot-testing isn't readily available makes fixups hard to justify. Will all platforms be able to boot with said fixup? It could happen that a fixup restores booting on a certain platform, but breaks another platform which was not initially affected. It might be unlikely, but it could happen. And since hardware is made out of cursed sand, no one really knows!
Despite the uncertain nature of boot failures, it's reasonable to assume the following: if a given platform stopped booting after a certain change was submitted, and reverting said change makes the affected platform able to boot again, the other platforms will either be affected in the same way (broke and got fixed) or not affected at all. Creating proper commits is important: when each commit does one thing, reverting the bad commit is easy.
# Examples
For example, when my huge SPDX commits cracked a jackpot and broke building master, Nico proposed CB:40140 to quickly fix things, and after thinking for a while CB:40240 was accepted as a definite solution. Reverting those SPDX changes was out of the question, as they were good to have, and it was easier to fix the `dead_code` macro.
When the allocator v4 changes broke master not too long after the 4.12 release, CB:41363 was proposed as a fixup. It fixed some platforms, but not all; this prompted for some more fixes like CB:41364 and similar. While the platform-specific fixups were eventually submitted, the first fixup was abandoned in favor of reverting everything and re-adding allocator v4 alongside v3.
Also, CB:42219 broke several things, and then CB:42434 was intended to be a fixup for the problem. I did not like that a fixup without much explanation was being pushed in using this rule, and I created CB:42166 to propose a revert. It went in much more easily.
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 61: * Trivial changes that deal with minor issues like inconsistencies in Mention that such changes need to be XS? (or maybe S, but that would be a bit of a stretch)
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 63: should point out in their commit : messages that they're trivial I've got mixed feelings about this. If a change is trivial because it meets the other criteria, but the author didn't state it is trivial, then it needs to wait 24h?
Hello build bot (Jenkins), Marshall Dawson, Jay Talbott, Jeremy Soller, Matt DeVillier, Stefan Reinauer, Angel Pons, Eric Peers, Kyösti Mälkki, Patrick Rudolph, Martin Roth, David Hendricks, Werner Zeh, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43484
to look at the new patch set (#3).
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Documentation: Revise "24 hours wait period" rules
They're more or less the same but reworked for hopefully some more clarity. There have been some best practices around documenting the reason for expedited processing so let's make them official, too.
Change-Id: I620e48016a1ceda2ac43f26624ed21af01f6a0a5 Signed-off-by: Patrick Georgi pgeorgi@google.com --- M Documentation/getting_started/gerrit_guidelines.md 1 file changed, 33 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43484/3
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/1/Documentation/getting_start... PS1, Line 64: TIMELESS
One could store the artifacts' SHA256 hashes for each commit somewhere, then compare. […]
To keep the hashes somewhere, we'd have to do TIMELESS builds on jenkins in the first place (their hashes are different from regular builds) which doubles the build effort. And it's down the rabbit hole from there.
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 53: recently introduced
works as an adjective: recently-introduced
Ack
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 56: The change itself should be as limited in scope : and impact as possible to make it simple to assess the impact.
I just thought about it for a while, and I noticed a certain pattern: fixup commits work best for bu […]
To be honest, this section is already longer than I'm comfortable with. It may be useful to add something like this (but ideally without calling out individual commits because that can seem to call out their developers and they also require context that will fade over time) as a "strategies / how to" section later in the document. Are you up for it?
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 61: * Trivial changes that deal with minor issues like inconsistencies in
Mention that such changes need to be XS? (or maybe S, but that would be a bit of a stretch)
I defused the entire "trivial change" exception by noting that if they're trivial enough they can also wait. That way this rule is about submitters seeing a change and wanting to get them in because they're obviously trivial and not be bothered about it _again_ (sometimes next day), and not about a right the patch editor can claim.
A change that replaces 100 uses of parralel with parallel might be trivial even if it's not "XS" or even "S" anymore. It still should be simple to follow that there's no adverse impact (hence: timeless builds).
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 63: should point out in their commit : messages that they're trivial
I've got mixed feelings about this. […]
Right, for trivial changes it's more reasonable to have the submitter state that assessment in a comment (also gives an implicit four-eyes review on the question of triviality). Done.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... PS3, Line 59: from the same organization, although that's not a hard requirement. When there is no enforcement of documentation policies, the quality of commit messages becomes a major factor for understanding design concepts. Therefore reviewers should be made aware beforehand that fast-tracking is to be expected. Reviewers themselves should self-assess if they are in their comfort-zone to allow fast-tracking.
So what should be done when this fast-tracking backfires? Who should have the responsibility of answering and addressing the questions that appeared after the submit? There are times when simple revert could be the choice, often other dependencies prevent from doing this.
Workarounds don't just magically get fixed upstream. When a commit was identified as such, should it be compulsory to add it to issue-tracker and assigned with a due-date?
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... PS3, Line 58: a change can be merged early with 3 Code-Review+2, ideally not all suggest adding not all from the same project or organization. Offers a little bit more oversight / neutral party to think about it. Otherwise lgtm.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... PS3, Line 58: a change can be merged early with 3 Code-Review+2, ideally not all
suggest adding not all from the same project or organization. […]
Do you mean adding "project or" to the text in this patchset? I'm not sure if I understood your suggestion correctly.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3: Code-Review+1
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... PS3, Line 59: from the same organization, although that's not a hard requirement.
When there is no enforcement of documentation policies, the quality of commit messages becomes a maj […]
Sorry, but I don't understand what you're getting at here, at all.
An emergency fix doesn't need a design concept: It needs to be clear what problem it is fixing, it needs to be clear what impact it has on other hardware, and if it's complex enough to need a design concept it's probably not an emergency fix.
But to be honest, "design concept" is stretching it for pretty much everything I've seen land in coreboot in the last 20 years, so it feels to me that you're shifting goal posts here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... PS3, Line 59: from the same organization, although that's not a hard requirement.
Sorry, but I don't understand what you're getting at here, at all. […]
To be able to build stages of a board for different ARCH definetly needs some symmetry of headerfiles we place under arch/. If you don't want to call it design concept, call it communication and coordination between developers then.
Only reading CB:43310 commit message, did it appear as an emergency fix to you? It was not an existing regression in upstream even with CB:42226 merged. To me it very much appeared as a regular commit, needed to workaround the spinlock headers, to build with the single mentioned Kconfig enabled. AFAICS, that particular Kconfig does not have much relevance in upstream.
It was not until I saw your (PG) comment on the meeting minutes agenda, when I understood the fix was necessary for the downstream chromeos repository. After the merge Martin had commented "Yes, this was to fix a breakage that would have hit in chromeos". I thought he just meant builds with CHROMEOS=y, so he could have been more clear there about not being able to submit CB:42226 in downstream chromeos, if that was the case.
So my complaint here is really about the commit messages not answering the questions, which those with interest did not have a chance to ask. Having one outside reviewer for fast-tracking should help here.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 3: Code-Review+2
Hello build bot (Jenkins), Marshall Dawson, Jay Talbott, Jeremy Soller, Matt DeVillier, Stefan Reinauer, Angel Pons, Kyösti Mälkki, Eric Peers, Patrick Rudolph, Martin Roth, David Hendricks, Werner Zeh, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43484
to look at the new patch set (#4).
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Documentation: Revise "24 hours wait period" rules
They're more or less the same but reworked for hopefully some more clarity. There have been some best practices around documenting the reason for expedited processing so let's make them official, too.
Change-Id: I620e48016a1ceda2ac43f26624ed21af01f6a0a5 Signed-off-by: Patrick Georgi pgeorgi@google.com --- M Documentation/getting_started/gerrit_guidelines.md 1 file changed, 36 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/43484/4
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... PS3, Line 58: a change can be merged early with 3 Code-Review+2, ideally not all
Do you mean adding "project or" to the text in this patchset? I'm not sure if I understood your sugg […]
I reworked that part to be stronger and less ambiguous. See next patch set.
https://review.coreboot.org/c/coreboot/+/43484/3/Documentation/getting_start... PS3, Line 59: from the same organization, although that's not a hard requirement.
To be able to build stages of a board for different ARCH definetly needs some symmetry of headerfile […]
So better documentation why this is an emergency and, if it can at all be helped (which it usually can), an outside reviewer - I emphasized both a bit more in the next revision.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 56: The change itself should be as limited in scope : and impact as possible to make it simple to assess the impact.
To be honest, this section is already longer than I'm comfortable with. […]
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Patch Set 4: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 56: The change itself should be as limited in scope : and impact as possible to make it simple to assess the impact.
Ack
I agree that mentioning particular changes isn't a good idea. I hope I can revisit this someday.
https://review.coreboot.org/c/coreboot/+/43484/2/Documentation/getting_start... PS2, Line 61: * Trivial changes that deal with minor issues like inconsistencies in
I defused the entire "trivial change" exception by noting that if they're trivial enough they can al […]
Ack
https://review.coreboot.org/c/coreboot/+/43484/4/Documentation/getting_start... File Documentation/getting_started/gerrit_guidelines.md:
https://review.coreboot.org/c/coreboot/+/43484/4/Documentation/getting_start... PS4, Line 81: our code, the focus should be on fixing the issue, not on assigning blame. +3
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43484 )
Change subject: Documentation: Revise "24 hours wait period" rules ......................................................................
Documentation: Revise "24 hours wait period" rules
They're more or less the same but reworked for hopefully some more clarity. There have been some best practices around documenting the reason for expedited processing so let's make them official, too.
Change-Id: I620e48016a1ceda2ac43f26624ed21af01f6a0a5 Signed-off-by: Patrick Georgi pgeorgi@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43484 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Documentation/getting_started/gerrit_guidelines.md 1 file changed, 36 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/Documentation/getting_started/gerrit_guidelines.md b/Documentation/getting_started/gerrit_guidelines.md index 59f675a..4547f91 100644 --- a/Documentation/getting_started/gerrit_guidelines.md +++ b/Documentation/getting_started/gerrit_guidelines.md @@ -43,15 +43,42 @@ clarification, see the Developer's Certificate of Origin in the coreboot [Signed-off-by policy](https://www.coreboot.org/Development_Guidelines#Sign-off_Procedure).
-* Let non-trivial patches sit in a review state for at least 24 hours -before submission. Remember that there are coreboot developers 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 those that don’t impact the final binary output. The -24-hour period would start at submission, and would be restarted at any -update which significantly changes any part of the patch. Patches can be -'Fast-tracked' and submitted in under 24 hours with the agreement of at -least 3 +2 votes. +* In general, patches should remain open for review for at least 24 hours +since the last significant modification to the change. The purpose is to +let coreboot developers around the world have a chance to review. Complex +reworks, even if they don't change the purpose of the patch but the way +it's implemented, should restart the wait period. + +* A change can go in without the wait period if its purpose is to fix +a recently-introduced issue (build, boot or OS-level compatibility, not +necessarily identified by coreboot.org facilities). Its commit message +has to explain what change introduced the problem and the nature of +the problem so that the emergency need becomes apparent. The change +itself should be as limited in scope and impact as possible to make it +simple to assess the impact. Such a change can be merged early with 3 +Code-Review+2. For emergency fixes that affect a single project (SoC, +mainboard, ...) it's _strongly_ recommended to get a review by somebody +not involved with that project to ensure that the documentation of the +issue is clear enough. + +* Trivial changes that deal with minor issues like inconsistencies in +whitespace or spelling fixes that don't impact the final binary output +also don't need to wait. Such changes should point out in their commit +messages how the the author verified that the binary output is identical +(e.g. a TIMELESS build for a given configuration). When submitting +such changes early, the submitter must be different from the author +and must document the intent in the Gerrit discussion, e.g. "landed the +change early because it's trivial". Note that trivial fixes shouldn't +necessarily be expedited: Just like they're not critical enough for +things to go wrong because of them, they're not critical enough to +require quick handling. This exception merely serves to acknowledge that +a round-the-world review just isn't necessary for some types of changes. + +* As explained in our Code of Conduct, we try to assume the best of each +other in this community. It's okay to discuss mistakes (e.g. isolated +instances of non-trivial and non-critical changes submitted early) but +try to keep such inquiries blameless. If a change leads to problems with +our code, the focus should be on fixing the issue, not on assigning blame.
* 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
-
Angel Pons (Code Review) -
Eric Peers (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)