Fix .pydeps PRESUBMIT check not always triggering

The check was never updated to support --gn-paths.

Also adds missing --output arg to error messages when missing.

Bug: 1141255
Change-Id: I1e5fa5b6926247f3f00ae2687c72b894c666e2c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2491521Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819963}

PRESUBMIT.py

@@ -3635,11 +3635,17 @@ class PydepsChecker(object):
 
   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)
+    pydeps_data = self._LoadFile(pydeps_path)
+    uses_gn_paths = '--gn-paths' in pydeps_data
+    entries = (l for l in pydeps_data.splitlines() if not l.startswith('#'))
+    if uses_gn_paths:
+      # Paths look like: //foo/bar/baz
+      return (e[2:] for e in entries)
+    else:
+      # Paths look like: path/relative/to/file.pydeps
+      os_path = self._input_api.os_path
+      pydeps_dir = os_path.dirname(pydeps_path)
+      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."""
@@ -3679,6 +3685,8 @@ class PydepsChecker(object):
     old_pydeps_data = self._LoadFile(pydeps_path).splitlines()
     if old_pydeps_data:
       cmd = old_pydeps_data[1][1:].strip()
+      if '--output' not in cmd:
+        cmd += ' --output ' + pydeps_path
       old_contents = old_pydeps_data[2:]
     else:
       # A default cmd that should work in most cases (as long as pydeps filename

PRESUBMIT_test.py

@@ -732,9 +732,9 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
     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',
-        'D.pydeps': '# Generated by:\n# CMD D\nD.py\n',
+        'A.pydeps': '# Generated by:\n# CMD --output A.pydeps A\nA.py\nC.py\n',
+        'B.pydeps': '# Generated by:\n# CMD --output B.pydeps B\nB.py\nC.py\n',
+        'D.pydeps': '# Generated by:\n# CMD --output D.pydeps D\nD.py\n',
     }
 
   def tearDown(self):
@@ -762,7 +762,7 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
             include_deletes=True)])
     results = self._RunCheck()
     self.assertEqual(1, len(results))
-    self.assertTrue('PYDEPS_FILES' in str(results[0]))
+    self.assertIn('PYDEPS_FILES', str(results[0]))
 
   def testPydepNotInSrc(self):
     self.mock_input_api.files = [
@@ -785,7 +785,7 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
             include_deletes=True)])
     results = self._RunCheck()
     self.assertEqual(1, len(results))
-    self.assertTrue('PYDEPS_FILES' in str(results[0]))
+    self.assertIn('PYDEPS_FILES', str(results[0]))
 
   def testRandomPyIgnored(self):
     # PRESUBMIT.CheckPydepsNeedsUpdating is only implemented for Linux.
@@ -809,7 +809,7 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
     ]
 
     def mock_check_output(cmd, shell=False, env=None):
-      self.assertEqual('CMD A --output ""', 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
@@ -827,14 +827,14 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
     ]
 
     def mock_check_output(cmd, shell=False, env=None):
-      self.assertEqual('CMD A --output ""', cmd)
+      self.assertEqual('CMD --output A.pydeps 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]))
+    self.assertIn('File is stale', str(results[0]))
 
   def testRelevantPyTwoChanges(self):
     # PRESUBMIT.CheckPydepsNeedsUpdating is only implemented for Linux.
@@ -852,8 +852,8 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
 
     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]))
+    self.assertIn('File is stale', str(results[0]))
+    self.assertIn('File is stale', str(results[1]))
 
   def testRelevantAndroidPyInNonAndroidCheckout(self):
     # PRESUBMIT.CheckPydepsNeedsUpdating is only implemented for Linux.
@@ -865,7 +865,7 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
     ]
 
     def mock_check_output(cmd, shell=False, env=None):
-      self.assertEqual('CMD D --output ""', cmd)
+      self.assertEqual('CMD --output D.pydeps D --output ""', cmd)
       return 'changed data'
 
     self.mock_input_api.subprocess.check_output = mock_check_output
@@ -873,8 +873,33 @@ class PydepsNeedsUpdatingTest(unittest.TestCase):
 
     results = self._RunCheck()
     self.assertEqual(1, len(results))
-    self.assertTrue('Android' in str(results[0]))
-    self.assertTrue('D.pydeps' in str(results[0]))
+    self.assertIn('Android', str(results[0]))
+    self.assertIn('D.pydeps', str(results[0]))
+
+  def testGnPathsAndMissingOutputFlag(self):
+    # PRESUBMIT.CheckPydepsNeedsUpdating is only implemented for Linux.
+    if self.mock_input_api.platform != 'linux2':
+      return []
+
+    self.checker._file_cache = {
+        'A.pydeps': '# Generated by:\n# CMD --gn-paths A\n//A.py\n//C.py\n',
+        'B.pydeps': '# Generated by:\n# CMD --gn-paths B\n//B.py\n//C.py\n',
+        'D.pydeps': '# Generated by:\n# CMD --gn-paths D\n//D.py\n',
+    }
+
+    self.mock_input_api.files = [
+      MockAffectedFile('A.py', []),
+    ]
+
+    def mock_check_output(cmd, shell=False, env=None):
+      self.assertEqual('CMD --gn-paths A --output A.pydeps --output ""', cmd)
+      return 'changed data'
+
+    self.mock_input_api.subprocess.check_output = mock_check_output
+
+    results = self._RunCheck()
+    self.assertEqual(1, len(results))
+    self.assertIn('File is stale', str(results[0]))
 
 
 class IncludeGuardTest(unittest.TestCase):