OWNERS file checking now enabled.

[Dirk Pranke]

Hi all,

We are now enforcing "OWNERS file" approvals in chromium.

This means that by default in order for you to land your change you
need an approval in your code review from someone listed in an OWNERS
file for every file you change, as documented here:

http://dev.chromium.org/developers/owners-files-1

You approve changes by adding a comment with the acronym "lgtm" (case
insensitive) to the issue. Eventually we'll add a check box to be
fancy :) We also plan to add a way to delegate approval.

This check is enforced by the presubmit hooks in git-cl and gcl. This
means you can bypass the checks with --force or --bypass-hooks (git
cl) or --no_presubmit (gcl), so you're on something of an honors
system.

The good news is that for now there is a wildcard OWNERS file at the
top of the tree, so, with few exceptions, any committer is an OWNER
for any file. The main exception is for stuff under the (newish)
"content" directory, which does not recurse up to the top file. We're
going to field test this with that directory first while we gradually
propagate the correct owners elsewhere.

For now we only check the owners on commit. Shortly (as early as later
today, or over the weekend), I will also enable the check on upload,
which will attempt to suggest reviewers for you based on the contents
of the OWNERS files and your change. I'm not enabling it yet because
the initial algorithm I used has turned out to be too annoyingly
stupid to be useful :)

Feedback welcome. Please let me know if there are any problems.

-- Dirk

[David Levin]

As we move to enforce this more broadly, are we going to move the DEPS file? :)

If not, gardening is going to be a lot more painful...

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Dirk Pranke]

The DEPS file and the test_expectations file will be wildcarded for
the foreseeable future,
as will any other file that is touched by a great many people.

We have talked about adding glob-based rules and rules that track
directory modifications
(files or directories being added or deleted, but not changes to
existing files) as other
ways to minimize the notification burden as well.

It is my hope that the OWNERS implementation turns out to be a net
improvement in
productivity (mostly through suggesting the right reviewers and
streamlining the approval
process). If it isn't, please let me know.

-- Dirk

[Gaurav Shah]

+chromium-os-dev

Since this is a git-cl side check, will this also work for chromium-os
repos? (I am guessing we'll need one at least one OWNER file per
repo). Also, how does git-cl handle commits if no OWNERS file was
found (top level or in the subtree)?

On Fri, Mar 18, 2011 at 9:20 AM, Dirk Pranke <dpr...@chromium.org> wrote:

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
>    http://groups.google.com/a/chromium.org/group/chromium-dev
>

--
-g

[Dirk Pranke]

It would work for chormium-os as well, but my understanding is that
you guys are using Gerrit and hence don't need or want OWNERS files?

-- Dirk

[Thiago Farina]

Just a reminder,

When reviewing a patch, please use "LGTM" instead of just "LG",
otherwise a presubmit warning will be emitted.

[Scott Violet]

Dirk,

It looks like you are explicitly looking for LGTM. Many folks use LG
and OK as well. Can the script look for those as well?

-Scott

-Scott

On Fri, Mar 18, 2011 at 9:20 AM, Dirk Pranke <dpr...@chromium.org> wrote:

[Denis Lagno]

and what about conditional sentences like: "You will get my LGTM only
after adding unit tests"?

[Miranda Callahan]

[correct acct]

I don't think we need to load down (or completely remove) this check
for the sake of NLP heuristics -- you'll always be able to override
with a flag for these edge case, right? (Having said that, +1 to
adding LG).

[Dirk Pranke]

Yes, it's easy to change the heuristics. I'll update the wiki with the
revised rules and reply to this when the change has landed.

-- Dirk

[Marc-Antoine Ruel]

I prefer to keep the heuristics simple, especially that the commit queue has the same regexp. It also simplifies audit, if ever necessary.

Unless you tell me typing two more letters lowers your productivity. :)

The other solution is to add a 'lgtm' button to rietveld but I'd prefer not go that route, that would kill review over email the same way.

M-A

[Scott Violet]

It's not the typing that is hard, it's the unforgetting OK and LG.

-Scott

[Anush Elangovan(அனுஷ்)]

On Fri, Mar 18, 2011 at 1:45 PM, Dirk Pranke <dpr...@chromium.org> wrote:

It would work for chormium-os as well, but my understanding is that
you guys are using Gerrit and hence don't need or want OWNERS files?

We are still in the transition to gerrit, but once we fully transition over we get the same effect as OWNERS files on a per repo level. 

Thanks

Anush

[Denis Lagno]

I see that currently presubmit check emits text like:

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/browser/tab_contents/tab_contents.cc

I'd be more useful if it emitted list of owners that are required to
approve, so to take off burden to scan OWNERS files manually.

[Dirk Pranke]

I can add that.

-- Dirk

