diff options
Diffstat (limited to 'docs/process/contributing.rst')
| -rw-r--r-- | docs/process/contributing.rst | 214 |
1 files changed, 151 insertions, 63 deletions
diff --git a/docs/process/contributing.rst b/docs/process/contributing.rst index f569fcbe7..15c2b4529 100644 --- a/docs/process/contributing.rst +++ b/docs/process/contributing.rst @@ -4,17 +4,22 @@ Contributor's Guide Getting Started --------------- -- Make sure you have a Github account and you are logged on - `developer.trustedfirmware.org`_. -- Create an `issue`_ for your work if one does not already exist. This gives - everyone visibility of whether others are working on something similar. +- Make sure you have a Github account and you are logged on both + `developer.trustedfirmware.org`_ and `review.trustedfirmware.org`_. - - If you intend to include Third Party IP in your contribution, please - raise a separate `issue`_ for this and ensure that the changes that - include Third Party IP are made on a separate topic branch. +- If you plan to contribute a major piece of work, it is usually a good idea to + start a discussion around it on the mailing list. This gives everyone + visibility of what is coming up, you might learn that somebody else is + already working on something similar or the community might be able to + provide some early input to help shaping the design of the feature. + + If you intend to include Third Party IP in your contribution, please mention + it explicitly in the email thread and ensure that the changes that include + Third Party IP are made in a separate patch (or patch series). - Clone `Trusted Firmware-A`_ on your own machine as described in :ref:`prerequisites_get_source`. + - Create a local topic branch based on the `Trusted Firmware-A`_ ``master`` branch. @@ -23,61 +28,25 @@ Making Changes - Make commits of logical units. See these general `Git guidelines`_ for contributing to a project. -- Follow the :ref:`Coding Style & Guidelines`. - - - Use the checkpatch.pl script provided with the Linux source tree. A - Makefile target is provided for convenience. - Keep the commits on topic. If you need to fix another bug or make another - enhancement, please create a separate `issue`_ and address it on a separate - topic branch. -- Avoid long commit series. If you do have a long series, consider whether - some commits should be squashed together or addressed in a separate topic. -- Make sure your commit messages are in the proper format. If a commit fixes - an `issue`_, include a reference. -- Where appropriate, please update the documentation. + enhancement, please address it on a separate topic branch. - - Consider whether the :ref:`Porting Guide`, - :ref:`Firmware Design` document or other in-source documentation needs - updating. - - Ensure that each changed file has the correct copyright and license - information. Files that entirely consist of contributions to this - project should have a copyright notice and BSD-3-Clause SPDX license - identifier of the form as shown in :ref:`license`. Files that contain - changes to imported Third Party IP files should retain their original - copyright and license notices. For significant contributions you may - add your own copyright notice in following format: - - :: - - Portions copyright (c) [XXXX-]YYYY, <OWNER>. All rights reserved. - - where XXXX is the year of first contribution (if different to YYYY) and - YYYY is the year of most recent contribution. <OWNER> is your name or - your company name. - - If you are submitting new files that you intend to be the technical - sub-maintainer for (for example, a new platform port), then also update - the :ref:`maintainers` file. - - For topics with multiple commits, you should make all documentation - changes (and nothing else) in the last commit of the series. Otherwise, - include the documentation changes within the single commit. +- Split the patch in manageable units. Small patches are usually easier to + review so this will speed up the review process. -- Please test your changes. As a minimum, ensure that Linux boots on the - Foundation FVP. See :ref:`Arm Fixed Virtual Platforms (FVP)` for more - information. For more extensive testing, consider running the `TF-A Tests`_ - against your patches. - -Submitting Changes ------------------- +- Avoid long commit series. If you do have a long series, consider whether + some commits should be squashed together or addressed in a separate topic. - Ensure that each commit in the series has at least one ``Signed-off-by:`` line, using your real name and email address. The names in the - ``Signed-off-by:`` and ``Author:`` lines must match. If anyone else - contributes to the commit, they must also add their own ``Signed-off-by:`` - line. By adding this line the contributor certifies the contribution is made - under the terms of the + ``Signed-off-by:`` and ``Commit:`` lines must match. By adding this line the + contributor certifies the contribution is made under the terms of the :download:`Developer Certificate of Origin <../../dco.txt>`. + There might be multiple ``Signed-off-by:`` lines, depending on the history + of the patch. + More details may be found in the `Gerrit Signed-off-by Lines guidelines`_. - Ensure that each commit also has a unique ``Change-Id:`` line. If you have @@ -87,27 +56,144 @@ Submitting Changes More details may be found in the `Gerrit Change-Ids documentation`_. +- Write informative and comprehensive commit messages. A good commit message + provides all the background information needed for reviewers to understand + the intent and rationale of the patch. This information is also useful for + future reference. + + For example: + + - What does the patch do? + - What motivated it? + - What impact does it have? + - How was it tested? + - Have alternatives been considered? Why did you choose this approach over + another one? + - If it fixes an `issue`_, include a reference. + +- Follow the :ref:`Coding Style` and :ref:`Coding Guidelines`. + + - Use the checkpatch.pl script provided with the Linux source tree. A + Makefile target is provided for convenience, see :ref:`this + section<automatic-compliance-checking>` for more details. + +- Where appropriate, please update the documentation. + + - Consider whether the :ref:`Porting Guide`, :ref:`Firmware Design` document + or other in-source documentation needs updating. + + - If you are submitting new files that you intend to be the code owner for + (for example, a new platform port), then also update the + :ref:`code owners` file. + + - For topics with multiple commits, you should make all documentation changes + (and nothing else) in the last commit of the series. Otherwise, include + the documentation changes within the single commit. + +.. _copyright-license-guidance: + +- Ensure that each changed file has the correct copyright and license + information. Files that entirely consist of contributions to this project + should have a copyright notice and BSD-3-Clause SPDX license identifier of + the form as shown in :ref:`license`. Files that contain changes to imported + Third Party IP files should retain their original copyright and license + notices. + + For significant contributions you may add your own copyright notice in the + following format: + + :: + + Portions copyright (c) [XXXX-]YYYY, <OWNER>. All rights reserved. + + where XXXX is the year of first contribution (if different to YYYY) and YYYY + is the year of most recent contribution. <OWNER> is your name or your company + name. + +- Ensure that each patch in the patch series compiles in all supported + configurations. Patches which do not compile will not be merged. + +- Please test your changes. As a minimum, ensure that Linux boots on the + Foundation FVP. See :ref:`Arm Fixed Virtual Platforms (FVP)` for more + information. For more extensive testing, consider running the `TF-A Tests`_ + against your patches. + +- Ensure that all CI automated tests pass. Failures should be fixed. They might + block a patch, depending on how critical they are. + +Submitting Changes +------------------ + - Submit your changes for review at https://review.trustedfirmware.org targeting the ``integration`` branch. - - The changes will then undergo further review and testing by the - :ref:`maintainers`. Any review comments will be made directly on your - patch. This may require you to do some rework. +- Add reviewers for your patch: + + - At least one code owner for each module modified by the patch. See the list + of modules and their :ref:`code owners`. + + - At least one maintainer. See the list of :ref:`maintainers`. + + - If some module has no code owner, try to identify a suitable (non-code + owner) reviewer. Running ``git blame`` on the module's source code can + help, as it shows who has been working the most recently on this area of + the code. + + Alternatively, if it is impractical to identify such a reviewer, you might + send an email to the `TF-A mailing list`_ to broadcast your review request + to the community. + + Note that self-reviewing a patch is prohibited, even if the patch author is + the only code owner of a module modified by the patch. Getting a second pair + of eyes on the code is essential to keep up with the quality standards the + project aspires to. + +- The changes will then undergo further review by the designated people. Any + review comments will be made directly on your patch. This may require you to + do some rework. For controversial changes, the discussion might be moved to + the `TF-A mailing list`_ to involve more of the community. Refer to the `Gerrit Uploading Changes documentation`_ for more details. +- The patch submission rules are the following. For a patch to be approved + and merged in the tree, it must get: + + - One ``Code-Owner-Review+1`` for each of the modules modified by the patch. + - A ``Maintainer-Review+1``. + + In the case where a code owner could not be found for a given module, + ``Code-Owner-Review+1`` is substituted by ``Code-Review+1``. + + In addition to these various code review labels, the patch must also get a + ``Verified+1``. This is usually set by the Continuous Integration (CI) bot + when all automated tests passed on the patch. Sometimes, some of these + automated tests may fail for reasons unrelated to the patch. In this case, + the maintainers might (after analysis of the failures) override the CI bot + score to certify that the patch has been correctly tested. + + In the event where the CI system lacks proper tests for a patch, the patch + author or a reviewer might agree to perform additional manual tests + in their review and the reviewer incorporates the review of the additional + testing in the ``Code-Review+1`` or ``Code-Owner-Review+1`` as applicable to + attest that the patch works as expected. Where possible additional tests should + be added to the CI system as a follow up task. For example, for a + platform-dependent patch where the said platform is not available in the CI + system's board farm. + - When the changes are accepted, the :ref:`maintainers` will integrate them. - Typically, the :ref:`maintainers` will merge the changes into the ``integration`` branch. + - If the changes are not based on a sufficiently-recent commit, or if they cannot be automatically rebased, then the :ref:`maintainers` may rebase it - on the ``master`` branch or ask you to do so. + on the ``integration`` branch or ask you to do so. + - After final integration testing, the changes will make their way into the - ``master`` branch. If a problem is found during integration, the merge - commit will be removed from the ``integration`` branch and the - :ref:`maintainers` will ask you to create a new patch set to resolve the - problem. + ``master`` branch. If a problem is found during integration, the + :ref:`maintainers` will request your help to solve the issue. They may + revert your patches and ask you to resubmit a reworked version of them or + they may ask you to provide a fix-up patch. Binary Components ----------------- @@ -128,15 +214,17 @@ Binary Components -------------- -*Copyright (c) 2013-2019, Arm Limited and Contributors. All rights reserved.* +*Copyright (c) 2013-2020, Arm Limited and Contributors. All rights reserved.* .. _developer.trustedfirmware.org: https://developer.trustedfirmware.org +.. _review.trustedfirmware.org: https://review.trustedfirmware.org .. _issue: https://developer.trustedfirmware.org/project/board/1/ .. _Trusted Firmware-A: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git .. _Git guidelines: http://git-scm.com/book/ch5-2.html .. _Gerrit Uploading Changes documentation: https://review.trustedfirmware.org/Documentation/user-upload.html .. _Gerrit Signed-off-by Lines guidelines: https://review.trustedfirmware.org/Documentation/user-signedoffby.html .. _Gerrit Change-Ids documentation: https://review.trustedfirmware.org/Documentation/user-changeid.html -.. _TF-A Tests: https://git.trustedfirmware.org/TF-A/tf-a-tests.git/about/ +.. _TF-A Tests: https://trustedfirmware-a-tests.readthedocs.io .. _Trusted Firmware binary repository: https://review.trustedfirmware.org/admin/repos/tf-binaries .. _tf-binaries-readme: https://git.trustedfirmware.org/tf-binaries.git/tree/readme.rst +.. _TF-A mailing list: https://lists.trustedfirmware.org/mailman/listinfo/tf-a |