421 lines
16 KiB
ReStructuredText
421 lines
16 KiB
ReStructuredText
.. _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.
|