[content/test/gpu] Run pylint check of gpu tests in unittest instead of PRESUBMIT

[content/test/gpu] Run pylint check of gpu tests in unittest instead of PRESUBMIT

This CL also adds pylint/ & logilab/astroid, logilab/common into
src/third_party. (these are copied from depot_tools/third_party)

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/3bb6d28634c0eb384281e06ca359715ba5445f91
Cr-Commit-Position: refs/heads/master@{#392160}

[nednguyen, 2016-04-27 00:29:10]

Description was changed from

==========
[content/test/gpu] Run PRESUBMIT of gpu in unittest


BUG=606854
==========

to

==========
[content/test/gpu] Run PRESUBMIT of gpu in unittest


BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[nednguyen(REVIEW IN OTHER ACC), 2016-04-27 01:02:22]

nednguyen@chromium.org changed reviewers:
+ kbr@chromium.org, nednguyen@chromium.org

[nednguyen(REVIEW IN OTHER ACC), 2016-04-27 01:02:24]

The extra libs are for pylint. We can't use pylint from depot_tools cuz it's not
easy to puy depot_tools in the isolate.

[nednguyen(REVIEW IN OTHER ACC), 2016-04-27 01:04:08]

On 2016/04/27 01:02:24, nednguyen(REVIEW IN OTHER ACC) wrote:
> The extra libs are for pylint. We can't use pylint from depot_tools cuz it's
not
> easy to puy depot_tools in the isolate.

If the approach looks ok to you, I gonna update the isolate & Build.gn

[nednguyen, 2016-04-27 03:56:08]

Description was changed from

==========
[content/test/gpu] Run PRESUBMIT of gpu in unittest


BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[content/test/gpu] Run pylint check of gpu tests in unittest instead of
PRESUBMIT

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-04-27 09:54:43]

kbr@chromium.org changed reviewers:
+ dpranke@chromium.org

[Ken Russell (switch to Gerrit), 2016-04-27 09:54:45]

This looks good overall to me. I realize it's a lot of third-party code but it's
really necessary that these pylint checks run as part of the CQ's tests and this
is the only way to do so.

CC'ing Dirk in case he has any comments.

One high-level comment then lgtm.

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT.py
File content/test/gpu/PRESUBMIT.py (left):

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT....
content/test/gpu/PRESUBMIT.py:60: results.extend(_PyLintChecks(input_api,
output_api))
Can we run all of the GPU unit tests upon upload? It's important that the lint
checks be done at this point rather than the CQ failing the CL.

[nednguyen, 2016-04-27 17:30:02]

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT.py
File content/test/gpu/PRESUBMIT.py (left):

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT....
content/test/gpu/PRESUBMIT.py:60: results.extend(_PyLintChecks(input_api,
output_api))
On 2016/04/27 09:54:45, Ken Russell OOO till 5-2-2016 wrote:
> Can we run all of the GPU unit tests upon upload? It's important that the lint
> checks be done at this point rather than the CQ failing the CL.

We already did at line 61 & 67. I remember you was the one who set it this way
:-)

[Ken Russell (switch to Gerrit), 2016-04-27 17:33:29]

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT.py
File content/test/gpu/PRESUBMIT.py (left):

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT....
content/test/gpu/PRESUBMIT.py:60: results.extend(_PyLintChecks(input_api,
output_api))
On 2016/04/27 17:30:02, nednguyen wrote:
> On 2016/04/27 09:54:45, Ken Russell OOO till 5-2-2016 wrote:
> > Can we run all of the GPU unit tests upon upload? It's important that the
lint
> > checks be done at this point rather than the CQ failing the CL.
> 
> We already did at line 61 & 67. I remember you was the one who set it this way
> :-)

It wasn't obvious to me any more that "_WebGLTextExpectationsTests" ran all of
the unittests. Could you rename that function to make it more obvious what it
does? Thanks.

[nednguyen, 2016-04-27 17:42:31]

Ping Dirk again

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT.py
File content/test/gpu/PRESUBMIT.py (left):

https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT....
content/test/gpu/PRESUBMIT.py:60: results.extend(_PyLintChecks(input_api,
output_api))
On 2016/04/27 17:33:28, Ken Russell OOO till 5-2-2016 wrote:
> On 2016/04/27 17:30:02, nednguyen wrote:
> > On 2016/04/27 09:54:45, Ken Russell OOO till 5-2-2016 wrote:
> > > Can we run all of the GPU unit tests upon upload? It's important that the
> lint
> > > checks be done at this point rather than the CQ failing the CL.
> > 
> > We already did at line 61 & 67. I remember you was the one who set it this
way
> > :-)
> 
> It wasn't obvious to me any more that "_WebGLTextExpectationsTests" ran all of
> the unittests. Could you rename that function to make it more obvious what it
> does? Thanks.

Done in https://codereview.chromium.org/1923183003

[Ken Russell (switch to Gerrit), 2016-04-27 17:59:57]

On 2016/04/27 17:42:31, nednguyen wrote:
> Ping Dirk again
> 
>
https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT.py
> File content/test/gpu/PRESUBMIT.py (left):
> 
>
https://codereview.chromium.org/1920403002/diff/1/content/test/gpu/PRESUBMIT....
> content/test/gpu/PRESUBMIT.py:60: results.extend(_PyLintChecks(input_api,
> output_api))
> On 2016/04/27 17:33:28, Ken Russell OOO till 5-2-2016 wrote:
> > On 2016/04/27 17:30:02, nednguyen wrote:
> > > On 2016/04/27 09:54:45, Ken Russell OOO till 5-2-2016 wrote:
> > > > Can we run all of the GPU unit tests upon upload? It's important that
the
> > lint
> > > > checks be done at this point rather than the CQ failing the CL.
> > > 
> > > We already did at line 61 & 67. I remember you was the one who set it this
> way
> > > :-)
> > 
> > It wasn't obvious to me any more that "_WebGLTextExpectationsTests" ran all
of
> > the unittests. Could you rename that function to make it more obvious what
it
> > does? Thanks.
> 
> Done in https://codereview.chromium.org/1923183003

Thanks. Still lgtm, though you'll need a third_party owner's review and update
the isolates, as you pointed out.

[Dirk Pranke, 2016-04-28 01:03:23]

Adding anything to //third_party needs approval from a //third_party OWNER *and*
approval from the OSPO.

See: open-https://www.chromium.org/developers/adding-3rd-party-libraries.

The README.chromium files should contain the versions that are pulled of each
package and instructions for how to update them.

We should move the pylintrc file to some place generic (e.g.
//third_party/pylint or //pylintrc) so that we can use a single file for
everything in Chromium.

To be clear: adding all of this stuff will not cause a catapult roll to run
presubmit checks against the //content/test/gpu code, right, since you're not
modifying the top-leve //PRESUBMIT.py ? So we still will need to add an actual
test step to one of the bots in the CQ? 

How long does it take to run these tests?

[nednguyen, 2016-04-28 03:37:52]

Hi Dirk, I will fix the README.chromium after the high level idea seems ok to
you.

On 2016/04/28 01:03:23, Dirk Pranke wrote:
> Adding anything to //third_party needs approval from a //third_party OWNER
*and*
> approval from the OSPO.
> 
> See: open-https://www.chromium.org/developers/adding-3rd-party-libraries.
> 
> The README.chromium files should contain the versions that are pulled of each
> package and instructions for how to update them.
> 
> We should move the pylintrc file to some place generic (e.g.
> //third_party/pylint or //pylintrc) so that we can use a single file for
> everything in Chromium.
> 
> To be clear: adding all of this stuff will not cause a catapult roll to run
> presubmit checks against the //content/test/gpu code, right, since you're not
> modifying the top-leve //PRESUBMIT.py ? So we still will need to add an actual
> test step to one of the bots in the CQ? 
> 
> How long does it take to run these tests?

The idea is we now move linting to content/test/gpu/gpu_tests/lint_unittest.py.
Since gpu_unittest is trigger on every CL (using swarming),  we will have lint
coverage on every CL, including the catapult roll.

[Ken Russell (switch to Gerrit), 2016-04-28 10:20:00]

On 2016/04/28 03:37:52, nednguyen wrote:
> Hi Dirk, I will fix the README.chromium after the high level idea seems ok to
> you.
> 
> On 2016/04/28 01:03:23, Dirk Pranke wrote:
> > Adding anything to //third_party needs approval from a //third_party OWNER
> *and*
> > approval from the OSPO.
> > 
> > See: open-https://www.chromium.org/developers/adding-3rd-party-libraries.
> > 
> > The README.chromium files should contain the versions that are pulled of
each
> > package and instructions for how to update them.
> > 
> > We should move the pylintrc file to some place generic (e.g.
> > //third_party/pylint or //pylintrc) so that we can use a single file for
> > everything in Chromium.
> > 
> > To be clear: adding all of this stuff will not cause a catapult roll to run
> > presubmit checks against the //content/test/gpu code, right, since you're
not
> > modifying the top-leve //PRESUBMIT.py ? So we still will need to add an
actual
> > test step to one of the bots in the CQ? 
> > 
> > How long does it take to run these tests?
> 
> The idea is we now move linting to
content/test/gpu/gpu_tests/lint_unittest.py.
> Since gpu_unittest is trigger on every CL (using swarming),  we will have lint
> coverage on every CL, including the catapult roll.

Also, these tests - including the lint checks - take a few seconds at most to
run.

[Dirk Pranke, 2016-04-28 22:28:55]

On 2016/04/28 03:37:52, nednguyen (ooo til 5-4) wrote:
> Hi Dirk, I will fix the README.chromium after the high level idea seems ok to
> you.
> 

...

> The idea is we now move linting to
content/test/gpu/gpu_tests/lint_unittest.py.
> Since gpu_unittest is trigger on every CL (using swarming),  we will have lint
> coverage on every CL, including the catapult roll.

Right, that's fine.

[nednguyen, 2016-04-29 03:10:06]

On 2016/04/28 22:28:55, Dirk Pranke wrote:
> On 2016/04/28 03:37:52, nednguyen (ooo til 5-4) wrote:
> > Hi Dirk, I will fix the README.chromium after the high level idea seems ok
to
> > you.
> > 
> 
> ...
> 
> > The idea is we now move linting to
> content/test/gpu/gpu_tests/lint_unittest.py.
> > Since gpu_unittest is trigger on every CL (using swarming),  we will have
lint
> > coverage on every CL, including the catapult roll.
> 
> Right, that's fine.

I mostly copy the logilab/, pylint/ from depot_tools/third_party. The reason we
need both is because pylint/ depends on modules in logilab/ (logilab/common &
logilab/astroid). So I wonder whether I should  jove logilab/common &
logilab/astroid to pylint/ also. The nice thing about doing this is people who
uses third_party/pylint no need to also reference third_party/logilab

[nednguyen, 2016-04-29 03:11:58]

nednguyen@google.com changed reviewers:
+ thakis@chromium.org

[nednguyen, 2016-04-29 03:12:00]

+Nico, third_party's OWNER. Can you give some advice on how I should organize
pylint, logilab/common/ & logilab/astroid in third_party?

[nednguyen(REVIEW IN OTHER ACC), 2016-04-29 21:51:49]

Ping

[Nico, 2016-05-02 18:54:58]

Since depot_tools has pylint in its third_party, I'd follow the organization
that's used there (see directory tree on the lhs on
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/thir...).

[Nico, 2016-05-02 18:57:02]

(and as dirk said, you need license approval and security approval for new
third-party stuff. presubmits should tell you that you need to add "security
critical" fields to README.chromium -- see a template here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/README...

Since depot_tools already has this lib, both should hopefully be quick)

[nednguyen, 2016-05-05 02:16:47]

On 2016/05/02 18:54:58, Nico (hiding Wed May 4) wrote:
> Since depot_tools has pylint in its third_party, I'd follow the organization
> that's used there (see directory tree on the lhs on
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/thir...).

Nico, the reason I don't follow its location there is because that would
requires adding chromium/src/third_party which may accidentally include a lot of
third_party python modules that are "shallow".

[nednguyen, 2016-05-05 02:58:09]

On 2016/05/05 02:16:47, nednguyen wrote:
> On 2016/05/02 18:54:58, Nico (hiding Wed May 4) wrote:
> > Since depot_tools has pylint in its third_party, I'd follow the organization
> > that's used there (see directory tree on the lhs on
> >
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/thir...).
> 
> Nico, the reason I don't follow its location there is because that would
> requires adding chromium/src/third_party which may accidentally include a lot
of
> third_party python modules that are "shallow".

Updated README.chromium files, PTAL

@Dirk: I think the topic of whether we should have 1 centralized pylintrc file
or mutliple files should be in a different topic. For the record, I find that
trying to define 1 single pylintrc for all chromium python projects is not an
easy task, since some projects are more disciplined in resolving the lint error
than others. This patch is quite big hence let's keep it simple for now :-)

[nednguyen, 2016-05-05 02:58:22]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-05 02:58:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920403002/10001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920403002/10001

[nednguyen, 2016-05-05 04:43:22]

nednguyen@google.com changed reviewers:
+ phajdan.jr@chromium.org

[nednguyen, 2016-05-05 04:43:23]

checklicenses.py is failing, can I just whitelist these pylint libs since we are
using them in depot_tools/ anyway?


'third_party/logilab/logilab/astroid/test_utils.py' has non-whitelisted license
'UNKNOWN'
'third_party/logilab/logilab/astroid/brain/py2stdlib.py' has non-whitelisted
license 'UNKNOWN'
'third_party/logilab/logilab/astroid/brain/builtin_inference.py' has
non-whitelisted license 'UNKNOWN'
'third_party/logilab/logilab/astroid/brain/py2pytest.py' has non-whitelisted
license 'UNKNOWN'
'third_party/logilab/logilab/astroid/brain/py2gi.py' has non-whitelisted license
'UNKNOWN'
'third_party/logilab/logilab/astroid/brain/py2mechanize.py' has non-whitelisted
license 'UNKNOWN'
'third_party/logilab/logilab/astroid/brain/py2qt4.py' has non-whitelisted
license 'UNKNOWN'
'third_party/logilab/logilab/common/urllib2ext.py' has non-whitelisted license
'UNKNOWN'
'third_party/logilab/logilab/common/contexts.py' has non-whitelisted license
'UNKNOWN'
'third_party/pylint/pylint/testutils.py' has non-whitelisted license 'GPL (v2 or
later)'
'third_party/pylint/pylint/__main__.py' has non-whitelisted license 'UNKNOWN'
'third_party/pylint/pylint/config.py' has non-whitelisted license 'GPL (v2 or
later)'
'third_party/pylint/pylint/reporters/html.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/reporters/guireporter.py' has non-whitelisted license
'UNKNOWN'
'third_party/pylint/pylint/reporters/text.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/reporters/__init__.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/reporters/json.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/misc.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/stdlib.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/raw_metrics.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/python3.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/design_analysis.py' has non-whitelisted
license 'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/spelling.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/format.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/exceptions.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/utils.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/variables.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/typecheck.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/imports.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/strings.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/classes.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/newstyle.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/__init__.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/checkers/similar.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/logging.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/checkers/base.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/lint.py' has non-whitelisted license 'GPL (v2 or
later)'
'third_party/pylint/pylint/__pkginfo__.py' has non-whitelisted license 'GPL (v2
or later)'
'third_party/pylint/pylint/utils.py' has non-whitelisted license 'GPL (v2 or
later)'
'third_party/pylint/pylint/gui.py' has non-whitelisted license 'GPL (v2 or
later)'
'third_party/pylint/pylint/__init__.py' has non-whitelisted license 'GPL (v2 or
later)'
'third_party/pylint/pylint/pyreverse/diadefslib.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/pyreverse/main.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/pyreverse/diagrams.py' has non-whitelisted license
'GPL (v2 or later)'
'third_party/pylint/pylint/pyreverse/writer.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/pyreverse/utils.py' has non-whitelisted license 'GPL
(v2 or later)'
'third_party/pylint/pylint/pyreverse/__init__.py' has non-whitelisted license
'UNKNOWN'
'third_party/pylint/pylint/interfaces.py' has non-whitelisted license 'GPL (v2
or later)'

