From ed2b4fe59bb58986a4228606f3fa06dd16883514 Mon Sep 17 00:00:00 2001 From: "bratell@opera.com" Date: Mon, 16 Dec 2013 14:34:12 +0000 Subject: [PATCH] Handle conflicting os deps better in gclient. It's possible in gclient to list multiple operating systems on the command line and while handling them all perfectly might not be possible it's possible to make it better than today. This patch makes sure None never overrides any previous value and it adds some output to indicate what kind of dangerous choices it made. BUG=248168 Review URL: https://codereview.chromium.org/23875029 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@240892 0039d316-1c4b-4281-b951-d872f2087c98 --- gclient.py | 59 +++++++++++++++++++++--------- tests/gclient_test.py | 83 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 124 insertions(+), 18 deletions(-) diff --git a/gclient.py b/gclient.py index c3e295c42..d1b5a8733 100755 --- a/gclient.py +++ b/gclient.py @@ -466,6 +466,46 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): raise gclient_utils.Error('Unknown url type') + @staticmethod + def MergeWithOsDeps(deps, deps_os, target_os_list): + """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 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) + if not possible_values: + 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) + # 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) + return new_deps + def ParseDepsFile(self): """Parses the DEPS file for this dependency.""" assert not self.deps_parsed @@ -502,22 +542,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): 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]) - else: - target_os_deps.update(os_deps) - - # deps_os overrides paths from deps - deps.update(target_os_deps) + target_os_list = self.target_os + if 'deps_os' in local_scope and target_os_list: + deps = self.MergeWithOsDeps(deps, local_scope['deps_os'], target_os_list) # If a line is in custom_deps, but not in the solution, we want to append # this line to the solution. diff --git a/tests/gclient_test.py b/tests/gclient_test.py index 73353dbc5..b758718cc 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -9,6 +9,7 @@ See gclient_smoketest.py for integration tests. """ import Queue +import copy import logging import os import sys @@ -475,8 +476,85 @@ class GclientTest(trial_dir.TestCase): ], sorted(self._get_processed())) + def testUpdateWithOsDeps(self): + """Verifies that complicated deps_os constructs result in the + correct data also with multple operating systems. Also see + testDepsOsOverrideDepsInDepsFile.""" + + 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'}, + {'os1': { 'foo': None } }, + ['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'}, + {'os1': { 'bar': 'os1_bar' }}, + ['os1'], + {'foo': 'default_foo', + 'bar': 'os1_bar'} + ), + ( + # One OS wants to add a module. One doesn't care. + {'foo': 'default_foo'}, + {'os1': { 'bar': 'os1_bar' }}, + ['os1', 'os2'], + {'foo': 'default_foo', + 'bar': 'os1_bar'} + ), + ( + # Two OSes want to add a module with the same definition. + {'foo': 'default_foo'}, + {'os1': { 'bar': 'os12_bar' }, + 'os2': { 'bar': 'os12_bar' }}, + ['os1', 'os2'], + {'foo': 'default_foo', + 'bar': 'os12_bar'} + ), + ] + for deps, deps_os, target_os_list, expected_deps in test_data: + orig_deps = copy.deepcopy(deps) + result = gclient.Dependency.MergeWithOsDeps(deps, deps_os, target_os_list) + self.assertEqual(result, expected_deps) + self.assertEqual(deps, orig_deps) + def testDepsOsOverrideDepsInDepsFile(self): - """Verifies that a 'deps_os' path can override a 'deps' path. + """Verifies that a 'deps_os' path can override a 'deps' path. Also + see testUpdateWithOsDeps above. """ write( @@ -495,7 +573,8 @@ class GclientTest(trial_dir.TestCase): 'deps_os = {\n' ' "unix": { "foo/unix": "/unix",' ' "foo/src": "/src_unix"},\n' - ' "baz": { "foo/baz": "/baz", },\n' + ' "baz": { "foo/baz": "/baz",\n' + ' "foo/src": None},\n' ' "jaz": { "foo/jaz": "/jaz", },\n' '}')