From 66945379c69dca334b61ebb4728b4a6ee95fde79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Hajdan=2C=20Jr?= Date: Mon, 12 Jun 2017 19:34:52 +0200 Subject: [PATCH] gclient: throw errors if values from deps_os override deps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: a0ad8ad9c9e72291caa387d4ad1d556da3aa860f https://codereview.chromium.org/11368067 https://bugs.chromium.org/p/chromium/issues/detail?id=157979 ed2b4fe59bb58986a4228606f3fa06dd16883514 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/531029 Reviewed-by: Dirk Pranke Reviewed-by: Andrii Shyshkalov Commit-Queue: Paweł Hajdan Jr. --- gclient.py | 14 ++++++--- tests/gclient_test.py | 71 ++++++++++++++++++++++--------------------- 2 files changed, 45 insertions(+), 40 deletions(-) diff --git a/gclient.py b/gclient.py index b25dfaa31..0481644ce 100755 --- a/gclient.py +++ b/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): diff --git a/tests/gclient_test.py b/tests/gclient_test.py index baa3d377f..7c19c3dc4 100755 --- a/tests/gclient_test.py +++ b/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) - obj.RunOnDeps('None', []) + 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()))