gclient: throw errors if values from deps_os override deps

With 'flatten' work and in general, we assume deps_os only add to deps,
without attempting to override entries there.

This removes significant edge cases from flatten code,
and ensures DEPS are easier to reason about.

This reverses some past patches and decisions:

a0ad8ad9
https://codereview.chromium.org/11368067
https://bugs.chromium.org/p/chromium/issues/detail?id=157979

ed2b4fe5
https://codereview.chromium.org/23875029
https://bugs.chromium.org/p/chromium/issues/detail?id=248168

These are rather old though (2012-2013), and not expected to be used.

Bug: 570091
Change-Id: I143e95bdaef9d10c937a5f678e6be7e26899ad4d
Reviewed-on: https://chromium-review.googlesource.com/531029Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

gclient.py

@@ -567,15 +567,19 @@ class Dependency(gclient_utils.WorkItem, DependencySettings):
         target_os_deps[os_dep_key] = None
       else:
         if len(possible_values) > 1:
-          # It would be possible to abort here but it would be
-          # unfortunate if we end up preventing any kind of checkout.
-          logging.error('Conflicting dependencies for %s: %s. (target_os=%s)',
-                        os_dep_key, os_dep_value, target_os_list)
+          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()
-    new_deps.update(target_os_deps)
+    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):

tests/gclient_test.py

@@ -535,20 +535,6 @@ class GclientTest(trial_dir.TestCase):
 
     test_data = [
       # Tuples of deps, deps_os, os_list and expected_deps.
-      (
-        # OS doesn't need module.
-        {'foo': 'default_foo'},
-        {'os1': { 'foo': None } },
-        ['os1'],
-        {'foo': None}
-        ),
-      (
-        # OS wants a different version of module.
-        {'foo': 'default_foo'},
-        {'os1': { 'foo': 'os1_foo'} },
-        ['os1'],
-        {'foo': 'os1_foo'}
-        ),
       (
         # OS with no overrides at all.
         {'foo': 'default_foo'},
@@ -556,22 +542,6 @@ class GclientTest(trial_dir.TestCase):
         ['os2'],
         {'foo': 'default_foo'}
         ),
-      (
-        # One OS doesn't need module, one OS wants the default.
-        {'foo': 'default_foo'},
-        {'os1': { 'foo': None },
-         'os2': {}},
-        ['os1', 'os2'],
-        {'foo': 'default_foo'}
-        ),
-      (
-        # One OS doesn't need module, another OS wants a special version.
-        {'foo': 'default_foo'},
-        {'os1': { 'foo': None },
-         'os2': { 'foo': 'os2_foo'}},
-        ['os1', 'os2'],
-        {'foo': 'os2_foo'}
-        ),
       (
         # One OS wants to add a module.
         {'foo': 'default_foo'},
@@ -604,6 +574,39 @@ class GclientTest(trial_dir.TestCase):
       self.assertEqual(result, expected_deps)
       self.assertEqual(deps, orig_deps)
 
+  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'},
+        {'os1': { 'foo': None },
+         'os2': { 'foo': 'os2_foo'}},
+        ['os1', 'os2'],
+        ),
+      ]
+    for deps, deps_os, target_os_list in test_data:
+      with self.assertRaises(gclient_utils.Error):
+        gclient.Dependency.MergeWithOsDeps(deps, deps_os, target_os_list)
 
   def testLateOverride(self):
     """Verifies expected behavior of LateOverride."""
@@ -614,7 +617,7 @@ class GclientTest(trial_dir.TestCase):
     self.assertEquals(url, late_url)
 
   def testDepsOsOverrideDepsInDepsFile(self):
-    """Verifies that a 'deps_os' path can override a 'deps' path. Also
+    """Verifies that a 'deps_os' path cannot override a 'deps' path. Also
     see testUpdateWithOsDeps above.
     """
 
@@ -644,14 +647,12 @@ class GclientTest(trial_dir.TestCase):
     options.deps_os = 'unix'
 
     obj = gclient.GClient.LoadCurrentConfig(options)
+    with self.assertRaises(gclient_utils.Error):
       obj.RunOnDeps('None', [])
     self.assertEqual(['unix'], sorted(obj.enforced_os))
     self.assertEquals(
         [
           ('foo', 'svn://example.com/foo'),
-          ('foo/baz', 'svn://example.com/foo/baz'),
-          ('foo/src', 'svn://example.com/foo/src_unix'),
-          ('foo/unix', 'svn://example.com/foo/unix'),
           ],
         sorted(self._get_processed()))