Add presubmit check to prevent package boundary violations.

Add presubmit check to prevent package boundary violations.

BUG=webrtc:6954
NOTRY=True

Review-Url: https://codereview.webrtc.org/2629723004
Cr-Commit-Position: refs/heads/master@{#16357}
Committed: https://chromium.googlesource.com/external/webrtc/+/4fb97462a8443fd0879953f0b30d59811828f851

[ehmaldonado_webrtc, 2017-01-13 14:49:24]

Description was changed from

==========
Add presubmit check to prevent package boundary violations.

BUG=
==========

to

==========
Add presubmit check to prevent package boundary violations.

BUG=webrtc:6954
==========

[ehmaldonado_webrtc, 2017-01-13 14:49:24]

ehmaldonado@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2017-01-16 07:26:10]

Can you share some numbers on how long this check takes to run on a CL that
changes in webrtc/BUILD.gn, without setting --max_message ?

https://codereview.webrtc.org/2629723004/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/2629723004/diff/1/PRESUBMIT.py#newcode310
PRESUBMIT.py:310: 'There are package boundary violations in the following GN
files:\n\n'
I think since most people are not familiar with what package boundary violations
are you need to either explain in detail or link to Bazel docs about it

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
File tools-webrtc/check_package_boundaries.py (right):

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:15: import re
sort

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:36: self.logLevel = DISPLAY_LEVEL
The style guide says local variables and class/global variables should be named
lowercase with underscores. The Chromium exception with CapsWords style only
applies to function and method names:
http://dev.chromium.org/chromium-os/python-style-guidelines

I realize style violations like that aren't checked by PyLint, which is
unfortunate, but AFAIK there's no check for that available either (we're already
using stricter checks than Chromium by our
https://chromium.googlesource.com/external/webrtc/+/master/pylintrc file - maybe
it's possible to tighten it up further?).

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:48: class RegexAux(object):
I'm a bit confused by the name Aux, is RegexUtil better? Since AFAICS this class
only exists to make the code more readable?

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:52: def matches(self, regex, target):
Rename target->line to avoid confusion here since we're dealing with GN targets
in this script.

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:67: def _LongestPrefix(trie, query):
+1 blank line for top level methods

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:87: if regexAux.matches(TARGET_RE,
line):
I find it hard to follow what's going on here since there's a lot of state etc.
Is it not possible to solve this problem easier with multiline-regexp, like in
_CheckNoSourcesAbove in
https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#231? 

Either way, I'd like to see unit tests to prove that this code works for a few
simple cases. For this I think you need to build a testdata dir that looks
something like this:
testdata/BIULD.gn - with two targets: one that has no sources and one that has
one: dummy.cc, which depends on subdir/BUILD.gn
tesdata/dummy.cc - includes subdir/subdir2/dummy3.h (package boundary violation)
testdata/subdir/BUILD.gn - with one target that includes dummy2.{h,cc}
testdata/subdir/dummy2.cc
testdata/subdir/dummy2.h
testdata/subdir/subdir2/dummy3.h

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:123: help='If set, the maximum number
of violations to be '
Does this exist only as a performance optimization, i.e. to fail fast when a
violation is found? Is it not better to return all errors to the user so one
doesn't have to rerun presubmit checks many times?

[ehmaldonado_webrtc, 2017-01-16 08:52:51]

Without setting --max_messages, and without specifying BUILD.gn files (so that
all BUILD.gn files are processed) this takes about 0.07s

I'll try to make the code clearer :)

https://codereview.webrtc.org/2629723004/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/2629723004/diff/1/PRESUBMIT.py#newcode310
PRESUBMIT.py:310: 'There are package boundary violations in the following GN
files:\n\n'
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> I think since most people are not familiar with what package boundary
violations
> are you need to either explain in detail or link to Bazel docs about it

Acknowledged.

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
File tools-webrtc/check_package_boundaries.py (right):

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:15: import re
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> sort

Acknowledged.

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:36: self.logLevel = DISPLAY_LEVEL
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> The style guide says local variables and class/global variables should be
named
> lowercase with underscores. The Chromium exception with CapsWords style only
> applies to function and method names:
> http://dev.chromium.org/chromium-os/python-style-guidelines
> 
> I realize style violations like that aren't checked by PyLint, which is
> unfortunate, but AFAIK there's no check for that available either (we're
already
> using stricter checks than Chromium by our
> https://chromium.googlesource.com/external/webrtc/+/master/pylintrc file -
maybe
> it's possible to tighten it up further?).

Acknowledged.

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:48: class RegexAux(object):
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> I'm a bit confused by the name Aux, is RegexUtil better? Since AFAICS this
class
> only exists to make the code more readable?

Acknowledged.

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:52: def matches(self, regex, target):
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> Rename target->line to avoid confusion here since we're dealing with GN
targets
> in this script.

Acknowledged.

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:67: def _LongestPrefix(trie, query):
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> +1 blank line for top level methods

Acknowledged.

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:87: if regexAux.matches(TARGET_RE,
line):
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> I find it hard to follow what's going on here since there's a lot of state
etc.
> Is it not possible to solve this problem easier with multiline-regexp, like in
> _CheckNoSourcesAbove in
> https://chromium.googlesource.com/external/webrtc/+/master/PRESUBMIT.py#231? 
> 
> Either way, I'd like to see unit tests to prove that this code works for a few
> simple cases. For this I think you need to build a testdata dir that looks
> something like this:
> testdata/BIULD.gn - with two targets: one that has no sources and one that has
> one: dummy.cc, which depends on subdir/BUILD.gn
> tesdata/dummy.cc - includes subdir/subdir2/dummy3.h (package boundary
violation)
> testdata/subdir/BUILD.gn - with one target that includes dummy2.{h,cc}
> testdata/subdir/dummy2.cc
> testdata/subdir/dummy2.h
> testdata/subdir/subdir2/dummy3.h
> 

I'll try to make this clearer.
You're right, this is missing test cases :)

https://codereview.webrtc.org/2629723004/diff/1/tools-webrtc/check_package_bo...
tools-webrtc/check_package_boundaries.py:123: help='If set, the maximum number
of violations to be '
On 2017/01/16 07:26:10, kjellander_webrtc OOO to Jan19 wrote:
> Does this exist only as a performance optimization, i.e. to fail fast when a
> violation is found? Is it not better to return all errors to the user so one
> doesn't have to rerun presubmit checks many times?

No, it's not for performance optimization.
If you don't pass the argument, then all errors will be shown. I just find it
useful to focus on a few of them at a time.

[ehmaldonado_webrtc, 2017-01-16 10:57:55]

PTAL
Tests on the way... :)

[ehmaldonado_webrtc, 2017-01-16 13:14:26]

On 2017/01/16 10:57:55, ehmaldonado_webrtc wrote:
> PTAL
> Tests on the way... :)

And tests added

[kjellander_webrtc, 2017-01-19 10:51:26]

First round. I will review the actual tests later.

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode301
PRESUBMIT.py:301: def _CheckNoPackageBoundaryViolations(input_api, gn_files,
output_api):
nit: +1 blank line for top-level functions.

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode305
PRESUBMIT.py:305: command = ['python', script_path, '--max_messages=1',
webrtc_path]
Use sys.executable instead instead of 'python'.  It's what's commonly used and I
think it's safer (avoids problems with incorrect PATH on Windows etc).

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode436
PRESUBMIT.py:436: join('tools-webrtc', 'autoroller', 'unittests'),
Please add an entry here to get your test to run instead. Then you can use the
Python unittest module like our other tests.
Now that we have our own tools directory with only our own stuff, maybe we
should add all modules matching *_test.py recursively to avoid having to
remember updating this list (it was necessary when we had lots of Chromium
tools/ stuff since we don't want to waste time running those tests).

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/PRESUBMIT.py
File tools-webrtc/PRESUBMIT.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/PRESUBMIT.py...
tools-webrtc/PRESUBMIT.py:44: def _CheckPackageBoundariesTest(input_api,
output_api):
Please remove this and make the test use the Python unittest module instead.

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
File tools-webrtc/check_package_boundaries.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
tools-webrtc/check_package_boundaries.py:88: found_violations = True
Seems like a bug was found with the tests? :)

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
File tools-webrtc/check_package_boundaries_test.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
tools-webrtc/check_package_boundaries_test.py:27: 
+1 blank line

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
tools-webrtc/check_package_boundaries_test.py:28: def _CheckEqual(actual,
expected, message=''):
Please use unittest module instead.

[ehmaldonado_webrtc, 2017-01-19 13:33:56]

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode301
PRESUBMIT.py:301: def _CheckNoPackageBoundaryViolations(input_api, gn_files,
output_api):
On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> nit: +1 blank line for top-level functions.

