Reland chromium#2 of Include isolate.py in data for Android unit tests

Now unconditionally including some directory trees to capture imports that differ based on python version (at least those that have come up so far).

BUG=589318, 599692

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

Cr-Commit-Position: refs/heads/master@{#384907}

PRESUBMIT.py

@@ -306,6 +306,17 @@
 )
 
 
+_ANDROID_SPECIFIC_PYDEPS_FILES = [
+    'build/android/test_runner.pydeps',
+]
+
+_GENERIC_PYDEPS_FILES = [
+    'build/secondary/tools/swarming_client/isolate.pydeps',
+]
+
+_ALL_PYDEPS_FILES = _ANDROID_SPECIFIC_PYDEPS_FILES + _GENERIC_PYDEPS_FILES
+
+
 def _CheckNoProductionCodeUsingTestOnlyFunctions(input_api, output_api):
   """Attempts to prevent use of functions intended only for testing in
   non-testing code. For now this is just a best-effort implementation
@@ -1509,6 +1520,99 @@ def _CheckAndroidNewMdpiAssetLocation(input_api, output_api):
   return results
 
 
+class PydepsChecker(object):
+  def __init__(self, input_api, pydeps_files):
+    self._file_cache = {}
+    self._input_api = input_api
+    self._pydeps_files = pydeps_files
+
+  def _LoadFile(self, path):
+    """Returns the list of paths within a .pydeps file relative to //."""
+    if path not in self._file_cache:
+      with open(path) as f:
+        self._file_cache[path] = f.read()
+    return self._file_cache[path]
+
+  def _ComputeNormalizedPydepsEntries(self, pydeps_path):
+    """Returns an interable of paths within the .pydep, relativized to //."""
+    os_path = self._input_api.os_path
+    pydeps_dir = os_path.dirname(pydeps_path)
+    entries = (l.rstrip() for l in self._LoadFile(pydeps_path).splitlines()
+               if not l.startswith('*'))
+    return (os_path.normpath(os_path.join(pydeps_dir, e)) for e in entries)
+
+  def _CreateFilesToPydepsMap(self):
+    """Returns a map of local_path -> list_of_pydeps."""
+    ret = {}
+    for pydep_local_path in self._pydeps_files:
+      for path in self._ComputeNormalizedPydepsEntries(pydep_local_path):
+        ret.setdefault(path, []).append(pydep_local_path)
+    return ret
+
+  def ComputeAffectedPydeps(self):
+    """Returns an iterable of .pydeps files that might need regenerating."""
+    affected_pydeps = set()
+    file_to_pydeps_map = None
+    for f in self._input_api.AffectedFiles(include_deletes=True):
+      local_path = f.LocalPath()
+      if local_path  == 'DEPS':
+        return self._pydeps_files
+      elif local_path.endswith('.pydeps'):
+        if local_path in self._pydeps_files:
+          affected_pydeps.add(local_path)
+      elif local_path.endswith('.py'):
+        if file_to_pydeps_map is None:
+          file_to_pydeps_map = self._CreateFilesToPydepsMap()
+        affected_pydeps.update(file_to_pydeps_map.get(local_path, ()))
+    return affected_pydeps
+
+  def DetermineIfStale(self, pydeps_path):
+    """Runs print_python_deps.py to see if the files is stale."""
+    old_pydeps_data = self._LoadFile(pydeps_path).splitlines()
+    cmd = old_pydeps_data[1][1:].strip()
+    new_pydeps_data = self._input_api.subprocess.check_output(
+        cmd  + ' --output ""', shell=True)
+    if old_pydeps_data[2:] != new_pydeps_data.splitlines()[2:]:
+      return cmd
+
+
+def _CheckPydepsNeedsUpdating(input_api, output_api, checker_for_tests=None):
+  """Checks if a .pydeps file needs to be regenerated."""
+  # TODO(agrieve): Update when there's a better way to detect this: crbug/570091
+  is_android = input_api.os_path.exists('third_party/android_tools')
+  pydeps_files = _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):
+    if f.LocalPath().endswith('.pydeps'):
+      if f.Action() == 'D' and f.LocalPath() in _ALL_PYDEPS_FILES:
+        results.append(output_api.PresubmitError(
+            'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
+            'remove %s' % f.LocalPath()))
+      elif f.Action() != 'D' and f.LocalPath() not in _ALL_PYDEPS_FILES:
+        results.append(output_api.PresubmitError(
+            'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
+            'include %s' % f.LocalPath()))
+
+  if results:
+    return results
+
+  checker = checker_for_tests or PydepsChecker(input_api, pydeps_files)
+
+  for pydep_path in checker.ComputeAffectedPydeps():
+    try:
+      cmd = checker.DetermineIfStale(pydep_path)
+      if cmd:
+        results.append(output_api.PresubmitError(
+            'File is stale: %s\nTo regenerate, run:\n\n    %s' %
+            (pydep_path, cmd)))
+    except input_api.subprocess.CalledProcessError as error:
+      return [output_api.PresubmitError('Error running: %s' % error.cmd,
+          long_text=error.output)]
+
+  return results
+
+
 def _CheckForCopyrightedCode(input_api, output_api):
   """Verifies that newly added code doesn't contain copyrighted material
   and is properly licensed under the standard Chromium license.
@@ -1712,6 +1816,7 @@ def _CommonChecks(input_api, output_api):
   results.extend(_CheckForWindowsLineEndings(input_api, output_api))
   results.extend(_CheckSingletonInHeaders(input_api, output_api))
   results.extend(_CheckNoDeprecatedCompiledResourcesGYP(input_api, output_api))
+  results.extend(_CheckPydepsNeedsUpdating(input_api, output_api))
 
   if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
     results.extend(input_api.canned_checks.RunUnitTestsInDirectory(

PRESUBMIT_test.py

@@ -3,12 +3,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-import glob
-import json
-import os
 import re
 import subprocess
-import sys
 import unittest
 
 import PRESUBMIT
@@ -835,6 +831,103 @@ def testUserMetricsActionNotAddedToActions(self):
       output[0].message)
 
 
+class PydepsNeedsUpdatingTest(unittest.TestCase):
+
+  class MockSubprocess(object):
+    CalledProcessError = subprocess.CalledProcessError
+
+  def setUp(self):
+    mock_all_pydeps = ['A.pydeps', 'B.pydeps']
+    self.old_ALL_PYDEPS_FILES = PRESUBMIT._ALL_PYDEPS_FILES
+    PRESUBMIT._ALL_PYDEPS_FILES = mock_all_pydeps
+    self.mock_input_api = MockInputApi()
+    self.mock_output_api = MockOutputApi()
+    self.mock_input_api.subprocess = PydepsNeedsUpdatingTest.MockSubprocess()
+    self.checker = PRESUBMIT.PydepsChecker(self.mock_input_api, mock_all_pydeps)
+    self.checker._file_cache = {
+        'A.pydeps': '# Generated by:\n# CMD A\nA.py\nC.py\n',
+        'B.pydeps': '# Generated by:\n# CMD B\nB.py\nC.py\n',
+    }
+
+  def tearDown(self):
+    PRESUBMIT._ALL_PYDEPS_FILES = self.old_ALL_PYDEPS_FILES
+
+  def _RunCheck(self):
+    return PRESUBMIT._CheckPydepsNeedsUpdating(self.mock_input_api,
+                                               self.mock_output_api,
+                                               checker_for_tests=self.checker)
+
+  def testAddedPydep(self):
+    self.mock_input_api.files = [
+      MockAffectedFile('new.pydeps', [], action='A'),
+    ]
+
+    results = self._RunCheck()
+    self.assertEqual(1, len(results))
+    self.assertTrue('PYDEPS_FILES' in str(results[0]))
+
+  def testRemovedPydep(self):
+    self.mock_input_api.files = [
+      MockAffectedFile(PRESUBMIT._ALL_PYDEPS_FILES[0], [], action='D'),
+    ]
+
+    results = self._RunCheck()
+    self.assertEqual(1, len(results))
+    self.assertTrue('PYDEPS_FILES' in str(results[0]))
+
+  def testRandomPyIgnored(self):
+    self.mock_input_api.files = [
+      MockAffectedFile('random.py', []),
+    ]
+
+    results = self._RunCheck()
+    self.assertEqual(0, len(results), 'Unexpected results: %r' % results)
+
+  def testRelevantPyNoChange(self):
+    self.mock_input_api.files = [
+      MockAffectedFile('A.py', []),
+    ]
+
+    def mock_check_output(cmd, shell=False):
+      self.assertEqual('CMD A --output ""', cmd)
+      return self.checker._file_cache['A.pydeps']
+
+    self.mock_input_api.subprocess.check_output = mock_check_output
+
+    results = self._RunCheck()
+    self.assertEqual(0, len(results), 'Unexpected results: %r' % results)
+
+  def testRelevantPyOneChange(self):
+    self.mock_input_api.files = [
+      MockAffectedFile('A.py', []),
+    ]
+
+    def mock_check_output(cmd, shell=False):
+      self.assertEqual('CMD A --output ""', cmd)
+      return 'changed data'
+
+    self.mock_input_api.subprocess.check_output = mock_check_output
+
+    results = self._RunCheck()
+    self.assertEqual(1, len(results))
+    self.assertTrue('File is stale' in str(results[0]))
+
+  def testRelevantPyTwoChanges(self):
+    self.mock_input_api.files = [
+      MockAffectedFile('C.py', []),
+    ]
+
+    def mock_check_output(cmd, shell=False):
+      return 'changed data'
+
+    self.mock_input_api.subprocess.check_output = mock_check_output
+
+    results = self._RunCheck()
+    self.assertEqual(2, len(results))
+    self.assertTrue('File is stale' in str(results[0]))
+    self.assertTrue('File is stale' in str(results[1]))
+
+
 class LogUsageTest(unittest.TestCase):
 
   def testCheckAndroidCrLogUsage(self):

PRESUBMIT_test_mocks.py

@@ -26,7 +26,7 @@ def __init__(self):
     self.is_committing = False
     self.change = MockChange([])
 
-  def AffectedFiles(self, file_filter=None):
+  def AffectedFiles(self, file_filter=None, include_deletes=False):
     return self.files
 
   def AffectedSourceFiles(self, file_filter=None):
@@ -92,13 +92,14 @@ class MockFile(object):
   MockInputApi for presubmit unittests.
   """
 
-  def __init__(self, local_path, new_contents):
+  def __init__(self, local_path, new_contents, action='A'):
     self._local_path = local_path
     self._new_contents = new_contents
     self._changed_contents = [(i + 1, l) for i, l in enumerate(new_contents)]
+    self._action = action
 
   def Action(self):
-    return 'A'  # TODO(dbeam): feel free to change if your test actually uses.
+    return self._action
 
   def ChangedContents(self):
     return self._changed_contents

build/android/BUILD.gn

@@ -84,30 +84,25 @@ action("cpplib_stripped") {
   ]
 }
 
-group("devil_py") {
-  data = [
-    "devil_chromium.json",
-    "devil_chromium.py",
-    "//third_party/android_tools/sdk/build-tools/23.0.1/aapt",
-    "//third_party/android_tools/sdk/build-tools/23.0.1/dexdump",
-    "//third_party/android_tools/sdk/build-tools/23.0.1/lib/libc++.so",
-    "//third_party/android_tools/sdk/build-tools/23.0.1/split-select",
-    "//third_party/android_tools/sdk/platform-tools/adb",
-    "//third_party/catapult/catapult_base/catapult_base/",
-    "//third_party/catapult/dependency_manager/dependency_manager/",
-    "//third_party/catapult/third_party/gsutil/",
-    "//third_party/catapult/devil/devil/",
-  ]
-}
-
 group("test_runner_py") {
-  data = [
-    "test_runner.py",
-    "pylib/",
-    "//build/util/lib/common/",
-  ]
+  _py_files = read_file("test_runner.pydeps", "list lines")
+
+  # Filter out comments.
+  set_sources_assignment_filter([ "#*" ])
+  sources = _py_files
+
+  data = sources + [
+           "devil_chromium.json",
+           "//third_party/android_tools/sdk/build-tools/23.0.1/aapt",
+           "//third_party/android_tools/sdk/build-tools/23.0.1/dexdump",
+           "//third_party/android_tools/sdk/build-tools/23.0.1/lib/libc++.so",
+           "//third_party/android_tools/sdk/build-tools/23.0.1/split-select",
+           "//third_party/android_tools/sdk/platform-tools/adb",
+           "//third_party/catapult/third_party/gsutil/",
+           "//third_party/catapult/devil/devil/devil_dependencies.json",
+         ]
   data_deps = [
-    ":devil_py",
+    "//tools/swarming_client:isolate_py",
   ]
 }