[nednguyen, 2016-05-05 13:20:51]

Description was changed from

==========
[content/test/gpu] Run pylint check of gpu tests in unittest instead of
PRESUBMIT

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[content/test/gpu] Run pylint check of gpu tests in unittest instead of
PRESUBMIT

This CL also adds pylint/ & logilab/astroid, logilab/common into 
src/third_party. (these are copied from depot_tools/third_party)

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Paweł Hajdan Jr., 2016-05-05 13:24:56]

Feel free to whitelist GPLv2.

For UNKNOWN files, file an *upstream* - *NOT* chromium - bug, and link to that
bug.

[nednguyen, 2016-05-05 13:59:52]

On 2016/05/05 13:24:56, Paweł Hajdan Jr. wrote:
> Feel free to whitelist GPLv2.
> 
> For UNKNOWN files, file an *upstream* - *NOT* chromium - bug, and link to that
> bug.

Done

[Paweł Hajdan Jr., 2016-05-05 14:03:29]

https://codereview.chromium.org/1920403002/diff/40001/tools/checklicenses/che...
File tools/checklicenses/checklicenses.py (right):

https://codereview.chromium.org/1920403002/diff/40001/tools/checklicenses/che...
tools/checklicenses/checklicenses.py:64: 'GPL (v2 or later)',
No, not globally. Just for the test-only code.

Globally GPL is *NOT* okay!

[nednguyen, 2016-05-05 14:22:11]

https://codereview.chromium.org/1920403002/diff/40001/tools/checklicenses/che...
File tools/checklicenses/checklicenses.py (right):

https://codereview.chromium.org/1920403002/diff/40001/tools/checklicenses/che...
tools/checklicenses/checklicenses.py:64: 'GPL (v2 or later)',
On 2016/05/05 14:03:29, Paweł Hajdan Jr. wrote:
> No, not globally. Just for the test-only code.
> 
> Globally GPL is *NOT* okay!

Done.

[Paweł Hajdan Jr., 2016-05-05 15:57:23]

checklicenses LGTM

[Nico, 2016-05-05 16:27:22]

On 2016/05/05 02:16:47, nednguyen wrote:
> On 2016/05/02 18:54:58, Nico (hiding Wed May 4) wrote:
> > Since depot_tools has pylint in its third_party, I'd follow the organization
> > that's used there (see directory tree on the lhs on
> >
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/thir...).
> 
> Nico, the reason I don't follow its location there is because that would
> requires adding chromium/src/third_party which may accidentally include a lot
of
> third_party python modules that are "shallow".

Adding where? To pypath? (If so, that makes sense.)

Do you have security and license approval from the relevant people?

[nednguyen, 2016-05-05 16:48:49]

On 2016/05/05 16:27:22, Nico wrote:
> On 2016/05/05 02:16:47, nednguyen wrote:
> > On 2016/05/02 18:54:58, Nico (hiding Wed May 4) wrote:
> > > Since depot_tools has pylint in its third_party, I'd follow the
organization
> > > that's used there (see directory tree on the lhs on
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/thir...).
> > 
> > Nico, the reason I don't follow its location there is because that would
> > requires adding chromium/src/third_party which may accidentally include a
lot
> of
> > third_party python modules that are "shallow".
> 
> Adding where? To pypath? (If so, that makes sense.)
> 
> Do you have security and license approval from the relevant people?

Sorry for missing this, I just sent an email to security@chromium.org &
open-source-third-party-reviews@google.com

[Ken Russell (switch to Gerrit), 2016-05-05 18:08:56]

On 2016/05/05 16:48:49, nednguyen wrote:
> On 2016/05/05 16:27:22, Nico wrote:
> > On 2016/05/05 02:16:47, nednguyen wrote:
> > > On 2016/05/02 18:54:58, Nico (hiding Wed May 4) wrote:
> > > > Since depot_tools has pylint in its third_party, I'd follow the
> organization
> > > > that's used there (see directory tree on the lhs on
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/thir...).
> > > 
> > > Nico, the reason I don't follow its location there is because that would
> > > requires adding chromium/src/third_party which may accidentally include a
> lot
> > of
> > > third_party python modules that are "shallow".
> > 
> > Adding where? To pypath? (If so, that makes sense.)
> > 
> > Do you have security and license approval from the relevant people?
> 
> Sorry for missing this, I just sent an email to mailto:security@chromium.org &
> mailto:open-source-third-party-reviews@google.com

I hope those reviews will be prompt. These libraries are already in the
open-source depot_tools; does that help alleviate any concerns?

[Dirk Pranke, 2016-05-05 18:32:36]

On 2016/05/05 18:08:56, Ken Russell wrote:
> On 2016/05/05 16:48:49, nednguyen wrote:
> > Sorry for missing this, I just sent an email to mailto:security@chromium.org
&
> > mailto:open-source-third-party-reviews@google.com
> 
> I hope those reviews will be prompt. These libraries are already in the
> open-source depot_tools; does that help alleviate any concerns?

I'm sure it'll help, but as I understand the policies it doesn't mean we can
skip them.

[Dirk Pranke, 2016-05-05 23:30:15]

