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 484 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])
+    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)
+          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_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.
     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 1358 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: