Handle conflicting os deps better in gclient.

gclient.py

Index: gclient.py
diff --git a/gclient.py b/gclient.py
index 407b9ad4e99f1efd3f31270f3695e18f458b1fcb..e8b4e7d12057ee9dd6d6b409aa8cc254cde98753 100755
--- a/gclient.py
+++ b/gclient.py
@@ -488,22 +488,42 @@ 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)
+    if 'deps_os' in local_scope and self.target_os:

[Isaac (away), 2013/12/09 10:33:13]

self.target_os is a recursive function, let's save to a local variable.

target_os_list = self.target_os
if ....

[Daniel Bratell, 2013/12/11 18:09:49]

To make the code faster?

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

yes, also because in theory the order could change on successive calls.
#premature optimization

+      target_os_deps = {}
+      the_first_os = self.target_os[0]

[Isaac (away), 2013/12/09 10:33:13]

It seems like we're going to have two decision paths: 1 os dep, and multiple. 
So let's explicitly separate those.  For just 1, all we need is
target_os_deps.update(os_deps). 

For more than 1 OS, let's split this into two parts.
a) data processing
b) decision making.

target_os_list = self.target_os
if 'deps_os' in local_scope and target_os_list:
  os_deps_f = lambda os_key : local_scope['deps_os'].get(os_key, {})
  if len(target_os_list) == 1:
    os_deps = os_deps_f(target_os_list[0])
  elif len(target_os_list) > 1:
    <new logic here>.....

  target_os.update(os_deps)



os_deps_d = {}  #  deps -> [(os, val), ...]
for os_key in target_os_list:
  for os_dep_key, os_dep_value in os_deps_f(os_key).iteritems():
    os_deps_d.setdefault(os_dep_key, []).append((os_key, os_dep_value))

os_deps = {}
for dep_key, os_deps_vals in os_deps_d.iteritems():
  if all(val is None for_, val in os_deps_vals) and len(os_deps_vals) ==
len(target_os_list):
    os_deps[dep_key] = None
  else:
    non_nones = sorted(dep_tup for dep_tup in os_deps_vals if dep_tup is not
None)
    if len(non_nones) > 1:
      logging.error('Choosing alphabetically.  You better know what you're
doing!').
    if non_nones:
       os_deps[deps_key] = non_nones[0][1] 

+      # |set_by_os| tracks what deps are overridden and by whom.
+      set_by_os = dict.fromkeys(deps.keys(), the_first_os)
+
+      os_deps = local_scope['deps_os'].get(the_first_os, {})
+      for os_dep_key, os_dep_value in os_deps.iteritems():
+        target_os_deps[os_dep_key] = os_dep_value
+        set_by_os[os_dep_key] = the_first_os
+
+      for the_target_os in self.target_os[1:]:
+        # Ignore (but warn) for 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.
+        os_deps = local_scope['deps_os'].get(the_target_os, {})
+        for os_dep_key, os_dep_value in os_deps.iteritems():
+          prev_value = target_os_deps.get(os_dep_key, deps.get(os_dep_key))
+          if os_dep_key in set_by_os and prev_value is not None:
+            # Some platforms override with None just to create a
+            # smaller checkout. No reason to warn about that.
+            if os_dep_value is not None and os_dep_value != prev_value:
+              print('Conflicting dependencies for %s. %s wants %s '

[Isaac (away), 2013/12/09 10:33:13]

Use logging calls instead of print

+                    'while %s wants %s. '
+                    '%s will be used since %s was listed before %s.' %

[Isaac (away), 2013/12/09 10:33:13]

Order is completely arbitrary in this list since target_os is passed through
set() multiple times.  I'd say we should just fail if multiple targets oses
specify conflicting revisions, which is just weird.

[Daniel Bratell, 2013/12/11 18:09:49]

The problem with that is that it won't be the person that makes the mistake that
will be affected but someone else innocent. It's possible that both conflicting
versions would have "worked", but instead there will be no build at all. It's
always more pleasurable punishing the one causing the conflict than the innocent
victim. :-)

But I'll raise an error. If it happens, hopefully the victim knows how to
manually edit DEPS files.

+                    (os_dep_key, the_target_os, os_dep_value,
+                     set_by_os[os_dep_key], prev_value,
+                     prev_value, set_by_os[os_dep_key], the_target_os))
+          else:
+            target_os_deps[os_dep_key] = os_dep_value
+            set_by_os[os_dep_key] = the_target_os
 
-    # deps_os overrides paths from deps
-    deps.update(target_os_deps)
+      # deps_os overrides paths from 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.