Move all mutations into a specific place.

gclient.py

 #!/usr/bin/env python
 # Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Meta checkout manager supporting both Subversion and GIT.
 
 Files
   .gclient      : Current client configuration, written by 'config' command.
                   Format is a Python script defining 'solutions', a list whose
@@ skipping 240 matching lines @@
     # hooks.
     self._file_list = []
     # If it is not set to True, the dependency wasn't processed for its child
     # dependency, i.e. its DEPS wasn't read.
     self._deps_parsed = False
     # This dependency has been processed, i.e. checked out
     self._processed = False
     # This dependency had its hook run
     self._hooks_ran = False
 
-    # Setup self.requirements and find any other dependency who would have self
-    # as a requirement.
+    if not self.name and self.parent:
+      raise gclient_utils.Error('Dependency without name')
+
+  def setup_requirements(self):

[M-A Ruel, 2011/10/05 00:40:52]

ok you can laugh at me for splitting it again.

+    """Setup self.requirements and find any other dependency who would have self
+    as a requirement.

[Dirk Pranke, 2011/10/06 19:34:29]

Nit: document the return value? It's not obvious that this is returning
something, or what it means if it returns true or false. Also I might rename the
method to something (not sure what) to clarify that.

[M-A Ruel, 2011/10/06 20:44:19]

documented

+    """
+    if self.name in [s.name for s in self.parent.dependencies]:
+      raise gclient_utils.Error(
+          'The same name "%s" appears multiple times in the deps section' %
+              self.name)
+    if self.should_process:
+      sibblings = [d for d in self.root.subtree(False) if d.name == self.name]

[Dirk Pranke, 2011/10/06 19:34:29]

Nit: there's only one "b" in "siblings". Here and the next few lines.

[M-A Ruel, 2011/10/06 20:44:19]

fixed

+      for sibbling in sibblings:
+        if self.url != sibbling.url:
+          raise gclient_utils.Error(
+              'Dependency %s specified more than once:\n  %s\nvs\n  %s' %
+              (self.name, sibbling.hierarchy(), self.hierarchy()))
+        # In theory we could keep it as a shadow of the other one. In
+        # practice, simply ignore it.
+        logging.warn('Won\'t process duplicate dependency %s' % sibbling)
+        return False
 
     # self.parent is implicitly a requirement. This will be recursive by
     # definition.
     if self.parent and self.parent.name:
       self.add_requirement(self.parent.name)
 
     # For a tree with at least 2 levels*, the leaf node needs to depend
     # on the level higher up in an orderly way.
     # This becomes messy for >2 depth as the DEPS file format is a dictionary,
     # thus unsorted, while the .gclient format is a list thus sorted.
@@ skipping 18 matching lines @@
     if self.name and self.should_process:
       for obj in self.root.depth_first_tree():
         if obj is self or not obj.name:
           continue
         # Step 1: Find any requirements self may need.
         if self.name.startswith(posixpath.join(obj.name, '')):
           self.add_requirement(obj.name)
         # Step 2: Find any requirements self may impose.
         if obj.name.startswith(posixpath.join(self.name, '')):
           obj.add_requirement(self.name)
-
-    if not self.name and self.parent:
-      raise gclient_utils.Error('Dependency without name')
+    return True
 
   def LateOverride(self, url):
     """Resolves the parsed url from url.
 
     Manages From() keyword accordingly. Do not touch self.parsed_url nor
     self.url because it may called with other urls due to From()."""
     assert self.parsed_url == None or not self.should_process, self.parsed_url
     parsed_url = self.get_custom_deps(self.name, url)
     if parsed_url != url:
       logging.info('LateOverride(%s, %s) -> %s' % (self.name, url, parsed_url))
@@ skipping 52 matching lines @@
       logging.info('LateOverride(%s, %s) -> %s (File)' % (self.name, url, url))
       return url
 
     if url is None:
       logging.info('LateOverride(%s, %s) -> %s' % (self.name, url, url))
       return url
     else:
       raise gclient_utils.Error('Unknown url type')
 
   def ParseDepsFile(self):
     """Parses the DEPS file for this dependency."""

