Disable DEPS review enforcement for cross-repo DEPS

When a new DEP is added, the presubmit check enforces reviews from
the owner of the additional DEP. However, when the DEP is from another
repo, the enforcement becomes trickier especially it is from a different
host.

This patch disables the check if a newly added DEP is from a submodule.

Change-Id: Id7c2feac005c1c769313dedb254d5a0b641ff68b
Bug: 324496840
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5656359
Reviewed-by: Josip Sokcevic <[email protected]>
Commit-Queue: Scott Lee <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1320079}

PRESUBMIT.py

@@ -3582,8 +3582,33 @@ def StripDeps(path):
         else:
             return path
 
+    submodule_paths = set(input_api.ListSubmodules())
+    def is_from_submodules(path, submodule_paths):
+        path = input_api.os_path.normpath(path)
+        while path:
+            if path in submodule_paths:
+                return True
+
+            # All deps should be a relative path from the checkout.
+            # i.e., shouldn't start with "/" or "c:\", for example.
+            #
+            # That said, this is to prevent an infinite loop, just in case
+            # an input dep path starts with "/", because
+            # os.path.dirname("/") => "/"
+            parent = input_api.os_path.dirname(path)
+            if parent == path:
+                break
+            path = parent
+
+        return False
+
     unapproved_dependencies = [
         "'+%s'," % StripDeps(path) for path in missing_files
+        # if a newly added dep is from a submodule, it becomes trickier
+        # to get suggested owners, especially it is from a different host.
+        #
+        # skip the review enforcement for cross-repo deps.
+        if not is_from_submodules(path, submodule_paths)
     ]
 
     if unapproved_dependencies:

PRESUBMIT_test.py

@@ -424,6 +424,88 @@ def testCalculateAddedDepsIgnoresPermutations(self):
                      self.calculate(old_include_rules, {}, new_include_rules,
                                     {}))
 
+  class FakeOwnersClient(object):
+    APPROVED = "APPROVED"
+    PENDING = "PENDING"
+    returns = {}
+
+    def ListOwners(self, *args, **kwargs):
+      return self.returns.get(self.ListOwners.__name__, "")
+
+    def mockListOwners(self, owners):
+      self.returns[self.ListOwners.__name__] = owners
+
+    def GetFilesApprovalStatus(self, *args, **kwargs):
+      return self.returns.get(self.GetFilesApprovalStatus.__name__, {})
+
+    def mockGetFilesApprovalStatus(self, status):
+      self.returns[self.GetFilesApprovalStatus.__name__] = status
+
+    def SuggestOwners(self, *args, **kwargs):
+      return ["eng1", "eng2", "eng3"]
+
+  class fakeGerrit(object):
+    def IsOwnersOverrideApproved(self, issue):
+      return False
+
+  def setUp(self):
+    self.input_api = input_api = MockInputApi()
+    input_api.environ = {}
+    input_api.owners_client = self.FakeOwnersClient()
+    input_api.gerrit = self.fakeGerrit()
+    input_api.change.issue = 123
+    self.mockOwnersAndReviewers("owner", set(["reviewer"]))
+    self.mockListSubmodules([])
+
+  def mockOwnersAndReviewers(self, owner, reviewers):
+    def mock(*args, **kwargs):
+      return [owner, reviewers]
+    self.input_api.canned_checks.GetCodereviewOwnerAndReviewers = mock
+
+  def mockListSubmodules(self, paths):
+    def mock(*args, **kwargs):
+      return paths
+    self.input_api.ListSubmodules = mock
+
+  def testApprovedAdditionalDep(self):
+    old_deps = """include_rules = []""".splitlines()
+    new_deps = """include_rules = ["+v8/123"]""".splitlines()
+    self.input_api.files = [MockAffectedFile("pdf/DEPS", new_deps, old_deps)]
+
+    # mark the additional dep as approved.
+    os_path = self.input_api.os_path
+    self.input_api.owners_client.mockGetFilesApprovalStatus(
+       {os_path.join('v8/123', 'DEPS'): self.FakeOwnersClient.APPROVED}
+    )
+    results = PRESUBMIT.CheckAddedDepsHaveTargetApprovals(
+        self.input_api, MockOutputApi())
+    # Then, the check should pass.
+    self.assertEqual([], results)
+
+  def testUnapprovedAdditionalDep(self):
+    old_deps = """include_rules = []""".splitlines()
+    new_deps = """include_rules = ["+v8/123"]""".splitlines()
+    self.input_api.files = [
+        MockAffectedFile('pdf/DEPS', new_deps, old_deps),
+    ]
+
+    # pending.
+    os_path = self.input_api.os_path
+    self.input_api.owners_client.mockGetFilesApprovalStatus(
+       {os_path.join('v8/123', 'DEPS'): self.FakeOwnersClient.PENDING}
+    )
+    results = PRESUBMIT.CheckAddedDepsHaveTargetApprovals(
+        self.input_api, MockOutputApi())
+    # the check should fail
+    self.assertIn('You need LGTM', results[0].message)
+    self.assertIn('+v8/123', results[0].message)
+
+    # unless the added dep is from a submodule.
+    self.mockListSubmodules(['v8'])
+    results = PRESUBMIT.CheckAddedDepsHaveTargetApprovals(
+        self.input_api, MockOutputApi())
+    self.assertEqual([], results)
+
 
 class JSONParsingTest(unittest.TestCase):
   def testSuccess(self):