diff --git a/gclient.py b/gclient.py index 01c40ffc1..1f8a309a9 100755 --- a/gclient.py +++ b/gclient.py @@ -543,43 +543,45 @@ 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 + + # Normalize value to be a dict which contains |should_process| metadata. + if isinstance(value, basestring): + value = {'url': value} + assert isinstance(value, collections.Mapping), (key, value) + value['should_process'] = dep_os in target_os_list + + # Handle collisions/overrides. + if key in new_deps and new_deps[key] != value: + # Normalize the existing new_deps entry. + if isinstance(new_deps[key], basestring): + new_deps[key] = {'url': new_deps[key]} + assert isinstance(new_deps[key], + collections.Mapping), (key, new_deps[key]) + + # It's OK if the "override" sets the key to the same value. + # This is mostly for legacy reasons to keep existing DEPS files + # working. Often mac/ios and unix/android will do this. + if value['url'] != new_deps[key]['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])) + + # We'd otherwise overwrite |should_process| metadata, but a dep should + # be processed if _any_ of its references call for that. + value['should_process'] = ( + value['should_process'] or + new_deps[key].get('should_process', True)) + + new_deps[key] = value + return new_deps def _postprocess_deps(self, deps, rel_prefix): @@ -625,6 +627,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..c185cb6ac 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,29 @@ 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'} + ), + ( + # No-op override. Regression test for http://crbug.com/735418 . + {'foo': 'default_foo'}, + {'os1': { 'foo': 'default_foo' } }, + [], + {'foo': {'should_process': True, 'url': 'default_foo'}} ), ] for deps, deps_os, target_os_list, expected_deps in test_data: @@ -577,25 +599,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'},