[PATCH] user: Document patch review process

[Sebastian Huber]

On 01/10/2019 09:22, Chris Johns wrote:
> On 1/10/19 3:50 pm, Sebastian Huber wrote:
>> On 01/10/2019 01:40, Chris Johns wrote:
>>> On 30/9/19 10:45 pm, Sebastian Huber wrote:
[...]
>>>> +* The patch builds.  All RTEMS tests link with this patch.
>>>> +
>>>> +* The patch does not introduce new compiler warnings.
>>>
>>> This step is not in the figure.
>>
>> You mean there should be a step mentioning this checklist?
> 
> Sorry, I missed by a line, I meant the patch builds. I am wondering if building
> should be in the figure, that is the figure and this text match. I think it is
> important to point out in a bold manner patches need to build and need to work.
> I know this is common sense but ....

What about this figure:

http://www.plantuml.com/plantuml/png/dLDXQnf14Fr-ls8u2bNAIHBwfVP3JKrf0ur8RA41lwntSzuqjxFNsJd5Vz-zNK5ReOK8WZlptfktRzoPLoFQspPx3QlbtO_YAvN87elx2bcf9fGfpEV5nwTYTLkydLnb0JXttK5esoYCvcEukRf-1sWtM5LOmKOCiOVFTlCb2vyedsNJMn73MuI3wmNAPlZjWNZDXW6DFu0w4DmHxi5mjURIDIZ82ftHiW6FGkZV3q9TrmPqWq45o-UMl4Bj9E4Iv9vtxXcdaETRYarhj8ZzF1_wA-GgAfnh3mOgt64x4qNh9qwsKJUPIZI5nG2x2QTzGot2w35sKNpWsc3yx6eN4pwCWJoCdj2FCu3fdOi8mLyz2PwOKKNGA881nltV2GJgzwuAxHI2ivOKB7fl8hiidLJ4s_QGiF_F2-0VYK6nWrUBc5lqNFOMMOzwoN0jpi8EnPFZ5D02ti3rcl_8e1xoChMYn69U54KEBJ56vLEuaHjhBzjJu1ntit3ZBACQHijp-jx4aB3JuSzwEF9GXlM4MNnQqBBtpSNuDQjBHN4_iTJ0xvmdTPBoPgS8yMs40y13xnKs29LZ7AOPZkN7RvyULc0DiOOloYHKW_78ph3roNrCd7nfv3A6U56gXmVckcmM3k4D_mO0

@startuml

start

:Arrange your changes in\nan easy to review and\ncoherent patch series;

:Apply the checklist for patches;

:Invoke: ""git format-patch"";

:Send the patch series to devel at rtems.org for review;

:Set N to 2;

while (Reviewers demand changes in the patch series?) is (Yes)
   :Do the required changes and create a new patch series;

   :Update the commit messages accordingly;

   :Apply the checklist for patches;

   :Invoke: ""git format-patch -v $N"";

   :Document the changes from version N to N + 1\nin the patch file 
after the "---" line;

   :Set N to N + 1;

   :Send the patch series to devel at rtems.org for review;
endwhile (No)

if (Patch series was accepted by reviewers?) then (Yes)
   :Push the patch series\nto the project repository;

   note right
     Must be done by an
     RTEMS maintainer.
   end note
else (No)
   :Discard the patch series;
endif

stop

@enduml

> 
>>>
>>> Built against which BSPs?
>>
>> This is for the user manual. I think at least one arbitrary BSP which is
>> affected by the patch should be sufficient.
>>
> 
> Joel and I now have the BSP builder building on a regular basis because we found
> things broke in weird ways across archs and bsps. If we had a patch smoke test
> tool available it would help but we do not.

Yes, this is the long term goal.

> 
>>>
>>>> +* The patch does not introduce new test failures in existing tests.
>>>
>>> This step is not in the figure.
>>>
>>> Again which BSPs? This assumes expected fails are valid for the bsps being
>>> tested.
>>
>> I think we should not add to many details to the figure.
> 
> The list of steps here does not matching the figure is a potential source of
> user problems if the user inspects the figure and skims the written text.
> 
>>>
>>> What about tickets and the "Closes ...", "Updates ..." etc tags?
>>
>> Yes, this is missing. What should be checked as well?
> 
> What does "checked" mean?

I mean items (individual checks) for the Checklist for Patches.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.