Handle conflicting os deps better in gclient.

gclient.py

Index: gclient.py
diff --git a/gclient.py b/gclient.py
index c3e295c42e1dca0fff893a61352cfa7deee4fe36..a9c0eb7e014b2cd381ec2d7e2bfe592000e646f8 100755
--- a/gclient.py
+++ b/gclient.py
@@ -502,22 +502,38 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
       self.local_target_os = local_scope['target_os']
     # load os specific dependencies if defined.  these dependencies may
     # override or extend the values defined by the 'deps' member.
-    target_os_deps = {}
-    if 'deps_os' in local_scope:
-      for deps_os_key in self.target_os:
-        os_deps = local_scope['deps_os'].get(deps_os_key, {})
-        if len(self.target_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.
-          target_os_deps.update(
-              [x for x in os_deps.items() if not x[0] in target_os_deps])
-        else:
-          target_os_deps.update(os_deps)
+    target_os_list = self.target_os
+    if 'deps_os' in local_scope and target_os_list:
+      if len(target_os_list) == 1:
+        target_os_deps = local_scope['deps_os'].get(target_os_list[0], {})
+      else:
+        os_overrides = {}
+        for the_target_os in target_os_list:
+          the_target_os_deps = local_scope['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))
+
+        for os_dep_key, os_dep_value in os_overrides.iteritems():
+          if len(os_dep_value) != len(target_os_list):

[Isaac (away), 2013/12/11 18:58:27]

What does this loop do?  Can you add a comment?

+            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])

[Isaac (away), 2013/12/11 18:58:27]

line length, but you can remove the unneeded brackets and get under 80.
i.e. set(x[1] for .... not None)

+          if len(possible_values) > 1:
+            logging.error('Conflicting dependencies for %s: %s' %
+                          (os_dep_key, os_dep_value))
+            raise gclient_utils.Error('Inconsistent checkout instructions.')

[Isaac (away), 2013/12/11 18:58:27]

I gave conflicting suggestions here.  in the first I suggest raising an error
and in the follow I said logging would be OK.  I think it's fine to remove this
and log the choices.  If we do pick one, we need to do so deterministically --
see the 'sorted' call in the sketch.

+
+          if len(possible_values) == 1:
+            target_os_deps[os_dep_key] = possible_values.pop()
+          else:
+            target_os_deps[os_dep_key] = None
 
-    # deps_os overrides paths from deps
-    deps.update(target_os_deps)
+      deps.update(target_os_deps)
 
     # If a line is in custom_deps, but not in the solution, we want to append
     # this line to the solution.