Implement ability to specify temporarily-allowed dependencies in DEPS

Implement ability to specify temporarily-allowed dependencies in DEPS
files, and use this ability in a few DEPS files where appropriate.
This has no effect on the normal running of checkdeps; "!"
dependencies are treated just like "+" dependencies when checkdeps is
run on our bots.

An upcoming change will use the new checkdeps.CheckAddedIncludes
function, and will error out if you add a new include that violates a
"-" rule, and show a presubmit warning when you add a new include that
violates a "!" rule (the warning will say something like "We are in
the process of removing dependencies from this directory to that file,
can you avoid adding more?"

While I was in there, fixed path handling so that checkdeps will work
on case-sensitive platforms with paths that include upper-case
characters (e.g. a checkout of Chrome at ~/c/Chrome/src rather than
~/c/chrome/src).

Since the pipes.quote method seems unreliable on Windows (it failed on
my setup), switched to subprocess.list2cmdline which I believe is
stable.

Added a small manual testing mode to checkdeps.  It currently only
verifies the CheckAddedIncludes function.

TBR=jam@chromium.org
BUG=138280
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149163

[Jói, 2012-07-20 18:42:50]

brettw: Main reviewer.
sky: chrome/browser/ui OWNER.

I've tested the checkdeps functionality manually on Windows and Linux, and added
a semi-automated test for the API that the PRESUBMIT.py script will use.

Cheers,
Jói

[Jói, 2012-07-20 18:45:29]

Meant to include jam@ on reviewers list so he is aware of the DEPS file changes
related to remaining temporary content/ includes.

Cheers,
Jói

[M-A Ruel, 2012-07-20 19:57:00]

This change will conflict with https://chromiumcodereview.appspot.com/10790014/

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py#n...
tools/checkdeps/checkdeps.py:245: (dir_name in GIT_SOURCE_DIRECTORY)):
are you sure?

[Jói, 2012-07-20 20:01:41]

Thanks for the heads up on the conflict.  Iain's change has been going for a few
days so it's fair that his goes in first.  I'll upload a new patch once I've
merged and re-tested.

Cheers,
Jói

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py#n...
tools/checkdeps/checkdeps.py:245: (dir_name in GIT_SOURCE_DIRECTORY)):
On 2012/07/20 19:57:01, Marc-Antoine Ruel wrote:
> are you sure?

Yes, or Linux (and Mac, I guess) is broken for checkouts that reside at a path
that includes capital letters.

[jam, 2012-07-20 20:08:49]

nice, just one comment

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py#n...
tools/checkdeps/checkdeps.py:27: show a warning if you add a new include of a
file that is only allowed
it seems that this should be an error for presubmit so that no new instances are
added? btw this is what i did for things like scopedallowio etc

[brettw, 2012-07-20 21:25:02]

This seems fine in concept. I'm sure maruel can review the python code, I didn't
read it.

[Jói, 2012-07-20 21:57:39]

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py#n...
tools/checkdeps/checkdeps.py:27: show a warning if you add a new include of a
file that is only allowed
On 2012/07/20 20:08:49, John Abd-El-Malek wrote:
> it seems that this should be an error for presubmit so that no new instances
are
> added? btw this is what i did for things like scopedallowio etc

In some cases, adding no new includes may be difficult to do in practice until
dependency breaking has been achieved.  I thought a warning would be good to
begin with, it's considerably more than what we have now (which is documentation
in a DEPS file that you may or may not see) without going all the way to a more
draconian rule (that may get overridden anyway by --bypass-hooks).  I'm pretty
much fine either way though.

[jam, 2012-07-20 23:23:33]

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/1/tools/checkdeps/checkdeps.py#n...
tools/checkdeps/checkdeps.py:27: show a warning if you add a new include of a
file that is only allowed
On 2012/07/20 21:57:39, Jói wrote:
> On 2012/07/20 20:08:49, John Abd-El-Malek wrote:
> > it seems that this should be an error for presubmit so that no new instances
> are
> > added? btw this is what i did for things like scopedallowio etc
> 
> In some cases, adding no new includes may be difficult to do in practice until
> dependency breaking has been achieved.  I thought a warning would be good to
> begin with, it's considerably more than what we have now (which is
documentation
> in a DEPS file that you may or may not see) without going all the way to a
more
> draconian rule (that may get overridden anyway by --bypass-hooks).  I'm pretty
> much fine either way though.

I don't feel strongly about this either, it's a personal preference (I would
have done it that way), but since you wrote it, feel free to pick whatever you
prefer.

[Jói, 2012-07-25 17:16:32]

Marc-Antoine:  Could you take a look?  I've merged after the large Java-checking
patch landed.

I decided to leave the temp-allowed checking as a C++ only thing for now, but
left myself a TODO to enable it for Java.  If it's desired for such code it
shouldn't be a ton of work, but this change has been open for a while, and I
want to get it in ASAP so that it starts helping us avoid unwanted dependencies.

I split the Rule and Rules classes into a separate file to avoid a circular
import (since cpp_checker.py and java_checker.py now need to use the Rule
class).

Cheers,
Jói

[Jói, 2012-07-26 10:04:16]

maruel: Friendly ping.

On Wed, Jul 25, 2012 at 5:16 PM,  <joi@chromium.org> wrote:
> Marc-Antoine:  Could you take a look?  I've merged after the large
> Java-checking
> patch landed.
>
> I decided to leave the temp-allowed checking as a C++ only thing for now,
> but
> left myself a TODO to enable it for Java.  If it's desired for such code it
> shouldn't be a ton of work, but this change has been open for a while, and I
> want to get it in ASAP so that it starts helping us avoid unwanted
> dependencies.
>
> I split the Rule and Rules classes into a separate file to avoid a circular
> import (since cpp_checker.py and java_checker.py now need to use the Rule
> class).
>
> Cheers,
> Jói
>
>
> http://codereview.chromium.org/10805042/

[M-A Ruel, 2012-07-26 13:06:02]

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:112: if
os.path.normcase(os.path.normpath(cur_dir)).startswith(
normcase is totally useless:
http://docs.python.org/library/os.path.html#os.path.normcase

I implemented the whole thing but it's "complicated". See the 3
get_native_path_case() implementations in:
http://src.chromium.org/viewvc/chrome/trunk/src/tools/isolate/trace_inputs.py...

This function is unit tested to work on each OS we care about. But that's
totally overkill in your case. The reason is simple: we do not allow in the
source tree 2 files in a directory where file1.lower() == file2.lower(). As
such, you can reduce the check to:

if os.path.normpath(cur_dir).lower().startswith(
    os.path.normpath(BASE_DIRECTORY).lower()):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:156: (dir_name in GIT_SOURCE_DIRECTORY)):
Same here:
any(dir_name.lower() == i.lower() for i in GIT_SOURCE_DIRECTORY)

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:303: def TestCheckAddedIncludes():
This function should be in a checkdeps_test.py and then calls from a
corresponding PRESUBMIT.py in the directory.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:304: """Run this by giving the argument
'manualtest' to checkdeps."""
Runs this

