From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f46.google.com (mail-lf0-f46.google.com [209.85.215.46]) by dpdk.org (Postfix) with ESMTP id 4575C91D0 for ; Thu, 15 Oct 2015 23:38:00 +0200 (CEST) Received: by lffv3 with SMTP id v3so50564631lff.0 for ; Thu, 15 Oct 2015 14:37:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=VAApIgJcsi/dmqFEMnWc7ZdetqLCLqnR9Am77Q8ggVY=; b=PiTvkLS1UTOf8ljm0ENppJ1uuLv1hs1klKblCyqDJKw+8WWR1nJ6In6c4NKlU2p1W1 6VsbVz9sQNZg3/s5dn1C47r9V3JxZlwxORjbQB4TQKb/djLM/8yVhc7vhX44GDYUCMAS D4g5MUuhOwKeODJYaGnf6T9G4FJZhX2mf2Rlruw7Dxfx4BkJ0SbMC272WlExHeKk0UoJ vpFQJE/+F52iO3Sg7S3yT8oa2CgKp/I47TcnpebI10Yn/ECeDaJWUvXNJZrpukY5eCY8 sNEWh3YpqP6AZi22Oqa8tk7XhcuXJ30We97Ft4O8Mo9gUQRXG9ROw+fdkE4zimz240GB kx3A== X-Gm-Message-State: ALoCoQnEo+6bc8COleNlgu4Gf6vtGAkLSE1pkVLN+oLkCDYtrPkVtz8yY6oKwpovWvdvIShByl7t X-Received: by 10.194.201.130 with SMTP id ka2mr13024669wjc.123.1444945079726; Thu, 15 Oct 2015 14:37:59 -0700 (PDT) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by smtp.gmail.com with ESMTPSA id xa5sm18708177wjc.20.2015.10.15.14.37.58 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Oct 2015 14:37:58 -0700 (PDT) From: Thomas Monjalon To: John McNamara Date: Thu, 15 Oct 2015 23:36:56 +0200 Message-ID: <1786715.z8JXW1Lmj7@xps13> Organization: 6WIND User-Agent: KMail/4.14.10 (Linux/4.1.6-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <1444927893-19845-2-git-send-email-john.mcnamara@intel.com> References: <1444927893-19845-1-git-send-email-john.mcnamara@intel.com> <1444927893-19845-2-git-send-email-john.mcnamara@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] doc: add contributors guide X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Oct 2015 21:38:00 -0000 Hi John, 2015-10-15 17:51, John McNamara: > Add a document to explain the DPDK patch submission and review proces= s. Thanks > +There are also DPDK mailing lists for: > + > +* users: `general usage questions `_. > +* announce: `release announcements `_ (also forwarded to the dev list). > +* dts: `test suite reviews and discussions `_. I think these lists are not relevant for patch submission. > +* test-reports: `test reports `_ (from continuous integration testing). [...] > +Getting the Source Code > +----------------------- > + > +The source code can be cloned using either of the following:: > + > + git clone git://dpdk.org/dpdk > + > + git clone http://dpdk.org/git/dpdk > + > +You can also `browse the source code `_ online. The online browse doesn't help for patch contribution. [...] > +* If you add new files or directories you should add your name to th= e ``MAINTAINERS`` file. yes > +* If your changes add new external functions then they should be add= ed to the local ``version.map`` file. > + See the :doc:`Guidelines for ABI policy and versioning `. > + > +* Most changes will require an addition to the release notes in ``do= c/guides/rel_notes/``. > + See the :ref:`Release Notes section of the Documentation Guideline= s ` for details. s/Most/Important/ ? > +* Don=E2=80=99t break compilation between commits with forward depen= dencies. > + Each commit should compile on its own to allow for ``git bisect`` = and continuous integration testing. no, please, don't break compilation :) > +* Add tests to the the ``app/test`` unit test framework where possib= le. > + > +* Add documentation, if required, in the form of Doxygen comments or= a User Guide in RST format. s/required/relevant/ ? > +The commits should be separated into logical patches in a patchset. Yes > +In general commits should be separated based on their directory such= as ``lib``, ``drivers``, ``scripts`` although > +some of these, such as ``drivers`` may require finer grained separat= ion. No. The directory is not so important. It must be easy to review first. If changes are not so big and do not require specific explanations, it's better to keep things together in the same patch. A good way of thinking about patch split is to consider backports: will it be easy to backport this change with its dependencies? will it be easy to backport this feature/fix without useless bloat? > +The easiest way of determining this is to do a ``git log`` on change= d or similar files. Yes > +Example of a logical patchset separation:: > + > + [patch 1/6] ethdev: add support for ieee1588 timestamping > + [patch 2/6] e1000: add support for ieee1588 timestamping > + [patch 3/6] ixgbe: add support for ieee1588 timestamping > + [patch 4/6] i40e: add support for ieee1588 timestamping > + [patch 5/6] app/testpmd: refactor ieee1588 forwarding > + [patch 6/6] doc: document ieee1588 forwarding mode The doc must be committed with the API change (ethdev). Splitting driver implementations is useful only if they are really big = or require some specific explanations in the commit message. > +* The summary line should be lowercase. The acronyms can be uppercase. > + For example:: > + > + ixgbe: fix bug in xyz After "fix", the word "bug" is useless. It's better to briefly explain the impact of the bug, e.g. "fix RSS on = 32-bit". So people interested in RSS or 32-bit will look at this fix. > + ixgbe: add refcount to foo struct Generally, using the name of a struct, a variable or a file in the titl= e reveals that you don't know how to explain your change simply. The implementation details may be explained in the long message. The title must help to catch the area and the impact of the change. > +If you are submitting a RFC draft of a feature you can use ``[RFC]``= instead of ``[PATCH]``. A RFC may be incomplete. It helps to have feedbacks before doing more. > +* You must provide a body to the commit message after the subject/su= mmary line. > + Do not leave it blank. When it is totally obvious, the Signed-off is enough. > +* When fixing a regression, it is a good idea to reference the id of= the commit which introduced the bug. > + You can generate the required text as follows:: > + > + git log -1 COMMIT_ID --abbrev=3D12 --format=3D'Fixes: %h ("%s")= ' git alias: fixline =3D log -1 --abbrev=3D12 --format=3D'Fixes: %h (\"%s= \")' > + Fixes: a4024448efa6 ("i40e: add ieee1588 timestamping") Yes it will help the backports. > +* When fixing an error or warning it is useful to add the error mess= age or output. The steps to reproduce the bugs are also required. > +* ``Reported-by:`` The reporter of the issue. > +* ``Tested-by:`` The tester of the change. > +* ``Reviewed-by:`` The reviewer of the change. > +* ``Suggested-by:`` The person who suggested the change. Yes, and Acked-by: When it is commented between 2 versions of the patch, it can be added i= n the new version if it is still relevant. > +Cover letters are useful for explaining a patchset. And it helps to have a correct threading of the patches. > +Version 2 and later of a patchset should also include a short log of= the changes so the reviewer knows what has changed. > +This can go either in the cover letter on the annotations. s/on/or/ > +The kernel guidelines that are tested by ``checkpatch`` don't match = the DPDK Coding Style guidelines exactly but > +they provide a good indication of conformance. > +Warnings about not using kernel data types or about split strings ca= n be ignored:: > + > + /path/checkpatch.pl --ignore PREFER_KERNEL_TYPES,SPLIT_STRING -q = files*.patch OK I plan to suggest a script with more checkpatch configurations. We should enforce using "make test" before sending. > +Patches should be sent to the mailing list using ``git send-email``.= > +This will require a working and configured ``sendmail`` or similar a= pplication. No, you can configure an external SMTP: =09smtpuser =3D name@domain.com =09smtpserver =3D smtp.domain.com =09smtpserverport =3D 465 =09smtpencryption =3D ssl > +If the patches are a change to existing files then you should CC the= maintainer(s) of the changed files. > +The appropriate maintainer can be found in the ``MAINTAINERS`` file:= : > + > + git send-email --to dev@dpdk.org --cc maintainer@some.org 000*.pa= tch I would say to send --to the maintainers and -cc dev@dpdk.org. Some maintainers can have stronger filter if their name is in the "To" = field. > +If the patch is in relation to a previous email thread you can add i= t to the same thread using the Message ID:: > + > + git send-email --to dev@dpdk.org --in-reply-to <1234-foo@bar.com>= 000*.patch Yes please. s/can/should/ > +Experienced commiters may send patches directly with ``git send-emai= l`` without the ``git format-patch`` step. The options --annotate and "confirm =3D always" are recommended to chec= k before sending. > +The more work you put into the previous steps the easier it will be = to get a patch accepted. Yes :) > +#. Submit the patch. Check the automatic test reports in the coming hours. > +#. Wait for review comments. While you are waiting review some other= patches. > +#. If the patch is deemed suitable for merging by the relevant maint= ainer(s) or other developers they will ``ack`` > + the patch with an email that includes something like:: We don"t use Reviewed-by a lot. My understanding is that "Acked-by" doesn't mean it has been fully revi= ewed and tested. But Reviewed-by is stronger without implying that we think it's the bes= t solution. It's an interpretation. Should it be explained here? > +#. If the patch isn't deemed suitable based on being out of scope or= conflicting with existing functionality > + it may receive a ``nack``. > + In this case you will need to make a more convincing technical ar= gument in favor of your patches. More generally, a patch should not be accepted if there are some commen= ts not addressed by a new version or some strong arguments. > +#. Acked patches will be merged in the next merge window. Next or current?