Clean up gclient MergeDependencies

gclient.py

 #!/usr/bin/env python
 # Copyright (c) 2012 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
 #                   entries each are maps binding the strings "name" and "url"
@@ skipping 86 matching lines @@
 import fix_encoding
 import gclient_scm
 import gclient_utils
 from third_party.repo.progress import Progress
 import subcommand
 import subprocess2
 from third_party import colorama
 
 
 class GClientKeywords(object):
-  class FromImpl(object):
+  class FromImpl(gclient_utils.BaseRecord):
     """Used to implement the From() syntax."""
 
     def __init__(self, module_name, sub_target_name=None):
       """module_name is the dep module we want to include from.  It can also be
       the name of a subdirectory to include from.
 
       sub_target_name is an optional parameter if the module name in the other
       DEPS file is different. E.g., you might want to map src/net to net."""
       self.module_name = module_name
       self.sub_target_name = sub_target_name
 
     def __str__(self):
       return 'From(%s, %s)' % (repr(self.module_name),
                                repr(self.sub_target_name))
 
-  class FileImpl(object):
+  class FileImpl(gclient_utils.BaseRecord):
     """Used to implement the File('') syntax which lets you sync a single file
     from a SVN repo."""
 
     def __init__(self, file_location):
       self.file_location = file_location
 
     def __str__(self):
       return 'File("%s")' % self.file_location
 
     def GetPath(self):
       return os.path.split(self.file_location)[0]
 
     def GetFilename(self):
       rev_tokens = self.file_location.split('@')
       return os.path.split(rev_tokens[0])[1]
 
     def GetRevision(self):
       rev_tokens = self.file_location.split('@')
       if len(rev_tokens) > 1:
         return rev_tokens[1]
       return None
 
-  class VarImpl(object):
+  class VarImpl(gclient_utils.BaseRecord):
     def __init__(self, custom_vars, local_scope):
       self._custom_vars = custom_vars
       self._local_scope = local_scope
 
     def Lookup(self, var_name):
       """Implements the Var syntax."""
       if var_name in self._custom_vars:
         return self._custom_vars[var_name]
       elif var_name in self._local_scope.get("vars", {}):
         return self._local_scope["vars"][var_name]
@@ skipping 308 matching lines @@
       return url
 
     raise gclient_utils.Error('Unknown url type')
 
   @staticmethod
   def MergeWithOsDeps(deps, deps_os, target_os_list):
     """Returns a new "deps" structure that is the deps sent in updated
     with information from deps_os (the deps_os section of the DEPS
     file) that matches the list of target os."""
     os_overrides = {}
-    for the_target_os in target_os_list:
+    # Sort target_os_list to ensure consistent conflict resolution.
+    for the_target_os in sorted(target_os_list):
       the_target_os_deps = deps_os.get(the_target_os, {})
       for os_dep_key, os_dep_value in the_target_os_deps.iteritems():
         overrides = os_overrides.setdefault(os_dep_key, [])
         overrides.append((the_target_os, os_dep_value))
 
-    # If any os didn't specify a value (we have fewer value entries
-    # than in the os list), then it wants to use the default value.
-    for os_dep_key, os_dep_value in os_overrides.iteritems():
-      if len(os_dep_value) != len(target_os_list):
-        # Record the default value too so that we don't accidently
-        # set it to None or miss a conflicting DEPS.
-        if os_dep_key in deps:
-          os_dep_value.append(('default', deps[os_dep_key]))
-
     target_os_deps = {}
-    for os_dep_key, os_dep_value in os_overrides.iteritems():
-      # os_dep_value is a list of (os, value) pairs.
-      possible_values = set(x[1] for x in os_dep_value if x[1] is not None)
-      if not possible_values:
-        target_os_deps[os_dep_key] = None
-      else:
-        if len(possible_values) > 1:
-          # It would be possible to abort here but it would be
-          # unfortunate if we end up preventing any kind of checkout.
-          logging.error('Conflicting dependencies for %s: %s. (target_os=%s)',
-                        os_dep_key, os_dep_value, target_os_list)
-        # Sorting to get the same result every time in case of conflicts.
-        target_os_deps[os_dep_key] = sorted(possible_values)[0]
-
-    new_deps = deps.copy()
-    new_deps.update(target_os_deps)
-    return new_deps
+    for os_dep_key, os_dep_values in os_overrides.iteritems():
+      # If any os didn't specify a value (we have fewer value entries
+      # than in the os list), then it wants to use the default value.
+      # We preferentially use default os val during conflicts.
+      if len(os_dep_values) < len(target_os_list):
+        os_dep_values.append(('default', deps.get(os_dep_key)))
+      if len(set(val for _, val in os_dep_values if val is not None)) > 1:
+        # It would be possible to abort here but it would be
+        # unfortunate if we end up preventing any kind of checkout.
+        logging.error('Conflicting dependencies for %s: %s. (target_os=%s)',
+                      os_dep_key, os_dep_values, target_os_list)
+      # Find the last non-None val, if it exists.
+      for _, val in reversed(os_dep_values):
+        if val is not None:
+          break
+      target_os_deps[os_dep_key] = val
+    return dict(deps, **target_os_deps)

[Daniel Bratell, 2013/12/17 08:38:19]

My pylint (not the one bundled) is noisily unhappy about ** constructs (because
they are hard to read/understand and maintain I guess; "Used * or ** magic"). In
other parts of gclient it looks like pylint's observations have been a factor
(comments to silence it). Is that a factor here as well?

 
   def ParseDepsFile(self):
     """Parses the DEPS file for this dependency."""
     assert not self.deps_parsed
     assert not self.dependencies
     # 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,
@@ skipping 1398 matching lines @@
     raise
   except (gclient_utils.Error, subprocess2.CalledProcessError), e:
     print >> sys.stderr, 'Error: %s' % str(e)
     return 1
 
 
 if '__main__' == __name__:
   sys.exit(Main(sys.argv[1:]))
 
 # vim: ts=2:sw=2:tw=80:et: