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

>> 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

On Nov 16, 2012, at 3:52 PM, Pawel Wodnicki <root at 32bitmicro.com>
wrote:
> 
>>> 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.

For this release, please just use the CODE_OWNERS.TXT file from 3.1.

-bw

----- Original Message -----
> From: "Bill Wendling" <wendling at apple.com>
> To: "Pawel Wodnicki" <root at 32bitmicro.com>
> Cc: "llvmdev" <llvmdev at cs.uiuc.edu>, "clang-dev
Developers" <cfe-dev at cs.uiuc.edu>
> Sent: Friday, November 16, 2012 6:19:30 PM
> Subject: Re: [LLVMdev] [cfe-dev] !!! 3.2 Release branch patching and the
Code Owners
> 
> On Nov 16, 2012, at 3:52 PM, Pawel Wodnicki <root at 32bitmicro.com>
> wrote:
> 
> > 
> >>> 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.
> 
> 
> For this release, please just use the CODE_OWNERS.TXT file from 3.1.
I don't think this helps the situation. Part of the point of declaring new
code owners was to help with the 3.2 release process. If these designations are
ambiguous, then we should make them more specific, or, make it the
responsibility of the requester to identify the appropriate code owner(s) in the
request.

 -Hal
> 
> -bw
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> 
-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory

> 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
Does not seem to me and many people around. If in doubt - ask at ML or IRC.
> "Exception handling, Windows codegen, ARM EABI"
Just for your information - this covers some lines in some files in
llvm/CodeGen, some files in lib/Target, some files in lib/Target/X86
and so on. Note that subjects are split across the files, we do not
have one-to-one or one-to-many mapping between subject and file. If
we'd follow your approach we'd need either specify too wide wildcards
(e.g. lib/CodeGen/*) or specify line ranges in the files, which does
not make any sense.

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

>> 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
> Does not seem to me and many people around. If in doubt - ask at ML or IRC.
> 
I have been through somewhat similar situations before
and the only solution that works is to have a quick and
unambiguous way of determining the required information.
Indirection through ML or IRC does not work particularly
well for a geographically spread team like llvm.
It might work locally but we are working in so many
different timezones that lag introduced by the indirection
is just another headache.
>> "Exception handling, Windows codegen, ARM EABI"
> Just for your information - this covers some lines in some files in
> llvm/CodeGen, some files in lib/Target, some files in lib/Target/X86
> and so on. Note that subjects are split across the files, we do not
> have one-to-one or one-to-many mapping between subject and file. If
> we'd follow your approach we'd need either specify too wide
wildcards
> (e.g. lib/CodeGen/*) or specify line ranges in the files, which does
> not make any sense.
I have checked-in this llvm/utils/wciia.py utility
(aka Whose Code Is It Anyway for lack of a better name).

It works of the CODE_OWNERS.TXT file as it tries to
determine the owner of the particular path or file.
Even this first rough version shows that it can be useful
without burdening the code owner with too much extra work.
It is not perfect and I have listed limitations in the
source code but it deals with overlapping ownership,
by listing all of the owners who have laid claim to the
particular path.

Anyway, it can be improved if needed, patches are welcome!

Here are couple of examples:

$ utils/wciia.py .
The owner(s) of the (.) is(are) : ['Chris Lattner']

$ utils/wciia.py lib/
The owner(s) of the (lib/) is(are) : ['Owen Anderson', 'Joe
Abbey','Justin Holewinski', 'Chris Lattner']

$ utils/wciia.py lib/Target/NVPTX/
The owner(s) of the (lib/Target/NVPTX/) is(are) : ['Justin Holewinski',
'Chris Lattner']

$ utils/wciia.py lib/Target/NAN
path (lib/Target/NAN) does not exist
> 
> --
> With best regards, Anton Korobeynikov
> Faculty of Mathematics and Mechanics, Saint Petersburg State University
> 
> 
Pawel

Hi Pawel, I guess the code owner could be noted in each source file.
Eg: in SelectionDAGBuilder.cpp, there could be a line in the comment
block at the start:

// Code owner: Owen Anderson (resistor at mac.com)

Then anyone working on a bug or with questions about code instantly knows
who to turn to.

Ciao, Duncan.

I think that the code owner process is becoming complicated and I am not sure if
it serves Chris's original intent. I don't think that we need to change
every file nor do we need an automatic tool to find the owner. I think that a
simple text file, or a section in the docs is enough.

On Nov 17, 2012, at 2:51, Duncan Sands <baldrick at free.fr> wrote:
> Hi Pawel, I guess the code owner could be noted in each source file.
> Eg: in SelectionDAGBuilder.cpp, there could be a line in the comment
> block at the start:
> 
> // Code owner: Owen Anderson (resistor at mac.com)
> 
> Then anyone working on a bug or with questions about code instantly knows
> who to turn to.
> 
> Ciao, Duncan.
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev