depot_tools: modify cpplint.py to allow CPPLINT.cfg overrides

depot_tools: modify cpplint.py to allow CPPLINT.cfg overrides

Added the ability to provide CPPLINT.cfg files to provide
linter message filters per sub-directory and special exclusion
rules. Each file can have instructions like:

filter=-build/include_order,+build/include_alpha
exclude_files=.*\.cc

The above disables build/include_order warning and enables
build/include_alpha as well as excludes all .cc from being
processed by linter, in the current directory (where the .cfg
file is located) and all sub-directories.

Related CL: https://chromium-review.googlesource.com/#/c/209384/

BUG=chromium:395296
TEST=ran cpplint.py on a bunch of directories in platform2/

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=285999

[Alex Vakulenko (Google), 2014-07-22 22:53:48]

In order to address https://code.google.com/p/chromium/issues/detail?id=395296
this is an attempt to modify cpplint.py to look for config overrides in local
directories.

With these changes, cpplint.py, as it processes a particular .cc/.h file, it
will start looking for CPPLINT.cfg file in the same directory as the cpp file
and goes up in the directory tree. It then loads all discovered .cfg files from
top-level dir to the bottom and reads statements like
"filter=+build/include_order,-build/include_alpha" and "exclude_files=.*\.cc"
and applies these config changes for the file being processed.

This allows us to override properties or exclude whole files on per-directory
level. This way, we can add exclude_files=.* to
src/platform2/minijail/CPPLINT.cfg and omit the whole minijail from being run
trough linter.

[sosa, 2014-07-23 17:35:00]

https://codereview.chromium.org/406373002/diff/1/cpplint.py
File cpplint.py (right):

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode685
cpplint.py:685: # backup of filter list. Used to restore the state after each
file.
indentation also this seems super private to the class so _filters_backup would
be more appropriate.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode814
cpplint.py:814: """Adds more filter overrides. Unlike _SetFilters, this function
does not
Unlike should be on a newline. We usually keep the first line of a docstring in
one line. Usual format:

Short description in one line.
<BREAK>
Longer description in many lines.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5480
cpplint.py:5480: """ Loads the configuration file and processes the config
overrides
nit about docstring

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5483
cpplint.py:5483: Config file contains a bunch of key=value pairs. Currently
This should probably go in the module docstring / help. Also maybe a commented
out example file would be nice.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5497
cpplint.py:5497: #lines = codecs.open(cfg_file, 'r', 'utf8',
'replace').read().split('\n')
What's the line commented out for?

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5498
cpplint.py:5498: with open(cfg_file) as lines:
Not completely correct naming. This is the file_handle that you can iterate with
as for line in file_handle

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5500
cpplint.py:5500: name, val = line.partition('=')[::2]
mmm, this works but not really the correct usage of a step. I usually see this
done as:

name, _, val = line.partition('=')

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5501
cpplint.py:5501: name = name.strip()
val = val.strip() here

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5504
cpplint.py:5504: elif name == 'exclude_files':
Should support comments but also error out for names that don't match so we
avoid people making bad files that are silently ignored.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5518
cpplint.py:5518: 
2 lines should separate top-level methods

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5520
cpplint.py:5520: """ Loads the configuration files and processes the config
overrides
same nit about docstrings.

[Alex Vakulenko (Google), 2014-07-23 19:04:51]

Fixed review issues

https://codereview.chromium.org/406373002/diff/1/cpplint.py
File cpplint.py (right):

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode685
cpplint.py:685: # backup of filter list. Used to restore the state after each
file.
On 2014/07/23 17:35:00, sosa wrote:
> indentation also this seems super private to the class so _filters_backup
would
> be more appropriate.

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode814
cpplint.py:814: """Adds more filter overrides. Unlike _SetFilters, this function
does not
On 2014/07/23 17:35:00, sosa wrote:
> Unlike should be on a newline. We usually keep the first line of a docstring
in
> one line. Usual format:
> 
> Short description in one line.
> <BREAK>
> Longer description in many lines.

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5480
cpplint.py:5480: """ Loads the configuration file and processes the config
overrides
On 2014/07/23 17:35:00, sosa wrote:
> nit about docstring

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5483
cpplint.py:5483: Config file contains a bunch of key=value pairs. Currently
On 2014/07/23 17:35:00, sosa wrote:
> This should probably go in the module docstring / help. Also maybe a commented
> out example file would be nice.

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5497
cpplint.py:5497: #lines = codecs.open(cfg_file, 'r', 'utf8',
'replace').read().split('\n')
On 2014/07/23 17:35:00, sosa wrote:
> What's the line commented out for?

Just forgot to remove. Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5498
cpplint.py:5498: with open(cfg_file) as lines:
On 2014/07/23 17:35:00, sosa wrote:
> Not completely correct naming. This is the file_handle that you can iterate
with
> as for line in file_handle

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5500
cpplint.py:5500: name, val = line.partition('=')[::2]
On 2014/07/23 17:35:00, sosa wrote:
> mmm, this works but not really the correct usage of a step. I usually see this
> done as:
> 
> name, _, val = line.partition('=')

I copied this from somewhere.
Fixed

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5501
cpplint.py:5501: name = name.strip()
On 2014/07/23 17:35:00, sosa wrote:
> val = val.strip() here 

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5504
cpplint.py:5504: elif name == 'exclude_files':
On 2014/07/23 17:35:00, sosa wrote:
> Should support comments but also error out for names that don't match so we
> avoid people making bad files that are silently ignored.

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5518
cpplint.py:5518: 
On 2014/07/23 17:35:00, sosa wrote:
> 2 lines should separate top-level methods

Done.

https://codereview.chromium.org/406373002/diff/1/cpplint.py#newcode5520
cpplint.py:5520: """ Loads the configuration files and processes the config
overrides
On 2014/07/23 17:35:00, sosa wrote:
> same nit about docstrings.

Done.

[Alex Vakulenko (Google), 2014-07-23 20:16:50]

Fixed a case where build/include_alpha fails on non-#include line that precedes
an #include, like in:

extern "C" {
#include "blah"
}

[sosa, 2014-07-23 20:25:42]

Technically looks good, but I'm not an owner here so I'm assuming someone from
depot_tools can jump in.

Also, it seems like these are changes we need to either upstream or should be
only done in cpplint_chromium.py (you can override the specific methods). I'll
let someone else jump in with a more authoritative answer.

https://codereview.chromium.org/406373002/diff/20001/cpplint.py
File cpplint.py (right):

https://codereview.chromium.org/406373002/diff/20001/cpplint.py#newcode5488
cpplint.py:5488: """ Looks for CPPLINT.cfg files in the directory containing
|filename| and
docstring issue

[Alex Vakulenko (Google), 2014-07-23 20:44:32]

Fixed last comment

https://codereview.chromium.org/406373002/diff/20001/cpplint.py
File cpplint.py (right):

https://codereview.chromium.org/406373002/diff/20001/cpplint.py#newcode5488
cpplint.py:5488: """ Looks for CPPLINT.cfg files in the directory containing
|filename| and
On 2014/07/23 20:25:42, sosa wrote:
> docstring issue

Done.

[Alex Vakulenko (Google), 2014-07-24 17:25:51]

Since I'm not sure who would be the best person to review this, I added everyone
from depot_tools/OWNERS file...

[iannucci, 2014-07-24 19:17:54]

maruel looks like the likely best reviewer based on blame.

[M-A Ruel, 2014-07-25 05:35:43]

On 2014/07/23 20:25:42, sosa wrote:
> Also, it seems like these are changes we need to either upstream or should be
> only done in cpplint_chromium.py (you can override the specific methods). I'll
> let someone else jump in with a more authoritative answer.

Chris is right, it should be put upstream first. This file is a third party in
practice.

[Alex Vakulenko (Google), 2014-07-25 15:01:07]

On 2014/07/25 05:35:43, M-A Ruel wrote:
> On 2014/07/23 20:25:42, sosa wrote:
> > Also, it seems like these are changes we need to either upstream or should
be
> > only done in cpplint_chromium.py (you can override the specific methods).
I'll
> > let someone else jump in with a more authoritative answer.
> 
> Chris is right, it should be put upstream first. This file is a third party in
> practice.

Sure I can do that. But the version in google style SVN is identical to that of
depot_tools (with the exception of the very first line with the script's
shebang). So putting it in SVN first or in depot tools doesn't really make much
difference and can be done quickly. What I'm more concerned about is the actual
approach I'm taking in this change. Are you guys Ok with it? Can you review it
and see if that's Ok to have it work like that. And then I can port this change
to the SVN repo and once that lands I can land this one (or do it in parallel).

