blob: 5e9a6678ff4c9371611d797a8784be382215f512 [file] [log] [blame]
Sandrine Bailleux398b1882020-08-17 08:52:33 +02001Code Review Guidelines
2======================
3
Sandrine Bailleux398b1882020-08-17 08:52:33 +02004Why do we do code reviews?
5--------------------------
6
7The main goal of code reviews is to improve the code quality. By reviewing each
8other's code, we can help catch issues that were missed by the author
9before they are integrated in the source tree. Different people bring different
10perspectives, depending on their past work, experiences and their current use
11cases of TF-A in their products.
12
13Code reviews also play a key role in sharing knowledge within the
14community. People with more expertise in one area of the code base can
15help those that are less familiar with it.
16
17Code reviews are meant to benefit everyone through team work. It is not about
18unfairly criticizing or belittling the work of any contributor.
19
20
Sandrine Bailleuxe4ec3d62023-05-25 15:46:01 +020021Overview of the code review process
22-----------------------------------
23
24All contributions to Trusted Firmware-A project are reviewed by the community to
25ensure they meet the project's expectations before they get merged, according to
26the `Project Maintenance Process`_ defined for all `Trusted Firmware` projects.
27
28Technical ownership of most parts of the codebase falls on the :ref:`code
29owners`. All patches are ultimately merged by the :ref:`maintainers`.
30
31Approval of a patch is tracked using Gerrit `labels`. For a patch to be merged,
32it must get all of the following votes:
33
34- At least one ``Code-Owner-Review+1`` up-vote, and no ``Code-Owner-Review-1``
35 down-vote.
36
37- At least one ``Maintainer-Review+1`` up-vote, and no ``Maintainer-Review-1``
38 down-vote.
39
40- ``Verified+1`` vote applied by the automated Continuous Integration (CI)
41 system.
42
43Note that, in some instances, the maintainers might give a waiver for some of
44the CI failures and manually override the ``Verified+1`` score.
45
46
47Good practices for all reviewers
48--------------------------------
Sandrine Bailleux398b1882020-08-17 08:52:33 +020049
50To ensure the code review gives the greatest possible benefit, participants in
51the project should:
52
53- Be considerate of other people and their needs. Participants may be working
54 to different timescales, and have different priorities. Keep this in
55 mind - be gracious while waiting for action from others, and timely in your
56 actions when others are waiting for you.
57
58- Review other people's patches where possible. The more active reviewers there
59 are, the more quickly new patches can be reviewed and merged. Contributing to
60 code review helps everyone in the long run, as it creates a culture of
61 participation which serves everyone's interests.
62
63
64Guidelines for patch contributors
65---------------------------------
66
67In addition to the rules outlined in the :ref:`Contributor's Guide`, as a patch
68contributor you are expected to:
69
70- Answer all comments from people who took the time to review your
71 patches.
72
73- Be patient and resilient. It is quite common for patches to go through
74 several rounds of reviews and rework before they get approved, especially
75 for larger features.
76
77 In the event that a code review takes longer than you would hope for, you
78 may try the following actions to speed it up:
79
80 - Ping the reviewers on Gerrit or on the mailing list. If it is urgent,
81 explain why. Please remain courteous and do not abuse this.
82
83 - If one code owner has become unresponsive, ask the other code owners for
84 help progressing the patch.
85
86 - If there is only one code owner and they have become unresponsive, ask one
87 of the project maintainers for help.
88
89- Do the right thing for the project, not the fastest thing to get code merged.
90
91 For example, if some existing piece of code - say a driver - does not quite
92 meet your exact needs, go the extra mile and extend the code with the missing
93 functionality you require - as opposed to copying the code into some other
94 directory to have the freedom to change it in any way. This way, your changes
95 benefit everyone and will be maintained over time.
96
Manish V Badarkhe352a61f2023-07-05 00:25:08 +010097- It is the patch-author's responsibility to respond to review comments within
98 21 days. In the event that the patch-author does not respond within this
99 timeframe, the maintainer is entitled to abandon the patch(es).
100 Patch author(s) may be busy with other priorities, causing a delay in
101 responding to active review comments after posting patch(es). In such a
102 situation, if the author's patch(es) is/are abandoned, they can restore
103 their work for review by resolving comments, merge-conflicts, and revising
104 their original submissions.
Sandrine Bailleux398b1882020-08-17 08:52:33 +0200105
106Guidelines for all reviewers
107----------------------------
108
109There are no good or bad review comments. If you have any doubt about a patch or
110need some clarifications, it's better to ask rather than letting a potential
111issue slip. Examples of review comments could be:
112
113- Questions ("Why do you need to do this?", "What if X happens?")
114- Bugs ("I think you need a logical \|\| rather than a bitwise \|.")
115- Design issues ("This won't scale well when we introduce feature X.")
116- Improvements ("Would it be better if we did Y instead?")
117
118
119Guidelines for code owners
120--------------------------
121
122Code owners are listed on the :ref:`Project Maintenance<code owners>` page,
123along with the module(s) they look after.
124
125When reviewing a patch, code owners are expected to check the following:
126
127- The patch looks good from a technical point of view. For example:
128
129 - The structure of the code is clear.
130
131 - It complies with the relevant standards or technical documentation (where
132 applicable).
133
134 - It leverages existing interfaces rather than introducing new ones
135 unnecessarily.
136
137 - It fits well in the design of the module.
138
139 - It adheres to the security model of the project. In particular, it does not
140 increase the attack surface (e.g. new SMCs) without justification.
141
142- The patch adheres to the TF-A :ref:`Coding Style`. The CI system should help
143 catch coding style violations.
144
145- (Only applicable to generic code) The code is MISRA-compliant (see
146 :ref:`misra-compliance`). The CI system should help catch violations.
147
148- Documentation is provided/updated (where applicable).
149
150- The patch has had an appropriate level of testing. Testing details are
151 expected to be provided by the patch author. If they are not, do not hesitate
152 to request this information.
153
154- All CI automated tests pass.
155
156If a code owner is happy with a patch, they should give their approval
157through the ``Code-Owner-Review+1`` label in Gerrit. If instead, they have
158concerns, questions, or any other type of blocking comment, they should set
159``Code-Owner-Review-1``.
160
161Code owners are expected to behave professionally and responsibly. Here are some
162guidelines for them:
163
164- Once you are engaged in a review, make sure you stay involved until the patch
165 is merged. Rejecting a patch and going away is not very helpful. You are
166 expected to monitor the patch author's answers to your review comments,
167 answer back if needed and review new revisions of their patch.
168
169- Provide constructive feedback. Just saying, "This is wrong, you should do X
170 instead." is usually not very helpful. The patch author is unlikely to
171 understand why you are requesting this change and might feel personally
172 attacked.
173
174- Be mindful when reviewing a patch. As a code owner, you are viewed as
175 the expert for the relevant module. By approving a patch, you are partially
176 responsible for its quality and the effects it has for all TF-A users. Make
177 sure you fully understand what the implications of a patch might be.
178
179
180Guidelines for maintainers
181--------------------------
182
183Maintainers are listed on the :ref:`Project Maintenance<maintainers>` page.
184
185When reviewing a patch, maintainers are expected to check the following:
186
187- The general structure of the patch looks good. This covers things like:
188
189 - Code organization.
190
191 - Files and directories, names and locations.
192
193 For example, platform code should be added under the ``plat/`` directory.
194
195 - Naming conventions.
196
197 For example, platform identifiers should be properly namespaced to avoid
198 name clashes with generic code.
199
200 - API design.
201
202- Interaction of the patch with other modules in the code base.
203
204- The patch aims at complying with any standard or technical documentation
205 that applies.
206
207- New files must have the correct license and copyright headers. See :ref:`this
208 paragraph<copyright-license-guidance>` for more information. The CI system
209 should help catch files with incorrect or no copyright/license headers.
210
211- There is no third party code or binary blobs with potential IP concerns.
212 Maintainers should look for copyright or license notices in code, and use
213 their best judgement. If they are unsure about a patch, they should ask
214 other maintainers for help.
215
216- Generally speaking, new driver code should be placed in the generic
217 layer. There are cases where a driver has to stay into the platform layer but
218 this should be the exception, rather than the rule.
219
220- Existing common drivers (in particular for Arm IPs like the GIC driver) should
221 not be copied into the platform layer to cater for platform quirks. This
222 type of code duplication hurts the maintainability of the project. The
223 duplicate driver is less likely to benefit from bug fixes and future
224 enhancements. In most cases, it is possible to rework a generic driver to
225 make it more flexible and fit slightly different use cases. That way, these
226 enhancements benefit everyone.
227
228- When a platform specific driver really is required, the burden lies with the
229 patch author to prove the need for it. A detailed justification should be
230 posted via the commit message or on the mailing list.
231
232- Before merging a patch, verify that all review comments have been addressed.
233 If this is not the case, encourage the patch author and the relevant
234 reviewers to resolve these together.
235
236If a maintainer is happy with a patch, they should give their approval
237through the ``Maintainer-Review+1`` label in Gerrit. If instead, they have
238concerns, questions, or any other type of blocking comment, they should set
239``Maintainer-Review-1``.
240
241--------------
242
Sandrine Bailleuxe4ec3d62023-05-25 15:46:01 +0200243*Copyright (c) 2020-2023, Arm Limited. All rights reserved.*
Sandrine Bailleux398b1882020-08-17 08:52:33 +0200244
Sandrine Bailleuxbe5a9472023-12-21 13:59:45 +0100245.. _Project Maintenance Process: https://trusted-firmware-docs.readthedocs.io/en/latest/generic_processes/project_maintenance_process.html