docs: Add Contributor Expectations document
Collect up all the contributor expectations and PR requirements into a
single place. Add additional guidelines about creating small PRs and how
to break up PRs into multiple commits.
Signed-off-by: Keith Short <keithshort@google.com>
diff --git a/doc/contribute/contributor_expectations.rst b/doc/contribute/contributor_expectations.rst
new file mode 100644
index 0000000..2d366e1
--- /dev/null
+++ b/doc/contribute/contributor_expectations.rst
@@ -0,0 +1,280 @@
+.. _contributor-expectations:
+
+Contributor Expectations
+########################
+
+Overview
+********
+
+The Zephyr project encourages :ref:`contributors <contributor>` to submit
+changes as smaller pull requests. Smaller pull requests (PRs) have the following
+benefits:
+
+- Reviewed more quickly and reviewed more thoroughly. It's easier for reviewers
+ to set aside a few minutes to review smaller changes several times than it is
+ to allocate large blocks of time to review a large PR.
+
+- Less wasted work if reviewers or maintainers reject the direction of the
+ change.
+
+- Easier to rebase and merge. Smaller PRs are less likely to conflict with other
+ changes in the tree.
+
+- Easier to revert if the PR breaks functionality.
+
+.. note::
+ This page does not apply to draft PRs which can have any size, any number of
+ commits and any combination of smaller PRs for testing and preview purposes.
+ Draft PRs have no review expectation and PRs created as drafts from the start
+ do not notify anyone by default.
+
+
+Defining Smaller PRs
+====================
+
+- Smaller PRs should encompass one self-contained logical change.
+
+- When adding a new large feature or API, the PR should address only one part of
+ the feature. In this case create an :ref:`RFC proposal <rfcs>` to describe the
+ additional parts of the feature for reviewers.
+
+- PRs should include tests or samples under the following conditions:
+
+ - Adding new features or functionality.
+
+ - Modifying a feature, especially for API behavior contract changes.
+
+ - Fixing a hardware agnostic bug. The test should fail without the bug fixed
+ and pass with the fix applied.
+
+- PRs must update any documentation affected by a functional code change.
+
+- If introducing a new API, the PR must include an example usage of the API.
+ This provides context to the reviewer and prevents submitting PRs with unused
+ APIs.
+
+
+Multiple Commits on a Single PR
+===============================
+
+Contributors are further encouraged to break up PRs into multiple commits. Keep
+in mind each commit in the PR must still build cleanly and pass all the CI
+tests.
+
+For example, when introducing an extension to an API, contributors can break up
+the PR into multiple commits targeting these specific changes:
+
+#. Introduce the new APIs, including shared devicetree bindings
+#. Update driver implementation X, with driver specific devicetree bindings
+#. Update driver implementation Y
+#. Add tests for the new API
+#. Add a sample using the API
+#. Update the documentation
+
+Large Changes
+=============
+
+Large changes to the Zephyr project must submit an :ref:`RFC proposal <rfcs>`
+describing the full scope of change and future work. The RFC proposal provides
+the required context to reviewers, but allows for smaller, incremental, PRs to
+get reviewed and merged into the project. The RFC should also define the minimum
+viable implementation.
+
+Changes which require an RFC proposal include:
+
+- Submitting a new feature.
+- Submitting a new API.
+- :ref:`treewide-changes`.
+- Other large changes that can benefit from the RFC proposal process.
+
+Maintainers have the discretion to request that contributors create an RFC for
+PRs that are too large or complicated.
+
+PR Requirements
+***************
+
+- Each commit in the PR must provide a commit message following the
+ :ref:`commit-guidelines`.
+
+- All files in the PR must comply with :ref:`Licensing
+ Requirements<licensing_requirements>`.
+
+- Follow the Zephyr :ref:`coding_style` and :ref:`coding_guidelines`.
+
+- PRs must pass all CI checks. This is a requirement to merge the PR.
+ Contributors may mark a PR as draft and explicitly request reviewers to
+ provide early feedback, even with failing CI checks.
+
+- When breaking a PR into multiple commits, each commit must build cleanly. The
+ CI system does not enforce this policy, so it is the PR author's
+ responsibility to verify.
+
+- When major new functionality is added, tests for the new functionality shall
+ be added to the automated test suite. All API functions should have test cases
+ and there should be tests for the behavior contracts of the API. Maintainers
+ and reviewers have the discretion to determine if the provided tests are
+ sufficient. The examples below demonstrate best practices on how to test APIs
+ effectively.
+
+ - :zephyr_file:`Kernel timer tests <tests/kernel/timer/timer_behavior>`
+ provide around 85% test coverage for the
+ :zephyr_file:`kernel timer <kernel/timer.c>`, measured by lines of code.
+ - Emulators for off-chip peripherals are an effective way to test driver
+ APIs. The :zephyr_file:`fuel gauge tests <tests/drivers/fuel_gauge/sbs_gauge>`
+ use the :zephyr_file:`smart battery emulator <drivers/sensor/sbs_gauge/emul_sbs_gauge.c>`,
+ providing test coverage for the
+ :zephyr_file:`fuel gauge API <include/zephyr/drivers/fuel_gauge.h>`
+ and the :zephyr_file:`smart battery driver <drivers/fuel_gauge/sbs_gauge/sbs_gauge.c>`.
+ - Code coverage reports for the Zephyr project are available on `Codecov`_.
+
+- Incompatible changes to APIs must also update the release notes for the
+ next release detailing the change. APIs marked as experimental are excluded
+ from this requirement.
+
+- Changes to APIs must increment the API version number according to the API
+ version rules.
+
+- PRs must also satisfy all :ref:`merge_criteria` before a member of the release
+ engineering team merges the PR into the zephyr tree.
+
+Maintainers may request that contributors break up a PR into smaller PRs and may
+request that they create an :ref:`RFC proposal <rfcs>`.
+
+.. _`Codecov`: https://app.codecov.io/gh/zephyrproject-rtos/zephyr
+
+Workflow Suggestions That Help Reviewers
+========================================
+
+- Unless they applied the reviewer's recommendation exactly, authors must not
+ resolve and hide comments, they must let the initial reviewer do it. The
+ Zephyr project does not require all comments to be resolved before merge.
+ Leaving some completed discussions open can sometimes be useful to understand
+ the greater picture.
+
+- Respond to comments using the "Start Review" and "Add Review" green buttons in
+ the "Files changed" view. This allows responding to multiple comments and
+ publishing the responses in bulk. This reduces the number of emails sent to
+ reviewers.
+
+- As GitHub does not implement |git range-diff|_, try to minimize rebases in the
+ middle of a review. If a rebase is required, push this as a separate update
+ with no other changes since the last push of the PR. When pushing a rebase
+ only, add a comment to the PR indicating which commit is the rebase.
+
+.. |git range-diff| replace:: ``git range-diff``
+.. _`git range-diff`: https://git-scm.com/docs/git-range-diff
+
+PR Review Escalation
+====================
+
+The Zephyr community is a diverse group of individuals, with different levels of
+commitment and priorities. As such, reviewers and maintainers may not get to a
+PR right away.
+
+The `Zephyr Dev Meeting`_ performs a triage of PRs missing reviewer approval,
+following this process:
+
+#. Identify and update PRs missing an Assignee.
+#. Identify PRs without any comments or reviews, ping the PR Assignee to start a
+ review or assign to a different maintainer.
+#. For PRs that have otherwise stalled, the Zephyr Dev Meeting pings the
+ Assignee and any reviewers that have left comments on the PR.
+
+Contributors may escalate PRs outside of the Zephyr Dev Meeting triage process
+as follows:
+
+- After 1 week of inactivity, ping the Assignee or reviewers on the PR by adding
+ a comment to the PR.
+
+- After 2 weeks of inactivity, post a message on the `#pr-help`_ channel on
+ Discord linking to the PR.
+
+- After 2 weeks of inactivity, add the `dev-review`_ label to the PR. This
+ explicitly adds the PR to the agenda for the next `Zephyr Dev Meeting`_
+ independent of the triage process. Not all contributors have the required
+ privileges to add labels to PRs, in this case the contributor should request
+ help on Discord or send an email to the `Zephyr devel mailing list`_.
+
+Note that for new PRs, contributors should generally wait for at least one
+Zephyr Dev Meeting before escalating the PR themselves.
+
+.. _Zephyr devel mailing list: https://lists.zephyrproject.org/g/devel
+
+
+.. _pr_technical_escalation:
+
+PR Technical Escalation
+=======================
+
+In cases where a contributor objects to change requests from reviewers, Zephyr
+defines the following escalation process for resolving technical disagreements.
+
+- Resolve in the PR among assignee, maintainers and reviewer.
+
+ - Assignee to act as moderator if applicable.
+
+- Optionally resolve in the next `Zephyr Dev Meeting`_ or `Architecture Working
+ Group`_ meeting with more Maintainers and project stakeholders.
+
+ - The involved parties and the Assignee to be present when
+ the (escalated) issue is discussed.
+
+- TSC: Assignees can escalate to the TSC voting members and get a binding
+ resolution in the TSC by adding the `tsc`_ label on the PR.
+
+- Assignee to ensure the resolution of the escalation is reflected in the PR
+ review.
+
+.. _#pr-help: https://discord.com/channels/720317445772017664/997527108844798012
+
+.. _dev-review: https://github.com/zephyrproject-rtos/zephyr/labels/dev-review
+
+.. _Zephyr Dev Meeting: https://github.com/zephyrproject-rtos/zephyr/wiki/Zephyr-Committee-and-Working-Group-Meetings#zephyr-dev-meeting
+
+.. _Architecture Project: https://github.com/zephyrproject-rtos/zephyr/projects/18
+
+.. _Architecture Working Group: https://github.com/zephyrproject-rtos/zephyr/wiki/Architecture-Working-Group
+
+.. _tsc: https://github.com/zephyrproject-rtos/zephyr/labels/tsc
+
+Reviewer Expectations
+#####################
+
+- Be respectful when commenting on PRs. Refer to the Zephyr `Code of Conduct`_
+ for more details.
+
+- The Zephyr Project recognizes that reviewers and maintainers have limited
+ bandwidth. Prioritize review requests in the following order:
+
+ #. PRs related to items in the `Zephyr Release Plan`_.
+ #. PRs that the reviewer has requested blocking changes.
+ #. PRs assigned to the reviewer as the area maintainer.
+ #. All other PRs.
+
+- Try to provide feedback on the entire PR in one shot. This provides the
+ contributor an opportunity to address all comments in the next PR update.
+
+- Partial reviews are permitted, but the reviewer must add a comment indicating
+ what portion of the PR they reviewed. Examples of useful partial reviews
+ include:
+
+ - Domain specific reviews (e.g. Devicetree).
+ - Code style changes that impact the readability of the PR.
+ - Reviewing commits separately when the requested changes cascade into the
+ later commits.
+
+- Avoid increasing scope of the PR by requesting new features, especially when
+ there is a corresponding :ref:`RFC <rfcs>` associated with the PR. Instead,
+ reviewers should add suggestions as a comment to the :ref:`RFC <rfcs>`. This
+ also encourages more collaboration as it is easier for multiple contributors
+ to work on a feature once the minimum implementation has merged.
+
+- When using the "Request Changes" option, mark trivial, non-functional,
+ requests as "Non-blocking" in the comment. Reviewers should approve PRs once
+ only non-blocking changes remain. The PR author has discretion as to whether
+ they address all non-blocking comments. PR authors should acknowledge every
+ review comment in some way, even if it's just with an emoticon.
+
+.. _Code of Conduct: https://github.com/zephyrproject-rtos/zephyr/blob/main/CODE_OF_CONDUCT.md
+
+.. _Zephyr Release Plan: https://github.com/orgs/zephyrproject-rtos/projects/13
diff --git a/doc/contribute/guidelines.rst b/doc/contribute/guidelines.rst
index de0c6bd..e5425a5 100644
--- a/doc/contribute/guidelines.rst
+++ b/doc/contribute/guidelines.rst
@@ -12,6 +12,8 @@
and enhancement requests, and submit patches to the project so your patch will
be accepted quickly in the codebase.
+.. _licensing_requirements:
+
Licensing
*********
@@ -816,25 +818,8 @@
Other Commit Expectations
=========================
-* Commits must build cleanly when applied on top of each other, thus avoiding
- breaking bisectability.
-
-* Commits must pass all CI checks (see `Continuous Integration`_ for more
- information)
-
-* Each commit must address a single identifiable issue and must be
- logically self-contained. Unrelated changes should be submitted as
- separate commits.
-
-* You may submit pull request RFCs (requests for comments) to send work
- proposals, progress snapshots of your work, or to get early feedback on
- features or changes that will affect multiple areas in the code base.
-
-* When major new functionality is added, tests for the new functionality MUST be
- added to the automated test suite. All new APIs MUST be documented and tested
- and tests MUST cover at least 80% of the added functionality using the code
- coverage tool and reporting provided by the project.
-
+See the :ref:`contributor-expectations` for a more complete discussion of
+contributor and reviewer expectations.
Submitting Proposals
====================
diff --git a/doc/contribute/index.rst b/doc/contribute/index.rst
index 36a3e44..f61b59d 100644
--- a/doc/contribute/index.rst
+++ b/doc/contribute/index.rst
@@ -7,6 +7,7 @@
:maxdepth: 1
guidelines.rst
+ contributor_expectations.rst
coding_guidelines/index.rst
documentation/index.rst
external.rst
diff --git a/doc/project/project_roles.rst b/doc/project/project_roles.rst
index cdd6446..4e2eeb8 100644
--- a/doc/project/project_roles.rst
+++ b/doc/project/project_roles.rst
@@ -23,6 +23,8 @@
particular code base areas or subsystems.
+.. _contributor:
+
Contributor
+++++++++++
@@ -108,7 +110,7 @@
of involvement.
* Right to make decisions in the relevant subsystems or areas of involvement,
in conjunction with the collaborators and submitters.
- See :ref:`escalation-process`.
+ See :ref:`pr_technical_escalation`.
* Responsibility to convey the direction of the relevant subsystem or areas to
the TSC
* Responsibility to ensure all contributions of the project have been reviewed
@@ -154,7 +156,9 @@
of the code
* Responsibility to drive the pull request to a mergeable state
* Solicit approvals from maintainers of the subsystems affected
-* Responsibility to drive the escalation process
+* Responsibility to drive the :ref:`pr_technical_escalation` process
+
+.. _release-engineering-team:
Release Engineering Team
++++++++++++++++++++++++
@@ -305,6 +309,8 @@
:align: center
:alt: Release Activity
+.. _merge_criteria:
+
Merge Criteria
++++++++++++++
@@ -341,27 +347,3 @@
* A minimum review period of 2 days, 4 hours for trivial changes (see
:ref:`review_time`). Hotfixes can be merged at any time after CI passes.
* All required checks are passing
-
-.. _escalation-process:
-
-Escalation Process
-++++++++++++++++++
-
-* Contributors may object to change requests or decisions made by
- Maintainers.
-* Process
-
- * Resolve in the PR among assignee, maintainers and reviewer
-
- * Assignee to act as moderator if applicable
-
- * Optionally resolve in the dev review meeting with more Maintainers
- and project stakeholders
-
- * The involved parties and the Assignee to be present when
- the (escalated) issue is discussed
-
- * TSC: Assignees can escalate to the TSC voting members and get
- a binding resolution in the TSC.
- * Assignee to ensure the resolution of the escalation is
- reflected in the PR review.