[Elliot Glaysher, 2014-07-25 17:29:20]

(I don't actually know enough python to credibly review this.)

[Elliot Glaysher, 2014-07-25 22:26:35]

On 2014/07/25 05:35:43, M-A Ruel wrote:
> On 2014/07/23 20:25:42, sosa wrote:
> > Also, it seems like these are changes we need to either upstream or should
be
> > only done in cpplint_chromium.py (you can override the specific methods).
I'll
> > let someone else jump in with a more authoritative answer.
> 
> Chris is right, it should be put upstream first. This file is a third party in
> practice.

There are no upstream owners that are knowledgeable enough to set policy here.

I'm willing to stamp the changes upstream, but I need someone who is
knowledgeable in python to review the approach taken here. I'll rubber stamp
whatever the consensus is here code wise in the upstream repository.

One thing that does jump out (and this should be handled as chrome policy) is
whether CPPLINT.cfg is a good name or not. My immediate reaction here is that it
should be a dot file, like various .gn and .gitattributes stuff, but if chrome
depot_tools owners want it unhidden, I'll just stamp whatever.

[Alex Vakulenko (Google), 2014-07-25 22:32:43]

> One thing that does jump out (and this should be handled as chrome policy) is
> whether CPPLINT.cfg is a good name or not. My immediate reaction here is that
it
> should be a dot file, like various .gn and .gitattributes stuff, but if chrome
> depot_tools owners want it unhidden, I'll just stamp whatever.

I also don't like the name CPPLINT.cfg particularly too much, because of
ALLCAPS. The only reason why I did it is to be consistent with existing
conventions like PRESUBMIT.cfg. But I'd be happy to rename this to something
like ".cpplint-config" if there are votes in favor of it.

[Alex Vakulenko (Google), 2014-07-28 18:42:29]

On 2014/07/25 22:32:43, Alex Vakulenko (Google) wrote:
> > One thing that does jump out (and this should be handled as chrome policy)
is
> > whether CPPLINT.cfg is a good name or not. My immediate reaction here is
that
> it
> > should be a dot file, like various .gn and .gitattributes stuff, but if
chrome
> > depot_tools owners want it unhidden, I'll just stamp whatever.
> 
> I also don't like the name CPPLINT.cfg particularly too much, because of
> ALLCAPS. The only reason why I did it is to be consistent with existing
> conventions like PRESUBMIT.cfg. But I'd be happy to rename this to something
> like ".cpplint-config" if there are votes in favor of it.

So, is anyone interested in contributing their opinions on this matter? If not,
I'd like to proceed with landing this CL as it blocks another bug on Chrome OS
side.

[M-A Ruel, 2014-07-28 19:04:40]

My observations;
- It's not malware.
- There's new code is doing things that may have value to some people.

That's enough for me. lgtm.

[Elliot Glaysher, 2014-07-28 19:11:16]

lgtm

I'm not in love with CPPLINT.cfg, but there's precedent with BUILD.gn,
PRESUBMIT.py, OWNERS, WATCHLISTS, and other SCREAMING.files.

[sosa, 2014-07-28 20:28:46]

lgtm w/ nits

https://codereview.chromium.org/406373002/diff/80001/cpplint.py
File cpplint.py (right):

https://codereview.chromium.org/406373002/diff/80001/cpplint.py#newcode5508
cpplint.py:5508: break  # Reached the root directory.
Too much indentation

https://codereview.chromium.org/406373002/diff/80001/cpplint.py#newcode5510
cpplint.py:5510: if os.path.isfile(cfg_file):
This is very nesty. I'd consider negating these if's with continue so you don't
have to indent as much. I'll leave it up to you (this one here and the
line.strip below are a couple examples).

[sosa, 2014-07-28 20:28:47]

lgtm w/ nits

[Alex Vakulenko (Google), 2014-07-28 22:11:02]

The CQ bit was checked by avakulenko@google.com

[commit-bot: I haz the power, 2014-07-28 22:11:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avakulenko@google.com/406373002/140001

[commit-bot: I haz the power, 2014-07-28 22:13:35]

Change committed as 285999

[Jason Robbins -- corp, 2014-12-03 22:02:25]

jrobbins@google.com changed reviewers:
- erg@chromium.or