gclient: include deps_os entries in dependencies (reland #1)

This is a reland of https://chromium-review.googlesource.com/c/541280/
with a fix for https://bugs.chromium.org/p/chromium/issues/detail?id=735418
(patchset 1 is original patch, patchset 2 has the fix).

Keep deps_os entries in dependencies, just with should_process set to False
for entries not applicable to target OS list.

This way gclient flatten has proper access to dependencies e.g. to evaluate
recursedeps referring to deps_os entries other than active OS.

Allow but ignore deps_os overriding a value with None, since that does not
fit the new model. There's no correctness harm in not checking out a repo.

Allow "overrides" setting given dependency to the same value. This seems
fairly common, especially for mac/ios and unix/android, even in chromium/src.

Bug: 570091, 735418
Change-Id: I6eba0e4be202212eb86cb959c18f2b2f0c1452b9
Reviewed-on: https://chromium-review.googlesource.com/543076
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

gclient.py

@@ -543,43 +543,45 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
     """Returns a new "deps" structure that is the deps sent in updated
     with information from deps_os (the deps_os section of the DEPS
     file) that matches the list of target os."""
-    os_overrides = {}
-    for the_target_os in target_os_list:
-      the_target_os_deps = 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 accidentally
-        # 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)
-      if not possible_values:
-        target_os_deps[os_dep_key] = None
-      else:
-        if len(possible_values) > 1:
-          raise gclient_utils.Error(
-              'Conflicting dependencies for %s: %s. (target_os=%s)' % (
-                  os_dep_key, os_dep_value, target_os_list))
-        # Sorting to get the same result every time in case of conflicts.
-        target_os_deps[os_dep_key] = sorted(possible_values)[0]
-
     new_deps = deps.copy()
-    for key, value in target_os_deps.iteritems():
-      if key in new_deps:
-        raise gclient_utils.Error(
-            ('Value from deps_os (%r: %r) conflicts with existing deps '
-             'entry (%r).') % (key, value, new_deps[key]))
-      new_deps[key] = value
+    for dep_os, os_deps in deps_os.iteritems():
+      for key, value in os_deps.iteritems():
+        if value is None:
+          # Make this condition very visible, so it's not a silent failure.
+          # It's unclear how to support None override in deps_os.
+          logging.error('Ignoring %r:%r in %r deps_os', key, value, dep_os)
+          continue
+
+        # Normalize value to be a dict which contains |should_process| metadata.
+        if isinstance(value, basestring):
+          value = {'url': value}
+        assert isinstance(value, collections.Mapping), (key, value)
+        value['should_process'] = dep_os in target_os_list
+
+        # Handle collisions/overrides.
+        if key in new_deps and new_deps[key] != value:
+          # Normalize the existing new_deps entry.
+          if isinstance(new_deps[key], basestring):
+            new_deps[key] = {'url': new_deps[key]}
+          assert isinstance(new_deps[key],
+                            collections.Mapping), (key, new_deps[key])
+
+          # It's OK if the "override" sets the key to the same value.
+          # This is mostly for legacy reasons to keep existing DEPS files
+          # working. Often mac/ios and unix/android will do this.
+          if value['url'] != new_deps[key]['url']:
+            raise gclient_utils.Error(
+                ('Value from deps_os (%r; %r: %r) conflicts with existing deps '
+                 'entry (%r).') % (dep_os, key, value, new_deps[key]))
+
+          # We'd otherwise overwrite |should_process| metadata, but a dep should
+          # be processed if _any_ of its references call for that.
+          value['should_process'] = (
+              value['should_process'] or
+              new_deps[key].get('should_process', True))
+
+        new_deps[key] = value
+
     return new_deps
 
   def _postprocess_deps(self, deps, rel_prefix):
@@ -625,6 +627,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         # This should be guaranteed by schema checking in gclient_eval.
         assert isinstance(dep_value, collections.Mapping)
         url = dep_value['url']
+        # Take into account should_process metadata set by MergeWithOsDeps.
+        should_process = (should_process and
+                          dep_value.get('should_process', True))
         condition = dep_value.get('condition')
       if condition:
         # TODO(phajdan.jr): should we also take custom vars into account?

testing_support/fake_repos.py

@@ -473,22 +473,16 @@ deps = {
 }
 deps_os ={
   'mac': {
-    'src/none_repo': None,
-
     # This entry should not appear in flattened DEPS' |deps|.
-    'src/os_repo': '/repo_5',
+    'src/mac_repo': '/repo_5',
   },
   'unix': {
-    'src/none_repo': None,
-
     # This entry should not appear in flattened DEPS' |deps|.
-    'src/os_repo': '/repo_5',
+    'src/unix_repo': '/repo_5',
   },
   'win': {
-    'src/none_repo': None,
-
     # This entry should not appear in flattened DEPS' |deps|.
-    'src/os_repo': '/repo_5',
+    'src/win_repo': '/repo_5',
   },
 }
 hooks = [

tests/gclient_smoketest.py

@@ -601,8 +601,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         'deps_os = {',
         '  "mac": {',
         '    deps = {',
-        '      # src -> src/os_repo',
-        '      "src/os_repo": {',
+        '      # src -> src/mac_repo',
+        '      "src/mac_repo": {',
         '        "url": "/repo_5",',
         '      },',
         '    ',
@@ -612,8 +612,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '  "unix": {',
         '    deps = {',
-        '      # src -> src/os_repo',
-        '      "src/os_repo": {',
+        '      # src -> src/unix_repo',
+        '      "src/unix_repo": {',
         '        "url": "/repo_5",',
         '      },',
         '    ',
@@ -623,8 +623,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '  "win": {',
         '    deps = {',
-        '      # src -> src/os_repo',
-        '      "src/os_repo": {',
+        '      # src -> src/win_repo',
+        '      "src/win_repo": {',
         '        "url": "/repo_5",',
         '      },',
         '    ',

tests/gclient_test.py

@@ -548,7 +548,7 @@ class GclientTest(trial_dir.TestCase):
         {'os1': { 'bar': 'os1_bar' }},
         ['os1'],
         {'foo': 'default_foo',
-         'bar': 'os1_bar'}
+         'bar': {'should_process': True, 'url': 'os1_bar'}}
         ),
       (
         # One OS wants to add a module. One doesn't care.
@@ -556,7 +556,7 @@ class GclientTest(trial_dir.TestCase):
         {'os1': { 'bar': 'os1_bar' }},
         ['os1', 'os2'],
         {'foo': 'default_foo',
-         'bar': 'os1_bar'}
+         'bar': {'should_process': True, 'url': 'os1_bar'}}
         ),
       (
         # Two OSes want to add a module with the same definition.
@@ -565,7 +565,29 @@ class GclientTest(trial_dir.TestCase):
          'os2': { 'bar': 'os12_bar' }},
         ['os1', 'os2'],
         {'foo': 'default_foo',
-         'bar': 'os12_bar'}
+         'bar': {'should_process': True, 'url': 'os12_bar'}}
+        ),
+      (
+        # One OS doesn't need module, one OS wants the default.
+        {'foo': 'default_foo'},
+        {'os1': { 'foo': None },
+         'os2': {}},
+        ['os1', 'os2'],
+        {'foo': 'default_foo'}
+        ),
+      (
+        # OS doesn't need module.
+        {'foo': 'default_foo'},
+        {'os1': { 'foo': None } },
+        ['os1'],
+        {'foo': 'default_foo'}
+        ),
+      (
+        # No-op override. Regression test for http://crbug.com/735418 .
+        {'foo': 'default_foo'},
+        {'os1': { 'foo': 'default_foo' } },
+        [],
+        {'foo': {'should_process': True, 'url': 'default_foo'}}
         ),
       ]
     for deps, deps_os, target_os_list, expected_deps in test_data:
@@ -577,25 +599,12 @@ class GclientTest(trial_dir.TestCase):
   def testUpdateWithOsDepsInvalid(self):
     test_data = [
       # Tuples of deps, deps_os, os_list.
-      (
-        # OS doesn't need module.
-        {'foo': 'default_foo'},
-        {'os1': { 'foo': None } },
-        ['os1'],
-        ),
       (
         # OS wants a different version of module.
         {'foo': 'default_foo'},
         {'os1': { 'foo': 'os1_foo'} },
         ['os1'],
         ),
-      (
-        # One OS doesn't need module, one OS wants the default.
-        {'foo': 'default_foo'},
-        {'os1': { 'foo': None },
-         'os2': {}},
-        ['os1', 'os2'],
-        ),
       (
         # One OS doesn't need module, another OS wants a special version.
         {'foo': 'default_foo'},