[Denis Lagno]

cool, do I understand correctly that for complex CLs this list is not
actually list but some expression like (avi@ or brettw@) and jam@ ?

[Dirk Pranke]

Not exactly, I think it depends on what you mean by "this list".

Any given OWNERS file is simply a list of email addresses (possibly
including a wildcard).

However, if you have a change that touches files in multiple
directories with different sets
of OWNERS, you can end up needing logic like you describe.

-- Dirk

[Lei Zhang]

I tried checking in r78963 for tools/heapcheck/suppressions.txt and
tools/valgrind/memcheck/suppressions.txt. There's no OWNERS file
anywhere in either tools/heapcheck or tools/valgrind, so it should
default to the top level OWNERS file of *, but I got:

Running presubmit hooks...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:

tools/heapcheck/suppressions.txt,tools/valgrind/memcheck/suppressions.txt

Any idea what went wrong?

On Fri, Mar 18, 2011 at 9:20 AM, Dirk Pranke <dpr...@chromium.org> wrote:

[Dirk Pranke]

I think the TBR= confused it. There are some known bugs in how it is
handling TBRs that I am just now testing fixes for.

-- Dirk

[David Levin]

Is there a command to get the list of which people still need to send LGTM for a change (before attempting the commit)?

Thanks,

dave

[Dirk Pranke]

No, but it is on my list of things to do.

-- Dirk

[Thomas Van Lenten]

Got a new error:

  Issue creation errors: {'reviewers': [u'Unknown user: *']}

I think it's because my CL spans a few directories, so the R= line got a bunch of real folks, and a '*'.

TVL

[Peter Kasting]

On Mon, Mar 21, 2011 at 10:02 AM, Scott Violet <s...@chromium.org> wrote:

It's not the typing that is hard, it's the unforgetting OK and LG.

FWIW, I'd prefer not to add these.  "OK" I use frequently in conversations on a review, so it doesn't indicate global approval.  "LG"  is part of other words, and for people who do this it's easier (than with "OK") to retrain to also add "TM" on the end.  I think the short-term transition pain is better than long-term heuristic complexity and false approval positives.

PK

[Dirk Pranke]

Yup, this is a known bug that I have a patch pending for. The "*" is
not supposed to be included in the line.

-- Dirk

[Greg Simon]

How do we deal with the situation that a folder has *one* entry in the OWNERS file and that person is on vacation/out of pocket ?

[Dirk Pranke]

Normally OWNERS files recurse up to the top of the tree, so any owners
in any parent directories would
cover for that person.

But, if the OWNERS file in question also had "set noparent", you'd
have a problem, in which case you'd use --force or
--bypass-hooks to override the warning, and then (hopefully) have
someone check in a new OWNERS file
with more OWNERS (using the same mechanism).

It might be a good idea to make sure every OWNERS file always has at
least two owners to minimize this risk.

Let's see how often this comes up in practice before trying to
implement a solution.

-- Dirk

[Peter Kasting]

On Wed, Mar 23, 2011 at 5:09 PM, Greg Simon <greg...@google.com> wrote:

How do we deal with the situation that a folder has *one* entry in the OWNERS file and that person is on vacation/out of pocket ?

Either no one is capable of doing an appropriate review for those changes, in which case the best thing to do is to backburner the change until said person can get back (and perhaps suggest they try and bring another engineer up to speed on their code), or else someone is, in which case we should probably change the OWNERS file.

It's easy to suggest that the change be checked in with --force or similar, TBR-style, but part of the reason for our rapid release cycle is to eliminate pressures to "get this change in now and review later, we've got to have it".

PK

[Thomas Van Lenten]

On Thu, Mar 24, 2011 at 2:34 PM, Robert Sesek <rse...@chromium.org> wrote:

On Fri, Mar 18, 2011 at 2:25 PM, Dirk Pranke <dpr...@chromium.org> wrote:

It is my hope that the OWNERS implementation turns out to be a net
improvement in
productivity (mostly through suggesting the right reviewers and
streamlining the approval
process). If it isn't, please let me know.

Has automatically adding reviewers and putting super-long R= lines in commit descriptions been added as part of this rollout? If so, can we disable that? Having these obnoxiously long R= lines just makes commit messages cluttered and doesn't reflect reality. If R= were to be automatically added, it should be at commit time, not at upload, and be the people whom issued the LGTM. I'd prefer no behavior around this, though.

See http://codereview.chromium.org/6705030/ for an example of how ridiculous this is.

I believe adding the R= line is how it knows who to send the reviews too in the first place.