What is "this" ?

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:385: global BASE_DIRECTORY
It'd be simpler if it was not a global. Same for GIT_SOURCE_DIRECTORY, globals
really complicate importing this file.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py
File tools/checkdeps/rules.py (right):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:8: # Specifies a single rule for an include, which can
be either allow or disallow.
Why not a class docstring?

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:41: if len(rule_string) < 1:
if not rule_string:  ?

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:42: raise Exception('The rule string "%s" is too
short\nin %s' %
is empty

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:53: class Rules:
class Rules(object):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:59: ret = "Rules = [\n"
return 'Rules = [\n%s]' % ''.join(' %s\n' % x for x in self._rules)
would generate a better formatting.

[Jói, 2012-07-26 13:32:12]

Thanks Marc-Antoine!  I've uploaded a new version that responds to your
feedback, see comments below.

Cheers,
Jói

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:112: if
os.path.normcase(os.path.normpath(cur_dir)).startswith(
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> normcase is totally useless:
> http://docs.python.org/library/os.path.html#os.path.normcase
> 
> I implemented the whole thing but it's "complicated". See the 3
> get_native_path_case() implementations in:
>
http://src.chromium.org/viewvc/chrome/trunk/src/tools/isolate/trace_inputs.py...
> 
> This function is unit tested to work on each OS we care about. But that's
> totally overkill in your case. The reason is simple: we do not allow in the
> source tree 2 files in a directory where file1.lower() == file2.lower(). As
> such, you can reduce the check to:
> 
> if os.path.normpath(cur_dir).lower().startswith(
>     os.path.normpath(BASE_DIRECTORY).lower()):

Done.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:156: (dir_name in GIT_SOURCE_DIRECTORY)):
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> Same here:
> any(dir_name.lower() == i.lower() for i in GIT_SOURCE_DIRECTORY)

Done, by making the paths lower-case when adding to GIT_SOURCE_DIRECTORY and
checking dir_name.lower() in GIT_SOURCE_DIRECTORY here.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:303: def TestCheckAddedIncludes():
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> This function should be in a checkdeps_test.py and then calls from a
> corresponding PRESUBMIT.py in the directory.

Made it a separate checkdeps_manualtest.py file (manualtest since it requires
manual validation of results, and may require updates to the test data based on
updates to checked-in files in the main Chrome source tree).  I don't think it's
right to put this test in a presubmit step because of the manual nature of
validation.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:304: """Run this by giving the argument
'manualtest' to checkdeps."""
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> Runs this
> 
> What is "this" ?

Removed this comment, it is meaningless in the new location of the test.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:385: global BASE_DIRECTORY
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> It'd be simpler if it was not a global. Same for GIT_SOURCE_DIRECTORY, globals
> really complicate importing this file.

I'm not sure I follow, I get that it's harder to use checkdeps as a module
because of the globals if you want to invoke multiple times on it, but it seems
fine for our current purposes?  Maybe I'm missing some complexity about
importing a module with globals that I'm unaware of?  They are really just
module scoped, not truly global, as far as I understand.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py
File tools/checkdeps/rules.py (right):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:8: # Specifies a single rule for an include, which can
be either allow or disallow.
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> Why not a class docstring?

Done.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:41: if len(rule_string) < 1:
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> if not rule_string:  ?

Done.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:42: raise Exception('The rule string "%s" is too
short\nin %s' %
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> is empty

Done.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:53: class Rules:
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> class Rules(object):

Done.

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:59: ret = "Rules = [\n"
On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> return 'Rules = [\n%s]' % ''.join(' %s\n' % x for x in self._rules)
> would generate a better formatting.

Done.

[M-A Ruel, 2012-07-26 13:50:51]

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:385: global BASE_DIRECTORY
On 2012/07/26 13:32:13, Jói wrote:
> On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> > It'd be simpler if it was not a global. Same for GIT_SOURCE_DIRECTORY,
globals
> > really complicate importing this file.
> 
> I'm not sure I follow, I get that it's harder to use checkdeps as a module
> because of the globals if you want to invoke multiple times on it, but it
seems
> fine for our current purposes?  Maybe I'm missing some complexity about
> importing a module with globals that I'm unaware of?  They are really just
> module scoped, not truly global, as far as I understand.

global as in module global, since this is python.

The problem is that CommonSetup() makes the calls implicitly stateful. So, if
two different PRESUBMIT.py imports checkdeps, the second one could (or could
not, depends if the module reference is leaked) reuse the state from the first
one.

This can be problematic for example if 2 presubmits are run for multiple
checkouts, since GIT_SOURCE_DIRECTORY will be wrong on the second one. This may
happen with complex gcl changes, but is rare with git cl.

Overall, this is a potential for Major Fun. This is why I first recommended for
shelling out instead of importing. I'd recommend to do one or the other:
1. shell out in PRESUBMIT
2. Get rid of BASE_DIRECTORY and GIT_SOURCE_DIRECTORY and pass them as
parameters. VERBOSE is borderline in that case, probably a good idea to do it
too.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:112: if
os.path.normpath(cur_dir).lower().startswith(
BTW, is cur_dir an absolute path?

FYI only: I like to sprinkle asserts and enforce absolute paths. But that means
realpath() must be called consistently otherwise symlinks will get in the way.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:335: BASE_DIRECTORY = os.path.abspath(
Did you mean normpath?

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:336:
os.path.join(os.path.abspath(os.path.dirname(__file__)), "../.."))
'..', '..' instead of '../..'

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
File tools/checkdeps/checkdeps_manualtest.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
tools/checkdeps/checkdeps_manualtest.py:15: # only run manually and may need to
be updated from time to time.
Optional: I think a data subdirectory should be created, with the content of the
following you wrote.

Then import checkdeps the same way you do in the presubmit and assert on the
return value.

Then the test becomes automatic. :)

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
tools/checkdeps/checkdeps_manualtest.py:40: 
remo

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/cpp_checke...
File tools/checkdeps/cpp_checker.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/cpp_checke...
tools/checkdeps/cpp_checker.py:39: Returns a triplet (is_include,
illegal_description. rule_type).
illegal_description, rule_type

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py
File tools/checkdeps/rules.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:1: #!/usr/bin/env python
This file is not executable

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:7: 
2 lines

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:74: self._rules = [x for x in self._rules if not
x.ParentOrMatch(rule_dir)]
I would probably have used instead:
self._rules = [Rule(add_rule, rule_dir, source)]
self._rules.extend(x for x in self._rules if not x.ParentOrMatch(rule_dir))

but I don't really mind.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:86: why_failed = rule.__str__()
str(rule)

[Jói, 2012-07-26 13:54:25]

OK, understood now about the imports.  I will get rid of the globals
to fix that, and address your other comments while I'm at it.

Cheers,
Jói


On Thu, Jul 26, 2012 at 1:50 PM,  <maruel@chromium.org> wrote:
>
>
http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps.py
> File tools/checkdeps/checkdeps.py (right):
>
>
http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:385: global BASE_DIRECTORY
> On 2012/07/26 13:32:13, Jói wrote:
>>
>> On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
>> > It'd be simpler if it was not a global. Same for
>
> GIT_SOURCE_DIRECTORY, globals
>>
>> > really complicate importing this file.
>
>
>> I'm not sure I follow, I get that it's harder to use checkdeps as a
>
> module
>>
>> because of the globals if you want to invoke multiple times on it, but
>
> it seems
>>
>> fine for our current purposes?  Maybe I'm missing some complexity
>
> about
>>
>> importing a module with globals that I'm unaware of?  They are really
>
> just
>>
>> module scoped, not truly global, as far as I understand.
>
>
> global as in module global, since this is python.
>
> The problem is that CommonSetup() makes the calls implicitly stateful.
> So, if two different PRESUBMIT.py imports checkdeps, the second one
> could (or could not, depends if the module reference is leaked) reuse
> the state from the first one.
>
> This can be problematic for example if 2 presubmits are run for multiple
> checkouts, since GIT_SOURCE_DIRECTORY will be wrong on the second one.
> This may happen with complex gcl changes, but is rare with git cl.
>
> Overall, this is a potential for Major Fun. This is why I first
> recommended for shelling out instead of importing. I'd recommend to do
> one or the other:
> 1. shell out in PRESUBMIT
> 2. Get rid of BASE_DIRECTORY and GIT_SOURCE_DIRECTORY and pass them as
> parameters. VERBOSE is borderline in that case, probably a good idea to
> do it too.
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps.py
> File tools/checkdeps/checkdeps.py (right):
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:112: if
> os.path.normpath(cur_dir).lower().startswith(
> BTW, is cur_dir an absolute path?
>
> FYI only: I like to sprinkle asserts and enforce absolute paths. But
> that means realpath() must be called consistently otherwise symlinks
> will get in the way.
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:335: BASE_DIRECTORY = os.path.abspath(
> Did you mean normpath?
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:336:
> os.path.join(os.path.abspath(os.path.dirname(__file__)), "../.."))
> '..', '..' instead of '../..'
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
> File tools/checkdeps/checkdeps_manualtest.py (right):
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
> tools/checkdeps/checkdeps_manualtest.py:15: # only run manually and may
> need to be updated from time to time.
> Optional: I think a data subdirectory should be created, with the
> content of the following you wrote.
>
> Then import checkdeps the same way you do in the presubmit and assert on
> the return value.
>
> Then the test becomes automatic. :)
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
> tools/checkdeps/checkdeps_manualtest.py:40:
> remo
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/cpp_checke...
> File tools/checkdeps/cpp_checker.py (right):
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/cpp_checke...
> tools/checkdeps/cpp_checker.py:39: Returns a triplet (is_include,
> illegal_description. rule_type).
> illegal_description, rule_type
>
> http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py
> File tools/checkdeps/rules.py (right):
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
> tools/checkdeps/rules.py:1: #!/usr/bin/env python
> This file is not executable
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
> tools/checkdeps/rules.py:7:
> 2 lines
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
> tools/checkdeps/rules.py:74: self._rules = [x for x in self._rules if
> not x.ParentOrMatch(rule_dir)]
> I would probably have used instead:
> self._rules = [Rule(add_rule, rule_dir, source)]
> self._rules.extend(x for x in self._rules if not
> x.ParentOrMatch(rule_dir))
>
> but I don't really mind.
>
>
http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
> tools/checkdeps/rules.py:86: why_failed = rule.__str__()
> str(rule)
>
>
> http://codereview.chromium.org/10805042/
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-dev" group.
> To post to this group, send email to browser-components-dev@chromium.org.
> To unsubscribe from this group, send email to
> browser-components-dev+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Jói, 2012-07-26 17:05:52]

Marc-Antoine, thanks again for the thorough review.  I think I've addressed all
of your points, please take a look at the latest patch set and the comments
below.

Cheers,
Jói

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/11001/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:385: global BASE_DIRECTORY
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> On 2012/07/26 13:32:13, Jói wrote:
> > On 2012/07/26 13:06:03, Marc-Antoine Ruel wrote:
> > > It'd be simpler if it was not a global. Same for GIT_SOURCE_DIRECTORY,
> globals
> > > really complicate importing this file.
> > 
> > I'm not sure I follow, I get that it's harder to use checkdeps as a module
> > because of the globals if you want to invoke multiple times on it, but it
> seems
> > fine for our current purposes?  Maybe I'm missing some complexity about
> > importing a module with globals that I'm unaware of?  They are really just
> > module scoped, not truly global, as far as I understand.
> 
> global as in module global, since this is python.
> 
> The problem is that CommonSetup() makes the calls implicitly stateful. So, if
> two different PRESUBMIT.py imports checkdeps, the second one could (or could
> not, depends if the module reference is leaked) reuse the state from the first
> one.
> 
> This can be problematic for example if 2 presubmits are run for multiple
> checkouts, since GIT_SOURCE_DIRECTORY will be wrong on the second one. This
may
> happen with complex gcl changes, but is rare with git cl.
> 
> Overall, this is a potential for Major Fun. This is why I first recommended
for
> shelling out instead of importing. I'd recommend to do one or the other:
> 1. shell out in PRESUBMIT
> 2. Get rid of BASE_DIRECTORY and GIT_SOURCE_DIRECTORY and pass them as
> parameters. VERBOSE is borderline in that case, probably a good idea to do it
> too.

Done, I made these members of a checkdeps.DepsChecker object that you
instantiate.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:112: if
os.path.normpath(cur_dir).lower().startswith(
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> BTW, is cur_dir an absolute path?
> 
> FYI only: I like to sprinkle asserts and enforce absolute paths. But that
means
> realpath() must be called consistently otherwise symlinks will get in the way.

It's an absolute path.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:335: BASE_DIRECTORY = os.path.abspath(
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> Did you mean normpath?

No, needs to be an absolute path.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:336:
os.path.join(os.path.abspath(os.path.dirname(__file__)), "../.."))
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> '..', '..' instead of '../..'

Done.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
File tools/checkdeps/checkdeps_manualtest.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
tools/checkdeps/checkdeps_manualtest.py:15: # only run manually and may need to
be updated from time to time.
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> Optional: I think a data subdirectory should be created, with the content of
the
> following you wrote.
> 
> Then import checkdeps the same way you do in the presubmit and assert on the
> return value.
> 
> Then the test becomes automatic. :)

