Changes to improve multiprocessing PRESUBMIT support in Windows

Changes to improve multiprocessing PRESUBMIT support in Windows
  * make RunTest's multiprocessing.Pool in the constructor of InputApi
    to avoid getting tripped up by chdir manipulation.
  * Don't do the split cyclic-import check when the invoker of the
    Pylint presubmit checks explicitly sends cyclic import check
    parameters via extra_args
  * fix pseudobug where ownership of the files variable was unclear,
    and pass all arguments on stdin (instead of mix of CLI + stdin).
  * fix bug in pylint which caused it to manipulate sys.path before
    spawning its subprocesses, which caused multiprocessing to fail
    on windows.
    * Note: This may carry a slight semantic change. Before, pylint would
      add all .py files' directories to sys.path while checking any of
      them. Now in parallel mode, pylint will only add the path of the
      single file to sys.path. This behavior actually mirrors Python's
      own behavior, so the check should be more-correct than before (and
      should cut down on pylint import scanning time with very large
      sys.path's).
    * If someone encounters an issue with this, please note that the
      GetPylint check also includes an extra_paths_list which is
      expressly for this purpose.

R=dpranke@chromium.org, kbr@chromium.org, maruel@chromium.org
BUG=501012
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295908

[iannucci, 2015-06-25 01:43:43]

Note that the sys.path manipulation fix was necessary because depot_tools
contains a file named 'pylint'. This would cause pylint's multiprocessing code
to import depot_tools\pylint.py instead of depot_tools\third_party\pylint.py.
This pylint.py has unguarded "main" code in the module, which essentially does a
sys.exit(0) before multiprocessing.forking.main is able to actually load
anything. Adding an `if __name__ == '__main__'` guard is insufficient, however,
because that would still cause sys.modules['pylint'] to be the wrong module
(e.g. it wouldn't contain any of the symbols that `import pylint` ought to have)

[iannucci, 2015-06-25 01:54:32]

https://chromiumcodereview.appspot.com/1208743002/diff/1/third_party/pylint/l...
File third_party/pylint/lint.py (right):

https://chromiumcodereview.appspot.com/1208743002/diff/1/third_party/pylint/l...
third_party/pylint/lint.py:674: with fix_import_path(files_or_modules):
note that all child checkers come through here

https://chromiumcodereview.appspot.com/1208743002/diff/1/third_party/pylint/l...
third_party/pylint/lint.py:1268: linter.generate_reports()
report generation doesn't actually require the path hacks as far as I can tell.

[iannucci, 2015-06-25 02:06:31]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-25 02:06:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1208743002/1

[iannucci, 2015-06-25 02:09:27]

The CQ bit was unchecked by iannucci@chromium.org

[iannucci, 2015-06-25 02:09:59]

(tests failed because depot_tools tests don't run on upload on windows! hooray!
pulling it into a linux machine to fix the tests, but still PTAL)

[Ken Russell (switch to Gerrit), 2015-06-25 07:33:46]

Thanks for fixing this. I don't know this code at all but the fix looks fine if
it's been tested. Are there any tests in depot_tools which would catch
regressions in this area more quickly in the future? It sounds like not a lot of
testing of depot_tools is done on Windows.

[iannucci, 2015-06-25 07:59:27]

On 2015/06/25 07:33:46, Ken Russell wrote:
> Thanks for fixing this. I don't know this code at all but the fix looks fine
if
> it's been tested. Are there any tests in depot_tools which would catch
> regressions in this area more quickly in the future? It sounds like not a lot
of
> testing of depot_tools is done on Windows.

Yeah it looks like the tests are explicitly disabled on windows (which implies
that they don't run, but I haven't verified that). There's a CQ, but it only
runs tests on linux (not on mac either).

We've added more code to depot tools over time, and we've tried to have
corresponding test coverage, but it's a pretty awesome mess right now (and it's
not a very high priority to un-mess it for anyone at the moment). Tragedy of the
commons and all that.

That said, I think we're building a really solid base with infra.git to
eventually move depot_tools to be more of a group of independently developed,
tested and packaged tools. Philippe is working on a mechanism of packaging
python programs so that they work in an isolated way (instead of the
interdependent mess in depot_tools). We're also getting pretty good at building
testable go tools (which would flat-out solve a bunch of the depot_tools mess
(including testing, but also multithreading, subprocess invocation and
multiplatform uniformity, to which a large chunk of code in depot_tools is
devoted), right off the bat). We will have cross compilation for all the
infra.git go tools pretty soon too (crbug.com/495174).

As it stands right now though, most of the code that is in depot_tools is not
designed to be tested and will probably never have real comprehensive tests
written for it :/ (the 'mock' tests which exist currently don't count for much
IMHO... they essentially give the same level of confidence in the program that
compiling a go application does (read: not much besides syntax checking)).

[iannucci, 2015-06-28 19:44:31]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-28 19:44:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1208743002/20001

[commit-bot: I haz the power, 2015-06-28 19:46:09]

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

[commit-bot: I haz the power, 2015-06-28 19:46:09]

Dry run: This issue passed the CQ dry run.

[iannucci, 2015-06-28 19:46:45]

PTAL: tests are passing again

[iannucci, 2015-06-28 19:47:43]

https://chromiumcodereview.appspot.com/1208743002/diff/20001/presubmit_suppor...
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/1208743002/diff/20001/presubmit_suppor...
presubmit_support.py:320: self._run_tests_pool =
multiprocessing.Pool(self.cpu_count)
this is so we pick up the mocked cpu_count

[Dirk Pranke, 2015-06-29 19:35:10]

lgtm

[iannucci, 2015-07-01 23:19:10]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-07-01 23:19:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1208743002/20001

[commit-bot: I haz the power, 2015-07-01 23:21:30]

Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295908