lgtm, I suppose, but after realizing actually what you're proposing I'm not wild
about doing pylinting as part of the unit tests (I think that'll be at least a
bit confusing, and I worry that by running it as part of an existing test suite
you'll be running this too late to catch some issues).

Ideally (to me) this would be a separate test step, potentially even one we
could extend to other blocks of python in the code base.

However, right now trying to ensure that lint checks are run correctly on all
the bots is a hassle, so I'm okay with leaving this as part of an existing test
step for now.

[Ken Russell (switch to Gerrit), 2016-05-06 00:20:17]

On 2016/05/05 23:30:15, Dirk Pranke wrote:
> lgtm, I suppose, but after realizing actually what you're proposing I'm not
wild
> about doing pylinting as part of the unit tests (I think that'll be at least a
> bit confusing, and I worry that by running it as part of an existing test
suite
> you'll be running this too late to catch some issues).
> 
> Ideally (to me) this would be a separate test step, potentially even one we
> could extend to other blocks of python in the code base.

Agree 100%. Unfortunately the presubmit checks for rolling catapult don't run
the pylint / presubmit checks for dependents elsewhere in the code base (i.e.,
src/content/test/gpu). The best way to make sure these dependents don't get
broken seems to be to incorporate them into a test step.

> However, right now trying to ensure that lint checks are run correctly on all
> the bots is a hassle, so I'm okay with leaving this as part of an existing
test
> step for now.

Would be happy to split this off into a separate test step later.

LGTM. Thanks for pushing this forward Ned.

[Martin Barbella, 2016-05-06 17:34:17]

On 2016/05/06 00:20:17, Ken Russell wrote:
> On 2016/05/05 23:30:15, Dirk Pranke wrote:
> > lgtm, I suppose, but after realizing actually what you're proposing I'm not
> wild
> > about doing pylinting as part of the unit tests (I think that'll be at least
a
> > bit confusing, and I worry that by running it as part of an existing test
> suite
> > you'll be running this too late to catch some issues).
> > 
> > Ideally (to me) this would be a separate test step, potentially even one we
> > could extend to other blocks of python in the code base.
> 
> Agree 100%. Unfortunately the presubmit checks for rolling catapult don't run
> the pylint / presubmit checks for dependents elsewhere in the code base (i.e.,
> src/content/test/gpu). The best way to make sure these dependents don't get
> broken seems to be to incorporate them into a test step.
> 
> > However, right now trying to ensure that lint checks are run correctly on
all
> > the bots is a hassle, so I'm okay with leaving this as part of an existing
> test
> > step for now.
> 
> Would be happy to split this off into a separate test step later.
> 
> LGTM. Thanks for pushing this forward Ned.

LGTM from security since these aren't shipping.

[xam, 2016-05-06 18:00:03]

On 2016/05/06 17:34:17, Martin Barbella wrote:
> On 2016/05/06 00:20:17, Ken Russell wrote:
> > On 2016/05/05 23:30:15, Dirk Pranke wrote:
> > > lgtm, I suppose, but after realizing actually what you're proposing I'm
not
> > wild
> > > about doing pylinting as part of the unit tests (I think that'll be at
least
> a
> > > bit confusing, and I worry that by running it as part of an existing test
> > suite
> > > you'll be running this too late to catch some issues).
> > > 
> > > Ideally (to me) this would be a separate test step, potentially even one
we
> > > could extend to other blocks of python in the code base.
> > 
> > Agree 100%. Unfortunately the presubmit checks for rolling catapult don't
run
> > the pylint / presubmit checks for dependents elsewhere in the code base
(i.e.,
> > src/content/test/gpu). The best way to make sure these dependents don't get
> > broken seems to be to incorporate them into a test step.
> > 
> > > However, right now trying to ensure that lint checks are run correctly on
> all
> > > the bots is a hassle, so I'm okay with leaving this as part of an existing
> > test
> > > step for now.
> > 
> > Would be happy to split this off into a separate test step later.
> > 
> > LGTM. Thanks for pushing this forward Ned.
> 
> LGTM from security since these aren't shipping.

LGTM from OSTPR

[nednguyen, 2016-05-06 20:28:10]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2016-05-06 20:28:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920403002/60001

[Nico, 2016-05-06 20:30:38]

third_party owners lgtm

[commit-bot: I haz the power, 2016-05-06 21:28:31]

Description was changed from

==========
[content/test/gpu] Run pylint check of gpu tests in unittest instead of
PRESUBMIT

This CL also adds pylint/ & logilab/astroid, logilab/common into 
src/third_party. (these are copied from depot_tools/third_party)

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[content/test/gpu] Run pylint check of gpu tests in unittest instead of
PRESUBMIT

This CL also adds pylint/ & logilab/astroid, logilab/common into 
src/third_party. (these are copied from depot_tools/third_party)

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-05-06 21:28:33]

Committed patchset #7 (id:60001)

[commit-bot: I haz the power, 2016-05-06 21:31:06]

Description was changed from

==========
[content/test/gpu] Run pylint check of gpu tests in unittest instead of
PRESUBMIT

This CL also adds pylint/ & logilab/astroid, logilab/common into 
src/third_party. (these are copied from depot_tools/third_party)

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[content/test/gpu] Run pylint check of gpu tests in unittest instead of
PRESUBMIT

This CL also adds pylint/ & logilab/astroid, logilab/common into
src/third_party. (these are copied from depot_tools/third_party)

BUG=606854
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/3bb6d28634c0eb384281e06ca359715ba5445f91
Cr-Commit-Position: refs/heads/master@{#392160}
==========

[commit-bot: I haz the power, 2016-05-06 21:31:09]

Patchset 7 (id:??) landed as
https://crrev.com/3bb6d28634c0eb384281e06ca359715ba5445f91
Cr-Commit-Position: refs/heads/master@{#392160}