You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
410 lines
16 KiB
410 lines
16 KiB
.. _dev-environment-and-tools: |
|
|
|
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. |
|
|
|
Pull requests should be appropriately :ref:`labeled<gh_labels>`, |
|
and linked to any relevant :ref:`bug or feature tracking issues<bug_reporting>` |
|
. |
|
|
|
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 Actions automatically assigns a responsible reviewer |
|
on a component basis, as defined in the :zephyr_file:`MAINTAINERS.yml` 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 main branch once |
|
reviews are complete. |
|
|
|
.. _review_time: |
|
|
|
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 and Working Groups |
|
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 :ref:`breaking API changes <breaking_api_changes>`, the |
|
proposal needs to be presented in the Architecture meeting so that the relevant |
|
stakeholders are made aware of the change. |
|
|
|
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 2 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 |
|
- 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. |
|
After one week without a response to a request, a second attempt to elicit |
|
a response from the contributor will be made. After one more week without a |
|
response the item may be closed (draft and DNM tagged pull requests are |
|
excluded). |
|
|
|
Continuous Integration |
|
*********************** |
|
|
|
All changes submitted to GitHub are subject to tests that are run on |
|
emulated platforms and architectures to identify breakage and regressions that |
|
can be immediately identified. Testing using Twister 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; see note) |
|
- Gitlint: Git commit style based on project requirements |
|
- License Check: Check for conflicting licenses |
|
- Run ``twister`` 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. |
|
|
|
.. note:: |
|
|
|
``checkpatch`` is a Perl script that uses regular expressions to |
|
extract information that requires a C language parser to process |
|
accurately. As such it sometimes issues false positives. Known |
|
cases include constructs like: |
|
|
|
.. code-block:: c |
|
|
|
static uint8_t __aligned(PAGE_SIZE) page_pool[PAGE_SIZE * POOL_PAGES]; |
|
IOPCTL_Type *base = config->base; |
|
|
|
Both lines produce a diagnostic regarding spaces around the ``*`` |
|
operator: the first is misidentified as a pointer type declaration |
|
that would be correct as ``PAGE_SIZE *POOL_PAGES`` while the second |
|
is misidentified as a multiplication expression that would be correct |
|
as ``IOPCTL_Type * base``. |
|
|
|
Maintainers can override the -1 in cases where the CI infrastructure |
|
gets the wrong answer. |
|
|
|
|
|
.. _gh_labels: |
|
|
|
Labeling issues and pull requests in GitHub |
|
******************************************* |
|
|
|
The project uses GitHub issues and pull requests (PRs) to track and manage |
|
daily and long-term work and contributions to the Zephyr project. We use |
|
GitHub **labels** to classify and organize these issues and PRs by area, type, |
|
priority, and more, making it easier to find and report on relevant items. |
|
|
|
All GitHub issues or pull requests must be appropriately labeled. |
|
Issues and PRs often have multiple labels assigned, |
|
to help classify them in the different available categories. |
|
When reviewing a PR, if it has missing or incorrect labels, maintainers shall |
|
fix it. |
|
|
|
This saves us all time when searching, reduces the chances of the PR or issue |
|
being forgotten, speeds up reviewing, avoids duplicate issue reports, etc. |
|
|
|
These are the labels we currently have, grouped by applicability: |
|
|
|
Labels applicable to issues only |
|
================================ |
|
|
|
.. list-table:: |
|
:header-rows: 1 |
|
|
|
* - Label |
|
- Description |
|
|
|
* - :guilabel:`priority: {high|medium|low}` |
|
- To classify the impact and importance of a bug or |
|
:ref:`feature <feature-tracking>`. |
|
|
|
Note: Issue priorities are generally set or changed during the bug-triage or TSC |
|
meetings. |
|
|
|
* - :guilabel:`Regression` |
|
- Something, which was working, but does not anymore (bug subtype). |
|
|
|
* - :guilabel:`Enhancement` |
|
- Changes/Updates/Additions to existing :ref:`features <feature-tracking>`. |
|
|
|
* - :guilabel:`Feature request` |
|
- A request for a new :ref:`feature <feature-tracking>`. |
|
|
|
* - :guilabel:`Feature` |
|
- A :ref:`planned feature<feature-tracking>` with a milestone. |
|
|
|
* - :guilabel:`Hardware Support` |
|
- Covers porting an existing feature (including Zephyr itself) to new hardware. |
|
|
|
* - :guilabel:`Duplicate` |
|
- This issue is a duplicate of another issue (please specify). |
|
|
|
* - :guilabel:`Good first issue` |
|
- Good for a first time contributor to take. |
|
|
|
* - :guilabel:`Release Notes` |
|
- Issues that need to be mentioned in release notes as known issues with |
|
additional information. |
|
|
|
Any issue must be classified and labeled as either *Bug*, *Enhancement*, *RFC*, |
|
*Feature*, *Feature Request* or *Hardware Support*. More information on how |
|
feature requests are handled and become features can be found in :ref:`Feature |
|
Tracking<feature-tracking>`. |
|
|
|
Labels applicable to pull requests only |
|
======================================= |
|
|
|
The issue or PR describes a change to a stable API. |
|
|
|
.. list-table:: |
|
:header-rows: 1 |
|
|
|
* - Label |
|
- Description |
|
|
|
* - :guilabel:`Hotfix` |
|
- Fix for an issue blocking development. |
|
|
|
* - :guilabel:`Trivial` |
|
- Simple changes that can have shorter review time and be reviewed by anyone, i.e. typos, |
|
straightforward one-liner bug fixes, etc. |
|
|
|
* - :guilabel:`Maintainer` |
|
- Maintainer review required. |
|
|
|
* - :guilabel:`Security Review` |
|
- To be reviewed by a security expert. |
|
|
|
* - :guilabel:`DNM` |
|
- This PR should not be merged (Do Not Merge). For work in progress, GitHub |
|
"draft" PRs are preferred. |
|
|
|
* - :guilabel:`Needs review` |
|
- The PR needs attention from the maintainers. |
|
|
|
* - :guilabel:`Backport` |
|
- The PR is a backport or should be backported. |
|
|
|
* - :guilabel:`Licensing` |
|
- The PR has licensing issues which require a licensing expert to review it. |
|
|
|
.. note:: |
|
For all labels applicable to PRs: Please note that the label, together with |
|
PR complexity, affects how long a merge should be held to ensure proper |
|
review. See :ref:`review process <review_time>` for details. |
|
|
|
|
|
Labels applicable to both pull requests and issues |
|
================================================== |
|
|
|
.. list-table:: |
|
:header-rows: 1 |
|
|
|
* - Label |
|
- Description |
|
|
|
* - :guilabel:`area: {area-name}` |
|
- Indicates Zephyr subsystems (e.g, :guilabel:`area: Kernel`, :guilabel:`area: I2C`, |
|
:guilabel:`area: Memory Management`), project functions (e.g., :guilabel:`area: Debugging`, |
|
:guilabel:`area: Documentation`, :guilabel:`area: Process`), or other categories (e.g., |
|
:guilabel:`area: Coding Style`, :guilabel:`area: MISRA-C`) affected by the bug or the pull request. |
|
|
|
An area maintainer should be able to filter by an area label and find all issues |
|
and PRs which relate to that area. |
|
|
|
* - :guilabel:`platform: {platform-name}` |
|
- An issue or PR which affects only a particular platform. |
|
|
|
* - :guilabel:`dev-review` |
|
- The issue is to be discussed in the following `dev-review`_ if time |
|
permits. |
|
|
|
.. _`dev-review`: https://github.com/zephyrproject-rtos/zephyr/wiki/Zephyr-Committee-and-Working-Groups#zephyr-dev-meeting |
|
|
|
* - :guilabel:`TSC` |
|
- TSC stands for Technical Steering Committee. The issue is to be discussed in the |
|
following `TSC meeting`_ if time permits. |
|
|
|
.. _`TSC meeting`: https://github.com/zephyrproject-rtos/zephyr/wiki/Technical-Steering-Committee-(TSC) |
|
|
|
* - :guilabel:`Breaking API Change` |
|
- The issue or PR describes a breaking change to a stable API. See additional information |
|
in :ref:`breaking_api_changes`. |
|
|
|
* - :guilabel:`bug` |
|
- The issue is a bug, or the PR is fixing a bug. |
|
|
|
* - :guilabel:`Coverity` |
|
- A Coverity detected issue or its fix. |
|
|
|
* - :guilabel:`Waiting for response` |
|
- The Zephyr developers are waiting for the submitter to respond to a question, or |
|
address an issue. |
|
|
|
* - :guilabel:`Blocked` |
|
- Blocked by another PR or issue. |
|
|
|
* - :guilabel:`Stale` |
|
- An issue or a PR which seems abandoned, and requires attention by the author. |
|
|
|
* - :guilabel:`In progress` |
|
- For PRs: is work in progress and should not be merged yet. For issues: Is being |
|
worked on. |
|
|
|
* - :guilabel:`RFC` |
|
- The author would like input from the community. For a PR it should be considered |
|
a draft. |
|
|
|
* - :guilabel:`LTS` |
|
- Long term release branch related. |
|
|
|
* - :guilabel:`EXT` |
|
- Related to an external component.
|
|
|