[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-dim-tools
Subject: Re: [PATCH 08/11] doc: add common committer guidelines file
From: Jani Nikula <jani.nikula () intel ! com>
Date: 2018-10-30 13:31:48
Message-ID: 87h8h3o917.fsf () intel ! com
[Download RAW message or body]
On Thu, 25 Oct 2018, Rodrigo Vivi <rodrigo.vivi@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@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@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@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@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@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@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@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
_______________________________________________
dim-tools mailing list
dim-tools@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dim-tools
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic