[RFC PATCH 0/4] docs/devel suggestions for discussion

[RFC PATCH 0/4] docs/devel suggestions for discussion

[Alex Bennée]

Hi,

This is an attempt to improve our processes documentation by:

 - adding an explicit section on maintainers
 - reducing the up-front verbiage in patch submission
 - emphasising the importance to respectful reviews

I'm sure the language could be improved further so I humbly submit
this RFC for discussion.

Alex Bennée (4):
  docs/devel: add a maintainers section to development process
  docs/devel: make language a little less code centric
  docs/devel: simplify the minimal checklist
  docs/devel: try and improve the language around patch review

 docs/devel/code-of-conduct.rst           |   2 +
 docs/devel/index-process.rst             |   1 +
 docs/devel/maintainers.rst               |  84 +++++++++++++++++++
 docs/devel/submitting-a-patch.rst        | 101 +++++++++++++++--------
 docs/devel/submitting-a-pull-request.rst |  12 +--
 roms/qboot                               |   2 +-
 6 files changed, 157 insertions(+), 45 deletions(-)
 create mode 100644 docs/devel/maintainers.rst

-- 
2.34.1

[RFC PATCH 1/4] docs/devel: add a maintainers section to development process

[Alex Bennée]

We don't currently have a clear place in the documentation to describe
the rolls and responsibilities of a maintainer. Lets create one so we
can. I've moved a few small bits out of other files to try and keep
everything in one place.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/code-of-conduct.rst           |  2 +
 docs/devel/index-process.rst             |  1 +
 docs/devel/maintainers.rst               | 84 ++++++++++++++++++++++++
 docs/devel/submitting-a-pull-request.rst | 12 ++--
 4 files changed, 91 insertions(+), 8 deletions(-)
 create mode 100644 docs/devel/maintainers.rst

diff --git a/docs/devel/code-of-conduct.rst b/docs/devel/code-of-conduct.rst
index 195444d1b4..f734ed0317 100644
--- a/docs/devel/code-of-conduct.rst
+++ b/docs/devel/code-of-conduct.rst
@@ -1,3 +1,5 @@
+.. _code_of_conduct:
+
 Code of Conduct
 ===============
 
diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst
index d0d7a200fd..d50dd74c3e 100644
--- a/docs/devel/index-process.rst
+++ b/docs/devel/index-process.rst
@@ -8,6 +8,7 @@ Notes about how to interact with the community and how and where to submit patch
 
    code-of-conduct
    conflict-resolution
+   maintainers
    style
    submitting-a-patch
    trivial-patches
diff --git a/docs/devel/maintainers.rst b/docs/devel/maintainers.rst
new file mode 100644
index 0000000000..e3c7003bfa
--- /dev/null
+++ b/docs/devel/maintainers.rst
@@ -0,0 +1,84 @@
+.. _maintainers:
+
+The Roll of Maintainers
+=======================
+
+Maintainers are a critical part of the projects contributor ecosystem.
+They come from a wide range of backgrounds from unpaid hobbyists
+working in their spare time to employees who work on the project as
+part of their job. Maintainer activities include:
+
+  - reviewing patches and suggesting changes
+  - preparing pull requests for their subsystems
+  - participating other project activities
+
+They are also human and subject to the same pressures as everyone else
+including overload and burn out. Like everyone else they are subject
+to projects :ref:`code_of_conduct`.
+
+The MAINTAINERS file
+--------------------
+
+The `MAINTAINERS
+<https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS>`__
+file contains the canonical list of who is a maintainer. The file
+is machine readable so an appropriately configured git (see
+:ref:`cc_the_relevant_maintainer`) can automatically Cc them on
+patches that touch their area of code.
+
+The file also describes the status of the area of code to give an idea
+of how actively that section is maintained.
+
+.. list-table:: Meaning of support status in MAINTAINERS
+   :widths: 25 75
+   :header-rows: 1
+
+   * - Status
+     - Meaning
+   * - Supported
+     - Someone is actually paid to look after this.
+   * - Maintained
+     - Someone actually looks after it.
+   * - Odd Fixes
+     - It has a maintainer but they don't have time to do
+       much other than throw the odd patch in.
+   * - Orphan
+     - No current maintainer.
+   * - Obsolete
+     - Old obsolete code, should use something else.
+
+Please bare in mind that even if someone is paid to support something
+it does not mean they are paid to support you. This is open source and
+the code comes with no warranty and the project makes no guarantees
+about dealing with bugs or features requests.
+
+Becoming a maintainer
+---------------------
+
+Maintainers are volunteers who put themselves forward to keep an eye
+on an area of code. They are generally accepted by the community to
+have a good understanding of the subsystem and able to make a positive
+contribution to the project.
+
+The process is simple - simply sent a patch to the list that updates
+the ``MAINTAINERS`` file. Sometimes this is done as part of a larger
+series when a new sub-system is being added to the code base. This can
+also be done by a retiring maintainer who nominates their replacement
+after discussion with other contributors.
+
+Once the patch is reviewed and merged the only other step is to make
+sure your GPG key is signed.
+
+.. _maintainer_keys:
+
+Maintainer GPG Keys
+~~~~~~~~~~~~~~~~~~~
+
+GPG is used to sign pull requests so they can be identified as really
+coming from the maintainer. If your key is not already signed by
+members of the QEMU community, you should make arrangements to attend
+a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
+example at KVM Forum) or make alternative arrangements to have your
+key signed by an attendee. Key signing requires meeting another
+community member **in person** so please make appropriate
+arrangements.
diff --git a/docs/devel/submitting-a-pull-request.rst b/docs/devel/submitting-a-pull-request.rst
index c9d1e8afd9..a4cd7ebbb6 100644
--- a/docs/devel/submitting-a-pull-request.rst
+++ b/docs/devel/submitting-a-pull-request.rst
@@ -53,14 +53,10 @@ series) and that "make check" passes before sending out the pull
 request. As a submaintainer you're one of QEMU's lines of defense
 against bad code, so double check the details.
 
-**All pull requests must be signed**. If your key is not already signed
-by members of the QEMU community, you should make arrangements to attend
-a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
-example at KVM Forum) or make alternative arrangements to have your key
-signed by an attendee.  Key signing requires meeting another community
-member \*in person\* so please make appropriate arrangements.  By
-"signed" here we mean that the pullreq email should quote a tag which is
-a GPG-signed tag (as created with 'gpg tag -s ...').
+**All pull requests must be signed**. By "signed" here we mean that
+the pullreq email should quote a tag which is a GPG-signed tag (as
+created with 'gpg tag -s ...'). See :ref:`maintainer_keys` for
+details.
 
 **Pull requests not for master should say "not for master" and have
 "PULL SUBSYSTEM whatever" in the subject tag**. If your pull request is
-- 
2.34.1

[RFC PATCH 2/4] docs/devel: make language a little less code centric

[Alex Bennée]

We welcome all sorts of patches.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/submitting-a-patch.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
index fec33ce148..fb1673e974 100644
--- a/docs/devel/submitting-a-patch.rst
+++ b/docs/devel/submitting-a-patch.rst
@@ -3,11 +3,11 @@
 Submitting a Patch
 ==================
 
-QEMU welcomes contributions of code (either fixing bugs or adding new
-functionality). However, we get a lot of patches, and so we have some
-guidelines about submitting patches. If you follow these, you'll help
-make our task of code review easier and your patch is likely to be
-committed faster.
+QEMU welcomes contributions to fix bugs, add functionality or improve
+the documentation. However, we get a lot of patches, and so we have
+some guidelines about submitting patches. If you follow these, you'll
+help make our task of code review easier and your patch is likely to
+be committed faster.
 
 This page seems very long, so if you are only trying to post a quick
 one-shot fix, the bare minimum we ask is that:
-- 
2.34.1

[RFC PATCH 3/4] docs/devel: simplify the minimal checklist

[Alex Bennée]

The bullet points are quite long and contain process tips. Move those
bits of the bullet to the relevant sections and link to them. Use a
table for nicer formatting of the checklist.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
 roms/qboot                        |  2 +-
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
index fb1673e974..41771501bf 100644
--- a/docs/devel/submitting-a-patch.rst
+++ b/docs/devel/submitting-a-patch.rst
@@ -12,25 +12,18 @@ be committed faster.
 This page seems very long, so if you are only trying to post a quick
 one-shot fix, the bare minimum we ask is that:
 
--  You **must** provide a Signed-off-by: line (this is a hard
-   requirement because it's how you say "I'm legally okay to contribute
-   this and happy for it to go into QEMU", modeled after the `Linux kernel
-   <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
-   policy.) ``git commit -s`` or ``git format-patch -s`` will add one.
--  All contributions to QEMU must be **sent as patches** to the
-   qemu-devel `mailing list <https://wiki.qemu.org/Contribute/MailingLists>`__.
-   Patch contributions should not be posted on the bug tracker, posted on
-   forums, or externally hosted and linked to. (We have other mailing lists too,
-   but all patches must go to qemu-devel, possibly with a Cc: to another
-   list.) ``git send-email`` (`step-by-step setup
-   guide <https://git-send-email.io/>`__ and `hints and
-   tips <https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
-   works best for delivering the patch without mangling it, but
-   attachments can be used as a last resort on a first-time submission.
--  You must read replies to your message, and be willing to act on them.
-   Note, however, that maintainers are often willing to manually fix up
-   first-time contributions, since there is a learning curve involved in
-   making an ideal patch submission.
+.. list-table:: Minimal Checklist for Patches
+   :widths: 35 65
+   :header-rows: 1
+
+   * - Check
+     - Reason
+   * - Patches contain Signed-off-by: Author Name <author@email>
+     - States you are legally able to contribute the code. See :ref:`patch_emails_must_include_a_signed_off_by_line`
+   * - Sent as patch emails to ``qemu-devel@nongnu.org``
+     - The project uses an email list based workflow. See :ref:`submitting_your_patches`
+   * - Be prepared to respond to review comments
+     - Code that doesn't pass review will not get merged. See :ref:`participating_in_code_review`
 
 You do not have to subscribe to post (list policy is to reply-to-all to
 preserve CCs and keep non-subscribers in the loop on the threads they
@@ -229,6 +222,19 @@ bisection doesn't land on a known-broken state.
 Submitting your Patches
 -----------------------
 
+The QEMU project uses a public email based workflow for reviewing and
+merging patches. As a result all contributions to QEMU must be **sent
+as patches** to the qemu-devel `mailing list
+<https://wiki.qemu.org/Contribute/MailingLists>`__. Patch
+contributions should not be posted on the bug tracker, posted on
+forums, or externally hosted and linked to. (We have other mailing
+lists too, but all patches must go to qemu-devel, possibly with a Cc:
+to another list.) ``git send-email`` (`step-by-step setup guide
+<https://git-send-email.io/>`__ and `hints and tips
+<https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
+works best for delivering the patch without mangling it, but
+attachments can be used as a last resort on a first-time submission.
+
 .. _if_you_cannot_send_patch_emails:
 
 If you cannot send patch emails
@@ -314,10 +320,12 @@ git repository to fetch the original commit.
 Patch emails must include a ``Signed-off-by:`` line
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-For more information see `SubmittingPatches 1.12
-<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
-This is vital or we will not be able to apply your patch! Please use
-your real name to sign a patch (not an alias or acronym).
+Your patches **must** include a Signed-off-by: line. This is a hard
+requirement because it's how you say "I'm legally okay to contribute
+this and happy for it to go into QEMU". The process is modelled after
+the `Linux kernel
+<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
+policy.
 
 If you wrote the patch, make sure your "From:" and "Signed-off-by:"
 lines use the same spelling. It's okay if you subscribe or contribute to
@@ -327,6 +335,11 @@ include a "From:" line in the body of the email (different from your
 envelope From:) that will give credit to the correct author; but again,
 that author's Signed-off-by: line is mandatory, with the same spelling.
 
+There are various tooling options for automatically adding these tags
+include using ``git commit -s`` or ``git format-patch -s``. For more
+information see `SubmittingPatches 1.12
+<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
+
 .. _include_a_meaningful_cover_letter:
 
 Include a meaningful cover letter
@@ -397,9 +410,19 @@ Participating in Code Review
 ----------------------------
 
 All patches submitted to the QEMU project go through a code review
-process before they are accepted. Some areas of code that are well
-maintained may review patches quickly, lesser-loved areas of code may
-have a longer delay.
+process before they are accepted. This will often mean a series will
+go through a number of iterations before being picked up by
+:ref:`maintainers<maintainers>`. You therefor should be prepared to
+read replies to your messages and be willing to act on them.
+
+Maintainers are often willing to manually fix up first-time
+contributions, since there is a learning curve involved in making an
+ideal patch submission. However for the best results you should
+proactively respond to suggestions with changes or justifications for
+your current approach.
+
+Some areas of code that are well maintained may review patches
+quickly, lesser-loved areas of code may have a longer delay.
 
 .. _stay_around_to_fix_problems_raised_in_code_review:
 
diff --git a/roms/qboot b/roms/qboot
index 8ca302e86d..a5300c4949 160000
--- a/roms/qboot
+++ b/roms/qboot
@@ -1 +1 @@
-Subproject commit 8ca302e86d685fa05b16e2b208888243da319941
+Subproject commit a5300c4949b8d4de2d34bedfaed66793f48ec948
-- 
2.34.1

[RFC PATCH 4/4] docs/devel: try and improve the language around patch review

[Alex Bennée]

It is important that contributors take the review process seriously
and we collaborate in a respectful way while avoiding personal
attacks. Try and make this clear in the language.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/submitting-a-patch.rst | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
index 41771501bf..5b6dc7ddf1 100644
--- a/docs/devel/submitting-a-patch.rst
+++ b/docs/devel/submitting-a-patch.rst
@@ -434,14 +434,20 @@ developers will identify bugs, or suggest a cleaner approach, or even
 just point out code style issues or commit message typos. You'll need to
 respond to these, and then send a second version of your patches with
 the issues fixed. This takes a little time and effort on your part, but
-if you don't do it then your changes will never get into QEMU. It's also
-just polite -- it is quite disheartening for a developer to spend time
-reviewing your code and suggesting improvements, only to find that
-you're not going to do anything further and it was all wasted effort.
+if you don't do it then your changes will never get into QEMU.
+
+Remember that a maintainer is under no obligation to take your
+patches. If someone has spent the time reviewing your code and
+suggesting improvements and you simply re-post without either
+addressing the comment directly or providing additional justification
+for the change then it becomes wasted effort. You cannot demand others
+merge and then fix up your code after the fact.
 
 When replying to comments on your patches **reply to all and not just
 the sender** -- keeping discussion on the mailing list means everybody
-can follow it.
+can follow it. Remember the spirit of the :ref:`code_of_conduct` and
+keep discussions respectful and collaborative and avoid making
+personal comments.
 
 .. _pay_attention_to_review_comments:
 
-- 
2.34.1

Re: [RFC PATCH 3/4] docs/devel: simplify the minimal checklist

[Daniel P. Berrangé]

On Wed, Oct 12, 2022 at 01:11:51PM +0100, Alex Bennée wrote:
> The bullet points are quite long and contain process tips. Move those
> bits of the bullet to the relevant sections and link to them. Use a
> table for nicer formatting of the checklist.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
>  roms/qboot                        |  2 +-

git submodule surprise !


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 1/4] docs/devel: add a maintainers section to development process

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 7093 bytes --]

On Wed, Oct 12, 2022 at 01:11:49PM +0100, Alex Bennée wrote:
> We don't currently have a clear place in the documentation to describe
> the rolls and responsibilities of a maintainer. Lets create one so we

s/roll/role/ in the commit description and the patch.

> can. I've moved a few small bits out of other files to try and keep
> everything in one place.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/code-of-conduct.rst           |  2 +
>  docs/devel/index-process.rst             |  1 +
>  docs/devel/maintainers.rst               | 84 ++++++++++++++++++++++++
>  docs/devel/submitting-a-pull-request.rst | 12 ++--
>  4 files changed, 91 insertions(+), 8 deletions(-)
>  create mode 100644 docs/devel/maintainers.rst
> 
> diff --git a/docs/devel/code-of-conduct.rst b/docs/devel/code-of-conduct.rst
> index 195444d1b4..f734ed0317 100644
> --- a/docs/devel/code-of-conduct.rst
> +++ b/docs/devel/code-of-conduct.rst
> @@ -1,3 +1,5 @@
> +.. _code_of_conduct:
> +
>  Code of Conduct
>  ===============
>  
> diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst
> index d0d7a200fd..d50dd74c3e 100644
> --- a/docs/devel/index-process.rst
> +++ b/docs/devel/index-process.rst
> @@ -8,6 +8,7 @@ Notes about how to interact with the community and how and where to submit patch
>  
>     code-of-conduct
>     conflict-resolution
> +   maintainers
>     style
>     submitting-a-patch
>     trivial-patches
> diff --git a/docs/devel/maintainers.rst b/docs/devel/maintainers.rst
> new file mode 100644
> index 0000000000..e3c7003bfa
> --- /dev/null
> +++ b/docs/devel/maintainers.rst
> @@ -0,0 +1,84 @@
> +.. _maintainers:
> +
> +The Roll of Maintainers
> +=======================
> +
> +Maintainers are a critical part of the projects contributor ecosystem.

project's

> +They come from a wide range of backgrounds from unpaid hobbyists
> +working in their spare time to employees who work on the project as
> +part of their job. Maintainer activities include:
> +
> +  - reviewing patches and suggesting changes
> +  - preparing pull requests for their subsystems
> +  - participating other project activities
> +
> +They are also human and subject to the same pressures as everyone else
> +including overload and burn out. Like everyone else they are subject
> +to projects :ref:`code_of_conduct`.

to the project's

(Although "project's" can be dropped without changing the meaning.)

> +
> +The MAINTAINERS file
> +--------------------
> +
> +The `MAINTAINERS
> +<https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS>`__
> +file contains the canonical list of who is a maintainer. The file
> +is machine readable so an appropriately configured git (see
> +:ref:`cc_the_relevant_maintainer`) can automatically Cc them on
> +patches that touch their area of code.
> +
> +The file also describes the status of the area of code to give an idea
> +of how actively that section is maintained.
> +
> +.. list-table:: Meaning of support status in MAINTAINERS
> +   :widths: 25 75
> +   :header-rows: 1
> +
> +   * - Status
> +     - Meaning
> +   * - Supported
> +     - Someone is actually paid to look after this.
> +   * - Maintained
> +     - Someone actually looks after it.
> +   * - Odd Fixes
> +     - It has a maintainer but they don't have time to do
> +       much other than throw the odd patch in.
> +   * - Orphan
> +     - No current maintainer.
> +   * - Obsolete
> +     - Old obsolete code, should use something else.
> +
> +Please bare in mind that even if someone is paid to support something
> +it does not mean they are paid to support you. This is open source and
> +the code comes with no warranty and the project makes no guarantees
> +about dealing with bugs or features requests.
> +
> +Becoming a maintainer
> +---------------------
> +
> +Maintainers are volunteers who put themselves forward to keep an eye
> +on an area of code. They are generally accepted by the community to
> +have a good understanding of the subsystem and able to make a positive
> +contribution to the project.
> +
> +The process is simple - simply sent a patch to the list that updates
> +the ``MAINTAINERS`` file. Sometimes this is done as part of a larger
> +series when a new sub-system is being added to the code base. This can
> +also be done by a retiring maintainer who nominates their replacement
> +after discussion with other contributors.
> +
> +Once the patch is reviewed and merged the only other step is to make
> +sure your GPG key is signed.
> +
> +.. _maintainer_keys:
> +
> +Maintainer GPG Keys
> +~~~~~~~~~~~~~~~~~~~
> +
> +GPG is used to sign pull requests so they can be identified as really
> +coming from the maintainer. If your key is not already signed by
> +members of the QEMU community, you should make arrangements to attend
> +a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
> +example at KVM Forum) or make alternative arrangements to have your
> +key signed by an attendee. Key signing requires meeting another
> +community member **in person** so please make appropriate
> +arrangements.

In practice there are maintainers who have not yet been able to perform
key signing in person. It is still possible to become a maintainer
without a signed GPG key but the goal should be to get it signed as soon
as practically possible. In this case the maintainer still signs pull
requests with their GPG key but the commit logs will show that the key
is not trusted and extra scrutiny is applied when merging code.

> diff --git a/docs/devel/submitting-a-pull-request.rst b/docs/devel/submitting-a-pull-request.rst
> index c9d1e8afd9..a4cd7ebbb6 100644
> --- a/docs/devel/submitting-a-pull-request.rst
> +++ b/docs/devel/submitting-a-pull-request.rst
> @@ -53,14 +53,10 @@ series) and that "make check" passes before sending out the pull
>  request. As a submaintainer you're one of QEMU's lines of defense
>  against bad code, so double check the details.
>  
> -**All pull requests must be signed**. If your key is not already signed
> -by members of the QEMU community, you should make arrangements to attend
> -a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
> -example at KVM Forum) or make alternative arrangements to have your key
> -signed by an attendee.  Key signing requires meeting another community
> -member \*in person\* so please make appropriate arrangements.  By
> -"signed" here we mean that the pullreq email should quote a tag which is
> -a GPG-signed tag (as created with 'gpg tag -s ...').
> +**All pull requests must be signed**. By "signed" here we mean that
> +the pullreq email should quote a tag which is a GPG-signed tag (as
> +created with 'gpg tag -s ...'). See :ref:`maintainer_keys` for
> +details.
>  
>  **Pull requests not for master should say "not for master" and have
>  "PULL SUBSYSTEM whatever" in the subject tag**. If your pull request is
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/4] docs/devel: simplify the minimal checklist

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 7131 bytes --]

On Wed, Oct 12, 2022 at 01:11:51PM +0100, Alex Bennée wrote:
> The bullet points are quite long and contain process tips. Move those
> bits of the bullet to the relevant sections and link to them. Use a
> table for nicer formatting of the checklist.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
>  roms/qboot                        |  2 +-
>  2 files changed, 50 insertions(+), 27 deletions(-)
> 
> diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
> index fb1673e974..41771501bf 100644
> --- a/docs/devel/submitting-a-patch.rst
> +++ b/docs/devel/submitting-a-patch.rst
> @@ -12,25 +12,18 @@ be committed faster.
>  This page seems very long, so if you are only trying to post a quick
>  one-shot fix, the bare minimum we ask is that:
>  
> --  You **must** provide a Signed-off-by: line (this is a hard
> -   requirement because it's how you say "I'm legally okay to contribute
> -   this and happy for it to go into QEMU", modeled after the `Linux kernel
> -   <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
> -   policy.) ``git commit -s`` or ``git format-patch -s`` will add one.
> --  All contributions to QEMU must be **sent as patches** to the
> -   qemu-devel `mailing list <https://wiki.qemu.org/Contribute/MailingLists>`__.
> -   Patch contributions should not be posted on the bug tracker, posted on
> -   forums, or externally hosted and linked to. (We have other mailing lists too,
> -   but all patches must go to qemu-devel, possibly with a Cc: to another
> -   list.) ``git send-email`` (`step-by-step setup
> -   guide <https://git-send-email.io/>`__ and `hints and
> -   tips <https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
> -   works best for delivering the patch without mangling it, but
> -   attachments can be used as a last resort on a first-time submission.
> --  You must read replies to your message, and be willing to act on them.
> -   Note, however, that maintainers are often willing to manually fix up
> -   first-time contributions, since there is a learning curve involved in
> -   making an ideal patch submission.
> +.. list-table:: Minimal Checklist for Patches
> +   :widths: 35 65
> +   :header-rows: 1
> +
> +   * - Check
> +     - Reason
> +   * - Patches contain Signed-off-by: Author Name <author@email>
> +     - States you are legally able to contribute the code. See :ref:`patch_emails_must_include_a_signed_off_by_line`
> +   * - Sent as patch emails to ``qemu-devel@nongnu.org``
> +     - The project uses an email list based workflow. See :ref:`submitting_your_patches`
> +   * - Be prepared to respond to review comments
> +     - Code that doesn't pass review will not get merged. See :ref:`participating_in_code_review`
>  
>  You do not have to subscribe to post (list policy is to reply-to-all to
>  preserve CCs and keep non-subscribers in the loop on the threads they
> @@ -229,6 +222,19 @@ bisection doesn't land on a known-broken state.
>  Submitting your Patches
>  -----------------------
>  
> +The QEMU project uses a public email based workflow for reviewing and
> +merging patches. As a result all contributions to QEMU must be **sent
> +as patches** to the qemu-devel `mailing list
> +<https://wiki.qemu.org/Contribute/MailingLists>`__. Patch
> +contributions should not be posted on the bug tracker, posted on
> +forums, or externally hosted and linked to. (We have other mailing
> +lists too, but all patches must go to qemu-devel, possibly with a Cc:
> +to another list.) ``git send-email`` (`step-by-step setup guide
> +<https://git-send-email.io/>`__ and `hints and tips
> +<https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
> +works best for delivering the patch without mangling it, but
> +attachments can be used as a last resort on a first-time submission.
> +
>  .. _if_you_cannot_send_patch_emails:
>  
>  If you cannot send patch emails
> @@ -314,10 +320,12 @@ git repository to fetch the original commit.
>  Patch emails must include a ``Signed-off-by:`` line
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> -For more information see `SubmittingPatches 1.12
> -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
> -This is vital or we will not be able to apply your patch! Please use
> -your real name to sign a patch (not an alias or acronym).
> +Your patches **must** include a Signed-off-by: line. This is a hard
> +requirement because it's how you say "I'm legally okay to contribute
> +this and happy for it to go into QEMU". The process is modelled after
> +the `Linux kernel
> +<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
> +policy.
>  
>  If you wrote the patch, make sure your "From:" and "Signed-off-by:"
>  lines use the same spelling. It's okay if you subscribe or contribute to
> @@ -327,6 +335,11 @@ include a "From:" line in the body of the email (different from your
>  envelope From:) that will give credit to the correct author; but again,
>  that author's Signed-off-by: line is mandatory, with the same spelling.
>  
> +There are various tooling options for automatically adding these tags
> +include using ``git commit -s`` or ``git format-patch -s``. For more
> +information see `SubmittingPatches 1.12
> +<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
> +
>  .. _include_a_meaningful_cover_letter:
>  
>  Include a meaningful cover letter
> @@ -397,9 +410,19 @@ Participating in Code Review
>  ----------------------------
>  
>  All patches submitted to the QEMU project go through a code review
> -process before they are accepted. Some areas of code that are well
> -maintained may review patches quickly, lesser-loved areas of code may
> -have a longer delay.
> +process before they are accepted. This will often mean a series will
> +go through a number of iterations before being picked up by
> +:ref:`maintainers<maintainers>`. You therefor should be prepared to

therefore

> +read replies to your messages and be willing to act on them.
> +
> +Maintainers are often willing to manually fix up first-time
> +contributions, since there is a learning curve involved in making an
> +ideal patch submission. However for the best results you should
> +proactively respond to suggestions with changes or justifications for
> +your current approach.
> +
> +Some areas of code that are well maintained may review patches
> +quickly, lesser-loved areas of code may have a longer delay.
>  
>  .. _stay_around_to_fix_problems_raised_in_code_review:

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/4] docs/devel suggestions for discussion

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

On Wed, Oct 12, 2022 at 01:11:48PM +0100, Alex Bennée wrote:
> Hi,
> 
> This is an attempt to improve our processes documentation by:
> 
>  - adding an explicit section on maintainers
>  - reducing the up-front verbiage in patch submission
>  - emphasising the importance to respectful reviews
> 
> I'm sure the language could be improved further so I humbly submit
> this RFC for discussion.
> 
> Alex Bennée (4):
>   docs/devel: add a maintainers section to development process
>   docs/devel: make language a little less code centric
>   docs/devel: simplify the minimal checklist
>   docs/devel: try and improve the language around patch review
> 
>  docs/devel/code-of-conduct.rst           |   2 +
>  docs/devel/index-process.rst             |   1 +
>  docs/devel/maintainers.rst               |  84 +++++++++++++++++++
>  docs/devel/submitting-a-patch.rst        | 101 +++++++++++++++--------
>  docs/devel/submitting-a-pull-request.rst |  12 +--
>  roms/qboot                               |   2 +-
>  6 files changed, 157 insertions(+), 45 deletions(-)
>  create mode 100644 docs/devel/maintainers.rst
> 
> -- 
> 2.34.1
> 

Modulo comments:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/4] docs/devel: make language a little less code centric

[Paolo Bonzini]

On 10/12/22 14:11, Alex Bennée wrote:
> +QEMU welcomes contributions to fix bugs, add functionality or improve
> +the documentation. However, we get a lot of patches, and so we have
> +some guidelines about submitting patches. If you follow these, you'll

While we're at it, "about submitting them".

Paolo

> +help make our task of code review easier and your patch is likely to
> +be committed faster.

Re: [RFC PATCH 0/4] docs/devel suggestions for discussion

[Paolo Bonzini]

On 10/12/22 14:11, Alex Bennée wrote:
> Hi,
> 
> This is an attempt to improve our processes documentation by:
> 
>   - adding an explicit section on maintainers
>   - reducing the up-front verbiage in patch submission
>   - emphasising the importance to respectful reviews
> 
> I'm sure the language could be improved further so I humbly submit
> this RFC for discussion.
> 
> Alex Bennée (4):
>    docs/devel: add a maintainers section to development process
>    docs/devel: make language a little less code centric
>    docs/devel: simplify the minimal checklist
>    docs/devel: try and improve the language around patch review
> 
>   docs/devel/code-of-conduct.rst           |   2 +
>   docs/devel/index-process.rst             |   1 +
>   docs/devel/maintainers.rst               |  84 +++++++++++++++++++
>   docs/devel/submitting-a-patch.rst        | 101 +++++++++++++++--------
>   docs/devel/submitting-a-pull-request.rst |  12 +--
>   roms/qboot                               |   2 +-
>   6 files changed, 157 insertions(+), 45 deletions(-)
>   create mode 100644 docs/devel/maintainers.rst

Thanks, these are useful improvements.  On top we could probably merge 
some content from Linux and make the documentation standalone.  But still:

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

after addressing comments from Stefan and myself.

Paolo

Re: [RFC PATCH 1/4] docs/devel: add a maintainers section to development process

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> We don't currently have a clear place in the documentation to describe
> the rolls and responsibilities of a maintainer. Lets create one so we
> can. I've moved a few small bits out of other files to try and keep
> everything in one place.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/code-of-conduct.rst           |  2 +
>  docs/devel/index-process.rst             |  1 +
>  docs/devel/maintainers.rst               | 84 ++++++++++++++++++++++++
>  docs/devel/submitting-a-pull-request.rst | 12 ++--
>  4 files changed, 91 insertions(+), 8 deletions(-)
>  create mode 100644 docs/devel/maintainers.rst
>
> diff --git a/docs/devel/code-of-conduct.rst b/docs/devel/code-of-conduct.rst
> index 195444d1b4..f734ed0317 100644
> --- a/docs/devel/code-of-conduct.rst
> +++ b/docs/devel/code-of-conduct.rst
> @@ -1,3 +1,5 @@
> +.. _code_of_conduct:
> +
>  Code of Conduct
>  ===============
>  
> diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst
> index d0d7a200fd..d50dd74c3e 100644
> --- a/docs/devel/index-process.rst
> +++ b/docs/devel/index-process.rst
> @@ -8,6 +8,7 @@ Notes about how to interact with the community and how and where to submit patch
>  
>     code-of-conduct
>     conflict-resolution
> +   maintainers
>     style
>     submitting-a-patch
>     trivial-patches
> diff --git a/docs/devel/maintainers.rst b/docs/devel/maintainers.rst
> new file mode 100644
> index 0000000000..e3c7003bfa
> --- /dev/null
> +++ b/docs/devel/maintainers.rst
> @@ -0,0 +1,84 @@
> +.. _maintainers:
> +
> +The Roll of Maintainers

Do you mean "Role"?

> +=======================
> +
> +Maintainers are a critical part of the projects contributor ecosystem.
> +They come from a wide range of backgrounds from unpaid hobbyists
> +working in their spare time to employees who work on the project as
> +part of their job. Maintainer activities include:
> +
> +  - reviewing patches and suggesting changes
> +  - preparing pull requests for their subsystems
> +  - participating other project activities

participating in

I think this doesn't quite do justice to what we expect maintainers to
do.

Besides shepherding patches, we expect maintainers to guard the
integrity of their subsystem and the "health" of the developer
community.

We generally defer to a maintainer's reasoned judgement.  This means a
maintainer has a certain power to say no.  With power comes
responsibility.

> +
> +They are also human and subject to the same pressures as everyone else
> +including overload and burn out. Like everyone else they are subject

burnout

> +to projects :ref:`code_of_conduct`.

Arguably even more so than "ordinary" contributors, because by their
visibility they necessarily serve as role models.  With power comes
responsibility.

Should we add something on how a maintainer could get advice?  Say when
they have to deal with bad behavior.

> +
> +The MAINTAINERS file
> +--------------------
> +
> +The `MAINTAINERS
> +<https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS>`__
> +file contains the canonical list of who is a maintainer. The file
> +is machine readable so an appropriately configured git (see
> +:ref:`cc_the_relevant_maintainer`) can automatically Cc them on
> +patches that touch their area of code.
> +
> +The file also describes the status of the area of code to give an idea
> +of how actively that section is maintained.
> +
> +.. list-table:: Meaning of support status in MAINTAINERS
> +   :widths: 25 75
> +   :header-rows: 1
> +
> +   * - Status
> +     - Meaning
> +   * - Supported
> +     - Someone is actually paid to look after this.
> +   * - Maintained
> +     - Someone actually looks after it.
> +   * - Odd Fixes
> +     - It has a maintainer but they don't have time to do
> +       much other than throw the odd patch in.
> +   * - Orphan
> +     - No current maintainer.
> +   * - Obsolete
> +     - Old obsolete code, should use something else.
> +
> +Please bare in mind that even if someone is paid to support something

bear in mind

> +it does not mean they are paid to support you. This is open source and
> +the code comes with no warranty and the project makes no guarantees
> +about dealing with bugs or features requests.
> +
> +Becoming a maintainer
> +---------------------
> +
> +Maintainers are volunteers who put themselves forward to keep an eye
> +on an area of code.

"Volunteers who put themselves forward"...  The press gangs wielding
clubs are a figment of your drunken imagination!

>                      They are generally accepted by the community to
> +have a good understanding of the subsystem and able to make a positive
> +contribution to the project.
> +
> +The process is simple - simply sent a patch to the list that updates
> +the ``MAINTAINERS`` file. Sometimes this is done as part of a larger
> +series when a new sub-system is being added to the code base. This can
> +also be done by a retiring maintainer who nominates their replacement
> +after discussion with other contributors.
> +
> +Once the patch is reviewed and merged the only other step is to make
> +sure your GPG key is signed.
> +
> +.. _maintainer_keys:
> +
> +Maintainer GPG Keys
> +~~~~~~~~~~~~~~~~~~~
> +
> +GPG is used to sign pull requests so they can be identified as really
> +coming from the maintainer. If your key is not already signed by
> +members of the QEMU community, you should make arrangements to attend
> +a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
> +example at KVM Forum) or make alternative arrangements to have your
> +key signed by an attendee. Key signing requires meeting another
> +community member **in person** so please make appropriate
> +arrangements.
> diff --git a/docs/devel/submitting-a-pull-request.rst b/docs/devel/submitting-a-pull-request.rst
> index c9d1e8afd9..a4cd7ebbb6 100644
> --- a/docs/devel/submitting-a-pull-request.rst
> +++ b/docs/devel/submitting-a-pull-request.rst
> @@ -53,14 +53,10 @@ series) and that "make check" passes before sending out the pull
>  request. As a submaintainer you're one of QEMU's lines of defense
>  against bad code, so double check the details.
>  
> -**All pull requests must be signed**. If your key is not already signed
> -by members of the QEMU community, you should make arrangements to attend
> -a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
> -example at KVM Forum) or make alternative arrangements to have your key
> -signed by an attendee.  Key signing requires meeting another community
> -member \*in person\* so please make appropriate arrangements.  By
> -"signed" here we mean that the pullreq email should quote a tag which is
> -a GPG-signed tag (as created with 'gpg tag -s ...').
> +**All pull requests must be signed**. By "signed" here we mean that
> +the pullreq email should quote a tag which is a GPG-signed tag (as
> +created with 'gpg tag -s ...'). See :ref:`maintainer_keys` for
> +details.
>  
>  **Pull requests not for master should say "not for master" and have
>  "PULL SUBSYSTEM whatever" in the subject tag**. If your pull request is

Re: [RFC PATCH 4/4] docs/devel: try and improve the language around patch review

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> It is important that contributors take the review process seriously
> and we collaborate in a respectful way while avoiding personal
> attacks. Try and make this clear in the language.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [RFC PATCH 1/4] docs/devel: add a maintainers section to development process

[Mark Cave-Ayland]

On 12/10/2022 13:11, Alex Bennée wrote:

> We don't currently have a clear place in the documentation to describe
> the rolls and responsibilities of a maintainer. Lets create one so we
> can. I've moved a few small bits out of other files to try and keep
> everything in one place.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   docs/devel/code-of-conduct.rst           |  2 +
>   docs/devel/index-process.rst             |  1 +
>   docs/devel/maintainers.rst               | 84 ++++++++++++++++++++++++
>   docs/devel/submitting-a-pull-request.rst | 12 ++--
>   4 files changed, 91 insertions(+), 8 deletions(-)
>   create mode 100644 docs/devel/maintainers.rst
> 
> diff --git a/docs/devel/code-of-conduct.rst b/docs/devel/code-of-conduct.rst
> index 195444d1b4..f734ed0317 100644
> --- a/docs/devel/code-of-conduct.rst
> +++ b/docs/devel/code-of-conduct.rst
> @@ -1,3 +1,5 @@
> +.. _code_of_conduct:
> +
>   Code of Conduct
>   ===============
>   
> diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst
> index d0d7a200fd..d50dd74c3e 100644
> --- a/docs/devel/index-process.rst
> +++ b/docs/devel/index-process.rst
> @@ -8,6 +8,7 @@ Notes about how to interact with the community and how and where to submit patch
>   
>      code-of-conduct
>      conflict-resolution
> +   maintainers
>      style
>      submitting-a-patch
>      trivial-patches
> diff --git a/docs/devel/maintainers.rst b/docs/devel/maintainers.rst
> new file mode 100644
> index 0000000000..e3c7003bfa
> --- /dev/null
> +++ b/docs/devel/maintainers.rst
> @@ -0,0 +1,84 @@
> +.. _maintainers:
> +
> +The Roll of Maintainers
> +=======================
> +
> +Maintainers are a critical part of the projects contributor ecosystem.
> +They come from a wide range of backgrounds from unpaid hobbyists
> +working in their spare time to employees who work on the project as
> +part of their job. Maintainer activities include:
> +
> +  - reviewing patches and suggesting changes
> +  - preparing pull requests for their subsystems
> +  - participating other project activities

missing word: participating in other project activities

> +They are also human and subject to the same pressures as everyone else
> +including overload and burn out. Like everyone else they are subject
> +to projects :ref:`code_of_conduct`.
> +
> +The MAINTAINERS file
> +--------------------
> +
> +The `MAINTAINERS
> +<https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS>`__
> +file contains the canonical list of who is a maintainer. The file
> +is machine readable so an appropriately configured git (see
> +:ref:`cc_the_relevant_maintainer`) can automatically Cc them on
> +patches that touch their area of code.
> +
> +The file also describes the status of the area of code to give an idea
> +of how actively that section is maintained.
> +
> +.. list-table:: Meaning of support status in MAINTAINERS
> +   :widths: 25 75
> +   :header-rows: 1
> +
> +   * - Status
> +     - Meaning
> +   * - Supported
> +     - Someone is actually paid to look after this.
> +   * - Maintained
> +     - Someone actually looks after it.
> +   * - Odd Fixes
> +     - It has a maintainer but they don't have time to do
> +       much other than throw the odd patch in.
> +   * - Orphan
> +     - No current maintainer.
> +   * - Obsolete
> +     - Old obsolete code, should use something else.
> +
> +Please bare in mind that even if someone is paid to support something

s/bare/bear/

> +it does not mean they are paid to support you. This is open source and
> +the code comes with no warranty and the project makes no guarantees
> +about dealing with bugs or features requests.
> +
> +Becoming a maintainer
> +---------------------
> +
> +Maintainers are volunteers who put themselves forward to keep an eye
> +on an area of code. They are generally accepted by the community to
> +have a good understanding of the subsystem and able to make a positive
> +contribution to the project.

Is it worth making this a bit stronger such as "having a demonstrable track record of 
providing accepted upstream patches"? I'm not sure if this is being a bit too 
nit-picky, however someone could have good understanding of a subsystem such as PCI 
but be still unfamiliar with the QEMU's PCI APIs and how they should be used.

> +The process is simple - simply sent a patch to the list that updates
> +the ``MAINTAINERS`` file. Sometimes this is done as part of a larger
> +series when a new sub-system is being added to the code base. This can
> +also be done by a retiring maintainer who nominates their replacement
> +after discussion with other contributors.

Should we also request that a patch adding to MAINTAINERS needs an R-B tag from an 
existing maintainer?

> +Once the patch is reviewed and merged the only other step is to make
> +sure your GPG key is signed.
> +
> +.. _maintainer_keys:
> +
> +Maintainer GPG Keys
> +~~~~~~~~~~~~~~~~~~~
> +
> +GPG is used to sign pull requests so they can be identified as really
> +coming from the maintainer. If your key is not already signed by
> +members of the QEMU community, you should make arrangements to attend
> +a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
> +example at KVM Forum) or make alternative arrangements to have your
> +key signed by an attendee. Key signing requires meeting another
> +community member **in person** so please make appropriate
> +arrangements.
> diff --git a/docs/devel/submitting-a-pull-request.rst b/docs/devel/submitting-a-pull-request.rst
> index c9d1e8afd9..a4cd7ebbb6 100644
> --- a/docs/devel/submitting-a-pull-request.rst
> +++ b/docs/devel/submitting-a-pull-request.rst
> @@ -53,14 +53,10 @@ series) and that "make check" passes before sending out the pull
>   request. As a submaintainer you're one of QEMU's lines of defense
>   against bad code, so double check the details.
>   
> -**All pull requests must be signed**. If your key is not already signed
> -by members of the QEMU community, you should make arrangements to attend
> -a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
> -example at KVM Forum) or make alternative arrangements to have your key
> -signed by an attendee.  Key signing requires meeting another community
> -member \*in person\* so please make appropriate arrangements.  By
> -"signed" here we mean that the pullreq email should quote a tag which is
> -a GPG-signed tag (as created with 'gpg tag -s ...').
> +**All pull requests must be signed**. By "signed" here we mean that
> +the pullreq email should quote a tag which is a GPG-signed tag (as
> +created with 'gpg tag -s ...'). See :ref:`maintainer_keys` for
> +details.
>   
>   **Pull requests not for master should say "not for master" and have
>   "PULL SUBSYSTEM whatever" in the subject tag**. If your pull request is


ATB,

Mark.

Re: [RFC PATCH 3/4] docs/devel: simplify the minimal checklist

[Mark Cave-Ayland]

On 12/10/2022 13:11, Alex Bennée wrote:

> The bullet points are quite long and contain process tips. Move those
> bits of the bullet to the relevant sections and link to them. Use a
> table for nicer formatting of the checklist.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
>   roms/qboot                        |  2 +-
>   2 files changed, 50 insertions(+), 27 deletions(-)
> 
> diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
> index fb1673e974..41771501bf 100644
> --- a/docs/devel/submitting-a-patch.rst
> +++ b/docs/devel/submitting-a-patch.rst
> @@ -12,25 +12,18 @@ be committed faster.
>   This page seems very long, so if you are only trying to post a quick
>   one-shot fix, the bare minimum we ask is that:
>   
> --  You **must** provide a Signed-off-by: line (this is a hard
> -   requirement because it's how you say "I'm legally okay to contribute
> -   this and happy for it to go into QEMU", modeled after the `Linux kernel
> -   <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
> -   policy.) ``git commit -s`` or ``git format-patch -s`` will add one.
> --  All contributions to QEMU must be **sent as patches** to the
> -   qemu-devel `mailing list <https://wiki.qemu.org/Contribute/MailingLists>`__.
> -   Patch contributions should not be posted on the bug tracker, posted on
> -   forums, or externally hosted and linked to. (We have other mailing lists too,
> -   but all patches must go to qemu-devel, possibly with a Cc: to another
> -   list.) ``git send-email`` (`step-by-step setup
> -   guide <https://git-send-email.io/>`__ and `hints and
> -   tips <https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
> -   works best for delivering the patch without mangling it, but
> -   attachments can be used as a last resort on a first-time submission.
> --  You must read replies to your message, and be willing to act on them.
> -   Note, however, that maintainers are often willing to manually fix up
> -   first-time contributions, since there is a learning curve involved in
> -   making an ideal patch submission.
> +.. list-table:: Minimal Checklist for Patches
> +   :widths: 35 65
> +   :header-rows: 1
> +
> +   * - Check
> +     - Reason
> +   * - Patches contain Signed-off-by: Author Name <author@email>
> +     - States you are legally able to contribute the code. See :ref:`patch_emails_must_include_a_signed_off_by_line`
> +   * - Sent as patch emails to ``qemu-devel@nongnu.org``
> +     - The project uses an email list based workflow. See :ref:`submitting_your_patches`
> +   * - Be prepared to respond to review comments
> +     - Code that doesn't pass review will not get merged. See :ref:`participating_in_code_review`
>   
>   You do not have to subscribe to post (list policy is to reply-to-all to
>   preserve CCs and keep non-subscribers in the loop on the threads they
> @@ -229,6 +222,19 @@ bisection doesn't land on a known-broken state.
>   Submitting your Patches
>   -----------------------
>   
> +The QEMU project uses a public email based workflow for reviewing and
> +merging patches. As a result all contributions to QEMU must be **sent
> +as patches** to the qemu-devel `mailing list
> +<https://wiki.qemu.org/Contribute/MailingLists>`__. Patch
> +contributions should not be posted on the bug tracker, posted on
> +forums, or externally hosted and linked to. (We have other mailing
> +lists too, but all patches must go to qemu-devel, possibly with a Cc:
> +to another list.) ``git send-email`` (`step-by-step setup guide
> +<https://git-send-email.io/>`__ and `hints and tips
> +<https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
> +works best for delivering the patch without mangling it, but
> +attachments can be used as a last resort on a first-time submission.
> +
>   .. _if_you_cannot_send_patch_emails:
>   
>   If you cannot send patch emails
> @@ -314,10 +320,12 @@ git repository to fetch the original commit.
>   Patch emails must include a ``Signed-off-by:`` line
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   
> -For more information see `SubmittingPatches 1.12
> -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
> -This is vital or we will not be able to apply your patch! Please use
> -your real name to sign a patch (not an alias or acronym).

I think it is worth keeping that part about using a real name rather than an 
alias/acronym - perhaps that could be included in the Minimal Checklist table above?

> +Your patches **must** include a Signed-off-by: line. This is a hard
> +requirement because it's how you say "I'm legally okay to contribute
> +this and happy for it to go into QEMU". The process is modelled after
> +the `Linux kernel
> +<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
> +policy.
>   
>   If you wrote the patch, make sure your "From:" and "Signed-off-by:"
>   lines use the same spelling. It's okay if you subscribe or contribute to
> @@ -327,6 +335,11 @@ include a "From:" line in the body of the email (different from your
>   envelope From:) that will give credit to the correct author; but again,
>   that author's Signed-off-by: line is mandatory, with the same spelling.
>   
> +There are various tooling options for automatically adding these tags
> +include using ``git commit -s`` or ``git format-patch -s``. For more
> +information see `SubmittingPatches 1.12
> +<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
> +
>   .. _include_a_meaningful_cover_letter:
>   
>   Include a meaningful cover letter
> @@ -397,9 +410,19 @@ Participating in Code Review
>   ----------------------------
>   
>   All patches submitted to the QEMU project go through a code review
> -process before they are accepted. Some areas of code that are well
> -maintained may review patches quickly, lesser-loved areas of code may
> -have a longer delay.
> +process before they are accepted. This will often mean a series will
> +go through a number of iterations before being picked up by
> +:ref:`maintainers<maintainers>`. You therefor should be prepared to
> +read replies to your messages and be willing to act on them.
> +
> +Maintainers are often willing to manually fix up first-time
> +contributions, since there is a learning curve involved in making an
> +ideal patch submission. However for the best results you should
> +proactively respond to suggestions with changes or justifications for
> +your current approach.
> +
> +Some areas of code that are well maintained may review patches
> +quickly, lesser-loved areas of code may have a longer delay.
>   
>   .. _stay_around_to_fix_problems_raised_in_code_review:
>   
> diff --git a/roms/qboot b/roms/qboot
> index 8ca302e86d..a5300c4949 160000
> --- a/roms/qboot
> +++ b/roms/qboot
> @@ -1 +1 @@
> -Subproject commit 8ca302e86d685fa05b16e2b208888243da319941
> +Subproject commit a5300c4949b8d4de2d34bedfaed66793f48ec948


ATB,

Mark.

Re: [RFC PATCH 0/4] docs/devel suggestions for discussion

[Mark Cave-Ayland]

On 12/10/2022 13:11, Alex Bennée wrote:

> Hi,
> 
> This is an attempt to improve our processes documentation by:
> 
>   - adding an explicit section on maintainers
>   - reducing the up-front verbiage in patch submission
>   - emphasising the importance to respectful reviews
> 
> I'm sure the language could be improved further so I humbly submit
> this RFC for discussion.
> 
> Alex Bennée (4):
>    docs/devel: add a maintainers section to development process
>    docs/devel: make language a little less code centric
>    docs/devel: simplify the minimal checklist
>    docs/devel: try and improve the language around patch review
> 
>   docs/devel/code-of-conduct.rst           |   2 +
>   docs/devel/index-process.rst             |   1 +
>   docs/devel/maintainers.rst               |  84 +++++++++++++++++++
>   docs/devel/submitting-a-patch.rst        | 101 +++++++++++++++--------
>   docs/devel/submitting-a-pull-request.rst |  12 +--
>   roms/qboot                               |   2 +-
>   6 files changed, 157 insertions(+), 45 deletions(-)
>   create mode 100644 docs/devel/maintainers.rst

Hi Alex,

I've replied with a couple of things I noticed, but in general I think this is a 
really nice improvement.

If you're looking at documenting some of the maintainer processes better, there are a 
few other things I have been thinking about that it may be worth discussing:


i) Requiring an R-B tag for all patches before merge

- Is this something we should insist on and document?

ii) Unresponsive maintainers

- Should we have a process for this? When Blue Swirl (the previous SPARC maintainer) 
disappeared abruptly, I think it took nearly 3 months to get my first patches merged 
since no-one knew if they were still active. If a maintainer has been unresponsive 
for e.g. 2 months should that then allow a process where other maintainers can merge 
patches on their behalf and/or start a process of maintainer transition?

iii) Differences(?) between maintainers

- There have been a few instances where I have been delayed in finding time for patch 
review, and in the meantime someone has stepped up to review the patch and given it 
an R-B tag which is great. However I have then reviewed the patch and noticed 
something amiss, and so it needs a bit more work before being merged. I think in 
these cases the review of the maintainer of the code in question should take priority 
over other maintainer reviews: do we need to explicitly document this? I can 
certainly see how this can be confusing to newcomers having an R-B tag as a 
pre-requisite for merge coming from one person, and then a NACK from someone else later.


ATB,

Mark.

Re: [RFC PATCH 1/4] docs/devel: add a maintainers section to development process

[Markus Armbruster]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> On 12/10/2022 13:11, Alex Bennée wrote:

[...]

>> +Becoming a maintainer
>> +---------------------
>> +
>> +Maintainers are volunteers who put themselves forward to keep an eye
>> +on an area of code. They are generally accepted by the community to

Do you mean "expected by the community"?

>> +have a good understanding of the subsystem and able to make a positive
>> +contribution to the project.
>
> Is it worth making this a bit stronger such as "having a demonstrable track record of providing accepted upstream patches"? I'm not sure if this is being a bit too 
> nit-picky, however someone could have good understanding of a subsystem such as PCI but be still unfamiliar with the QEMU's PCI APIs and how they should be used.

I think existing practice varies.

For something that is widely used, we generally require enough of a
track record (contributions *and* reviews) to inspire confidence.

But if you submit something new, say a machine, we may ask you to stick
around and maintain it as a prerequisite for merging.

[...]

Re: [RFC PATCH 0/4] docs/devel suggestions for discussion

[Markus Armbruster]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> On 12/10/2022 13:11, Alex Bennée wrote:
>
>> Hi,
>> This is an attempt to improve our processes documentation by:
>>   - adding an explicit section on maintainers
>>   - reducing the up-front verbiage in patch submission
>>   - emphasising the importance to respectful reviews
>> I'm sure the language could be improved further so I humbly submit
>> this RFC for discussion.
>> Alex Bennée (4):
>>    docs/devel: add a maintainers section to development process
>>    docs/devel: make language a little less code centric
>>    docs/devel: simplify the minimal checklist
>>    docs/devel: try and improve the language around patch review
>>   docs/devel/code-of-conduct.rst           |   2 +
>>   docs/devel/index-process.rst             |   1 +
>>   docs/devel/maintainers.rst               |  84 +++++++++++++++++++
>>   docs/devel/submitting-a-patch.rst        | 101 +++++++++++++++--------
>>   docs/devel/submitting-a-pull-request.rst |  12 +--
>>   roms/qboot                               |   2 +-
>>   6 files changed, 157 insertions(+), 45 deletions(-)
>>   create mode 100644 docs/devel/maintainers.rst
>
> Hi Alex,
>
> I've replied with a couple of things I noticed, but in general I think this is a really nice improvement.
>
> If you're looking at documenting some of the maintainer processes better, there are a few other things I have been thinking about that it may be worth discussing:
>
>
> i) Requiring an R-B tag for all patches before merge
>
> - Is this something we should insist on and document?
>
> ii) Unresponsive maintainers
>
> - Should we have a process for this? When Blue Swirl (the previous SPARC maintainer) disappeared abruptly, I think it took nearly 3 months to get my first patches merged 
> since no-one knew if they were still active. If a maintainer has been unresponsive for e.g. 2 months should that then allow a process where other maintainers can merge 
> patches on their behalf and/or start a process of maintainer transition?
>
> iii) Differences(?) between maintainers
>
> - There have been a few instances where I have been delayed in finding time for patch review, and in the meantime someone has stepped up to review the patch and given it 
> an R-B tag which is great. However I have then reviewed the patch and noticed something amiss, and so it needs a bit more work before being merged. I think in 
> these cases the review of the maintainer of the code in question should take priority over other maintainer reviews: do we need to explicitly document this? I can 
> certainly see how this can be confusing to newcomers having an R-B tag as a pre-requisite for merge coming from one person, and then a NACK from someone else later.