What do you mean? Should I also add a blank line before the functions above?

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode305
PRESUBMIT.py:305: command = ['python', script_path, '--max_messages=1',
webrtc_path]
On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> Use sys.executable instead instead of 'python'.  It's what's commonly used and
I
> think it's safer (avoids problems with incorrect PATH on Windows etc).

Done.
I also decided to get rid of --max-messages=1.

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode436
PRESUBMIT.py:436: join('tools-webrtc', 'autoroller', 'unittests'),
On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> Please add an entry here to get your test to run instead. Then you can use the
> Python unittest module like our other tests.
> Now that we have our own tools directory with only our own stuff, maybe we
> should add all modules matching *_test.py recursively to avoid having to
> remember updating this list (it was necessary when we had lots of Chromium
> tools/ stuff since we don't want to waste time running those tests).

Like this?

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
File tools-webrtc/check_package_boundaries.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
tools-webrtc/check_package_boundaries.py:88: found_violations = True
On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> Seems like a bug was found with the tests? :)

Yup :)

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
File tools-webrtc/check_package_boundaries_test.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/tools-webrtc/check_packag...
tools-webrtc/check_package_boundaries_test.py:28: def _CheckEqual(actual,
expected, message=''):
On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> Please use unittest module instead.

Done.

[kjellander_webrtc, 2017-01-24 09:33:20]

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode301
PRESUBMIT.py:301: def _CheckNoPackageBoundaryViolations(input_api, gn_files,
output_api):
On 2017/01/19 13:33:56, ehmaldonado_webrtc wrote:
> On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> > nit: +1 blank line for top-level functions.
> 
> What do you mean? Should I also add a blank line before the functions above?

Yeah, all top-level definitions are to be separated with two blank lines
according to the Python style guide (I agree that's a bit silly):
https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode305
PRESUBMIT.py:305: command = ['python', script_path, '--max_messages=1',
webrtc_path]
On 2017/01/19 13:33:56, ehmaldonado_webrtc wrote:
> On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> > Use sys.executable instead instead of 'python'.  It's what's commonly used
and
> I
> > think it's safer (avoids problems with incorrect PATH on Windows etc).
> 
> Done.
> I also decided to get rid of --max-messages=1.

Acknowledged.

https://codereview.webrtc.org/2629723004/diff/40001/PRESUBMIT.py#newcode436
PRESUBMIT.py:436: join('tools-webrtc', 'autoroller', 'unittests'),
On 2017/01/19 13:33:56, ehmaldonado_webrtc wrote:
> On 2017/01/19 10:51:26, kjellander_webrtc wrote:
> > Please add an entry here to get your test to run instead. Then you can use
the
> > Python unittest module like our other tests.
> > Now that we have our own tools directory with only our own stuff, maybe we
> > should add all modules matching *_test.py recursively to avoid having to
> > remember updating this list (it was necessary when we had lots of Chromium
> > tools/ stuff since we don't want to waste time running those tests).
> 
> Like this?

Yeah, that looks great. Thanks!

https://codereview.webrtc.org/2629723004/diff/60001/tools-webrtc/check_packag...
File tools-webrtc/check_package_boundaries_test.py (right):

https://codereview.webrtc.org/2629723004/diff/60001/tools-webrtc/check_packag...
tools-webrtc/check_package_boundaries_test.py:33:
os.chdir(os.path.dirname(os.path.abspath(__file__)))
It's generally bad practice to use os.chdir in scripts and IMO it's often a
source of bugs and hard-to-read code (although not here). 
To send the right signal to future readers, could you you create: 
SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
and then join with that below instead? Otherwise I'm afraid if we add another
test case in the future, it will still have cwd set to what you change it to
here (it applies to the currently running interpreter executable IIRC).

https://codereview.webrtc.org/2629723004/diff/60001/tools-webrtc/check_packag...
tools-webrtc/check_package_boundaries_test.py:39: test_command = ['python',
'check_package_boundaries.py', 'testdata']
Please refactor the check_package_boundaries module so you can invoke a function
in it here instead of firing up another Python process.
I'm afraid we loose several benefits with the unit test framework if we launch
another Python instance (like easy debugging).

https://codereview.webrtc.org/2629723004/diff/60001/tools-webrtc/testdata/exp...
File tools-webrtc/testdata/expected_stderr (right):

https://codereview.webrtc.org/2629723004/diff/60001/tools-webrtc/testdata/exp...
tools-webrtc/testdata/expected_stderr:1: testdata/subdir/BUILD.gn:21 in target
'package_boundary_violation':
I think it's better to write more fine-grained test cases so we don't need to
resort to just comparing stdout/stderr. When a test fails it's likely going to
be a harder-to-read failure message with this approach.

[ehmaldonado_webrtc, 2017-01-24 14:09:05]

PTAL :)

[kjellander_webrtc, 2017-01-24 17:46:10]

On 2017/01/24 14:09:05, ehmaldonado_webrtc wrote:
> PTAL :)

Woho, solid testing work here. LGTM!

[kjellander_webrtc, 2017-01-24 17:46:27]

..but please address my comments

https://codereview.webrtc.org/2629723004/diff/100001/tools-webrtc/check_packa...
File tools-webrtc/check_package_boundaries_test.py (right):

https://codereview.webrtc.org/2629723004/diff/100001/tools-webrtc/check_packa...
tools-webrtc/check_package_boundaries_test.py:59: def test_common_preffix(self):
preffix -> prefix

https://codereview.webrtc.org/2629723004/diff/100001/tools-webrtc/check_packa...
tools-webrtc/check_package_boundaries_test.py:64: 
nit: +1 blank line

https://codereview.webrtc.org/2629723004/diff/100001/tools-webrtc/testdata/co...
File tools-webrtc/testdata/common_preffix/expected.pyl (right):

https://codereview.webrtc.org/2629723004/diff/100001/tools-webrtc/testdata/co...
tools-webrtc/testdata/common_preffix/expected.pyl:1: []
Change the dirname common_preffix -> common_prefix

[ehmaldonado_webrtc, 2017-01-24 17:56:29]

Description was changed from

==========
Add presubmit check to prevent package boundary violations.

BUG=webrtc:6954
==========

to

==========
Add presubmit check to prevent package boundary violations.

BUG=webrtc:6954
NOTRY=True
==========

[ehmaldonado_webrtc, 2017-01-24 17:57:33]

Done :)
Maybe this should wait until https://codereview.webrtc.org/2649563002/ is
submitted?
Otherwise people modifiying webrtc/modules/BUILD.gn might get an error.

[kjellander_webrtc, 2017-01-25 15:53:06]

On 2017/01/24 17:57:33, ehmaldonado_webrtc wrote:
> Done :)
> Maybe this should wait until https://codereview.webrtc.org/2649563002/ is
> submitted?
> Otherwise people modifiying webrtc/modules/BUILD.gn might get an error.

Yeah, that makes sense.

[ehmaldonado_webrtc, 2017-01-30 12:31:47]

The CQ bit was checked by ehmaldonado@webrtc.org

[ehmaldonado_webrtc, 2017-01-30 12:31:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2629723004/#ps120001 (title:
"preffix->prefix")

[commit-bot: I haz the power, 2017-01-30 12:31:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-30 12:37:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-30 12:37:06]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12813)

[ehmaldonado_webrtc, 2017-01-30 13:24:36]

The CQ bit was checked by ehmaldonado@webrtc.org

[ehmaldonado_webrtc, 2017-01-30 13:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2629723004/#ps140001 (title:
"Sort lists before comparing.")

[commit-bot: I haz the power, 2017-01-30 13:24:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-30 13:27:23]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485782676283710,
"parent_rev": "9cbb0a16e5a02538dc9cb18ec9d447e2d2058ec3", "commit_rev":
"4fb97462a8443fd0879953f0b30d59811828f851"}

[commit-bot: I haz the power, 2017-01-30 13:27:26]

Description was changed from

==========
Add presubmit check to prevent package boundary violations.

BUG=webrtc:6954
NOTRY=True
==========

to

==========
Add presubmit check to prevent package boundary violations.

BUG=webrtc:6954
NOTRY=True

