[PATCH 08/11] doc: add common committer guidelines file

Tue Oct 30 13:31:48 UTC 2018

On Thu, 25 Oct 2018, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> But I believe any of my 2 thoughts can be done in follow-up work.
> This patch here looks correct, so:

This is just text movement, changes should be separate.

> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

Thanks,
Jani.

>
>
>
>> +
>> +On Confidence, Complexity, and Transparency
>> +===========================================
>> +
>> +* Reviewed-by/Acked-by/Tested-by must include the name and email of a real
>> +  person for transparency. Anyone can give these, and therefore you have to
>> +  value them according to the merits of the person. Quality matters, not
>> +  quantity. Be suspicious of rubber stamps.
>> +
>> +* Reviewed-by/Acked-by/Tested-by can be asked for and given informally (on the
>> +  list, IRC, in person, in a meeting) but must be added to the commit.
>> +
>> +* Reviewed-by. All patches must be reviewed, no exceptions. Please see
>> +  "Reviewer's statement of oversight" in `Documentation/process/submitting-patches
>> +  <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_
>> +  and `review training
>> +  <http://blog.ffwll.ch/2014/08/review-training-slides.html>`_.
>> +
>> +* Acked-by. Indicates acceptance. No reply is not a sign of acceptance, unless
>> +  you've involved the person (added Cc:, explicitly included in discussion).
>> +
>> +* Tested-by. Please solicit separate Tested-by especially from the bug
>> +  reporters, or people with specific hardware that you or the author do not
>> +  have.
>> +
>> +* There must not be open issues or unresolved or conflicting feedback from
>> +  anyone. Clear them up first. Defer to maintainers as needed.
>> +
>> +* For patches that are simple, isolated, non-functional, not visible to
>> +  userspace, and/or are in author or reviewer's domain of expertise, one
>> +  reviewer is enough. Author with commit access can push after review, or
>> +  reviewer with commit access can push after review.
>> +
>> +* The more complicated the patch gets, the greater the impact, involves ABI,
>> +  touches several areas or platforms, is outside of author's domain of
>> +  expertise, the more eyeballs are needed. For example, several reviewers, or
>> +  separate author, reviewer and committer. Make sure you have experienced
>> +  reviewers. Involve the domain expert(s). Possibly involve people across
>> +  team/group boundaries. Possibly involve the maintainers. Give people more time
>> +  to voice their concerns. Aim for what's described below in more detail as
>> +  "rough consensus".
>> +
>> +* Most patches fall somewhere in between. You have to be the judge, and ensure
>> +  you have involved enough people to feel comfortable if the justification for
>> +  the commit is questioned afterwards.
>> +
>> +On Rough Consensus
>> +==================
>> +
>> +For really big features with a lot of impact on the code, new cross-driver ABI
>> +(like new atomic properties), or features that will take a few patch series (and
>> +maybe a few kernel releases) aim for rough consensus among a wide group of
>> +stakeholders. There's three components for that:
>> +
>> +* Identify stakeholders beyond just the patch author and reviewers. This
>> +  includes contributors with experience in code ancillary to your feature,
>> +  domain experts, maybe maintainers. Also include maintainers and reviewers of
>> +  the userspace component for new ABI, which often means non-Intel people. In
>> +  case of doubt ask maintainers for a reasonable list of people. Make sure you
>> +  gather their input actively, don't expect them to deliver it on their own -
>> +  most are really busy.
>> +
>> +* Have agreement among all these stakeholders what the code should look like in
>> +  the end. Generally disagreement on the end state is considered a blocker, and
>> +  overruling such disagreement by maintainers is done only in exceptional cases.
>> +  Generally what happens is that maintainers instead do all the work of
>> +  soliciting input, which doesn't scale and should be the patch author's and
>> +  reviewer's duty. Try to document this consensus somewhere, either in a summary
>> +  email, interface patch to describe the data structures and vfuncs. Or maybe
>> +  even a more formal design spec, although in the past that didn't work out when
>> +  non-Intel people are involved, and the actually merged interface differed from
>> +  the spec a lot. Just assumed consensus on IRC tends to result in
>> +  misunderstandings.
>> +
>> +* Have a concrete plan how to get to the agreed end state in the codebase. Often
>> +  it makes sense to have an intermediate patch set merged first, and then polish
>> +  it in tree. Or merge new ABI in stages. Usually this means that a new feature
>> +  or ABI is disabled by default at first. For the actual plan some unhappiness
>> +  from stakeholders about the execution is acceptable, as long as the overall
>> +  stability and other ongoing work isn't negatively affected. As a rule of thumb
>> +  new ABI or features should never be in a partial/limbo stage for more than 2-3
>> +  kernel releases. If that seems unlikely, more work should happen pre-merge.
>> +
>> +
>> +Try to reach rough consensus before spending months writing code you might need
>> +to throw away or at least entirely rewrite again. Also make sure that all
>> +discussions happen in public forums, and make sure there's a searchable
>> +permanent record of any discussions for later reference. This means that for
>> +most things internal meetings are not the most suitable venue.
>> +
>> +Continuous Integration and Pre-Merge Testing
>> +============================================
>> +
>> +The requirements for CI_ pre-merge testing are:
>> +
>> +* ``checkpatch.pl`` does not complain. (Some of the more subjective warnings may
>> +  be ignored at the committer's discretion.)
>> +
>> +* The patch does not introduce new ``sparse`` warnings.
>> +
>> +* Patch series must pass IGT Basic Acceptance Tests (BAT) on all the CI machines
>> +  without causing regressions.
>> +
>> +* Patch series must pass full IGT tests on CI shard machines without causing
>> +  regressions.
>> +
>> +* Patch series must pass GPU piglit tests on all CI machines without causing
>> +  regressions.
>> +
>> +The CI bots will send results to the patch author and intel-gfx for any patches
>> +tracked by patchwork. The results are also available on patchwork_ and the CI_
>> +site.
>> +
>> +Check CI failures and make sure any sporadic failures are a) pre-existing,
>> +and b) tracked in bugzilla. If there's anything dubious that you can't track
>> +down to pre-existing and tracked issues please don't push, but instead figure
>> +out what's going on.
>> +
>> +.. _CI: https://intel-gfx-ci.01.org/
>> +
>> +.. _patchwork: https://patchwork.freedesktop.org/project/intel-gfx/series/
>> +
>> +Labeling Fixes Before Pushing
>> +=============================
>> +
>> +To label fixes that should be cherry-picked to the current -rc development
>> +kernel or drm-next, the commit message should have the 'Fixes:' tag.
>> +
>> +If the fix is relevant for a released kernel please also
>> +
>> +	Cc: stable at vger.kernel.org
>> +
>> +'Fixes:' tag needs to added to any patch fixing a regression or an incorrect
>> +behavior from previous patches. This tag help maintainers and tools to decide
>> +where to cherry-pick a patch to. This also extremely useful for
>> +product groups to know which bugfixes they must include. To follow the
>> +recommended format please generate the Fixes: line using ::
>> +
>> +        $ dim fixes $regressing_commit
>> +
>> +This will also add the correct Cc: line if one is needed.
>> +
>> +If the Cc: or Fixes: was forgotten, you can still reply to the list with that,
>> +just like any other tags, and they should be picked up by whoever pushes the
>> +patch.
>> +
>> +The maintainers will cherry-pick labeled patches from drm-intel-next-queued to
>> +the appropriate branches.
>> +
>> +'Fixes:' tag is described in
>> +`Documentation/process/submitting-patches
>> +<https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_.
>> +
>> +Requesting Fixes Cherry-Pick Afterwards
>> +=======================================
>> +
>> +It's not uncommon for a patch to have been committed before it's identified as a
>> +fix needing to be backported.
>> +
>> +If the patch is already in Linus' tree, please follow `stable kernel rules
>> +<https://01.org/linuxgraphics/gfx-docs/drm/process/stable-kernel-rules.html>`_.
>> +
>> +Otherwise, send an email to intel-gfx at lists.freedesktop.org containing the subject of the patch, the commit id, why you think it should be applied, and what branch you wish it to be applied to.
>> +
>> +Replying to the original patch is also fine, but please mention the commit id.
>> +
>> +Alternatively, if the cherry-pick has conflicts, please send a patch to
>> +intel-gfx at lists.freedesktop.org with
>> +subject prefix "drm-intel-fixes PATCH" or "drm-intel-next-fixes PATCH" depending
>> +on the branch. Please add 'git cherry-pick -x' style annotation above your
>> +Signed-off-by: line in the commit message:
>> +
>> +	(cherry picked from commit 0bff4858653312a10c83709e0009c3adb87e6f1e)
>> +
>> +Tooling
>> +=======
>> +
>> +drm-intel git repositories are managed with dim_.
>> +
>> +.. _dim: dim.html
>> diff --git a/committer-guidelines.rst b/committer-guidelines.rst
>> new file mode 100644
>> index 000000000000..c0ed6d942aa7
>> --- /dev/null
>> +++ b/committer-guidelines.rst
>> @@ -0,0 +1,12 @@
>> +.. _committer-guidelines:
>> +
>> +======================
>> + Committer Guidelines
>> +======================
>> +
>> +This document gathers together committer guidelines.
>> +
>> +.. toctree::
>> +   :maxdepth: 2
>> +
>> +   committer-drm-intel
>> diff --git a/drm-intel.rst b/drm-intel.rst
>> index 087402cf985d..4587396452f2 100644
>> --- a/drm-intel.rst
>> +++ b/drm-intel.rst
>> @@ -44,7 +44,7 @@ Features
>>  --------
>>  
>>  Features are picked up and pushed to drm-intel-next-queued by committers and
>> -maintainers. See committer guidelines below for details.
>> +maintainers. See :ref:`committer-drm-intel` for details.
>>  
>>  Fixes
>>  -----
>> @@ -55,61 +55,9 @@ a timely manner. Fixes that are relevant for stable, current development
>>  kernels, or drm-next, will be cherry-picked by maintainers to drm-intel-fixes or
>>  drm-intel-next-fixes.
>>  
>> -To make this work, patches should be labeled as fixes (see below), and extra
>> -care should be put into making fixes the first patches in series, not depending
>> -on preparatory work or cleanup.
>> -
>> -Labeling Fixes Before Pushing
>> -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> -
>> -To label fixes that should be cherry-picked to the current -rc development
>> -kernel or drm-next, the commit message should have the 'Fixes:' tag.
>> -
>> -If the fix is relevant for a released kernel please also
>> -
>> -	Cc: stable at vger.kernel.org
>> -
>> -'Fixes:' tag needs to added to any patch fixing a regression or an incorrect
>> -behavior from previous patches. This tag help maintainers and tools to decide
>> -where to cherry-pick a patch to. This also extremely useful for
>> -product groups to know which bugfixes they must include. To follow the
>> -recommended format please generate the Fixes: line using ::
>> -
>> -        $ dim fixes $regressing_commit
>> -
>> -This will also add the correct Cc: line if one is needed.
>> -
>> -If the Cc: or Fixes: was forgotten, you can still reply to the list with that,
>> -just like any other tags, and they should be picked up by whoever pushes the
>> -patch.
>> -
>> -The maintainers will cherry-pick labeled patches from drm-intel-next-queued to
>> -the appropriate branches.
>> -
>> -'Fixes:' tag is described in
>> -`Documentation/process/submitting-patches
>> -<https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_.
>> -
>> -Requesting Fixes Cherry-Pick Afterwards
>> -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> -
>> -It's not uncommon for a patch to have been committed before it's identified as a
>> -fix needing to be backported.
>> -
>> -If the patch is already in Linus' tree, please follow `stable kernel rules
>> -<https://01.org/linuxgraphics/gfx-docs/drm/process/stable-kernel-rules.html>`_.
>> -
>> -Otherwise, send an email to intel-gfx at lists.freedesktop.org containing the subject of the patch, the commit id, why you think it should be applied, and what branch you wish it to be applied to.
>> -
>> -Replying to the original patch is also fine, but please mention the commit id.
>> -
>> -Alternatively, if the cherry-pick has conflicts, please send a patch to
>> -intel-gfx at lists.freedesktop.org with
>> -subject prefix "drm-intel-fixes PATCH" or "drm-intel-next-fixes PATCH" depending
>> -on the branch. Please add 'git cherry-pick -x' style annotation above your
>> -Signed-off-by: line in the commit message:
>> -
>> -	(cherry picked from commit 0bff4858653312a10c83709e0009c3adb87e6f1e)
>> +To make this work, patches should be labeled as fixes (see XXX), and extra care
>> +should be put into making fixes the first patches in series, not depending on
>> +preparatory work or cleanup.
>>  
>>  Merge Timeline
>>  ==============
>> @@ -123,213 +71,3 @@ release. There are no fast paths.
>>  
>>  For predictions on the future merge windows and releases, see
>>  http://phb-crystal-ball.org/.
>> -
>> -Committer Guidelines
>> -====================
>> -
>> -This section describes the guidelines for pushing patches. Strict rules covering
>> -all cases are impossible to write and follow. We put a lot of trust in the sound
>> -judgement of the people with commit access, and that only develops with
>> -experience. These guidelines are primarily for the committers to aid in making
>> -the right decisions, and for others to set their the expectations right.
>> -
>> -The short list:
>> -
>> -* Only push patches changing `drivers/gpu/drm/i915`.
>> -
>> -* Only push patches to `drm-intel-next-queued` branch.
>> -
>> -* Ensure certain details are covered, see separate list below.
>> -
>> -* You have confidence in the patches you push, proportionate to the complexity
>> -  and impact they have. The confidence must be explicitly documented with
>> -  special tags (Reviewed-by, Acked-by, Tested-by, Bugzilla, etc.) in the commit
>> -  message. This is also your insurance, and you want to have it when the commit
>> -  comes back haunting you. The complexity and impact are properties of the patch
>> -  that must be justified in the commit message.
>> -
>> -* Last but not least, especially when getting started, you can't go wrong with
>> -  asking or deferring to the maintainers. If you don't feel comfortable pushing
>> -  a patch for any reason (technical concerns, unresolved or conflicting
>> -  feedback, management or peer pressure, or anything really) it's best to defer
>> -  to the maintainers. This is what the maintainers are there for.
>> -
>> -* After pushing, please reply to the list that you've done so.
>> -
>> -Detail Check List
>> ------------------
>> -
>> -An inexhaustive list of details to check:
>> -
>> -* The patch conforms to `Documentation/process/submitting-patches
>> -  <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_
>> -
>> -* The commit message is sensible, and includes adequate details and references.
>> -
>> -* Bug fixes are clearly indicated as such.
>> -
>> -* IGT test cases, if applicable, must be complete and reviewed. Please see
>> -  `details on testing requirements
>> -  <http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html>`_.
>> -
>> -* The patch series has passed CI pre-merge testing. See CI details below.
>> -
>> -* An open source userspace, reviewed and ready for merging by the upstream
>> -  project, must be available for new kernel ABI. Please see `details on
>> -  upstreaming requirements
>> -  <http://blog.ffwll.ch/2015/05/gfx-kernel-upstreaming-requirements.html>`_.
>> -
>> -* Relevant documentation must be updated as part of the patch or series.
>> -
>> -* Patch series builds and is expected to boot every step of the way, i.e. is
>> -  bisectable.
>> -
>> -* Each patch is of a sensible size. A good rule of thumb metric is, would you
>> -  (or the author) stand a chance to fairly quickly understand what goes wrong if
>> -  the commit is reported to cause a regression?
>> -
>> -* When pushing someone else's patch you must add your own signed-off per
>> -  http://developercertificate.org/. dim apply-branch should do this
>> -  automatically for you.
>> -
>> -* For patches that move around lots of code (file rename or extraction) please
>> -  coordinate with maintainers to avoid unnecessary pain with conflicts. Usually
>> -  some explicit merges are needed to avoid git getting lost.
>> -
>> -* As a general rule, do not modify the patches while applying, apart from the
>> -  commit message. If the patch conflicts, or needs to be changed due to review,
>> -  have the author rebase, update and resend. Any change at this stage is a
>> -  potential issue bypassing CI.
>> -
>> -  At most, minor comment and whitespace tweaks are acceptable.
>> -
>> -* Please notify maintainers (IRC is fine) of new merge conflicts during drm-tip
>> -  rebuild, so that they can do backmerges as needed.
>> -
>> -On Confidence, Complexity, and Transparency
>> --------------------------------------------
>> -
>> -* Reviewed-by/Acked-by/Tested-by must include the name and email of a real
>> -  person for transparency. Anyone can give these, and therefore you have to
>> -  value them according to the merits of the person. Quality matters, not
>> -  quantity. Be suspicious of rubber stamps.
>> -
>> -* Reviewed-by/Acked-by/Tested-by can be asked for and given informally (on the
>> -  list, IRC, in person, in a meeting) but must be added to the commit.
>> -
>> -* Reviewed-by. All patches must be reviewed, no exceptions. Please see
>> -  "Reviewer's statement of oversight" in `Documentation/process/submitting-patches
>> -  <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_
>> -  and `review training
>> -  <http://blog.ffwll.ch/2014/08/review-training-slides.html>`_.
>> -
>> -* Acked-by. Indicates acceptance. No reply is not a sign of acceptance, unless
>> -  you've involved the person (added Cc:, explicitly included in discussion).
>> -
>> -* Tested-by. Please solicit separate Tested-by especially from the bug
>> -  reporters, or people with specific hardware that you or the author do not
>> -  have.
>> -
>> -* There must not be open issues or unresolved or conflicting feedback from
>> -  anyone. Clear them up first. Defer to maintainers as needed.
>> -
>> -* For patches that are simple, isolated, non-functional, not visible to
>> -  userspace, and/or are in author or reviewer's domain of expertise, one
>> -  reviewer is enough. Author with commit access can push after review, or
>> -  reviewer with commit access can push after review.
>> -
>> -* The more complicated the patch gets, the greater the impact, involves ABI,
>> -  touches several areas or platforms, is outside of author's domain of
>> -  expertise, the more eyeballs are needed. For example, several reviewers, or
>> -  separate author, reviewer and committer. Make sure you have experienced
>> -  reviewers. Involve the domain expert(s). Possibly involve people across
>> -  team/group boundaries. Possibly involve the maintainers. Give people more time
>> -  to voice their concerns. Aim for what's described below in more detail as
>> -  "rough consensus".
>> -
>> -* Most patches fall somewhere in between. You have to be the judge, and ensure
>> -  you have involved enough people to feel comfortable if the justification for
>> -  the commit is questioned afterwards.
>> -
>> -On Rough Consensus
>> -------------------
>> -
>> -For really big features with a lot of impact on the code, new cross-driver ABI
>> -(like new atomic properties), or features that will take a few patch series (and
>> -maybe a few kernel releases) aim for rough consensus among a wide group of
>> -stakeholders. There's three components for that:
>> -
>> -* Identify stakeholders beyond just the patch author and reviewers. This
>> -  includes contributors with experience in code ancillary to your feature,
>> -  domain experts, maybe maintainers. Also include maintainers and reviewers of
>> -  the userspace component for new ABI, which often means non-Intel people. In
>> -  case of doubt ask maintainers for a reasonable list of people. Make sure you
>> -  gather their input actively, don't expect them to deliver it on their own -
>> -  most are really busy.
>> -
>> -* Have agreement among all these stakeholders what the code should look like in
>> -  the end. Generally disagreement on the end state is considered a blocker, and
>> -  overruling such disagreement by maintainers is done only in exceptional cases.
>> -  Generally what happens is that maintainers instead do all the work of
>> -  soliciting input, which doesn't scale and should be the patch author's and
>> -  reviewer's duty. Try to document this consensus somewhere, either in a summary
>> -  email, interface patch to describe the data structures and vfuncs. Or maybe
>> -  even a more formal design spec, although in the past that didn't work out when
>> -  non-Intel people are involved, and the actually merged interface differed from
>> -  the spec a lot. Just assumed consensus on IRC tends to result in
>> -  misunderstandings.
>> -
>> -* Have a concrete plan how to get to the agreed end state in the codebase. Often
>> -  it makes sense to have an intermediate patch set merged first, and then polish
>> -  it in tree. Or merge new ABI in stages. Usually this means that a new feature
>> -  or ABI is disabled by default at first. For the actual plan some unhappiness
>> -  from stakeholders about the execution is acceptable, as long as the overall
>> -  stability and other ongoing work isn't negatively affected. As a rule of thumb
>> -  new ABI or features should never be in a partial/limbo stage for more than 2-3
>> -  kernel releases. If that seems unlikely, more work should happen pre-merge.
>> -
>> -
>> -Try to reach rough consensus before spending months writing code you might need
>> -to throw away or at least entirely rewrite again. Also make sure that all
>> -discussions happen in public forums, and make sure there's a searchable
>> -permanent record of any discussions for later reference. This means that for
>> -most things internal meetings are not the most suitable venue.
>> -
>> -Continuous Integration and Pre-Merge Testing
>> ---------------------------------------------
>> -
>> -The requirements for CI_ pre-merge testing are:
>> -
>> -* ``checkpatch.pl`` does not complain. (Some of the more subjective warnings may
>> -  be ignored at the committer's discretion.)
>> -
>> -* The patch does not introduce new ``sparse`` warnings.
>> -
>> -* Patch series must pass IGT Basic Acceptance Tests (BAT) on all the CI machines
>> -  without causing regressions.
>> -
>> -* Patch series must pass full IGT tests on CI shard machines without causing
>> -  regressions.
>> -
>> -* Patch series must pass GPU piglit tests on all CI machines without causing
>> -  regressions.
>> -
>> -The CI bots will send results to the patch author and intel-gfx for any patches
>> -tracked by patchwork. The results are also available on patchwork_ and the CI_
>> -site.
>> -
>> -Check CI failures and make sure any sporadic failures are a) pre-existing,
>> -and b) tracked in bugzilla. If there's anything dubious that you can't track
>> -down to pre-existing and tracked issues please don't push, but instead figure
>> -out what's going on.
>> -
>> -.. _CI: https://intel-gfx-ci.01.org/
>> -
>> -.. _patchwork: https://patchwork.freedesktop.org/project/intel-gfx/series/
>> -
>> -Tooling
>> -=======
>> -
>> -drm-intel git repositories are managed with dim_:
>> -
>> -.. _dim: dim.html
>> diff --git a/index.rst b/index.rst
>> index 2a73f5de5bed..88b49edbeece 100644
>> --- a/index.rst
>> +++ b/index.rst
>> @@ -26,6 +26,7 @@ Contents:
>>     drm-tip
>>     drm-misc
>>     drm-intel
>> +   committer-guidelines
>>     commit-access
>>     getting-started
>>     dim
>> -- 
>> 2.11.0
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center