(and since that's my CL, it would be longer, but I removed the "*," that caused other errors by being included.)  :)

TVL

 

rsesek / @chromium.org

[Robert Sesek]

On Fri, Mar 18, 2011 at 2:25 PM, Dirk Pranke <dpr...@chromium.org> wrote:

It is my hope that the OWNERS implementation turns out to be a net
improvement in
productivity (mostly through suggesting the right reviewers and
streamlining the approval
process). If it isn't, please let me know.

Has automatically adding reviewers and putting super-long R= lines in commit descriptions been added as part of this rollout? If so, can we disable that? Having these obnoxiously long R= lines just makes commit messages cluttered and doesn't reflect reality. If R= were to be automatically added, it should be at commit time, not at upload, and be the people whom issued the LGTM. I'd prefer no behavior around this, though.

See http://codereview.chromium.org/6705030/ for an example of how ridiculous this is.

rsesek / @chromium.org

[Dirk Pranke]

Hi all,

First, thanks for bearing with me over the week. The code has been
buggier than I had hoped it would be (mostly due to differences in the
way gcl and git-cl do things), and in addition, we've been changing
what we actually want the tools to do. Let me sum up where we're at:

1) There are still a couple of open bugs that may be screwing things
up. I hope to land both of them shortly, but any CLs that you've
created over the past couple days may be treated somewhat
inconsistently, so I apologize in advance. In particular, handling
TBR= is a bit broken.

2) The initial algorithm I used to find suggested reviewers was very
brain dead; it simply listed everyone it found. I disabled this for
git-cl before I enabled the checks, but missed a path though gcl, so
you gcl users have been seeing the stupidly-long R= lines. As of last
night, those should be gone now as well.

The net result is that right now both git-cl and gcl should not be
adding R= lines to your changes under any circumstances; the only
check that should be happening is the actual LGTM check on submit.

In the next couple days, I plan to modify the logic in the following ways:

1) When an issue is first created, we should suggest a list of
reviewers; that list should be kept to a maximum length of either 1-3
people (depending on how smart I can get things) or the minimal
covering set of owners needed.

2) Subsequent changes to the issue or uploads will not automatically
modify *any* part of your CL or description. Ideally we'll eventually
add an optional field suggesting additional reviewers if you modify
the fields affected by your CL, but this may take some experimentation
to make useful.

3) On submit, your description will be modified to include an R= line
with the people that actually approved your change (unless you have a
TBR in your description already).

4) There will be two new commands added to git-cl and gcl to suggest
reviewers and tell which approvals are still needed for your review.

It's pretty clear that there's no single policy that will make
everyone happy, but I'll try to get the best compromise I can. As
always, feel free to report and bugs/issues/suggestions to me. If you
are filing bugs against this, the Area is "Build" and you can use the
"Build-OWNERS" label as well.

-- Dirk

[Alexander Potapenko]

Can we request an exception for
tools/valgrind/{memcheck,tsan}/suppressions*.txt and
tools/heapcheck/suppressions.txt?
These files are used to suppress the errors on the memory bots and
should be edited by anyone who's in the Memory sheriff rotation (and
ideally many of those who aren't, because we want to encourage people
to at least suppress their bugs)

--
Alexander Potapenko
Software Engineer
Google Moscow

[Marc-Antoine Ruel]

From what I understand, add a OWNERS file with * in it in tools/valgrind.

[Dirk Pranke]

What he said. You can use
webkit/tools/layout_tests/test_expectations.txt as an example.

(There are also names in that file because there are other files in
that directory).

It won't stop you from getting prompted, but anyone can approve it. So
far we have not
agreed to have a "changes to this file don't ever need to be reviewed" concept.

-- Dirk

[Scott Hess]

On Wed, Mar 23, 2011 at 5:29 PM, Peter Kasting <pkas...@chromium.org> wrote:
> On Wed, Mar 23, 2011 at 5:09 PM, Greg Simon <greg...@google.com> wrote:
>> How do we deal with the situation that a folder has *one* entry in the
>> OWNERS file and that person is on vacation/out of pocket ?
>
> Either no one is capable of doing an appropriate review for those changes,
> in which case the best thing to do is to backburner the change until said
> person can get back (and perhaps suggest they try and bring another engineer
> up to speed on their code), or else someone is, in which case we should
> probably change the OWNERS file.

I think this response should be stronger: Code shouldn't be allowed
into the tree unless there are multiple people willing to be in
OWNERS. If there is literally only one person capable of
understanding the code, then the code cannot possibly be getting good
reviews.

-scott

[Dirk Pranke]

I wouldn't sweat this too much for now. While we have a lot of files
with only one OWNER
listed, we don't have any marked 'set noparent' at the moment, which
means that every
change can still be reviewed by anyone if necessary to pacify the trigger.

I agree that we should eventually try to get better coverage across
the directories, though.

-- Dirk

I wouldn't sweat this too much. AFAIK, we don't have
>
> -scott