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

Review URL: https://codereview.chromium.org/1208743002

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@295908 0039d316-1c4b-4281-b951-d872f2087c98

presubmit_canned_checks.py

@@ -771,29 +771,30 @@ def GetPylint(input_api, output_api, white_list=None, black_list=None,
   env['PYTHONPATH'] = input_api.os_path.pathsep.join(
       extra_paths_list + sys.path).encode('utf8')
 
-  def GetPylintCmd(files, extra, parallel):
+  def GetPylintCmd(flist, extra, parallel):
     # Windows needs help running python files so we explicitly specify
     # the interpreter to use. It also has limitations on the size of
     # the command-line, so we pass arguments via a pipe.
     cmd = [input_api.python_executable,
            input_api.os_path.join(_HERE, 'third_party', 'pylint.py'),
            '--args-on-stdin']
-    if len(files) == 1:
+    if len(flist) == 1:
-      description = files[0]
+      description = flist[0]
     else:
-      description = '%s files' % len(files)
+      description = '%s files' % len(flist)
 
+    args = extra_args[:]
     if extra:
-      cmd.extend(extra)
+      args.extend(extra)
       description += ' using %s' % (extra,)
     if parallel:
-      cmd.append('--jobs=%s' % input_api.cpu_count)
+      args.append('--jobs=%s' % input_api.cpu_count)
       description += ' on %d cores' % input_api.cpu_count
 
     return input_api.Command(
         name='Pylint (%s)' % description,
         cmd=cmd,
-        kwargs={'env': env, 'stdin': '\n'.join(files + extra_args)},
+        kwargs={'env': env, 'stdin': '\n'.join(args + flist)},
         message=error_type)
 
   # Always run pylint and pass it all the py files at once.
@@ -807,12 +808,18 @@ def GetPylint(input_api, output_api, white_list=None, black_list=None,
   if True:
     # pylint's cycle detection doesn't work in parallel, so spawn a second,
     # single-threaded job for just that check.
+
+    # Some PRESUBMITs explicitly mention cycle detection.
+    if not any('R0401' in a or 'cyclic-import' in a for a in extra_args):
       return [
         GetPylintCmd(files, ["--disable=cyclic-import"], True),
         GetPylintCmd(files, ["--disable=all", "--enable=cyclic-import"], False)
       ]
     else:
-    return map(lambda x: GetPylintCmd([x], extra_args, 1), files)
+      return [ GetPylintCmd(files, [], True) ]
+
+  else:
+    return map(lambda x: GetPylintCmd([x], [], 1), files)
 
 
 def RunPylint(input_api, *args, **kwargs):

presubmit_support.py

@@ -313,6 +313,12 @@ class InputApi(object):
 
     self.cpu_count = multiprocessing.cpu_count()
 
+    # this is done here because in RunTests, the current working directory has
+    # changed, which causes Pool() to explode fantastically when run on windows
+    # (because it tries to load the __main__ module, which imports lots of
+    # things relative to the current working directory).
+    self._run_tests_pool = multiprocessing.Pool(self.cpu_count)
+
     # The local path of the currently-being-processed presubmit script.
     self._current_presubmit_path = os.path.dirname(presubmit_path)
 
@@ -492,11 +498,8 @@ class InputApi(object):
         if self.verbose:
           t.info = _PresubmitNotifyResult
     if len(tests) > 1 and parallel:
-      pool = multiprocessing.Pool()
       # async recipe works around multiprocessing bug handling Ctrl-C
-      msgs.extend(pool.map_async(CallCommand, tests).get(99999))
+      msgs.extend(self._run_tests_pool.map_async(CallCommand, tests).get(99999))
-      pool.close()
-      pool.join()
     else:
       msgs.extend(map(CallCommand, tests))
     return [m for m in msgs if m]

tests/presubmit_unittest.py

@@ -2529,13 +2529,15 @@ class CannedChecksUnittest(PresubmitTestsBase):
     pylintrc = os.path.join(_ROOT, 'pylintrc')
 
     CommHelper(input_api,
-        ['pyyyyython', pylint, '--args-on-stdin', '--disable=cyclic-import',
+        ['pyyyyython', pylint, '--args-on-stdin'],
-         '--jobs=2'],
+        env=mox.IgnoreArg(), stdin=
-        env=mox.IgnoreArg(), stdin='file1.py\n--rcfile=%s' % pylintrc)
+               '--rcfile=%s\n--disable=cyclic-import\n--jobs=2\nfile1.py'
+               % pylintrc)
     CommHelper(input_api,
-        ['pyyyyython', pylint, '--args-on-stdin', '--disable=all',
+        ['pyyyyython', pylint, '--args-on-stdin'],
-         '--enable=cyclic-import'],
+        env=mox.IgnoreArg(), stdin=
-        env=mox.IgnoreArg(), stdin='file1.py\n--rcfile=%s' % pylintrc)
+               '--rcfile=%s\n--disable=all\n--enable=cyclic-import\nfile1.py'
+               % pylintrc)
     self.mox.ReplayAll()
 
     results = presubmit_canned_checks.RunPylint(

third_party/pylint/README.chromium

@@ -8,3 +8,46 @@ This directory contains the pylint module.
 
 Local Modifications:
 - applied upstream fix https://bitbucket.org/logilab/pylint/commits/5df347467ee0
+- applied fix to work around bad interaction between sys.path manipulation in
+  pylint itself and multiprocessing's implementation on Windows (DIFF1)
+
+
+Diffs:
+DIFF1
+diff --git a/third_party/pylint/lint.py b/third_party/pylint/lint.py
+index e10ae56..082d8b3 100644
+--- a/third_party/pylint/lint.py
++++ b/third_party/pylint/lint.py
+@@ -671,7 +671,8 @@ class PyLinter(configuration.OptionsManagerMixIn,
+             files_or_modules = (files_or_modules,)
+
+         if self.config.jobs == 1:
+-            self._do_check(files_or_modules)
++            with fix_import_path(files_or_modules):
++                self._do_check(files_or_modules)
+         else:
+             # Hack that permits running pylint, on Windows, with -m switch
+             # and with --jobs, as in 'python -2 -m pylint .. --jobs'.
+@@ -1252,8 +1253,8 @@ group are mutually exclusive.'),
+
+         # insert current working directory to the python path to have a correct
+         # behaviour
+-        with fix_import_path(args):
+-            if self.linter.config.profile:
++        if self.linter.config.profile:
++            with fix_import_path(args):
+                 print('** profiled run', file=sys.stderr)
+                 import cProfile, pstats
+                 cProfile.runctx('linter.check(%r)' % args, globals(), locals(),
+@@ -1262,9 +1263,9 @@ group are mutually exclusive.'),
+                 data.strip_dirs()
+                 data.sort_stats('time', 'calls')
+                 data.print_stats(30)
+-            else:
+-                linter.check(args)
+-            linter.generate_reports()
++        else:
++            linter.check(args)
++        linter.generate_reports()
+         if exit:
+             sys.exit(self.linter.msg_status)

third_party/pylint/lint.py

@@ -671,6 +671,7 @@ class PyLinter(configuration.OptionsManagerMixIn,
             files_or_modules = (files_or_modules,)
 
         if self.config.jobs == 1:
+            with fix_import_path(files_or_modules):
                 self._do_check(files_or_modules)
         else:
             # Hack that permits running pylint, on Windows, with -m switch
@@ -1252,8 +1253,8 @@ group are mutually exclusive.'),
 
         # insert current working directory to the python path to have a correct
         # behaviour
-        with fix_import_path(args):
         if self.linter.config.profile:
+            with fix_import_path(args):
                 print('** profiled run', file=sys.stderr)
                 import cProfile, pstats
                 cProfile.runctx('linter.check(%r)' % args, globals(), locals(),