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
diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py
index edd6332..620661f 100644
--- a/presubmit_canned_checks.py
+++ b/presubmit_canned_checks.py
@@ -771,29 +771,30 @@
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:
- description = files[0]
+ if len(flist) == 1:
+ 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 @@
if True:
# pylint's cycle detection doesn't work in parallel, so spawn a second,
# single-threaded job for just that check.
- return [
- GetPylintCmd(files, ["--disable=cyclic-import"], True),
- GetPylintCmd(files, ["--disable=all", "--enable=cyclic-import"], False)
- ]
+
+ # 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 [ GetPylintCmd(files, [], True) ]
+
else:
- return map(lambda x: GetPylintCmd([x], extra_args, 1), files)
+ return map(lambda x: GetPylintCmd([x], [], 1), files)
def RunPylint(input_api, *args, **kwargs):
diff --git a/presubmit_support.py b/presubmit_support.py
index 4baee77..523bb79 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -313,6 +313,12 @@
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 @@
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))
- pool.close()
- pool.join()
+ msgs.extend(self._run_tests_pool.map_async(CallCommand, tests).get(99999))
else:
msgs.extend(map(CallCommand, tests))
return [m for m in msgs if m]
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 3bec773..87832d4 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -2529,13 +2529,15 @@
pylintrc = os.path.join(_ROOT, 'pylintrc')
CommHelper(input_api,
- ['pyyyyython', pylint, '--args-on-stdin', '--disable=cyclic-import',
- '--jobs=2'],
- env=mox.IgnoreArg(), stdin='file1.py\n--rcfile=%s' % pylintrc)
+ ['pyyyyython', pylint, '--args-on-stdin'],
+ env=mox.IgnoreArg(), stdin=
+ '--rcfile=%s\n--disable=cyclic-import\n--jobs=2\nfile1.py'
+ % pylintrc)
CommHelper(input_api,
- ['pyyyyython', pylint, '--args-on-stdin', '--disable=all',
- '--enable=cyclic-import'],
- env=mox.IgnoreArg(), stdin='file1.py\n--rcfile=%s' % pylintrc)
+ ['pyyyyython', pylint, '--args-on-stdin'],
+ env=mox.IgnoreArg(), stdin=
+ '--rcfile=%s\n--disable=all\n--enable=cyclic-import\nfile1.py'
+ % pylintrc)
self.mox.ReplayAll()
results = presubmit_canned_checks.RunPylint(
diff --git a/third_party/pylint/README.chromium b/third_party/pylint/README.chromium
index 2c3ecd9..3e468c2 100644
--- a/third_party/pylint/README.chromium
+++ b/third_party/pylint/README.chromium
@@ -8,3 +8,46 @@
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)
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 @@
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 @@
# 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 @@
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)