Done, I was thinking about this and should have done it from the start to save
time manually testing each iteration.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/checkdeps_...
tools/checkdeps/checkdeps_manualtest.py:40: 
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> remo

Done.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/cpp_checke...
File tools/checkdeps/cpp_checker.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/cpp_checke...
tools/checkdeps/cpp_checker.py:39: Returns a triplet (is_include,
illegal_description. rule_type).
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> illegal_description, rule_type

Done.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py
File tools/checkdeps/rules.py (right):

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:1: #!/usr/bin/env python
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> This file is not executable

Done.

http://codereview.chromium.org/10805042/diff/14002/tools/checkdeps/rules.py#n...
tools/checkdeps/rules.py:86: why_failed = rule.__str__()
On 2012/07/26 13:50:51, Marc-Antoine Ruel wrote:
> str(rule)

Done.

[M-A Ruel, 2012-07-26 18:30:07]

lgtm with nits

http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:140: for (_, rule_str) in enumerate(includes):
I know you just moved the code but while at it, remove the enumerate() call
since the index is ignored.

http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:141: if not len(relative_dir):
if not relative_dir:

http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:261: 
one line is sufficient

http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/testdata/DEPS
File tools/checkdeps/testdata/DEPS (right):

http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/testdata/D...
tools/checkdeps/testdata/DEPS:1: include_rules = [
Are you sure this directory won't trigger in normal check_deps execution? You
can confirm on the try slave.

[Jói, 2012-07-26 22:40:30]

Thanks, addressed the nits.

> Are you sure this directory won't trigger in normal check_deps
> execution? You can confirm on the try slave.

It doesn't on my local machine (git checkout) but I will confirm on
the try slave (intended to do so to get SVN coverage).

Cheers,
Jói



On Thu, Jul 26, 2012 at 6:30 PM,  <maruel@chromium.org> wrote:
> lgtm with nits
>
>
>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps.py
> File tools/checkdeps/checkdeps.py (right):
>
>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:140: for (_, rule_str) in
> enumerate(includes):
> I know you just moved the code but while at it, remove the enumerate()
> call since the index is ignored.
>
>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:141: if not len(relative_dir):
> if not relative_dir:
>
>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:261:
> one line is sufficient
>
>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/testdata/DEPS
> File tools/checkdeps/testdata/DEPS (right):
>
>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/testdata/D...
> tools/checkdeps/testdata/DEPS:1: include_rules = [
> Are you sure this directory won't trigger in normal check_deps
> execution? You can confirm on the try slave.
>
> http://codereview.chromium.org/10805042/

[Jói, 2012-07-26 22:41:13]

Actually, fails with "too many values to unpack" if I remove
enumerate, so put that back in.

On Thu, Jul 26, 2012 at 10:39 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Thanks, addressed the nits.
>
>> Are you sure this directory won't trigger in normal check_deps
>> execution? You can confirm on the try slave.
>
> It doesn't on my local machine (git checkout) but I will confirm on
> the try slave (intended to do so to get SVN coverage).
>
> Cheers,
> Jói
>
>
>
> On Thu, Jul 26, 2012 at 6:30 PM,  <maruel@chromium.org> wrote:
>> lgtm with nits
>>
>>
>>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps.py
>> File tools/checkdeps/checkdeps.py (right):
>>
>>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
>> tools/checkdeps/checkdeps.py:140: for (_, rule_str) in
>> enumerate(includes):
>> I know you just moved the code but while at it, remove the enumerate()
>> call since the index is ignored.
>>
>>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
>> tools/checkdeps/checkdeps.py:141: if not len(relative_dir):
>> if not relative_dir:
>>
>>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/checkdeps....
>> tools/checkdeps/checkdeps.py:261:
>> one line is sufficient
>>
>>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/testdata/DEPS
>> File tools/checkdeps/testdata/DEPS (right):
>>
>>
http://codereview.chromium.org/10805042/diff/15010/tools/checkdeps/testdata/D...
>> tools/checkdeps/testdata/DEPS:1: include_rules = [
>> Are you sure this directory won't trigger in normal check_deps
>> execution? You can confirm on the try slave.
>>
>> http://codereview.chromium.org/10805042/

[commit-bot: I haz the power, 2012-07-30 12:33:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10805042/20002

[commit-bot: I haz the power, 2012-07-30 12:33:35]

Presubmit check for 10805042-20002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  tools/checkdeps/testdata/disallowed/allowed/test.h \
  tools/checkdeps/testdata/disallowed/test.h

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui
    chrome/test

/b/commit-queue/workdir/chromium/tools/checkdeps/checkdeps_test.py failed!
Command /b/commit-queue/workdir/chromium/tools/checkdeps/checkdeps_test.py
returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium/tools/checkdeps
.....F
======================================================================
FAIL: testRegularCheckDepsRun (__main__.CheckDepsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/b/commit-queue/workdir/chromium/tools/checkdeps/checkdeps_test.py",
line 29, in testRegularCheckDepsRun
    self.failUnless(index != -1)
AssertionError

----------------------------------------------------------------------
Ran 6 tests in 0.623s

FAILED (failures=1)
WARNING: multiple definitions of com.example.HelloLicenseServlet:
   
/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/examples/apps/hello-java/workspace/HelloLicense/src/com/example/HelloLicenseServlet.java
   
/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/examples/apps/hello-java/HelloLicenseServlet.java

Can't find dir /b/commit-queue/workdir/chromium/tools/checkdeps/testdata/.svn in
git or svn
Can't find dir
/b/commit-queue/workdir/chromium/tools/checkdeps/testdata/disallowed/.svn in git
or svn
Can't find dir
/b/commit-queue/workdir/chromium/tools/checkdeps/testdata/disallowed/allowed/.svn
in git or svn
Can't find dir
/b/commit-queue/workdir/chromium/tools/checkdeps/testdata/allowed/.svn in git or
svn


Presubmit checks took 2.1s to calculate.

[commit-bot: I haz the power, 2012-07-30 12:58:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10805042/21018

[commit-bot: I haz the power, 2012-07-30 12:58:28]

Presubmit check for 10805042-21018 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui
    chrome/test

/b/commit-queue/workdir/chromium/tools/checkdeps/checkdeps_test.py failed!
Command /b/commit-queue/workdir/chromium/tools/checkdeps/checkdeps_test.py
returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium/tools/checkdeps
.....F
======================================================================
FAIL: testRegularCheckDepsRun (__main__.CheckDepsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/b/commit-queue/workdir/chromium/tools/checkdeps/checkdeps_test.py",
line 31, in testRegularCheckDepsRun
    self.failUnless(index != -1)
AssertionError

----------------------------------------------------------------------
Ran 6 tests in 0.542s

FAILED (failures=1)
WARNING: multiple definitions of com.example.HelloLicenseServlet:
   
/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/examples/apps/hello-java/workspace/HelloLicense/src/com/example/HelloLicenseServlet.java
   
/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/examples/apps/hello-java/HelloLicenseServlet.java

[u'ERROR in
/b/commit-queue/workdir/chromium/tools/checkdeps/testdata/disallowed/test.h\nIllegal
include: "third_party/explicitly_disallowed/bad.h"\n    Because of
"-third_party/explicitly_disallowed" from tools/checkdeps/testdata\'s
include_rules.\nIllegal include: "third_party/allowed_may_use/bad.h"\n   
Because of no rule applying\nIllegal include: "third_party/no_rule/bad.h"\n   
Because of no rule applying\n', u'ERROR in
/b/commit-queue/workdir/chromium/tools/checkdeps/testdata/disallowed/allowed/test.h\nIllegal
include: "third_party/explicitly_disallowed/bad.h"\n    Because of
"-third_party/explicitly_disallowed" from tools/checkdeps/testdata\'s
include_rules.\nIllegal include: "third_party/allowed_may_use/bad.h"\n   
Because of no rule applying\nIllegal include: "third_party/no_rule/bad.h"\n   
Because of no rule applying\n', u'ERROR in
/b/commit-queue/workdir/chromium/tools/checkdeps/testdata/allowed/test.h\nIllegal
include: "tools/checkdeps/testdata/disallowed/bad.h"\n    Because of
"-tools/checkdeps/testdata/disallowed" from tools/checkdeps/testdata\'s
include_rules.\nIllegal include: "third_party/explicitly_disallowed/bad.h"\n   
Because of "-third_party/explicitly_disallowed" from tools/checkdeps/testdata\'s
include_rules.\nIllegal include: "third_party/no_rule/bad.h"\n    Because of no
rule applying\n']


Presubmit checks took 2.2s to calculate.

[commit-bot: I haz the power, 2012-07-30 13:12:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10805042/11007

[commit-bot: I haz the power, 2012-07-30 13:12:37]

Presubmit check for 10805042-11007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui
    chrome/test

Presubmit checks took 2.1s to calculate.

[commit-bot: I haz the power, 2012-07-30 13:18:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10805042/11007

[commit-bot: I haz the power, 2012-07-30 15:23:30]

Try job failure for 10805042-11007 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-31 09:23:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10805042/12028

[commit-bot: I haz the power, 2012-07-31 10:40:02]

Change committed as 149163