Review-Url: https://codereview.webrtc.org/2629723004
Cr-Commit-Position: refs/heads/master@{#16357}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4fb97462a8443fd0879953f0b...
==========

[commit-bot: I haz the power, 2017-01-30 13:27:27]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/4fb97462a8443fd0879953f0b...

[tommi, 2017-02-12 16:32:48]

On 2017/01/30 13:27:27, commit-bot: I haz the power wrote:
> Committed patchset #8 (id:140001) as
>
https://chromium.googlesource.com/external/webrtc/+/4fb97462a8443fd0879953f0b...

I'm getting an error on Windows whenever I do a git cl upload.  See below.  I
wonder if this could be because of assumptions that don't hold on all platforms?
(e.g. '/' vs '\\')

d:\src\webrtc\src\out\Default>git cl upload -m "Rebase"
Using 50% similarity for rename/copy detection. Override with --similarity.
Running presubmit upload checks ...

** Presubmit Warnings **
d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py (0.10s) failed
F.FF.
======================================================================
FAIL: test_all_build_files (__main__.UnitTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
63, in test_all_build_files
    self.RunTest(os.path.join(TESTDATA_DIR, 'all_build_files'), True)
  File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
48, in RunTest
    self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
AssertionError: Lists differ: [['subpackage1/BUILD.gn', '12'... != []

First list contains 4 additional elements.
First extra element 0:
['subpackage1/BUILD.gn', '12', 'error_1',
'subsubpackage1/dummy_subsubpackage1.cc', 'subsubpackage1']

+ []
- [['subpackage1/BUILD.gn',
-   '12',
-   'error_1',
-   'subsubpackage1/dummy_subsubpackage1.cc',
-   'subsubpackage1'],
-  ['subpackage1/BUILD.gn',
-   '13',
-   'error_1',
-   'subsubpackage1/dummy_subsubpackage1.h',
-   'subsubpackage1'],
-  ['subpackage2/BUILD.gn',
-   '12',
-   'error_2',
-   'subsubpackage2/dummy_subsubpackage2.cc',
-   'subsubpackage2'],
-  ['subpackage2/BUILD.gn',
-   '13',
-   'error_2',
-   'subsubpackage2/dummy_subsubpackage2.h',
-   'subsubpackage2']]

======================================================================
FAIL: test_multiple_errors_multiple_targets (__main__.UnitTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
57, in test_multiple_errors_multiple_targets
    self.RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_multiple_targets'))
  File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
48, in RunTest
    self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
AssertionError: Lists differ: [['BUILD.gn', '18', 'error_1',... != []

First list contains 4 additional elements.
First extra element 0:
['BUILD.gn', '18', 'error_1', 'subpackage1/dummy_subpackage1.cc', 'subpackage1']

+ []
- [['BUILD.gn',
-   '18',
-   'error_1',
-   'subpackage1/dummy_subpackage1.cc',
-   'subpackage1'],
-  ['BUILD.gn',
-   '19',
-   'error_1',
-   'subpackage1/dummy_subpackage1.h',
-   'subpackage1'],
-  ['BUILD.gn',
-   '25',
-   'error_2',
-   'subpackage1/dummy_subpackage2.cc',
-   'subpackage1'],
-  ['BUILD.gn',
-   '26',
-   'error_2',
-   'subpackage1/dummy_subpackage2.h',
-   'subpackage1']]

======================================================================
FAIL: test_multiple_errors_single_target (__main__.UnitTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
54, in test_multiple_errors_single_target
    self.RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_single_target'))
  File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
48, in RunTest
    self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
AssertionError: Lists differ: [['BUILD.gn', '11', 'dummy_tar... != []

First list contains 2 additional elements.
First extra element 0:
['BUILD.gn', '11', 'dummy_target', 'subpackage/dummy_subpackage_file.cc',
'subpackage']

+ []
- [['BUILD.gn',
-   '11',
-   'dummy_target',
-   'subpackage/dummy_subpackage_file.cc',
-   'subpackage'],
-  ['BUILD.gn',
-   '12',
-   'dummy_target',
-   'subpackage/dummy_subpackage_file.h',
-   'subpackage']]

----------------------------------------------------------------------
Ran 5 tests in 0.006s

FAILED (failures=3)


Presubmit checks took 2.5s to calculate.

There were presubmit warnings. Are you sure you wish to continue? (y/N): N

[kjellander_webrtc, 2017-02-13 06:29:12]

On 2017/02/12 16:32:48, tommi (webrtc) wrote:
> On 2017/01/30 13:27:27, commit-bot: I haz the power wrote:
> > Committed patchset #8 (id:140001) as
> >
>
https://chromium.googlesource.com/external/webrtc/+/4fb97462a8443fd0879953f0b...
> 
> I'm getting an error on Windows whenever I do a git cl upload.  See below.  I
> wonder if this could be because of assumptions that don't hold on all
platforms?
> (e.g. '/' vs '\\')

Seems likely. Edward: can you give this a try on your Windows machine? 
We could setup presubmit checks on Windows as well, if this turns out to be a
recurring problem (but I don't remember any other recent case).

 
> d:\src\webrtc\src\out\Default>git cl upload -m "Rebase"
> Using 50% similarity for rename/copy detection. Override with --similarity.
> Running presubmit upload checks ...
> 
> ** Presubmit Warnings **
> d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py (0.10s) failed
> F.FF.
> ======================================================================
> FAIL: test_all_build_files (__main__.UnitTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
> 63, in test_all_build_files
>     self.RunTest(os.path.join(TESTDATA_DIR, 'all_build_files'), True)
>   File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
> 48, in RunTest
>     self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
> AssertionError: Lists differ: [['subpackage1/BUILD.gn', '12'... != []
> 
> First list contains 4 additional elements.
> First extra element 0:
> ['subpackage1/BUILD.gn', '12', 'error_1',
> 'subsubpackage1/dummy_subsubpackage1.cc', 'subsubpackage1']
> 
> + []
> - [['subpackage1/BUILD.gn',
> -   '12',
> -   'error_1',
> -   'subsubpackage1/dummy_subsubpackage1.cc',
> -   'subsubpackage1'],
> -  ['subpackage1/BUILD.gn',
> -   '13',
> -   'error_1',
> -   'subsubpackage1/dummy_subsubpackage1.h',
> -   'subsubpackage1'],
> -  ['subpackage2/BUILD.gn',
> -   '12',
> -   'error_2',
> -   'subsubpackage2/dummy_subsubpackage2.cc',
> -   'subsubpackage2'],
> -  ['subpackage2/BUILD.gn',
> -   '13',
> -   'error_2',
> -   'subsubpackage2/dummy_subsubpackage2.h',
> -   'subsubpackage2']]
> 
> ======================================================================
> FAIL: test_multiple_errors_multiple_targets (__main__.UnitTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
> 57, in test_multiple_errors_multiple_targets
>     self.RunTest(os.path.join(TESTDATA_DIR,
'multiple_errors_multiple_targets'))
>   File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
> 48, in RunTest
>     self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
> AssertionError: Lists differ: [['BUILD.gn', '18', 'error_1',... != []
> 
> First list contains 4 additional elements.
> First extra element 0:
> ['BUILD.gn', '18', 'error_1', 'subpackage1/dummy_subpackage1.cc',
'subpackage1']
> 
> + []
> - [['BUILD.gn',
> -   '18',
> -   'error_1',
> -   'subpackage1/dummy_subpackage1.cc',
> -   'subpackage1'],
> -  ['BUILD.gn',
> -   '19',
> -   'error_1',
> -   'subpackage1/dummy_subpackage1.h',
> -   'subpackage1'],
> -  ['BUILD.gn',
> -   '25',
> -   'error_2',
> -   'subpackage1/dummy_subpackage2.cc',
> -   'subpackage1'],
> -  ['BUILD.gn',
> -   '26',
> -   'error_2',
> -   'subpackage1/dummy_subpackage2.h',
> -   'subpackage1']]
> 
> ======================================================================
> FAIL: test_multiple_errors_single_target (__main__.UnitTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
> 54, in test_multiple_errors_single_target
>     self.RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_single_target'))
>   File "d:\src\webrtc\src\tools-webrtc\check_package_boundaries_test.py", line
> 48, in RunTest
>     self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
> AssertionError: Lists differ: [['BUILD.gn', '11', 'dummy_tar... != []
> 
> First list contains 2 additional elements.
> First extra element 0:
> ['BUILD.gn', '11', 'dummy_target', 'subpackage/dummy_subpackage_file.cc',
> 'subpackage']
> 
> + []
> - [['BUILD.gn',
> -   '11',
> -   'dummy_target',
> -   'subpackage/dummy_subpackage_file.cc',
> -   'subpackage'],
> -  ['BUILD.gn',
> -   '12',
> -   'dummy_target',
> -   'subpackage/dummy_subpackage_file.h',
> -   'subpackage']]
> 
> ----------------------------------------------------------------------
> Ran 5 tests in 0.006s
> 
> FAILED (failures=3)
> 
> 
> Presubmit checks took 2.5s to calculate.
> 
> There were presubmit warnings. Are you sure you wish to continue? (y/N): N