Handle conflicting os deps better in gclient.

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 470 matching lines @@
     deps = local_scope.get('deps', {})
     if 'recursion' in local_scope:
       self.recursion_override = local_scope.get('recursion')
       logging.warning(
           'Setting %s recursion to %d.', self.name, self.recursion_limit)
     # If present, save 'target_os' in the local_target_os property.
     if 'target_os' in local_scope:
       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)
 
-    # deps_os overrides paths from deps
-    deps.update(target_os_deps)
+      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)
 
     # If a line is in custom_deps, but not in the solution, we want to append
     # this line to the solution.
     for d in self.custom_deps:
       if d not in deps:
         deps[d] = self.custom_deps[d]
 
     # If use_relative_paths is set in the DEPS file, regenerate
     # the dictionary using paths relative to the directory containing
     # the DEPS file.
@@ skipping 1280 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: