proposal: files with OWNERS exemptions

[John Grabowski]

All CLs require review scrutiny.  However, some CLs have simple changes that might not rise to the level of OWNERS scrutiny.

For example, http://codereview.chromium.org/10035034/ has some very simple changes to base/base.gypi (files added/removed).  Even the OWNER complained that "That must be some kind of joke. Your changes to this file don’t need my review."

I propose that *.gyp and *.gypi files be exempt from OWNERS requirements.  This will save valuable OWNER bandwidth, and would be particularly helpful for speed in the Chromium Android work.  Like any Chromium file, changes will still require committer review.  

Thoughts?

jrg

[Peter Kasting]

On Wed, Apr 25, 2012 at 5:37 PM, John Grabowski <j...@chromium.org> wrote:

I propose that *.gyp and *.gypi files be exempt from OWNERS requirements.  This will save valuable OWNER bandwidth, and would be particularly helpful for speed in the Chromium Android work.  Like any Chromium file, changes will still require committer review.

I am sympathetic to the desire to minimize useless overhead.  This is why in the past I've publicly advocated for TBRing the OWNERS portion of changes that are trivial and mechanical and thus truly do not need OWNERS to look closely.

However, others, most notably John, have rightly noted that carving out exceptions in our policy undermines the whole thing and makes it more confusing to understand what the right behavior is, more complicated to do presubmit checks for it, etc.  So there has not been team agreement on such proposals.

I suspect that similar tradeoffs apply to your proposal and thus there will be a similar lack of agreement.

FWIW, I'm not actually a huge fan of this one.  I think the filetype here is less relevant than the kind of change.  I can imagine major gyp[i] changes that really should be reviewed carefully, and many minor ones that are trivial and mechanical and thus would have fallen under my proposal of "TBR such changes".  Your proposal does have the advantage of being possible to code into the PRESUBMIT scripts, though.

PK

[Emmanuel Saint-loubert]

I strongly disagree. If you look at the mess that has become of chrome_browser.gypi (and which some of us have started to cleanup and refactor) it clearly needs *more* scrutiny.

-- Emmanuel

On Wed, Apr 25, 2012 at 5:37 PM, John Grabowski <j...@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

[John Abd-El-Malek]

This sounds good to me. I originally had the OWNERS for content not at the top level by design, so that OWNERS don't need to approve gypi changes :) But the OWNERS moved up to src\content once we got rid of "set noparent".

On Wed, Apr 25, 2012 at 5:37 PM, John Grabowski <j...@chromium.org> wrote:

[James Robinson]

On Wed, Apr 25, 2012 at 5:37 PM, John Grabowski <j...@chromium.org> wrote:

All CLs require review scrutiny.  However, some CLs have simple changes that might not rise to the level of OWNERS scrutiny.

For example, http://codereview.chromium.org/10035034/ has some very simple changes to base/base.gypi (files added/removed).  Even the OWNER complained that "That must be some kind of joke. Your changes to this file don’t need my review."

It seems like a better solution to this particular problem is to have base/base.gypi not list all of the files within subdirs of base/.  For example, base/base.gypi could set a gyp variable that is then extended by base/android/something.gypi.

I think that changes to a .gyp / .gypi that change the build configuration or dependencies are pretty interesting to OWNERS and worth looking at.  Adding/removing files typically isn't, but if someone has an OWNERS review for the file itself they typically will also have OWNERS on the .gypi listing the file, no?

- James

 

I propose that *.gyp and *.gypi files be exempt from OWNERS requirements.  This will save valuable OWNER bandwidth, and would be particularly helpful for speed in the Chromium Android work.  Like any Chromium file, changes will still require committer review.  

Thoughts?

jrg

[John Abd-El-Malek]

On Wed, Apr 25, 2012 at 5:50 PM, James Robinson <jam...@google.com> wrote:

On Wed, Apr 25, 2012 at 5:37 PM, John Grabowski <j...@chromium.org> wrote:

All CLs require review scrutiny.  However, some CLs have simple changes that might not rise to the level of OWNERS scrutiny.

For example, http://codereview.chromium.org/10035034/ has some very simple changes to base/base.gypi (files added/removed).  Even the OWNER complained that "That must be some kind of joke. Your changes to this file don’t need my review."

It seems like a better solution to this particular problem is to have base/base.gypi not list all of the files within subdirs of base/.  For example, base/base.gypi could set a gyp variable that is then extended by base/android/something.gypi.

I think that changes to a .gyp / .gypi that change the build configuration or dependencies are pretty interesting to OWNERS and worth looking at.  Adding/removing files typically isn't, but if someone has an OWNERS review for the file itself they typically will also have OWNERS on the .gypi listing the file, no?

The issue that annoyed people is that owners of subdirectories of content needed top level owners approval every time they added/renamed/deleted files in those subdirectories.

[John Abd-El-Malek]

These changes go to a number of content owners, so I don't see all of them. as these are usually rubberstamps, they don't bother me. but perhaps they feel like undue process to some?

Thinking more about it after reading the other messages, yes there have been instances where it's been good to see changes to the gypi files (I've tried to keep them simple and not turn into chrome_browser.gypi ;) ).

On Wed, Apr 25, 2012 at 5:47 PM, Erik Wright <erikw...@google.com> wrote:

Is the volume of changes such as this worth decreeing, implementing, and supporting such an exception?

[Dirk Pranke]

On Wed, Apr 25, 2012 at 5:37 PM, John Grabowski <j...@chromium.org> wrote:

There is an outstanding request for per-file OWNERS support [1] that I
hope to someday implement; it would be easy for that to support
wildcards. Typical uses for that that I can think of are the top level
DEPS file (which seems to always be TBRed for webkit at least) and the
webkit test_expectations.txt file that every gardener modifies all the
time w/o review.

That said, I don't think *gyp and *gypi files are a good use of this
feature. I've certainly seen (and made) changes to gyp files that have
introduced real bugs and needed review.

As others have noted, I think the right thing to do is probably to
refactor your files to get the levela of granularity and safety you
want instead.

-- Dirk

[1] http://code.google.com/p/chromium/issues/detail?id=119394

[John Grabowski]

I'd think TBR should be reserved for emergencies (e.g. broken build), but if that's the consensus, no problem.  

I will recommend our team TBR simple gyp/gypi changes and not wait for OWNERS.  And yes I mean simple.  

I am not proposing code land without committer review, nor am I proposing complicated gyp changes get TBRed.  Chromium has standard mechanisms for dealing with abuse (back out the CL, revoke committer bit, unsolicited negative perf, etc).

jrg