[Dirk Pranke, 2011/10/06 19:34:29]

Is this going to be called by multiple threads simultaneously?

[M-A Ruel, 2011/10/06 20:44:19]

Not starting with http://codereview.chromium.org/8143022.

     assert self.processed == True
     if self.deps_parsed:
       logging.debug('%s was already parsed' % self.name)
       return
     # One thing is unintuitive, vars= {} must happen before Var() use.
     local_scope = {}
     var = self.VarImpl(self.custom_vars, local_scope)
     global_scope = {
       'File': self.FileImpl,
       'From': self.FromImpl,
@@ skipping 22 matching lines @@
         os_deps = local_scope['deps_os'].get(deps_os_key, {})
         if len(enforced_os) > 1:
           # Ignore any conflict when including deps for more than one
           # platform, so we collect the broadest set of dependencies
           # available. We may end up with the wrong revision of something for
           # our platform, but this is the best we can do.
           deps.update([x for x in os_deps.items() if not x[0] in deps])
         else:
           deps.update(os_deps)
 
-    self._deps_hooks.extend(local_scope.get('hooks', []))
-
     # If a line is in custom_deps, but not in the solution, we want to append
     # this line to the solution.
     for d in self.custom_deps:
       if d not in deps:
         deps[d] = self.custom_deps[d]
 
     # If use_relative_paths is set in the DEPS file, regenerate
     # the dictionary using paths relative to the directory containing
     # the DEPS file.
     use_relative_paths = local_scope.get('use_relative_paths', False)
     if use_relative_paths:
       rel_deps = {}
       for d, url in deps.items():
         # normpath is required to allow DEPS to use .. in their
         # dependency local path.
         rel_deps[os.path.normpath(os.path.join(self.name, d))] = url
       deps = rel_deps
 
     # Convert the deps into real Dependency.
+    deps_to_add = []
     for name, url in deps.iteritems():
-      if name in [s.name for s in self._dependencies]:
-        raise gclient_utils.Error(
-            'The same name "%s" appears multiple times in the deps section' %
-                name)
       should_process = self.recursion_limit and self.should_process
-      if should_process:
-        tree = dict((d.name, d) for d in self.root.subtree(False))
-        if name in tree:
-          if url == tree[name].url:
-            logging.info('Won\'t process duplicate dependency %s' % tree[name])
-            # In theory we could keep it as a shadow of the other one. In
-            # practice, simply ignore it.
-            #should_process = False
-            continue
-          else:
-            raise gclient_utils.Error(
-                'Dependency %s specified more than once:\n  %s\nvs\n  %s' %
-                (name, tree[name].hierarchy(), self.hierarchy()))
-      self._dependencies.append(
-          Dependency(
-            self, name, url, None, None, None, None,
-            self.deps_file, should_process))
-    self._deps_parsed = True
-    logging.debug('Loaded: %s' % str(self))
+      deps_to_add.append(Dependency(
+          self, name, url, None, None, None, None,
+          self.deps_file, should_process))
+    self.add_dependencies_and_close(deps_to_add, local_scope.get('hooks', []))
+    logging.info('ParseDepsFile(%s) done' % self.name)
+
+  def add_dependencies_and_close(self, deps_to_add, hooks):

[Dirk Pranke, 2011/10/06 19:34:29]

does it make sense to make this a locked method? Otherwise you're locking and
unlocking in add_dependency() a lot. 

[M-A Ruel, 2011/10/06 20:44:19]

Yes and that's the purpose. The lock must be kept as little as possible, and the
code guarantees internal consistency all the time the lock is released.

[Dirk Pranke, 2011/10/06 20:50:46]

Well, there's nothing in setup_requirements that can block, right()? So I'm
pretty sure you'll hold the GIL the whole time and only one thread will be
actually executing add_dependencies_and_close() at a time, so it just seems like
it might be a bit more efficient. Not a big deal, though.

[M-A Ruel, 2011/10/06 20:57:35]

Yes there is something. setup_requirements() does a tree traversal, which calls
.dependencies on all objects. So no lock must be held when this function is
called.

Note that I've finally coded a fix for parallel processing, which I'll send you
shortly once I clean up the code. This will mark the end of this journey in
refactoring the crap out of this class.

+    """Adds the dependencies, hooks and mark the parsing as done."""
+    for dep in deps_to_add:
+      if dep.setup_requirements():
+        self.add_dependency(dep)
+    self._mark_as_parsed(hooks)

[Dirk Pranke, 2011/10/06 20:50:46]

I thought I had mentioned this in the earlier email, but I don't see the comment
... I'd probably move _mark_as_parsed out of this routine and after line 462
(and then rename this routine).

[M-A Ruel, 2011/10/06 20:57:35]

If I'd do that, I'd have to add this call at the other call site at line 802.
That's why I preferred to keep the call inside add_dependencies_and_close(). Not
much opinion though.

 
   # Arguments number differs from overridden method
   # pylint: disable=W0221
   def run(self, revision_overrides, command, args, work_queue, options):
     """Runs 'command' before parsing the DEPS in case it's a initial checkout
     or a revert."""
 
     def maybeGetParentRevision(options):
       """If we are performing an update and --transitive is set, set the
       revision to the parent's revision. If we have an explicit revision
@@ skipping 150 matching lines @@
         yield i
 
   def depth_first_tree(self):
     """Depth-first recursion including the root node."""
     yield self
     for i in self.dependencies:
       for j in i.depth_first_tree():
         if j.should_process:
           yield j
 
+  @gclient_utils.lockedmethod
+  def add_dependency(self, new_dep):
+    self._dependencies.append(new_dep)
+
+  @gclient_utils.lockedmethod
+  def _mark_as_parsed(self, new_hooks):
+    self._deps_hooks.extend(new_hooks)
+    self._deps_parsed = True
+
   @property
+  @gclient_utils.lockedmethod
   def dependencies(self):
     return tuple(self._dependencies)
 
   @property
   def deps_hooks(self):
     return tuple(self._deps_hooks)
 
   @property
   def parsed_url(self):
     return self._parsed_url
@@ skipping 112 matching lines @@
     self.config_content = None
 
   def SetConfig(self, content):
     assert not self.dependencies
     config_dict = {}
     self.config_content = content
     try:
       exec(content, config_dict)
     except SyntaxError, e:
       gclient_utils.SyntaxErrorToError('.gclient', e)
+
+    deps_to_add = []
     for s in config_dict.get('solutions', []):
       try:
-        tree = dict((d.name, d) for d in self.root.subtree(False))
-        if s['name'] in tree:
-          raise gclient_utils.Error(
-              'Dependency %s specified more than once in .gclient' % s['name'])
-        self._dependencies.append(Dependency(
+        deps_to_add.append(Dependency(
             self, s['name'], s['url'],
             s.get('safesync_url', None),
             s.get('managed', True),
             s.get('custom_deps', {}),
             s.get('custom_vars', {}),
             s.get('deps_file', 'DEPS'),
             True))
       except KeyError:
         raise gclient_utils.Error('Invalid .gclient file. Solution is '
                                   'incomplete: %s' % s)
-    # .gclient can have hooks.
-    self._deps_hooks = config_dict.get('hooks', [])
-    self._deps_parsed = True
+    self.add_dependencies_and_close(deps_to_add, config_dict.get('hooks', []))
+    logging.info('SetConfig() done')
 
   def SaveConfig(self):
     gclient_utils.FileWrite(os.path.join(self.root_dir,
                                          self._options.config_filename),
                             self.config_content)
 
   @staticmethod
   def LoadCurrentConfig(options):
     """Searches for and loads a .gclient file relative to the current working
     dir. Returns a GClient object."""
@@ skipping 649 matching lines @@
   except (gclient_utils.Error, subprocess2.CalledProcessError), e:
     print >> sys.stderr, 'Error: %s' % str(e)
     return 1
 
 
 if '__main__' == __name__:
   fix_encoding.fix_encoding()
   sys.exit(Main(sys.argv[1:]))
 
 # vim: ts=2:sw=2:tw=80:et: