[gclient] Use a single CIPD root for an entire gclient checkout.

gclient previously used a CIPD root per DEPS file. This didn't work
in cases in which a DEPS file wanted to specify a CIPD package outside
of its directory hierarchy, though, as is the case with buildspecs.

Bug: 755920
Change-Id: I0d6c3db567f17f9171c0feaaf9ed6bc64db22757
Reviewed-on: https://chromium-review.googlesource.com/955933
Commit-Queue: John Budorick <jbudorick@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Michael Moss <mmoss@chromium.org>

gclient.py

@@ -455,8 +455,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     ])
     return s
 
-
-
   @property
   def requirements(self):
     """Calculate the list of requirements."""
@@ -676,7 +674,6 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
   def _deps_to_objects(self, deps, use_relative_paths):
     """Convert a deps dict to a dict of Dependency objects."""
     deps_to_add = []
-    cipd_root = None
     for name, dep_value in deps.iteritems():
       should_process = self.recursion_limit and self.should_process
       deps_file = self.deps_file
@@ -709,14 +706,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
           should_process = should_process and condition_value
 
       if dep_type == 'cipd':
-        if not cipd_root:
-          cipd_root = gclient_scm.CipdRoot(
-              os.path.join(self.root.root_dir, self.name),
-              # TODO(jbudorick): Support other service URLs as necessary.
-              # Service URLs should be constant over the scope of a cipd
-              # root, so a var per DEPS file specifying the service URL
-              # should suffice.
-              'https://chrome-infra-packages.appspot.com')
+        cipd_root = self.GetCipdRoot()
         for package in dep_value.get('packages', []):
           if 'version' in package:
             # Matches version to vars value.
@@ -1171,6 +1161,13 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     for hook in self.pre_deps_hooks:
       hook.run(self.root.root_dir)
 
+  def GetCipdRoot(self):
+    if self.root is self:
+      # Let's not infinitely recurse. If this is root and isn't an
+      # instance of GClient, do nothing.
+      return None
+    return self.root.GetCipdRoot()
+
   def subtree(self, include_all):
     """Breadth first recursion excluding root node."""
     dependencies = self.dependencies
@@ -1399,6 +1396,7 @@ solutions = %(solution_list)s
     self._enforced_os = tuple(set(enforced_os))
     self._enforced_cpu = detect_host_arch.HostArch(),
     self._root_dir = root_dir
+    self._cipd_root = None
     self.config_content = None
 
   def _CheckConfig(self):
@@ -1637,6 +1635,9 @@ it or fix the checkout.
       print('Please fix your script, having invalid --revision flags will soon '
             'considered an error.', file=sys.stderr)
 
+    if self._cipd_root:
+      self._cipd_root.run(command)
+
     # Once all the dependencies have been processed, it's now safe to write
     # out any gn_args_files and run the hooks.
     if command == 'update':
@@ -1844,6 +1845,17 @@ it or fix the checkout.
     print('Loaded .gclient config in %s:\n%s' % (
         self.root_dir, self.config_content))
 
+  def GetCipdRoot(self):
+    if not self._cipd_root:
+      self._cipd_root = gclient_scm.CipdRoot(
+          self.root_dir,
+          # TODO(jbudorick): Support other service URLs as necessary.
+          # Service URLs should be constant over the scope of a cipd
+          # root, so a var per DEPS file specifying the service URL
+          # should suffice.
+          'https://chrome-infra-packages.appspot.com')
+    return self._cipd_root
+
   @property
   def root_dir(self):
     """Root directory of gclient checkout."""
@@ -1906,12 +1918,30 @@ class CipdDependency(Dependency):
       # TODO(jbudorick): Implement relative if necessary.
       raise gclient_utils.Error(
           'Relative CIPD dependencies are not currently supported.')
+    self._cipd_package = None
     self._cipd_root = cipd_root
-
     self._cipd_subdir = os.path.relpath(
         os.path.join(self.root.root_dir, name), cipd_root.root_dir)
-    self._cipd_package = self._cipd_root.add_package(
-        self._cipd_subdir, package, version)
+    self._package_name = package
+    self._package_version = version
+
+  #override
+  def run(self, revision_overrides, command, args, work_queue, options):
+    """Runs |command| then parse the DEPS file."""
+    logging.info('CipdDependency(%s).run()' % self.name)
+    if not self.should_process:
+      return
+    self._CreatePackageIfNecessary()
+    super(CipdDependency, self).run(revision_overrides, command, args,
+                                    work_queue, options)
+
+  def _CreatePackageIfNecessary(self):
+    # We lazily create the CIPD package to make sure that only packages
+    # that we want (as opposed to all packages defined in all DEPS files
+    # we parse) get added to the root and subsequently ensured.
+    if not self._cipd_package:
+      self._cipd_package = self._cipd_root.add_package(
+          self._cipd_subdir, self._package_name, self._package_version)
 
   def ParseDepsFile(self):
     """CIPD dependencies are not currently allowed to have nested deps."""
@@ -1933,6 +1963,7 @@ class CipdDependency(Dependency):
   def CreateSCM(self, url, root_dir=None, relpath=None, out_fh=None,
                 out_cb=None):
     """Create a Wrapper instance suitable for handling this CIPD dependency."""
+    self._CreatePackageIfNecessary()
     return gclient_scm.CipdWrapper(
         url, root_dir, relpath, out_fh, out_cb,
         root=self._cipd_root,
@@ -1941,12 +1972,13 @@ class CipdDependency(Dependency):
   def ToLines(self):
     """Return a list of lines representing this in a DEPS file."""
     s = []
+    self._CreatePackageIfNecessary()
     if self._cipd_package.authority_for_subdir:
       condition_part = (['    "condition": %r,' % self.condition]
                         if self.condition else [])
       s.extend([
           '  # %s' % self.hierarchy(include_url=False),
-          '  "%s": {' % (self.name,),
+          '  "%s": {' % (self.name.split(':')[0],),
           '    "packages": [',
       ])
       for p in self._cipd_root.packages(self._cipd_subdir):
@@ -1956,6 +1988,7 @@ class CipdDependency(Dependency):
             '        "version": "%s",' % p.version,
             '      },',
         ])
+
       s.extend([
           '    ],',
           '    "dep_type": "cipd",',

gclient_scm.py

@@ -1215,25 +1215,11 @@ class GitWrapper(SCMWrapper):
 class CipdPackage(object):
   """A representation of a single CIPD package."""
 
-  def __init__(self, name, version, authority_for_root, authority_for_subdir):
-    self._authority_for_root = authority_for_root
+  def __init__(self, name, version, authority_for_subdir):
     self._authority_for_subdir = authority_for_subdir
     self._name = name
     self._version = version
 
-  @property
-  def authority_for_root(self):
-    """Whether this package has authority to act on behalf of its root.
-
-    Some operations should only be performed once per cipd root. A package
-    that has authority for its cipd root is the only package that should
-    perform such operations.
-
-    Returns:
-      bool; whether this package has root authority.
-    """
-    return self._authority_for_root
-
   @property
   def authority_for_subdir(self):
     """Whether this package has authority to act on behalf of its subdir.
@@ -1284,7 +1270,6 @@ class CipdRoot(object):
     with self._mutator_lock:
       cipd_package = CipdPackage(
           package, version,
-          not self._packages_by_subdir,
           not self._packages_by_subdir[subdir])
       self._all_packages.add(cipd_package)
       self._packages_by_subdir[subdir].append(cipd_package)
@@ -1301,7 +1286,7 @@ class CipdRoot(object):
     packages.
     """
     with self._mutator_lock:
-      cipd_cache_dir = os.path.join(self._cipd_root, '.cipd')
+      cipd_cache_dir = os.path.join(self.root_dir, '.cipd')
       try:
         gclient_utils.rmtree(os.path.join(cipd_cache_dir))
       except OSError:
@@ -1336,6 +1321,13 @@ class CipdRoot(object):
         ]
         gclient_utils.CheckCallAndFilterAndHeader(cmd)
 
+  def run(self, command):
+    if command == 'update':
+      self.ensure()
+    elif command == 'revert':
+      self.clobber()
+      self.ensure()
+
   def created_package(self, package):
     """Checks whether this root created the given package.
 
@@ -1385,10 +1377,12 @@ class CipdWrapper(SCMWrapper):
     return True
 
   def revert(self, options, args, file_list):
-    """Deletes .cipd and reruns ensure."""
-    if self._package.authority_for_root:
-      self._root.clobber()
-      self._root.ensure()
+    """Does nothing.
+
+    CIPD packages should be reverted at the root by running
+    `CipdRoot.run('revert')`.
+    """
+    pass
 
   def diff(self, options, args, file_list):
     """CIPD has no notion of diffing."""
@@ -1422,6 +1416,9 @@ class CipdWrapper(SCMWrapper):
     pass
 
   def update(self, options, args, file_list):
-    """Runs ensure."""
-    if self._package.authority_for_root:
-      self._root.ensure()
+    """Does nothing.
+
+    CIPD packages should be updated at the root by running
+    `CipdRoot.run('update')`.
+    """
+    pass

tests/gclient_scm_test.py

@@ -929,27 +929,10 @@ class CipdWrapperTestCase(BaseTestCase):
 
     self.fail('Unable to find a satisfactory package.')
 
-  def testSingleRootAuthority(self):
-    """Checks that exactly one package has root authority."""
-    self.assertEquals(1, len([p for p in self._cipd_packages
-                              if p.authority_for_root]))
-
   def testRevert(self):
-    """Checks that revert w/ root authority clobbers and reruns ensure."""
-    scm = self.createScmWithPackageThatSatisfies(
-        lambda p: p.authority_for_root)
-
-    self._cipd_root.clobber()
-    self._cipd_root.ensure()
-
-    self.mox.ReplayAll()
-
-    scm.revert(None, (), [])
-
-  def testRevertWithoutAuthority(self):
-    """Checks that revert w/o root authority does nothing."""
+    """Checks that revert does nothing."""
     scm = self.createScmWithPackageThatSatisfies(
-        lambda p: not p.authority_for_root)
+        lambda _: True)
 
     self.mox.ReplayAll()
 
@@ -991,20 +974,9 @@ class CipdWrapperTestCase(BaseTestCase):
     self.assertEquals(revinfo, expected_revinfo)
 
   def testUpdate(self):
-    """Checks that update w/ root authority runs ensure."""
-    scm = self.createScmWithPackageThatSatisfies(
-        lambda p: p.authority_for_root)
-
-    self._cipd_root.ensure()
-
-    self.mox.ReplayAll()
-
-    scm.update(None, (), [])
-
-  def testUpdateWithoutAuthority(self):
-    """Checks that update w/o root authority does nothing."""
+    """Checks that update does nothing."""
     scm = self.createScmWithPackageThatSatisfies(
-        lambda p: not p.authority_for_root)
+        lambda _: True)
 
     self.mox.ReplayAll()
 

tests/gclient_smoketest.py

@@ -1265,7 +1265,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '  },',
         '',
         '  # src -> src/cipd_dep:package0',
-        '  "src/cipd_dep:package0": {',
+        '  "src/cipd_dep": {',
         '    "packages": [',
         '      {',
         '        "package": "package0",',