The opposite also happens: I review in my role as a maintainer, and give
my R-by, then somebody else has questions or objections.  These need to
be addressed.  My R-by as maintainer does not change that at all.

Maintainers' reviews are not special.  Issues raised in review need to
be addressed regardless of who raised them.  Maintainers' "specialness"
kicks in at the point where they make judgement calls, such as "this is
good enough, we can address the remaining issues on top".

If multiple maintainers are responsible and disagree, they need to
hammer out some kind of consensus.

Re: [RFC PATCH 1/4] docs/devel: add a maintainers section to development process

[Alex Bennée]

Markus Armbruster <armbru@redhat.com> writes:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
>
>> On 12/10/2022 13:11, Alex Bennée wrote:
>
> [...]
>
>>> +Becoming a maintainer
>>> +---------------------
>>> +
>>> +Maintainers are volunteers who put themselves forward to keep an eye
>>> +on an area of code. They are generally accepted by the community to
>
> Do you mean "expected by the community"?

Well I was trying to make clear how the "community" decides who should
be a maintainer. We don't leave it to who's currently holding the merge
keys so in practice its other contributors acknowledging that the
proposed maintainer knows their stuff (or at least didn't step backwards
fast enough when the call went out).

In practice when maintainership is passed down this is often just the
R-b by the previous maintainer. For areas where no maintainer currently
exists just gathering a few R-b's seems to be enough because having a
maintainer is better than not having one.

>
>>> +have a good understanding of the subsystem and able to make a positive
>>> +contribution to the project.
>>
>> Is it worth making this a bit stronger such as "having a
>> demonstrable track record of providing accepted upstream patches"?
>> I'm not sure if this is being a bit too
>> nit-picky, however someone could have good understanding of a
>> subsystem such as PCI but be still unfamiliar with the QEMU's PCI
>> APIs and how they should be used.
>
> I think existing practice varies.
>
> For something that is widely used, we generally require enough of a
> track record (contributions *and* reviews) to inspire confidence.

I can certainly add language about prior contributions. 

> But if you submit something new, say a machine, we may ask you to stick
> around and maintain it as a prerequisite for merging.
>
> [...]


-- 
Alex Bennée

Re: [RFC PATCH 0/4] docs/devel suggestions for discussion

[Alex Bennée]

Markus Armbruster <armbru@redhat.com> writes:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
>
>> On 12/10/2022 13:11, Alex Bennée wrote:
>>
>>> Hi,
>>> This is an attempt to improve our processes documentation by:
>>>   - adding an explicit section on maintainers
>>>   - reducing the up-front verbiage in patch submission
>>>   - emphasising the importance to respectful reviews
>>> I'm sure the language could be improved further so I humbly submit
>>> this RFC for discussion.
>>> Alex Bennée (4):
>>>    docs/devel: add a maintainers section to development process
>>>    docs/devel: make language a little less code centric
>>>    docs/devel: simplify the minimal checklist
>>>    docs/devel: try and improve the language around patch review
>>>   docs/devel/code-of-conduct.rst           |   2 +
>>>   docs/devel/index-process.rst             |   1 +
>>>   docs/devel/maintainers.rst               |  84 +++++++++++++++++++
>>>   docs/devel/submitting-a-patch.rst        | 101 +++++++++++++++--------
>>>   docs/devel/submitting-a-pull-request.rst |  12 +--
>>>   roms/qboot                               |   2 +-
>>>   6 files changed, 157 insertions(+), 45 deletions(-)
>>>   create mode 100644 docs/devel/maintainers.rst
>>
>> Hi Alex,
>>
>> I've replied with a couple of things I noticed, but in general I think this is a really nice improvement.
>>
>> If you're looking at documenting some of the maintainer processes
>> better, there are a few other things I have been thinking about that
>> it may be worth discussing:
>>
>>
>> i) Requiring an R-B tag for all patches before merge
>>
>> - Is this something we should insist on and document?
>>
>> ii) Unresponsive maintainers
>>
>> - Should we have a process for this? When Blue Swirl (the previous
>> SPARC maintainer) disappeared abruptly, I think it took nearly 3
>> months to get my first patches merged
>> since no-one knew if they were still active. If a maintainer has
>> been unresponsive for e.g. 2 months should that then allow a process
>> where other maintainers can merge
>> patches on their behalf and/or start a process of maintainer transition?
>>
>> iii) Differences(?) between maintainers
>>
>> - There have been a few instances where I have been delayed in
>> finding time for patch review, and in the meantime someone has
>> stepped up to review the patch and given it
>> an R-B tag which is great. However I have then reviewed the patch
>> and noticed something amiss, and so it needs a bit more work before
>> being merged. I think in
>> these cases the review of the maintainer of the code in question
>> should take priority over other maintainer reviews: do we need to
>> explicitly document this? I can
>> certainly see how this can be confusing to newcomers having an R-B
>> tag as a pre-requisite for merge coming from one person, and then a
>> NACK from someone else later.
>
> The opposite also happens: I review in my role as a maintainer, and give
> my R-by, then somebody else has questions or objections.  These need to
> be addressed.  My R-by as maintainer does not change that at all.
>
> Maintainers' reviews are not special.  Issues raised in review need to
> be addressed regardless of who raised them.  Maintainers' "specialness"
> kicks in at the point where they make judgement calls, such as "this is
> good enough, we can address the remaining issues on top".

This is my view as well - and we want to encourage as much reviewing as
possible rather than implying you need to be blessed to have a view on
the code. That said it is still going to be the maintainers call to take
a patch through their tree and hopefully we don't have too many cases
where that is subverted by going through other maintainers trees. 

> If multiple maintainers are responsible and disagree, they need to
> hammer out some kind of consensus.

Which reminds me I've quite often used the A-b tag to indicate I'm happy
for a patch that touches one of my areas to go through someone elses
tree. Do we actually codify that meaning anywhere?

-- 
Alex Bennée

Re: [RFC PATCH 1/4] docs/devel: add a maintainers section to development process

[Peter Maydell]

On Fri, 14 Oct 2022 at 14:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> >
> >> On 12/10/2022 13:11, Alex Bennée wrote:
> >
> > [...]
> >
> >>> +Becoming a maintainer
> >>> +---------------------
> >>> +
> >>> +Maintainers are volunteers who put themselves forward to keep an eye
> >>> +on an area of code. They are generally accepted by the community to
> >
> > Do you mean "expected by the community"?
>
> Well I was trying to make clear how the "community" decides who should
> be a maintainer. We don't leave it to who's currently holding the merge
> keys so in practice its other contributors acknowledging that the
> proposed maintainer knows their stuff (or at least didn't step backwards
> fast enough when the call went out).

I think there are also two parts or stages to this:
 (1) is there somebody who's agreed to review patches, look at
     incoming bugs, etc for a particular bit of the code and
     is probably the 'local expert' on it?
 (2) who is accumulating these patches and eventually sending
     a pull request?

For a lot of the smaller sub-areas of QEMU (e.g. specific board
types or specific devices) these might be two different people.
And we might trust somebody to be able to do code review and
patch triage but prefer the patches to flow through the tree
of some other maintainer who's more experienced with sending
pull requests.

-- PMM