Handle conflicting os deps better in gclient.

gclient.py

Index: gclient.py
diff --git a/gclient.py b/gclient.py
index c3e295c42e1dca0fff893a61352cfa7deee4fe36..dd874b5402c6b5c38f3d55cb784282c713788eae 100755
--- a/gclient.py
+++ b/gclient.py
@@ -502,22 +502,41 @@ 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])
+    target_os_list = self.target_os
+    if 'deps_os' in local_scope and target_os_list:
+      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))
+
+      # 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])

[Isaac (away), 2013/12/12 09:41:01]

remove brackets (it's best practice so python can use a generator instead of
constructing a list)

+        if len(possible_values) == 0:

[Isaac (away), 2013/12/12 09:41:01]

Usually written as "if not possible_values".  However simpler might be:

possible_values = set(x[1] for x in os_dep_value)
if len(possible_values) > 1:
  # log error
  possible_values.discard(None)
target_os_deps[os_dep_key] = sorted(possible_values)[0]

[Daniel Bratell, 2013/12/12 16:26:15]

It's not an error (or even unusual) to have more than one value if you include
None. None is used to optimize checkouts so it's very common but can safely be
ignored. That is the reason I remove all Nones from the list (and that is the
reason it looks more complicated that you would expect).

+          target_os_deps[os_dep_key] = None
         else:
-          target_os_deps.update(os_deps)
-
-    # deps_os overrides paths from deps
-    deps.update(target_os_deps)
+          if len(possible_values) > 1:
+            # It would be possible to abort here but it would be
+            # unfortunate to prevent people from checking out the
+            # source at all.
+            logging.error('Conflicting dependencies for %s: %s.' %

[Isaac (away), 2013/12/12 09:41:01]

Nit, best practice here is to use logging implicit string formatting.  While
less important in this case, it avoids computation on logging calls that fall
below the current error threshold.  In this case:

logging.error('Conflicting...', os_dep_key, os_dep_value)

[Daniel Bratell, 2013/12/12 16:26:15]

gclient is super super slow but I don't think it's because of python string
operations. I happened to have Process Explorer open while running it. It's
scary to see the number of sub processes it creates while running, as well as
the very heavy git commands they run even when gclient sync is a NOP. There is
plenty of room for optimizations for someone.

But I've planned to learn the logging formatting anyway so this was an excuse.

+                          (os_dep_key, os_dep_value))
+          # Sorting to get the same result every time in case of conflicts.
+          target_os_deps[os_dep_key] = sorted(possible_values)[0]
+
+      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.