Revert "gclient: include deps_os entries in dependencies"

This reverts commit 529d6a4e.

Reason for revert: broke developers and CI/Try checkouts.

Bug: 735418

Original change's description:
> gclient: include deps_os entries in dependencies
> 
> 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
> Change-Id: I2037a1ecc5fd2da6b5f73061548b81fc79ba2e72
> Reviewed-on: https://chromium-review.googlesource.com/541280
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

TBR=phajdan.jr@chromium.org,dpranke@chromium.org

Change-Id: Iaa0c39865908a5b25c15dda54ba61c0e76abcbea
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 570091
Reviewed-on: https://chromium-review.googlesource.com/543138Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

gclient.py

@@ -543,30 +543,43 @@ 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 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
-        if isinstance(value, basestring):
-          value = {'url': value}
-        if key in new_deps and new_deps[key] != value:
-          if isinstance(new_deps[key], basestring):
-            existing_url = new_deps[key]
-          else:
-            assert isinstance(new_deps[key],
-                              collections.Mapping), (key, new_deps[key])
-            existing_url = new_deps[key]['url']
-          if value['url'] != existing_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]))
-        assert isinstance(value, collections.Mapping), (key, value)
-        new_deps[key] = value
-        new_deps[key]['should_process'] = dep_os in target_os_list
+    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
     return new_deps
 
   def _postprocess_deps(self, deps, rel_prefix):
@@ -612,9 +625,6 @@ 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,16 +473,22 @@ deps = {
 }
 deps_os ={
   'mac': {
+    'src/none_repo': None,
+
     # This entry should not appear in flattened DEPS' |deps|.
-    'src/mac_repo': '/repo_5',
+    'src/os_repo': '/repo_5',
   },
   'unix': {
+    'src/none_repo': None,
+
     # This entry should not appear in flattened DEPS' |deps|.
-    'src/unix_repo': '/repo_5',
+    'src/os_repo': '/repo_5',
   },
   'win': {
+    'src/none_repo': None,
+
     # This entry should not appear in flattened DEPS' |deps|.
-    'src/win_repo': '/repo_5',
+    'src/os_repo': '/repo_5',
   },
 }
 hooks = [

tests/gclient_smoketest.py

@@ -601,8 +601,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         'deps_os = {',
         '  "mac": {',
         '    deps = {',
-        '      # src -> src/mac_repo',
-        '      "src/mac_repo": {',
+        '      # src -> src/os_repo',
+        '      "src/os_repo": {',
         '        "url": "/repo_5",',
         '      },',
         '    ',
@@ -612,8 +612,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '  "unix": {',
         '    deps = {',
-        '      # src -> src/unix_repo',
-        '      "src/unix_repo": {',
+        '      # src -> src/os_repo',
+        '      "src/os_repo": {',
         '        "url": "/repo_5",',
         '      },',
         '    ',
@@ -623,8 +623,8 @@ class GClientSmokeGIT(GClientSmokeBase):
         '',
         '  "win": {',
         '    deps = {',
-        '      # src -> src/win_repo',
-        '      "src/win_repo": {',
+        '      # src -> src/os_repo',
+        '      "src/os_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': {'should_process': True, 'url': 'os1_bar'}}
+         'bar': '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': {'should_process': True, 'url': 'os1_bar'}}
+         'bar': 'os1_bar'}
         ),
       (
         # Two OSes want to add a module with the same definition.
@@ -565,22 +565,7 @@ class GclientTest(trial_dir.TestCase):
          'os2': { 'bar': 'os12_bar' }},
         ['os1', 'os2'],
         {'foo': 'default_foo',
-         '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'}
+         'bar': 'os12_bar'}
         ),
       ]
     for deps, deps_os, target_os_list, expected_deps in test_data:
@@ -592,12 +577,25 @@ 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'},