Clean up gclient MergeDependencies

gclient.py

Index: gclient.py
diff --git a/gclient.py b/gclient.py
index d1b5a8733859fb76b7cc28e825b941f19ad8ab01..7a1959f34d3467cf752ed7028c0f4d31e3057208 100755
--- a/gclient.py
+++ b/gclient.py
@@ -104,7 +104,7 @@ 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):
@@ -120,7 +120,7 @@ class GClientKeywords(object):
       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."""
 
@@ -143,7 +143,7 @@ class GClientKeywords(object):
         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
@@ -472,39 +472,31 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     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."""