From 8f21332fec4c8adb5349ff90e30bb0e4f75e090e Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 30 Oct 2016 18:29:44 -0700 Subject: [PATCH] Bugfix: '::' only worked on top-level key in config. - generalized and fixed to work with any key in YAML file - simplified schema writing, as well - add more unit tests for the config system - Rename test/yaml.py to test/spack_yaml.py - Add test/yaml.pyc to ignored pyc files. --- bin/spack | 3 +- lib/spack/spack/config.py | 50 ++- lib/spack/spack/schema/compilers.py | 2 +- lib/spack/spack/schema/config.py | 16 +- lib/spack/spack/schema/mirrors.py | 2 +- lib/spack/spack/schema/modules.py | 2 +- lib/spack/spack/schema/packages.py | 2 +- lib/spack/spack/schema/repos.py | 2 +- lib/spack/spack/test/__init__.py | 2 +- lib/spack/spack/test/config.py | 322 +++++++++++------- .../spack/test/{yaml.py => spack_yaml.py} | 2 +- lib/spack/spack/util/spack_yaml.py | 2 + 12 files changed, 256 insertions(+), 151 deletions(-) rename lib/spack/spack/test/{yaml.py => spack_yaml.py} (98%) diff --git a/bin/spack b/bin/spack index 29991c070d..1f5dec0b3d 100755 --- a/bin/spack +++ b/bin/spack @@ -72,7 +72,8 @@ orphaned_pyc_files = [ os.path.join(SPACK_LIB_PATH, 'spack', 'platforms', 'cray_xc.pyc'), os.path.join(SPACK_LIB_PATH, 'spack', 'cmd', 'package-list.pyc'), os.path.join(SPACK_LIB_PATH, 'spack', 'cmd', 'test-install.pyc'), - os.path.join(SPACK_LIB_PATH, 'spack', 'cmd', 'url-parse.pyc') + os.path.join(SPACK_LIB_PATH, 'spack', 'cmd', 'url-parse.pyc'), + os.path.join(SPACK_LIB_PATH, 'spack', 'test', 'yaml.pyc') ] for pyc_file in orphaned_pyc_files: diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index c47eb68e09..989b3da169 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -266,7 +266,7 @@ def _read_config_file(filename, schema): try: tty.debug("Reading config file %s" % filename) with open(filename) as f: - data = syaml.load(f) + data = _mark_overrides(syaml.load(f)) if data: validate_section(data, schema) @@ -288,6 +288,34 @@ def clear_config_caches(): scope.clear() +def override(string): + """Test if a spack YAML string is an override. + + See ``spack_yaml`` for details. Keys in Spack YAML can end in `::`, + and if they do, their values completely replace lower-precedence + configs instead of merging into them. + + """ + return hasattr(string, 'override') and string.override + + +def _mark_overrides(data): + if isinstance(data, list): + return [_mark_overrides(elt) for elt in data] + + elif isinstance(data, dict): + marked = {} + for key, val in data.iteritems(): + if isinstance(key, basestring) and key.endswith(':'): + key = syaml.syaml_str(key[:-1]) + key.override = True + marked[key] = _mark_overrides(val) + return marked + + else: + return data + + def _merge_yaml(dest, source): """Merges source into dest; entries in source take precedence over dest. @@ -320,9 +348,11 @@ def they_are(t): # Source dict is merged into dest. elif they_are(dict): for sk, sv in source.iteritems(): - if sk not in dest: + if override(sk) or sk not in dest: + # if sk ended with ::, or if it's new, completely override dest[sk] = copy.copy(sv) else: + # otherwise, merge the YAML dest[sk] = _merge_yaml(dest[sk], source[sk]) return dest @@ -371,18 +401,18 @@ def get_config(section, scope=None): if not data or not isinstance(data, dict): continue - # Allow complete override of site config with '
::' - override_key = section + ':' - if not (section in data or override_key in data): + if section not in data: tty.warn("Skipping bad configuration file: '%s'" % scope.path) continue - if override_key in data: - merged_section = data[override_key] - else: - merged_section = _merge_yaml(merged_section, data[section]) + merged_section = _merge_yaml(merged_section, data) - return merged_section + # no config files -- empty config. + if section not in merged_section: + return {} + + # take the top key off before returning. + return merged_section[section] def get_config_filename(scope, section): diff --git a/lib/spack/spack/schema/compilers.py b/lib/spack/spack/schema/compilers.py index 1c7894d675..ea1071729f 100644 --- a/lib/spack/spack/schema/compilers.py +++ b/lib/spack/spack/schema/compilers.py @@ -35,7 +35,7 @@ 'type': 'object', 'additionalProperties': False, 'patternProperties': { - 'compilers:?': { # optional colon for overriding site config. + 'compilers': { 'type': 'array', 'items': { 'compiler': { diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py index 31d4b8a8a8..e51fa69afe 100644 --- a/lib/spack/spack/schema/config.py +++ b/lib/spack/spack/schema/config.py @@ -49,7 +49,6 @@ }, 'module_roots': { 'type': 'object', - 'default': {}, 'additionalProperties': False, 'properties': { 'tcl': {'type': 'string'}, @@ -59,18 +58,9 @@ }, 'source_cache': {'type': 'string'}, 'misc_cache': {'type': 'string'}, - 'verify_ssl': { - 'type': 'boolean', - 'default': True, - }, - 'checksum': { - 'type': 'boolean', - 'default': True, - }, - 'dirty': { - 'type': 'boolean', - 'default': False, - }, + 'verify_ssl': {'type': 'boolean'}, + 'checksum': {'type': 'boolean'}, + 'dirty': {'type': 'boolean'}, } }, }, diff --git a/lib/spack/spack/schema/mirrors.py b/lib/spack/spack/schema/mirrors.py index 9aa3a0f747..60b865bb42 100644 --- a/lib/spack/spack/schema/mirrors.py +++ b/lib/spack/spack/schema/mirrors.py @@ -35,7 +35,7 @@ 'type': 'object', 'additionalProperties': False, 'patternProperties': { - r'mirrors:?': { + r'mirrors': { 'type': 'object', 'default': {}, 'additionalProperties': False, diff --git a/lib/spack/spack/schema/modules.py b/lib/spack/spack/schema/modules.py index 2e776635d2..2059e14fa6 100644 --- a/lib/spack/spack/schema/modules.py +++ b/lib/spack/spack/schema/modules.py @@ -127,7 +127,7 @@ } }, 'patternProperties': { - r'modules:?': { + r'modules': { 'type': 'object', 'default': {}, 'additionalProperties': False, diff --git a/lib/spack/spack/schema/packages.py b/lib/spack/spack/schema/packages.py index 29c62150ef..bf5648b1b7 100644 --- a/lib/spack/spack/schema/packages.py +++ b/lib/spack/spack/schema/packages.py @@ -35,7 +35,7 @@ 'type': 'object', 'additionalProperties': False, 'patternProperties': { - r'packages:?': { + r'packages': { 'type': 'object', 'default': {}, 'additionalProperties': False, diff --git a/lib/spack/spack/schema/repos.py b/lib/spack/spack/schema/repos.py index 74dcee7cdc..c7a3495ae1 100644 --- a/lib/spack/spack/schema/repos.py +++ b/lib/spack/spack/schema/repos.py @@ -35,7 +35,7 @@ 'type': 'object', 'additionalProperties': False, 'patternProperties': { - r'repos:?': { + r'repos': { 'type': 'array', 'default': [], 'items': { diff --git a/lib/spack/spack/test/__init__.py b/lib/spack/spack/test/__init__.py index 457e5db9dc..c0a4c7354f 100644 --- a/lib/spack/spack/test/__init__.py +++ b/lib/spack/spack/test/__init__.py @@ -78,7 +78,7 @@ 'url_substitution', 'versions', 'provider_index', - 'yaml', + 'spack_yaml', # This test needs to be last until global compiler cache is fixed. 'cmd.test_compiler_cmd', ] diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py index d5e1791b40..adc0795916 100644 --- a/lib/spack/spack/test/config.py +++ b/lib/spack/spack/test/config.py @@ -25,6 +25,7 @@ import os import shutil import getpass +import yaml from tempfile import mkdtemp import spack @@ -34,109 +35,136 @@ from spack.test.mock_packages_test import * # Some sample compiler config data -a_comps = [ - {'compiler': { - 'paths': { - "cc": "/gcc473", - "cxx": "/g++473", - "f77": None, - "fc": None - }, - 'modules': None, - 'spec': 'gcc@4.7.3', - 'operating_system': 'CNL10' - }}, - {'compiler': { - 'paths': { - "cc": "/gcc450", - "cxx": "/g++450", - "f77": 'gfortran', - "fc": 'gfortran' - }, - 'modules': None, - 'spec': 'gcc@4.5.0', - 'operating_system': 'CNL10' - }}, - {'compiler': { - 'paths': { - "cc": "/gcc422", - "cxx": "/g++422", - "f77": 'gfortran', - "fc": 'gfortran' - }, - 'flags': { - "cppflags": "-O0 -fpic", - "fflags": "-f77", - }, - 'modules': None, - 'spec': 'gcc@4.2.2', - 'operating_system': 'CNL10' - }}, - {'compiler': { - 'paths': { - "cc": "", - "cxx": "", - "f77": '', - "fc": ''}, - 'modules': None, - 'spec': 'clang@3.3', - 'operating_system': 'CNL10' - }} -] +a_comps = { + 'compilers': [ + {'compiler': { + 'paths': { + "cc": "/gcc473", + "cxx": "/g++473", + "f77": None, + "fc": None + }, + 'modules': None, + 'spec': 'gcc@4.7.3', + 'operating_system': 'CNL10' + }}, + {'compiler': { + 'paths': { + "cc": "/gcc450", + "cxx": "/g++450", + "f77": 'gfortran', + "fc": 'gfortran' + }, + 'modules': None, + 'spec': 'gcc@4.5.0', + 'operating_system': 'CNL10' + }}, + {'compiler': { + 'paths': { + "cc": "/gcc422", + "cxx": "/g++422", + "f77": 'gfortran', + "fc": 'gfortran' + }, + 'flags': { + "cppflags": "-O0 -fpic", + "fflags": "-f77", + }, + 'modules': None, + 'spec': 'gcc@4.2.2', + 'operating_system': 'CNL10' + }}, + {'compiler': { + 'paths': { + "cc": "", + "cxx": "", + "f77": '', + "fc": ''}, + 'modules': None, + 'spec': 'clang@3.3', + 'operating_system': 'CNL10' + }} + ] +} -b_comps = [ - {'compiler': { - 'paths': { - "cc": "/icc100", - "cxx": "/icp100", - "f77": None, - "fc": None - }, - 'modules': None, - 'spec': 'icc@10.0', - 'operating_system': 'CNL10' - }}, - {'compiler': { - 'paths': { - "cc": "/icc111", - "cxx": "/icp111", - "f77": 'ifort', - "fc": 'ifort' - }, - 'modules': None, - 'spec': 'icc@11.1', - 'operating_system': 'CNL10' - }}, - {'compiler': { - 'paths': { - "cc": "/icc123", - "cxx": "/icp123", - "f77": 'ifort', - "fc": 'ifort' - }, - 'flags': { - "cppflags": "-O3", - "fflags": "-f77rtl", - }, - 'modules': None, - 'spec': 'icc@12.3', - 'operating_system': 'CNL10' - }}, - {'compiler': { - 'paths': { - "cc": "", - "cxx": "", - "f77": '', - "fc": ''}, - 'modules': None, - 'spec': 'clang@3.3', - 'operating_system': 'CNL10' - }} -] +b_comps = { + 'compilers': [ + {'compiler': { + 'paths': { + "cc": "/icc100", + "cxx": "/icp100", + "f77": None, + "fc": None + }, + 'modules': None, + 'spec': 'icc@10.0', + 'operating_system': 'CNL10' + }}, + {'compiler': { + 'paths': { + "cc": "/icc111", + "cxx": "/icp111", + "f77": 'ifort', + "fc": 'ifort' + }, + 'modules': None, + 'spec': 'icc@11.1', + 'operating_system': 'CNL10' + }}, + {'compiler': { + 'paths': { + "cc": "/icc123", + "cxx": "/icp123", + "f77": 'ifort', + "fc": 'ifort' + }, + 'flags': { + "cppflags": "-O3", + "fflags": "-f77rtl", + }, + 'modules': None, + 'spec': 'icc@12.3', + 'operating_system': 'CNL10' + }}, + {'compiler': { + 'paths': { + "cc": "", + "cxx": "", + "f77": '', + "fc": ''}, + 'modules': None, + 'spec': 'clang@3.3', + 'operating_system': 'CNL10' + }} + ] +} # Some Sample repo data -repos_low = ["/some/path"] -repos_high = ["/some/other/path"] +repos_low = {'repos': ["/some/path"]} +repos_high = {'repos': ["/some/other/path"]} + + +# sample config data +config_low = { + 'config': { + 'install_tree': 'install_tree_path', + 'build_stage': ['path1', 'path2', 'path3']}} + +config_override_all = { + 'config:': { + 'install_tree:': 'override_all'}} + +config_override_key = { + 'config': { + 'install_tree:': 'override_key'}} + +config_merge_list = { + 'config': { + 'build_stage': ['patha', 'pathb']}} + +config_override_list = { + 'config': { + 'build_stage:': ['patha', 'pathb']}} class ConfigTest(MockPackagesTest): @@ -144,19 +172,30 @@ class ConfigTest(MockPackagesTest): def setUp(self): super(ConfigTest, self).setUp() self.tmp_dir = mkdtemp('.tmp', 'spack-config-test-') - self.a_comp_specs = [ac['compiler']['spec'] for ac in a_comps] - self.b_comp_specs = [bc['compiler']['spec'] for bc in b_comps] + self.a_comp_specs = [ + ac['compiler']['spec'] for ac in a_comps['compilers']] + self.b_comp_specs = [ + bc['compiler']['spec'] for bc in b_comps['compilers']] spack.config.config_scopes = OrderedDict() for priority in ['low', 'high']: - spack.config.ConfigScope('test_{0}_priority'.format(priority), - os.path.join(self.tmp_dir, priority)) + scope_dir = os.path.join(self.tmp_dir, priority) + spack.config.ConfigScope(priority, scope_dir) def tearDown(self): super(ConfigTest, self).tearDown() shutil.rmtree(self.tmp_dir, True) - def check_config(self, comps, *compiler_names): + def write_config_file(self, config, data, scope): + scope_dir = os.path.join(self.tmp_dir, scope) + mkdirp(scope_dir) + + path = os.path.join(scope_dir, config + '.yaml') + with open(path, 'w') as f: + print yaml + yaml.dump(data, f) + + def check_compiler_config(self, comps, *compiler_names): """Check that named compilers in comps match Spack's config.""" config = spack.config.get_config('compilers') compiler_list = ['cc', 'cxx', 'f77', 'fc'] @@ -182,43 +221,50 @@ def check_config(self, comps, *compiler_names): self.assertEqual(expected, actual) def test_write_list_in_memory(self): - spack.config.update_config('repos', repos_low, 'test_low_priority') - spack.config.update_config('repos', repos_high, 'test_high_priority') + spack.config.update_config('repos', repos_low['repos'], scope='low') + spack.config.update_config('repos', repos_high['repos'], scope='high') + config = spack.config.get_config('repos') - self.assertEqual(config, repos_high + repos_low) + self.assertEqual(config, repos_high['repos'] + repos_low['repos']) def test_write_key_in_memory(self): # Write b_comps "on top of" a_comps. - spack.config.update_config('compilers', a_comps, 'test_low_priority') - spack.config.update_config('compilers', b_comps, 'test_high_priority') + spack.config.update_config( + 'compilers', a_comps['compilers'], scope='low') + spack.config.update_config( + 'compilers', b_comps['compilers'], scope='high') # Make sure the config looks how we expect. - self.check_config(a_comps, *self.a_comp_specs) - self.check_config(b_comps, *self.b_comp_specs) + self.check_compiler_config(a_comps['compilers'], *self.a_comp_specs) + self.check_compiler_config(b_comps['compilers'], *self.b_comp_specs) def test_write_key_to_disk(self): # Write b_comps "on top of" a_comps. - spack.config.update_config('compilers', a_comps, 'test_low_priority') - spack.config.update_config('compilers', b_comps, 'test_high_priority') + spack.config.update_config( + 'compilers', a_comps['compilers'], scope='low') + spack.config.update_config( + 'compilers', b_comps['compilers'], scope='high') # Clear caches so we're forced to read from disk. spack.config.clear_config_caches() # Same check again, to ensure consistency. - self.check_config(a_comps, *self.a_comp_specs) - self.check_config(b_comps, *self.b_comp_specs) + self.check_compiler_config(a_comps['compilers'], *self.a_comp_specs) + self.check_compiler_config(b_comps['compilers'], *self.b_comp_specs) def test_write_to_same_priority_file(self): # Write b_comps in the same file as a_comps. - spack.config.update_config('compilers', a_comps, 'test_low_priority') - spack.config.update_config('compilers', b_comps, 'test_low_priority') + spack.config.update_config( + 'compilers', a_comps['compilers'], scope='low') + spack.config.update_config( + 'compilers', b_comps['compilers'], scope='low') # Clear caches so we're forced to read from disk. spack.config.clear_config_caches() # Same check again, to ensure consistency. - self.check_config(a_comps, *self.a_comp_specs) - self.check_config(b_comps, *self.b_comp_specs) + self.check_compiler_config(a_comps['compilers'], *self.a_comp_specs) + self.check_compiler_config(b_comps['compilers'], *self.b_comp_specs) def check_canonical(self, var, expected): """Ensure that is substituted properly for in strings @@ -270,3 +316,39 @@ def test_substitute_tempdir(self): self.assertEqual(tempdir, canonicalize_path('$tempdir')) self.assertEqual(tempdir + '/foo/bar/baz', canonicalize_path('$tempdir/foo/bar/baz')) + + def test_read_config(self): + self.write_config_file('config', config_low, 'low') + self.assertEqual(spack.config.get_config('config'), + config_low['config']) + + def test_read_config_override_all(self): + self.write_config_file('config', config_low, 'low') + self.write_config_file('config', config_override_all, 'high') + self.assertEqual(spack.config.get_config('config'), { + 'install_tree': 'override_all' + }) + + def test_read_config_override_key(self): + self.write_config_file('config', config_low, 'low') + self.write_config_file('config', config_override_key, 'high') + self.assertEqual(spack.config.get_config('config'), { + 'install_tree': 'override_key', + 'build_stage': ['path1', 'path2', 'path3'] + }) + + def test_read_config_merge_list(self): + self.write_config_file('config', config_low, 'low') + self.write_config_file('config', config_merge_list, 'high') + self.assertEqual(spack.config.get_config('config'), { + 'install_tree': 'install_tree_path', + 'build_stage': ['patha', 'pathb', 'path1', 'path2', 'path3'] + }) + + def test_read_config_override_list(self): + self.write_config_file('config', config_low, 'low') + self.write_config_file('config', config_override_list, 'high') + self.assertEqual(spack.config.get_config('config'), { + 'install_tree': 'install_tree_path', + 'build_stage': ['patha', 'pathb'] + }) diff --git a/lib/spack/spack/test/yaml.py b/lib/spack/spack/test/spack_yaml.py similarity index 98% rename from lib/spack/spack/test/yaml.py rename to lib/spack/spack/test/spack_yaml.py index dedbd15d10..30ed1672e2 100644 --- a/lib/spack/spack/test/yaml.py +++ b/lib/spack/spack/test/spack_yaml.py @@ -56,7 +56,7 @@ ])} -class YamlTest(unittest.TestCase): +class SpackYamlTest(unittest.TestCase): def setUp(self): self.data = syaml.load(test_file) diff --git a/lib/spack/spack/util/spack_yaml.py b/lib/spack/spack/util/spack_yaml.py index 506f56633a..674c79bca1 100644 --- a/lib/spack/spack/util/spack_yaml.py +++ b/lib/spack/spack/util/spack_yaml.py @@ -91,7 +91,9 @@ def construct_yaml_str(self, node): value = value.encode('ascii') except UnicodeEncodeError: pass + value = syaml_str(value) + mark(value, node) return value