mirror of
https://github.com/ARM-software/arm-trusted-firmware.git
synced 2025-04-16 09:34:18 +00:00
doc: Improve contribution guidelines
- Add some guidance about the type of information a patch author should provide to facilitate the review (and for future reference). - Make a number of implicit expectations explicit: - Every patch must compile. - All CI tests must pass. - Mention that the patch author is expected to add reviewers and explain how to choose them. - Explain the patch submission rules in terms of Gerrit labels. Also do some cosmetic changes, like adding empty lines, shuffling some paragraphs around. Change-Id: I6dac486684310b5a35aac7353e10fe5474a81ec5 Signed-off-by: Sandrine Bailleux <sandrine.bailleux@arm.com>
This commit is contained in:
Sandrine Bailleux
joanna.farley
parent
3e1e08b799
commit
7969747e7f
2 changed files with 100 additions and 29 deletions
|
|
@ -19,6 +19,7 @@ support its functionality through plugins.
|
|||
|
||||
Use of the EditorConfig file is suggested but is not required.
|
||||
|
||||
.. _automatic-compliance-checking:
|
||||
|
||||
Automatic Compliance Checking
|
||||
-----------------------------
|
||||
|
|
|
|||
|
|
@ -29,19 +29,53 @@ Making Changes
|
|||
- Make commits of logical units. See these general `Git guidelines`_ for
|
||||
contributing to a project.
|
||||
|
||||
- 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.
|
||||
|
||||
- Keep the commits on topic. If you need to fix another bug or make another
|
||||
enhancement, please address it on a separate topic branch.
|
||||
|
||||
- Split the patch in manageable units. Small patches are usually easier to
|
||||
review so this will speed up the review process.
|
||||
|
||||
- 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.
|
||||
- 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 ``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
|
||||
cloned the repository with the "`Clone with commit-msg hook`" clone method
|
||||
(following the :ref:`Prerequisites` document), this should already be the
|
||||
case.
|
||||
|
||||
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.
|
||||
|
||||
|
|
@ -74,49 +108,85 @@ Making Changes
|
|||
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
|
||||
------------------
|
||||
|
||||
- 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
|
||||
:download:`Developer Certificate of Origin <../../dco.txt>`.
|
||||
|
||||
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
|
||||
cloned the repository with the "`Clone with commit-msg hook`" clone method
|
||||
(following the :ref:`Prerequisites` document), this should already be the
|
||||
case.
|
||||
|
||||
More details may be found in the `Gerrit Change-Ids documentation`_.
|
||||
|
||||
- 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:`code owners` and :ref:`maintainers`. 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.
|
||||
- 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 ``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
|
||||
:ref:`maintainers` will request your help to solve the issue. They may
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue