216 lines
8.9 KiB
ReStructuredText
216 lines
8.9 KiB
ReStructuredText
|
||
|
||
Development Environment and Tools
|
||
#################################
|
||
|
||
Code Review
|
||
************
|
||
|
||
GitHub is intended to provide a framework for reviewing every commit before it
|
||
is accepted into the code base. Changes, in the form of Pull Requests (PR) are
|
||
uploaded to GitHub but don’t actually become a part of the project until they’ve
|
||
been reviewed, passed a series of checks (CI), and are approved by maintainers.
|
||
GitHub is used to support the standard open source practice of submitting
|
||
patches, which are then reviewed by the project members before being applied to
|
||
the code base.
|
||
|
||
The Zephyr project uses GitHub for code reviews and Git tree management. When
|
||
submitting a change or an enhancement to any Zephyr component, a developer
|
||
should use GitHub. GitHub automatically assigns a responsible reviewer on a
|
||
component basis, as defined in the :zephyr_file:`CODEOWNERS` file stored with the code
|
||
tree in the Zephyr project repository. A limited set of release managers are
|
||
allowed to merge a pull request into the master branch once reviews are complete.
|
||
|
||
|
||
Give reviewers time to review before code merge
|
||
================================================
|
||
|
||
The Zephyr project is a global project that is not tied to a certain geography
|
||
or timezone. We have developers and contributors from across the globe. When
|
||
changes are proposed using pull request, we need to allow for a minimal review
|
||
time to give developers and contributors the opportunity to review and comment
|
||
on changes. There are different categories of changes and we know that some
|
||
changes do require reviews by subject matter experts and owners of the subsystem
|
||
being changed. Many changes fall under the “trivial” category that can be
|
||
addressed with general reviews and do not need to be queued for a maintainer or
|
||
code-owner review. Additionally, some changes might require further discussions
|
||
and a decision by the TSC or the Security working group. To summarize the above,
|
||
the diagram below proposes minimal review times for each category:
|
||
|
||
|
||
.. figure:: pull_request_classes.png
|
||
:align: center
|
||
:alt: Pull request classes
|
||
:figclass: align-center
|
||
|
||
Pull request classes
|
||
|
||
Workflow
|
||
---------
|
||
|
||
- An author of a change can suggest in his pull-request which category a change
|
||
should belong to. A project maintainers or TSC member monitoring the inflow of
|
||
changes can change the label of a pull request by adding a comment justifying
|
||
why a change should belong to another category.
|
||
- The project will use the label system to categorize the pull requests.
|
||
- Changes should not be merged before the minimal time has expired.
|
||
|
||
Categories/Labels
|
||
-----------------
|
||
|
||
Hotfix
|
||
++++++
|
||
|
||
Any change that is a fix to an issue that blocks developers from doing their
|
||
daily work, for example CI breakage, Test breakage, Minor documentation fixes
|
||
that impact the user experience.
|
||
|
||
Such fixes can be merged at any time after they have passed CI checks. Depending
|
||
on the fix, severity, and availability of someone to review them (other than the
|
||
author) they can be merged with justification without review by one of the
|
||
project owners.
|
||
|
||
Trivial
|
||
+++++++
|
||
|
||
Trivial changes are those that appear obvious enough and do not require maintainer or code-owner
|
||
involvement. Such changes should not change the logic or the design of a
|
||
subsystem or component. For example a trivial change can be:
|
||
|
||
- Documentation changes
|
||
- Configuration changes
|
||
- Minor Build System tweaks
|
||
- Minor optimization to code logic without changing the logic
|
||
- Test changes and fixes
|
||
- Sample modifications to support additional configuration or boards etc.
|
||
|
||
Maintainer
|
||
+++++++++++
|
||
|
||
Any changes that touch the logic or the original design of a subsystem or
|
||
component will need to be reviewed by the code owner or the designated subsystem
|
||
maintainer. If the code changes is initiated by a contributor or developer other
|
||
than the owner the pull request needs to be assigned to the code owner who will
|
||
have to drive the pull request to a mergeable state by giving feedback to the
|
||
author and asking for more reviews from other developers.
|
||
|
||
Security
|
||
+++++++++++
|
||
|
||
Changes that appear to have an impact to the overall security of the system need
|
||
to be reviewed by a security expert from the security working group.
|
||
|
||
TSC
|
||
++++
|
||
|
||
Changes that introduce new features or functionality or change the way the
|
||
overall system works need to be reviewed by the TSC or the responsible Working
|
||
Group. For example for API changes, the API working group needs to be consulted
|
||
and made aware of such changes.
|
||
|
||
|
||
A Pull-Request should have an Assignee
|
||
=======================================
|
||
|
||
- An assignee to a pull request should not be the same as the
|
||
author of the pull-request
|
||
- An assignee to a pull request is responsible for driving the
|
||
pull request to a mergeable state
|
||
- An assignee is responsible for dismissing stale reviews and seeking reviews
|
||
from additional developers and contributors
|
||
- Pull requests should not be merged without an approval by the assignee.
|
||
|
||
Pull Request should not be merged by author without review
|
||
===========================================================
|
||
|
||
All pull requests need to be reviewed and should not be merged by the author
|
||
without a review. The following exceptions apply:
|
||
|
||
- Hot fixes: Fixing CI issues, reverts, and system breakage
|
||
- Release related changes: Changing version file, applying tags and release
|
||
related activities without any code changes.
|
||
|
||
Developers and contributors should always seek review, however there are cases
|
||
when reviewers are not available and there is a need to get a code change into
|
||
the tree as soon as possible.
|
||
|
||
Reviewers shall not ‘Request Changes’ without comments or justification
|
||
=======================================================================
|
||
|
||
Any change requests (-1) on a pull request have to be justified. A reviewer
|
||
should avoid blocking a pull-request with no justification. If a reviewer feels
|
||
that a change should not be merged without their review, then: Request change
|
||
of the category: for example:
|
||
|
||
- Trivial -> Maintainer
|
||
- Assign Pull Request to yourself, this will mean that a pull request should
|
||
not be merged without your approval.
|
||
|
||
|
||
Pull Requests should have at least 2 approvals before they are merged
|
||
======================================================================
|
||
|
||
A pull-request shall be merged only with two positive reviews (approval). Beside
|
||
the person merging the pull-request (merging != approval), two additional
|
||
approvals are required to be able to merge a pull request. The person merging
|
||
the request can merge without approving or approve and merge to get to the 2
|
||
approvals required.
|
||
|
||
Reviewers should keep track of pull requests they have provided feedback to
|
||
===========================================================================
|
||
|
||
If a reviewer has requested changes in a pull request, he or she should monitor
|
||
the state of the pull request and/or respond to mention requests to see if his
|
||
feedback has been addressed. Failing to do so, negative reviews shall be
|
||
dismissed by the assignee or an owner of the repository. Reviews will be
|
||
dismissed following the criteria below:
|
||
|
||
- The feedback or concerns were visibly addressed by the author
|
||
- The reviewer did not revisit the pull request after 1 week and multiple pings
|
||
by the author
|
||
- The review is unrelated to the code change or asking for unjustified
|
||
structural changes such as:
|
||
|
||
- Split the PR
|
||
- Split the commits
|
||
- Can you fix this unrelated code that happens to appear in the diff
|
||
- Can you fix unrelated issues
|
||
- Etc.
|
||
|
||
Closing Stale Issues and Pull Requests
|
||
=======================================
|
||
|
||
- The Pull requests and issues sections on Github are NOT discussion forums.
|
||
They are items that we need to execute and drive to closure.
|
||
Use the mailing lists for discussions.
|
||
- In case of both issues and pull-requests the original poster needs to respond
|
||
to questions and provide clarifications regarding the issue or the change.
|
||
Failing to do so, an issue or a pull request will be closed automatically
|
||
after 6 months.
|
||
|
||
Continuous Integration
|
||
***********************
|
||
|
||
All changes submitted to GitHub are subject to sanity tests that are run on
|
||
emulated platforms and architectures to identify breakage and regressions that
|
||
can be immediately identified. Sanity testing additionally performs build tests
|
||
of all boards and platforms. Documentation changes are also verified
|
||
through review and build testing to verify doc generation will be successful.
|
||
|
||
Any failures found during the CI test run will result in a negative review
|
||
assigned automatically by the CI system.
|
||
Developers are expected to fix issues and rework their patches and submit again.
|
||
|
||
The CI infrastructure currently runs the following tests:
|
||
|
||
- Run ''checkpatch'' for code style issues (can vote -1 on errors)
|
||
- Gitlint: Git commit style based on project requirements
|
||
- License Check: Check for conflicting licenses
|
||
- Run ''sanitycheck'' script
|
||
|
||
- Run kernel tests in QEMU (can vote -1 on errors)
|
||
- Build various samples for different boards (can vote -1 on errors)
|
||
|
||
- Verify documentation builds correctly.
|
||
|