From 529d6a4e4aed18b48a8fc0e9351a5d4a07ca597d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Hajdan=2C=20Jr?= Date: Tue, 20 Jun 2017 13:17:03 +0200 Subject: [PATCH] gclient: include deps_os entries in dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Paweł Hajdan Jr. --- gclient.py | 62 +++++++++++++++-------------------- testing_support/fake_repos.py | 12 ++----- tests/gclient_smoketest.py | 12 +++---- tests/gclient_test.py | 34 ++++++++++--------- 4 files changed, 53 insertions(+), 67 deletions(-) diff --git a/gclient.py b/gclient.py index f78b62a5a..a1aef8c51 100755 --- a/gclient.py +++ b/gclient.py @@ -543,43 +543,30 @@ 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 + 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 return new_deps def _postprocess_deps(self, deps, rel_prefix): @@ -625,6 +612,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? diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index a5c6dc9f0..ed7dac258 100755 --- a/testing_support/fake_repos.py +++ b/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 = [ diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index 324a24a21..a0b7341f1 100755 --- a/tests/gclient_smoketest.py +++ b/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",', ' },', ' ', diff --git a/tests/gclient_test.py b/tests/gclient_test.py index 7c19c3dc4..409389327 100755 --- a/tests/gclient_test.py +++ b/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,22 @@ 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'} ), ] for deps, deps_os, target_os_list, expected_deps in test_data: @@ -577,25 +592,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'},