Reland "PRESUBMIT.py: Make CheckPydepsNeedsUpdating faster using concurrency"
This reverts commitcadd78c55c
. Reason for reland: Removed py3 syntax Original change's description: > Revert "PRESUBMIT.py: Make CheckPydepsNeedsUpdating faster using concurrency" > > This reverts commit09650bc010
. > > Reason for revert: Running as py2 for some. > > Original change's description: > > PRESUBMIT.py: Make CheckPydepsNeedsUpdating faster using concurrency > > > > For DEPS rolls, all .pydeps files are checked, which can take ~30 > > seconds when done sequentially. Check them all concurrently, which on my > > machine runs in < 2 seconds. > > > > Bug: None > > Change-Id: I621a04456d8c18b0522b586c8eea4befe0e9674f > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3433606 > > Reviewed-by: Erik Staab <estaab@chromium.org> > > Commit-Queue: Andrew Grieve <agrieve@chromium.org> > > Cr-Commit-Position: refs/heads/main@{#966727} > > Bug: None > Change-Id: Ic5d902ba6d42312ff9fde8430eec23e47e0977ed > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437351 > Auto-Submit: Andrew Grieve <agrieve@chromium.org> > Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> > Cr-Commit-Position: refs/heads/main@{#966840} Bug: None Change-Id: I1cab290df83d411d6d06a37c3b4563b608df38ce Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437577 Auto-Submit: Andrew Grieve <agrieve@chromium.org> Reviewed-by: Erik Staab <estaab@chromium.org> Commit-Queue: Erik Staab <estaab@chromium.org> Cr-Commit-Position: refs/heads/main@{#966930}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
609666abbc
commit
4deedb119c
62
PRESUBMIT.py
62
PRESUBMIT.py
@ -3469,7 +3469,31 @@ def _CheckAndroidInfoBarDeprecation(input_api, output_api):
|
||||
return infobar_deprecation.CheckDeprecationOnUpload(input_api, output_api)
|
||||
|
||||
|
||||
class PydepsChecker(object):
|
||||
class _PydepsCheckerResult:
|
||||
def __init__(self, cmd, pydeps_path, process, old_contents):
|
||||
self._cmd = cmd
|
||||
self._pydeps_path = pydeps_path
|
||||
self._process = process
|
||||
self._old_contents = old_contents
|
||||
|
||||
def GetError(self):
|
||||
"""Returns an error message, or None."""
|
||||
import difflib
|
||||
if self._process.wait() != 0:
|
||||
# STDERR should already be printed.
|
||||
return 'Command failed: ' + self._cmd
|
||||
new_contents = self._process.stdout.read().splitlines()[2:]
|
||||
if self._old_contents != new_contents:
|
||||
diff = '\n'.join(difflib.context_diff(self._old_contents, new_contents))
|
||||
return ('File is stale: {}\n'
|
||||
'Diff (apply to fix):\n'
|
||||
'{}\n'
|
||||
'To regenerate, run:\n\n'
|
||||
' {}').format(self._pydeps_path, diff, self._cmd)
|
||||
return None
|
||||
|
||||
|
||||
class PydepsChecker:
|
||||
def __init__(self, input_api, pydeps_files):
|
||||
self._file_cache = {}
|
||||
self._input_api = input_api
|
||||
@ -3526,9 +3550,8 @@ class PydepsChecker(object):
|
||||
affected_pydeps.update(file_to_pydeps_map.get(local_path, ()))
|
||||
return affected_pydeps
|
||||
|
||||
def DetermineIfStale(self, pydeps_path):
|
||||
def DetermineIfStaleAsync(self, pydeps_path):
|
||||
"""Runs print_python_deps.py to see if the files is stale."""
|
||||
import difflib
|
||||
import os
|
||||
|
||||
old_pydeps_data = self._LoadFile(pydeps_path).splitlines()
|
||||
@ -3546,11 +3569,10 @@ class PydepsChecker(object):
|
||||
old_contents = []
|
||||
env = dict(os.environ)
|
||||
env['PYTHONDONTWRITEBYTECODE'] = '1'
|
||||
new_pydeps_data = self._input_api.subprocess.check_output(
|
||||
cmd + ' --output ""', shell=True, env=env, encoding='utf-8')
|
||||
new_contents = new_pydeps_data.splitlines()[2:]
|
||||
if old_contents != new_contents:
|
||||
return cmd, '\n'.join(difflib.context_diff(old_contents, new_contents))
|
||||
process = self._input_api.subprocess.Popen(
|
||||
cmd + ' --output ""', shell=True, env=env,
|
||||
stdout=self._input_api.subprocess.PIPE, encoding='utf-8')
|
||||
return _PydepsCheckerResult(cmd, pydeps_path, process, old_contents)
|
||||
|
||||
|
||||
def _ParseGclientArgs():
|
||||
@ -3573,8 +3595,6 @@ def CheckPydepsNeedsUpdating(input_api, output_api, checker_for_tests=None):
|
||||
if not input_api.platform.startswith('linux'):
|
||||
return []
|
||||
|
||||
is_android = _ParseGclientArgs().get('checkout_android', 'false') == 'true'
|
||||
pydeps_to_check = _ALL_PYDEPS_FILES if is_android else _GENERIC_PYDEPS_FILES
|
||||
results = []
|
||||
# First, check for new / deleted .pydeps.
|
||||
for f in input_api.AffectedFiles(include_deletes=True):
|
||||
@ -3597,6 +3617,7 @@ def CheckPydepsNeedsUpdating(input_api, output_api, checker_for_tests=None):
|
||||
if results:
|
||||
return results
|
||||
|
||||
is_android = _ParseGclientArgs().get('checkout_android', 'false') == 'true'
|
||||
checker = checker_for_tests or PydepsChecker(input_api, _ALL_PYDEPS_FILES)
|
||||
affected_pydeps = set(checker.ComputeAffectedPydeps())
|
||||
affected_android_pydeps = affected_pydeps.intersection(
|
||||
@ -3611,19 +3632,14 @@ def CheckPydepsNeedsUpdating(input_api, output_api, checker_for_tests=None):
|
||||
'Possibly stale pydeps files:\n{}'.format(
|
||||
'\n'.join(affected_android_pydeps))))
|
||||
|
||||
affected_pydeps_to_check = affected_pydeps.intersection(set(pydeps_to_check))
|
||||
for pydep_path in affected_pydeps_to_check:
|
||||
try:
|
||||
result = checker.DetermineIfStale(pydep_path)
|
||||
if result:
|
||||
cmd, diff = result
|
||||
results.append(output_api.PresubmitError(
|
||||
'File is stale: %s\nDiff (apply to fix):\n%s\n'
|
||||
'To regenerate, run:\n\n %s' %
|
||||
(pydep_path, diff, cmd)))
|
||||
except input_api.subprocess.CalledProcessError as error:
|
||||
return [output_api.PresubmitError('Error running: %s' % error.cmd,
|
||||
long_text=error.output)]
|
||||
all_pydeps = _ALL_PYDEPS_FILES if is_android else _GENERIC_PYDEPS_FILES
|
||||
pydeps_to_check = affected_pydeps.intersection(all_pydeps)
|
||||
# Process these concurrently, as each one takes 1-2 seconds.
|
||||
pydep_results = [checker.DetermineIfStaleAsync(p) for p in pydeps_to_check]
|
||||
for result in pydep_results:
|
||||
error_msg = result.GetError()
|
||||
if error_msg:
|
||||
results.append(output_api.PresubmitError(error_msg))
|
||||
|
||||
return results
|
||||
|
||||
|
@ -3,6 +3,7 @@
|
||||
# Use of this source code is governed by a BSD-style license that can be
|
||||
# found in the LICENSE file.
|
||||
|
||||
import io
|
||||
import os.path
|
||||
import subprocess
|
||||
import unittest
|
||||
@ -506,9 +507,26 @@ class UserMetricsActionTest(unittest.TestCase):
|
||||
|
||||
|
||||
class PydepsNeedsUpdatingTest(unittest.TestCase):
|
||||
class MockPopen:
|
||||
def __init__(self, stdout_func):
|
||||
self._stdout_func = stdout_func
|
||||
|
||||
class MockSubprocess(object):
|
||||
def wait(self):
|
||||
self.stdout = io.StringIO(self._stdout_func())
|
||||
return 0
|
||||
|
||||
class MockSubprocess:
|
||||
CalledProcessError = subprocess.CalledProcessError
|
||||
PIPE = 0
|
||||
|
||||
def __init__(self):
|
||||
self._popen_func = None
|
||||
|
||||
def SetPopenCallback(self, func):
|
||||
self._popen_func = func
|
||||
|
||||
def Popen(self, cmd, *args, **kwargs):
|
||||
return PydepsNeedsUpdatingTest.MockPopen(lambda: self._popen_func(cmd))
|
||||
|
||||
def _MockParseGclientArgs(self, is_android=True):
|
||||
return lambda: {'checkout_android': 'true' if is_android else 'false' }
|
||||
@ -604,11 +622,11 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
|
||||
MockAffectedFile('A.py', []),
|
||||
]
|
||||
|
||||
def mock_check_output(cmd, shell=False, env=None, encoding=None):
|
||||
def popen_callback(cmd):
|
||||
self.assertEqual('CMD --output A.pydeps A --output ""', cmd)
|
||||
return self.checker._file_cache['A.pydeps']
|
||||
|
||||
self.mock_input_api.subprocess.check_output = mock_check_output
|
||||
self.mock_input_api.subprocess.SetPopenCallback(popen_callback)
|
||||
|
||||
results = self._RunCheck()
|
||||
self.assertEqual(0, len(results), 'Unexpected results: %r' % results)
|
||||
@ -622,14 +640,16 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
|
||||
MockAffectedFile('A.py', []),
|
||||
]
|
||||
|
||||
def mock_check_output(cmd, shell=False, env=None, encoding=None):
|
||||
def popen_callback(cmd):
|
||||
self.assertEqual('CMD --output A.pydeps A --output ""', cmd)
|
||||
return 'changed data'
|
||||
|
||||
self.mock_input_api.subprocess.check_output = mock_check_output
|
||||
self.mock_input_api.subprocess.SetPopenCallback(popen_callback)
|
||||
|
||||
results = self._RunCheck()
|
||||
self.assertEqual(1, len(results))
|
||||
# Check that --output "" is not included.
|
||||
self.assertNotIn('""', str(results[0]))
|
||||
self.assertIn('File is stale', str(results[0]))
|
||||
|
||||
def testRelevantPyTwoChanges(self):
|
||||
@ -641,10 +661,10 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
|
||||
MockAffectedFile('C.py', []),
|
||||
]
|
||||
|
||||
def mock_check_output(cmd, shell=False, env=None, encoding=None):
|
||||
def popen_callback(cmd):
|
||||
return 'changed data'
|
||||
|
||||
self.mock_input_api.subprocess.check_output = mock_check_output
|
||||
self.mock_input_api.subprocess.SetPopenCallback(popen_callback)
|
||||
|
||||
results = self._RunCheck()
|
||||
self.assertEqual(2, len(results))
|
||||
@ -660,11 +680,11 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
|
||||
MockAffectedFile('D.py', []),
|
||||
]
|
||||
|
||||
def mock_check_output(cmd, shell=False, env=None, encoding=None):
|
||||
def popen_callback(cmd):
|
||||
self.assertEqual('CMD --output D.pydeps D --output ""', cmd)
|
||||
return 'changed data'
|
||||
|
||||
self.mock_input_api.subprocess.check_output = mock_check_output
|
||||
self.mock_input_api.subprocess.SetPopenCallback(popen_callback)
|
||||
PRESUBMIT._ParseGclientArgs = self._MockParseGclientArgs(is_android=False)
|
||||
|
||||
results = self._RunCheck()
|
||||
@ -687,11 +707,11 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
|
||||
MockAffectedFile('A.py', []),
|
||||
]
|
||||
|
||||
def mock_check_output(cmd, shell=False, env=None, encoding=None):
|
||||
def popen_callback(cmd):
|
||||
self.assertEqual('CMD --gn-paths A --output A.pydeps --output ""', cmd)
|
||||
return 'changed data'
|
||||
|
||||
self.mock_input_api.subprocess.check_output = mock_check_output
|
||||
self.mock_input_api.subprocess.SetPopenCallback(popen_callback)
|
||||
|
||||
results = self._RunCheck()
|
||||
self.assertEqual(1, len(results))
|
||||
|
Reference in New Issue
Block a user