llvm dev - Nov 2012 - [LLVMdev] !!! 3.2 Release branch patching and the Code Owners

Hello,

 Recent code owner activities have led to
what I would call loss of referential integrity
in the CODE_OWNERS.TXT file.

 Changes are fine but the information in the
CODE_OWNERS.TXT does not allow to positively
identify code owner of the particular
file or patch.

 The problem stems from the usage of the
"description (D)" field which is overloaded
with meaning. Most people put only a textual
description of the code they own.
This approach is fine for casual reader but
does not work for scripting or any automated
way of dealing with the build.


 I would like to propose addition of the
"folder/file (F)" field. The format
would be the same as used by Joe,Owen
and Justin

F: (path/relative/to/llvm/src/root/*)

F: (path/relative/to/llvm/src/root/file.name)

Examples:

F: (lib/Bitcode/* include/llvm/Bitcode/*)

F: (lib/CodeGen/SelectionDAG/*)

F: (lib/Target/NVPTX/*)


 Situation is particularly bad for the 3.2 branch as
"old" owner is not necessarily the same as new one.
So for the time being I have adopted the policy
of using code owner from the trunk as the one
responsible for approving the patches.

 This worked for a while but there are more and
more patches requested with no clear way of
identifying the owner. Situation has got to
the point where AI (lame as it is) embedded
in the 3.2 integration bots simply says:

 DOES NOT COMPUTE!

 Therefor I have no choice but to suspend
accepting all of the 3.2 patches until the
situation gets resolved.


Pawel

> This approach is fine for casual reader but
> does not work for scripting or any automated
> way of dealing with the build.
Will you please clarify how the automation / scripting helps with the
patch approval process?
>  I would like to propose addition of the
> "folder/file (F)" field. The format
> would be the same as used by Joe,Owen
> and Justin
This won't cover all the cases, since code in question might be
scattered across the dirs, or single dir can contain several
maintainers depending on the subject.
>  Therefor I have no choice but to suspend
> accepting all of the 3.2 patches until the
> situation gets resolved.
I'd expect from release manager to understand the llvm/clang internal
organization and deduce the correct owner in most cases.

--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University

On Nov 16, 2012, at 2:17 PM, 32bitmicro <root at 32bitmicro.com> wrote:
> Hello,
> 
> Recent code owner activities have led to
> what I would call loss of referential integrity
> in the CODE_OWNERS.TXT file.
> 
> Changes are fine but the information in the
> CODE_OWNERS.TXT does not allow to positively
> identify code owner of the particular
> file or patch.
> 
> The problem stems from the usage of the
> "description (D)" field which is overloaded
> with meaning. Most people put only a textual
> description of the code they own.
> This approach is fine for casual reader but
> does not work for scripting or any automated
> way of dealing with the build.
> 
> 
> I would like to propose addition of the
> "folder/file (F)" field. The format
> would be the same as used by Joe,Owen
> and Justin
> 
> F: (path/relative/to/llvm/src/root/*)
> 
> F: (path/relative/to/llvm/src/root/file.name)
> 
> Examples:
> 
> F: (lib/Bitcode/* include/llvm/Bitcode/*)
> 
> F: (lib/CodeGen/SelectionDAG/*)
> 
> F: (lib/Target/NVPTX/*)
> 
> 
> Situation is particularly bad for the 3.2 branch as
> "old" owner is not necessarily the same as new one.
> So for the time being I have adopted the policy
> of using code owner from the trunk as the one
> responsible for approving the patches.
> 
> This worked for a while but there are more and
> more patches requested with no clear way of
> identifying the owner. Situation has got to
> the point where AI (lame as it is) embedded
> in the 3.2 integration bots simply says:
> 
> DOES NOT COMPUTE!
> 
> Therefor I have no choice but to suspend
> accepting all of the 3.2 patches until the
> situation gets resolved.
> 
> 
Hi Pawel,

That's a bit draconic. The code owners file is a text file and not important
to the execution of LLVM. Therefore, it's fine to omit patches for it if
there are conflicts.

-bw

On Nov 16, 2012, at 2:56 PM, Bill Wendling <wendling at apple.com> wrote:
> Hi Pawel,
> 
> That's a bit draconic. The code owners file is a text file and not
important to the execution of LLVM. Therefore, it's fine to omit patches for
it if there are conflicts.
> 
That is "omit patches for the code owners file", not all patches. :)

-bw

>> This approach is fine for casual reader but
>> does not work for scripting or any automated
>> way of dealing with the build.
> Will you please clarify how the automation / scripting helps with the
> patch approval process?

Generally release patch process works like this:

- patch gets checked-in on the trunk

- developer sends message to the code owner who
approves the patch and sends the *APPROVED* to the
release manager

- somebody (not specified who in the current flow)
merges the patch on to the release_branch

- release manager creates rc1/rc2/and final branches
from the release_branch verifying that each patch
has been approved by the code owner.

This process looks good on the screen but breaks  down
in practice because:

- patches get checked-in onto the release_branch (rare)

- patches get sent to the release manager bypassing code owner

I think the root cause for this is simply the problem in
identifying the code owner by the developer.

We can solve this problem by providing code owner tool that can
map patch to the code owner or owners who should be
notified for approval.


> 
>>  I would like to propose addition of the
>> "folder/file (F)" field. The format
>> would be the same as used by Joe,Owen
>> and Justin
> This won't cover all the cases, since code in question might be
> scattered across the dirs, or single dir can contain several
> maintainers depending on the subject.
In such case it would require the code owner to add multiple
F: fields for all of the dirs and files.

To simplify selecting multiple files we would allow
wildcard matching (globing patterns) at the end of the path

F: (lib/Bitcode/* include/llvm/Bitcode/*)

F: (include/llvm/*.h)

Since code ownership might overlap files and dirs we
should allow F: field to express that.

Code owner tool could easily walk-up the hierarchy
until it reduces the number of owners to 1 or 2.
> 
>>  Therefor I have no choice but to suspend
>> accepting all of the 3.2 patches until the
>> situation gets resolved.
> I'd expect from release manager to understand the llvm/clang internal
> organization and deduce the correct owner in most cases.
Understanding the internal llvm/clang structure is easy,
deducing the correct code owner is not due to the
vague and changing nature of the CODE_OWNERS.TXT


"Exception handling, Windows codegen, ARM EABI"


> 
> --
> With best regards, Anton Korobeynikov
> Faculty of Mathematics and Mechanics, Saint Petersburg State University
> 
> 

Pawel