diff options
Diffstat (limited to 'docs/process/code-review-guidelines.rst')
| -rw-r--r-- | docs/process/code-review-guidelines.rst | 245 |
1 files changed, 245 insertions, 0 deletions
diff --git a/docs/process/code-review-guidelines.rst b/docs/process/code-review-guidelines.rst new file mode 100644 index 0000000..bd42811 --- /dev/null +++ b/docs/process/code-review-guidelines.rst @@ -0,0 +1,245 @@ +Code Review Guidelines +====================== + +Why do we do code reviews? +-------------------------- + +The main goal of code reviews is to improve the code quality. By reviewing each +other's code, we can help catch issues that were missed by the author +before they are integrated in the source tree. Different people bring different +perspectives, depending on their past work, experiences and their current use +cases of TF-A in their products. + +Code reviews also play a key role in sharing knowledge within the +community. People with more expertise in one area of the code base can +help those that are less familiar with it. + +Code reviews are meant to benefit everyone through team work. It is not about +unfairly criticizing or belittling the work of any contributor. + + +Overview of the code review process +----------------------------------- + +All contributions to Trusted Firmware-A project are reviewed by the community to +ensure they meet the project's expectations before they get merged, according to +the `Project Maintenance Process`_ defined for all `Trusted Firmware` projects. + +Technical ownership of most parts of the codebase falls on the :ref:`code +owners`. All patches are ultimately merged by the :ref:`maintainers`. + +Approval of a patch is tracked using Gerrit `labels`. For a patch to be merged, +it must get all of the following votes: + +- At least one ``Code-Owner-Review+1`` up-vote, and no ``Code-Owner-Review-1`` + down-vote. + +- At least one ``Maintainer-Review+1`` up-vote, and no ``Maintainer-Review-1`` + down-vote. + +- ``Verified+1`` vote applied by the automated Continuous Integration (CI) + system. + +Note that, in some instances, the maintainers might give a waiver for some of +the CI failures and manually override the ``Verified+1`` score. + + +Good practices for all reviewers +-------------------------------- + +To ensure the code review gives the greatest possible benefit, participants in +the project should: + +- Be considerate of other people and their needs. Participants may be working + to different timescales, and have different priorities. Keep this in + mind - be gracious while waiting for action from others, and timely in your + actions when others are waiting for you. + +- Review other people's patches where possible. The more active reviewers there + are, the more quickly new patches can be reviewed and merged. Contributing to + code review helps everyone in the long run, as it creates a culture of + participation which serves everyone's interests. + + +Guidelines for patch contributors +--------------------------------- + +In addition to the rules outlined in the :ref:`Contributor's Guide`, as a patch +contributor you are expected to: + +- Answer all comments from people who took the time to review your + patches. + +- Be patient and resilient. It is quite common for patches to go through + several rounds of reviews and rework before they get approved, especially + for larger features. + + In the event that a code review takes longer than you would hope for, you + may try the following actions to speed it up: + + - Ping the reviewers on Gerrit or on the mailing list. If it is urgent, + explain why. Please remain courteous and do not abuse this. + + - If one code owner has become unresponsive, ask the other code owners for + help progressing the patch. + + - If there is only one code owner and they have become unresponsive, ask one + of the project maintainers for help. + +- Do the right thing for the project, not the fastest thing to get code merged. + + For example, if some existing piece of code - say a driver - does not quite + meet your exact needs, go the extra mile and extend the code with the missing + functionality you require - as opposed to copying the code into some other + directory to have the freedom to change it in any way. This way, your changes + benefit everyone and will be maintained over time. + +- It is the patch-author's responsibility to respond to review comments within + 21 days. In the event that the patch-author does not respond within this + timeframe, the maintainer is entitled to abandon the patch(es). + Patch author(s) may be busy with other priorities, causing a delay in + responding to active review comments after posting patch(es). In such a + situation, if the author's patch(es) is/are abandoned, they can restore + their work for review by resolving comments, merge-conflicts, and revising + their original submissions. + +Guidelines for all reviewers +---------------------------- + +There are no good or bad review comments. If you have any doubt about a patch or +need some clarifications, it's better to ask rather than letting a potential +issue slip. Examples of review comments could be: + +- Questions ("Why do you need to do this?", "What if X happens?") +- Bugs ("I think you need a logical \|\| rather than a bitwise \|.") +- Design issues ("This won't scale well when we introduce feature X.") +- Improvements ("Would it be better if we did Y instead?") + + +Guidelines for code owners +-------------------------- + +Code owners are listed on the :ref:`Project Maintenance<code owners>` page, +along with the module(s) they look after. + +When reviewing a patch, code owners are expected to check the following: + +- The patch looks good from a technical point of view. For example: + + - The structure of the code is clear. + + - It complies with the relevant standards or technical documentation (where + applicable). + + - It leverages existing interfaces rather than introducing new ones + unnecessarily. + + - It fits well in the design of the module. + + - It adheres to the security model of the project. In particular, it does not + increase the attack surface (e.g. new SMCs) without justification. + +- The patch adheres to the TF-A :ref:`Coding Style`. The CI system should help + catch coding style violations. + +- (Only applicable to generic code) The code is MISRA-compliant (see + :ref:`misra-compliance`). The CI system should help catch violations. + +- Documentation is provided/updated (where applicable). + +- The patch has had an appropriate level of testing. Testing details are + expected to be provided by the patch author. If they are not, do not hesitate + to request this information. + +- All CI automated tests pass. + +If a code owner is happy with a patch, they should give their approval +through the ``Code-Owner-Review+1`` label in Gerrit. If instead, they have +concerns, questions, or any other type of blocking comment, they should set +``Code-Owner-Review-1``. + +Code owners are expected to behave professionally and responsibly. Here are some +guidelines for them: + +- Once you are engaged in a review, make sure you stay involved until the patch + is merged. Rejecting a patch and going away is not very helpful. You are + expected to monitor the patch author's answers to your review comments, + answer back if needed and review new revisions of their patch. + +- Provide constructive feedback. Just saying, "This is wrong, you should do X + instead." is usually not very helpful. The patch author is unlikely to + understand why you are requesting this change and might feel personally + attacked. + +- Be mindful when reviewing a patch. As a code owner, you are viewed as + the expert for the relevant module. By approving a patch, you are partially + responsible for its quality and the effects it has for all TF-A users. Make + sure you fully understand what the implications of a patch might be. + + +Guidelines for maintainers +-------------------------- + +Maintainers are listed on the :ref:`Project Maintenance<maintainers>` page. + +When reviewing a patch, maintainers are expected to check the following: + +- The general structure of the patch looks good. This covers things like: + + - Code organization. + + - Files and directories, names and locations. + + For example, platform code should be added under the ``plat/`` directory. + + - Naming conventions. + + For example, platform identifiers should be properly namespaced to avoid + name clashes with generic code. + + - API design. + +- Interaction of the patch with other modules in the code base. + +- The patch aims at complying with any standard or technical documentation + that applies. + +- New files must have the correct license and copyright headers. See :ref:`this + paragraph<copyright-license-guidance>` for more information. The CI system + should help catch files with incorrect or no copyright/license headers. + +- There is no third party code or binary blobs with potential IP concerns. + Maintainers should look for copyright or license notices in code, and use + their best judgement. If they are unsure about a patch, they should ask + other maintainers for help. + +- Generally speaking, new driver code should be placed in the generic + layer. There are cases where a driver has to stay into the platform layer but + this should be the exception, rather than the rule. + +- Existing common drivers (in particular for Arm IPs like the GIC driver) should + not be copied into the platform layer to cater for platform quirks. This + type of code duplication hurts the maintainability of the project. The + duplicate driver is less likely to benefit from bug fixes and future + enhancements. In most cases, it is possible to rework a generic driver to + make it more flexible and fit slightly different use cases. That way, these + enhancements benefit everyone. + +- When a platform specific driver really is required, the burden lies with the + patch author to prove the need for it. A detailed justification should be + posted via the commit message or on the mailing list. + +- Before merging a patch, verify that all review comments have been addressed. + If this is not the case, encourage the patch author and the relevant + reviewers to resolve these together. + +If a maintainer is happy with a patch, they should give their approval +through the ``Maintainer-Review+1`` label in Gerrit. If instead, they have +concerns, questions, or any other type of blocking comment, they should set +``Maintainer-Review-1``. + +-------------- + +*Copyright (c) 2020-2023, Arm Limited. All rights reserved.* + +.. _Project Maintenance Process: https://developer.trustedfirmware.org/w/collaboration/project-maintenance-process/ |