refactor install_tree to use projections format (#18341)

* refactor install_tree to use projections format

* Add update method for config.yaml

* add test for config update config

etc/spack/defaults/config.yaml

@@ -16,7 +16,10 @@
 config:
   # This is the path to the root of the Spack install tree.
   # You can use $spack here to refer to the root of the spack instance.
-  install_tree: $spack/opt/spack
+  install_tree:
+    root: $spack/opt/spack
+    projections:
+      all: "${ARCHITECTURE}/${COMPILERNAME}-${COMPILERVER}/${PACKAGE}-${VERSION}-${HASH}"
 
 
   # Locations where templates should be found
@@ -24,10 +27,6 @@ config:
     - $spack/share/spack/templates
 
 
-  # Default directory layout
-  install_path_scheme: "${ARCHITECTURE}/${COMPILERNAME}-${COMPILERVER}/${PACKAGE}-${VERSION}-${HASH}"
-
-
   # Locations where different types of modules should be installed.
   module_roots:
     tcl:    $spack/share/spack/modules

lib/spack/spack/directory_layout.py

@@ -7,7 +7,6 @@
 import shutil
 import glob
 import tempfile
-import re
 from contextlib import contextmanager
 
 import ruamel.yaml as yaml
@@ -19,6 +18,11 @@
 from spack.error import SpackError
 
 
+default_projections = {'all': ('{architecture}/'
+                               '{compiler.name}-{compiler.version}/'
+                               '{name}-{version}-{hash}')}
+
+
 def _check_concrete(spec):
     """If the spec is not concrete, raise a ValueError"""
     if not spec.concrete:
@@ -179,24 +183,31 @@ class YamlDirectoryLayout(DirectoryLayout):
        The hash here is a SHA-1 hash for the full DAG plus the build
        spec.  TODO: implement the build spec.
 
-       The installation directory scheme can be modified with the
+       The installation directory projections can be modified with the
-       arguments hash_len and path_scheme.
+       projections argument.
     """
 
     def __init__(self, root, **kwargs):
         super(YamlDirectoryLayout, self).__init__(root)
-        self.hash_len       = kwargs.get('hash_len')
+        projections = kwargs.get('projections') or default_projections
-        self.path_scheme    = kwargs.get('path_scheme') or (
+        self.projections = dict((key, projection.lower())
-            "{architecture}/"
+                                for key, projection in projections.items())
-            "{compiler.name}-{compiler.version}/"
+
-            "{name}-{version}-{hash}")
+        # apply hash length as appropriate
-        self.path_scheme = self.path_scheme.lower()
+        self.hash_length = kwargs.get('hash_length', None)
-        if self.hash_len is not None:
+        if self.hash_length is not None:
-            if re.search(r'{hash:\d+}', self.path_scheme):
+            for when_spec, projection in self.projections.items():
-                raise InvalidDirectoryLayoutParametersError(
+                if '{hash}' not in projection:
-                    "Conflicting options for installation layout hash length")
+                    if '{hash' in projection:
-            self.path_scheme = self.path_scheme.replace(
+                        raise InvalidDirectoryLayoutParametersError(
-                "{hash}", "{hash:%d}" % self.hash_len)
+                            "Conflicting options for installation layout hash"
+                            " length")
+                    else:
+                        raise InvalidDirectoryLayoutParametersError(
+                            "Cannot specify hash length when the hash is not"
+                            " part of all install_tree projections")
+                self.projections[when_spec] = projection.replace(
+                    "{hash}", "{hash:%d}" % self.hash_length)
 
         # If any of these paths change, downstream databases may not be able to
         # locate files in older upstream databases
@@ -214,7 +225,8 @@ def hidden_file_paths(self):
     def relative_path_for_spec(self, spec):
         _check_concrete(spec)
 
-        path = spec.format(self.path_scheme)
+        projection = spack.projections.get_projection(self.projections, spec)
+        path = spec.format(projection)
         return path
 
     def write_spec(self, spec, path):
@@ -336,25 +348,32 @@ def all_specs(self):
         if not os.path.isdir(self.root):
             return []
 
-        path_elems = ["*"] * len(self.path_scheme.split(os.sep))
+        specs = []
-        path_elems += [self.metadata_dir, self.spec_file_name]
+        for _, path_scheme in self.projections.items():
-        pattern = os.path.join(self.root, *path_elems)
+            path_elems = ["*"] * len(path_scheme.split(os.sep))
-        spec_files = glob.glob(pattern)
+            path_elems += [self.metadata_dir, self.spec_file_name]
-        return [self.read_spec(s) for s in spec_files]
+            pattern = os.path.join(self.root, *path_elems)
+            spec_files = glob.glob(pattern)
+            specs.extend([self.read_spec(s) for s in spec_files])
+        return specs
 
     def all_deprecated_specs(self):
         if not os.path.isdir(self.root):
             return []
 
-        path_elems = ["*"] * len(self.path_scheme.split(os.sep))
+        deprecated_specs = set()
-        path_elems += [self.metadata_dir, self.deprecated_dir,
+        for _, path_scheme in self.projections.items():
-                       '*_' + self.spec_file_name]
+            path_elems = ["*"] * len(path_scheme.split(os.sep))
-        pattern = os.path.join(self.root, *path_elems)
+            path_elems += [self.metadata_dir, self.deprecated_dir,
-        spec_files = glob.glob(pattern)
+                           '*_' + self.spec_file_name]
-        get_depr_spec_file = lambda x: os.path.join(
+            pattern = os.path.join(self.root, *path_elems)
-            os.path.dirname(os.path.dirname(x)), self.spec_file_name)
+            spec_files = glob.glob(pattern)
-        return set((self.read_spec(s), self.read_spec(get_depr_spec_file(s)))
+            get_depr_spec_file = lambda x: os.path.join(
-                   for s in spec_files)
+                os.path.dirname(os.path.dirname(x)), self.spec_file_name)
+            deprecated_specs |= set((self.read_spec(s),
+                                     self.read_spec(get_depr_spec_file(s)))
+                                    for s in spec_files)
+        return deprecated_specs
 
     def specs_by_hash(self):
         by_hash = {}

lib/spack/spack/schema/config.py

@@ -8,7 +8,9 @@
 .. literalinclude:: _spack_root/lib/spack/spack/schema/config.py
    :lines: 13-
 """
-
+import six
+from llnl.util.lang import union_dicts
+import spack.schema.projections
 
 #: Properties for inclusion in other schemas
 properties = {
@@ -20,9 +22,20 @@
                 'type': 'string',
                 'enum': ['rpath', 'runpath']
             },
-            'install_tree': {'type': 'string'},
+            'install_tree': {
+                'anyOf': [
+                    {
+                        'type': 'object',
+                        'properties': union_dicts(
+                            {'root': {'type': 'string'}},
+                            spack.schema.projections.properties,
+                        ),
+                    },
+                    {'type': 'string'}  # deprecated
+                ],
+            },
             'install_hash_length': {'type': 'integer', 'minimum': 1},
-            'install_path_scheme': {'type': 'string'},
+            'install_path_scheme': {'type': 'string'},  # deprecated
             'build_stage': {
                 'oneOf': [
                     {'type': 'string'},
@@ -87,3 +100,45 @@
     'additionalProperties': False,
     'properties': properties,
 }
+
+
+def update(data):
+    """Update the data in place to remove deprecated properties.
+
+    Args:
+        data (dict): dictionary to be updated
+
+    Returns:
+        True if data was changed, False otherwise
+    """
+    # currently deprecated properties are
+    # install_tree: <string>
+    # install_path_scheme: <string>
+    # updated: install_tree: {root: <string>,
+    #                         projections: <projections_dict}
+    # root replaces install_tree, projections replace install_path_scheme
+    changed = False
+
+    install_tree = data.get('install_tree', None)
+    if isinstance(install_tree, six.string_types):
+        # deprecated short-form install tree
+        # add value as `root` in updated install_tree
+        data['install_tree'] = {'root': install_tree}
+        changed = True
+
+    install_path_scheme = data.pop('install_path_scheme', None)
+    if install_path_scheme:
+        projections_data = {
+            'projections': {
+                'all': install_path_scheme
+            }
+        }
+
+        # update projections with install_scheme
+        # whether install_tree was updated or not
+        # we merge the yaml to ensure we don't invalidate other projections
+        update_data = data.get('install_tree', {})
+        update_data = spack.config.merge_yaml(update_data, projections_data)
+        data['install_tree'] = update_data
+        changed = True
+    return changed

lib/spack/spack/schema/projections.py

@@ -14,7 +14,6 @@
 properties = {
     'projections': {
         'type': 'object',
-        'default': {},
         'patternProperties': {
             r'all|\w[\w-]*': {
                 'type': 'string'

lib/spack/spack/store.py

@@ -24,14 +24,16 @@
 
 """
 import os
+import six
 
 import llnl.util.lang
+import llnl.util.tty as tty
 
 import spack.paths
 import spack.config
 import spack.util.path
 import spack.database
-import spack.directory_layout
+import spack.directory_layout as dir_layout
 
 #: default installation root, relative to the Spack install path
 default_root = os.path.join(spack.paths.opt_path, 'spack')
@@ -56,12 +58,12 @@ class Store(object):
         hash_length (int): length of the hashes used in the directory
             layout; spec hash suffixes will be truncated to this length
     """
-    def __init__(self, root, path_scheme=None, hash_length=None):
+    def __init__(self, root, projections=None, hash_length=None):
         self.root = root
         self.db = spack.database.Database(
             root, upstream_dbs=retrieve_upstream_dbs())
-        self.layout = spack.directory_layout.YamlDirectoryLayout(
+        self.layout = dir_layout.YamlDirectoryLayout(
-            root, hash_len=hash_length, path_scheme=path_scheme)
+            root, projections=projections, hash_length=hash_length)
 
     def reindex(self):
         """Convenience function to reindex the store DB with its own layout."""
@@ -70,11 +72,31 @@ def reindex(self):
 
 def _store():
     """Get the singleton store instance."""
-    root = spack.config.get('config:install_tree', default_root)
+    install_tree = spack.config.get('config:install_tree', {})
-    root = spack.util.path.canonicalize_path(root)
 
-    return Store(root,
+    if isinstance(install_tree, six.string_types):
-                 spack.config.get('config:install_path_scheme'),
+        tty.warn("Using deprecated format for configuring install_tree")
+        root = install_tree
+
+        # construct projection from previous values for backwards compatibility
+        all_projection = spack.config.get(
+            'config:install_path_scheme',
+            dir_layout.default_projections['all'])
+
+        projections = {'all': all_projection}
+    else:
+        root = install_tree.get('root', default_root)
+        root = spack.util.path.canonicalize_path(root)
+
+        projections = install_tree.get(
+            'projections', dir_layout.default_projections)
+
+        path_scheme = spack.config.get('config:install_path_scheme', None)
+        if path_scheme:
+            tty.warn("Deprecated config value 'install_path_scheme' ignored"
+                     " when using new install_tree syntax")
+
+    return Store(root, projections,
                  spack.config.get('config:install_hash_length'))
 
 

lib/spack/spack/test/cmd/config.py

@@ -2,8 +2,8 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+import functools
 import os
-
 import pytest
 
 import llnl.util.filesystem as fs
@@ -16,26 +16,40 @@
 env = spack.main.SpackCommand('env')
 
 
+def _create_config(scope=None, data={}, section='packages'):
+    scope = scope or spack.config.default_modify_scope()
+    cfg_file = spack.config.config.get_config_filename(scope, section)
+    with open(cfg_file, 'w') as f:
+        syaml.dump(data, stream=f)
+    return cfg_file
+
+
 @pytest.fixture()
 def packages_yaml_v015(mutable_config):
     """Create a packages.yaml in the old format"""
-    def _create(scope=None):
+    old_data = {
-        old_data = {
+        'packages': {
-            'packages': {
+            'cmake': {
-                'cmake': {
+                'paths': {'cmake@3.14.0': '/usr'}
-                    'paths': {'cmake@3.14.0': '/usr'}
+            },
-                },
+            'gcc': {
-                'gcc': {
+                'modules': {'gcc@8.3.0': 'gcc-8'}
-                    'modules': {'gcc@8.3.0': 'gcc-8'}
-                }
             }
         }
-        scope = scope or spack.config.default_modify_scope()
+    }
-        cfg_file = spack.config.config.get_config_filename(scope, 'packages')
+    return functools.partial(_create_config, data=old_data, section='packages')
-        with open(cfg_file, 'w') as f:
+
-            syaml.dump(old_data, stream=f)
+
-        return cfg_file
+@pytest.fixture()
-    return _create
+def config_yaml_v015(mutable_config):
+    """Create a packages.yaml in the old format"""
+    old_data = {
+        'config': {
+            'install_tree': '/fake/path',
+            'install_path_scheme': '{name}-{version}',
+        }
+    }
+    return functools.partial(_create_config, data=old_data, section='config')
 
 
 def test_get_config_scope(mock_low_high_config):
@@ -469,7 +483,16 @@ def test_config_update_packages(packages_yaml_v015):
 
     # Check the entries have been transformed
     data = spack.config.get('packages')
-    check_update(data)
+    check_packages_updated(data)
+
+
+def test_config_update_config(config_yaml_v015):
+    config_yaml_v015()
+    config('update', '-y', 'config')
+
+    # Check the entires have been transformed
+    data = spack.config.get('config')
+    check_config_updated(data)
 
 
 def test_config_update_not_needed(mutable_config):
@@ -543,7 +566,7 @@ def test_updating_multiple_scopes_at_once(packages_yaml_v015):
 
     for scope in ('user', 'site'):
         data = spack.config.get('packages', scope=scope)
-        check_update(data)
+        check_packages_updated(data)
 
 
 @pytest.mark.regression('18031')
@@ -603,7 +626,7 @@ def test_config_update_works_for_empty_paths(mutable_config):
     assert '[backup=' in output
 
 
-def check_update(data):
+def check_packages_updated(data):
     """Check that the data from the packages_yaml_v015
     has been updated.
     """
@@ -615,3 +638,9 @@ def check_update(data):
     externals = data['gcc']['externals']
     assert {'spec': 'gcc@8.3.0', 'modules': ['gcc-8']} in externals
     assert 'modules' not in data['gcc']
+
+
+def check_config_updated(data):
+    assert isinstance(data['install_tree'], dict)
+    assert data['install_tree']['root'] == '/fake/path'
+    assert data['install_tree']['projections'] == {'all': '{name}-{version}'}

lib/spack/spack/test/config.py

@@ -29,16 +29,16 @@
 # sample config data
 config_low = {
     'config': {
-        'install_tree': 'install_tree_path',
+        'install_tree': {'root': 'install_tree_path'},
         'build_stage': ['path1', 'path2', 'path3']}}
 
 config_override_all = {
     'config:': {
-        'install_tree:': 'override_all'}}
+        'install_tree:': {'root': 'override_all'}}}
 
 config_override_key = {
     'config': {
-        'install_tree:': 'override_key'}}
+        'install_tree:': {'root': 'override_key'}}}
 
 config_merge_list = {
     'config': {
@@ -391,7 +391,9 @@ def test_read_config_override_all(mock_low_high_config, write_config_file):
     write_config_file('config', config_low, 'low')
     write_config_file('config', config_override_all, 'high')
     assert spack.config.get('config') == {
-        'install_tree': 'override_all'
+        'install_tree': {
+            'root': 'override_all'
+        }
     }
 
 
@@ -399,7 +401,9 @@ def test_read_config_override_key(mock_low_high_config, write_config_file):
     write_config_file('config', config_low, 'low')
     write_config_file('config', config_override_key, 'high')
     assert spack.config.get('config') == {
-        'install_tree': 'override_key',
+        'install_tree': {
+            'root': 'override_key'
+        },
         'build_stage': ['path1', 'path2', 'path3']
     }
 
@@ -408,7 +412,9 @@ def test_read_config_merge_list(mock_low_high_config, write_config_file):
     write_config_file('config', config_low, 'low')
     write_config_file('config', config_merge_list, 'high')
     assert spack.config.get('config') == {
-        'install_tree': 'install_tree_path',
+        'install_tree': {
+            'root': 'install_tree_path'
+        },
         'build_stage': ['patha', 'pathb', 'path1', 'path2', 'path3']
     }
 
@@ -417,7 +423,9 @@ def test_read_config_override_list(mock_low_high_config, write_config_file):
     write_config_file('config', config_low, 'low')
     write_config_file('config', config_override_list, 'high')
     assert spack.config.get('config') == {
-        'install_tree': 'install_tree_path',
+        'install_tree': {
+            'root': 'install_tree_path'
+        },
         'build_stage': config_override_list['config']['build_stage:']
     }
 
@@ -426,7 +434,7 @@ def test_internal_config_update(mock_low_high_config, write_config_file):
     write_config_file('config', config_low, 'low')
 
     before = mock_low_high_config.get('config')
-    assert before['install_tree'] == 'install_tree_path'
+    assert before['install_tree']['root'] == 'install_tree_path'
 
     # add an internal configuration scope
     scope = spack.config.InternalConfigScope('command_line')
@@ -435,12 +443,12 @@ def test_internal_config_update(mock_low_high_config, write_config_file):
     mock_low_high_config.push_scope(scope)
 
     command_config = mock_low_high_config.get('config', scope='command_line')
-    command_config['install_tree'] = 'foo/bar'
+    command_config['install_tree'] = {'root': 'foo/bar'}
 
     mock_low_high_config.set('config', command_config, scope='command_line')
 
     after = mock_low_high_config.get('config')
-    assert after['install_tree'] == 'foo/bar'
+    assert after['install_tree']['root'] == 'foo/bar'
 
 
 def test_internal_config_filename(mock_low_high_config, write_config_file):
@@ -714,12 +722,13 @@ def test_immutable_scope(tmpdir):
     with open(config_yaml, 'w') as f:
         f.write("""\
 config:
-    install_tree: dummy_tree_value
+    install_tree:
+      root: dummy_tree_value
 """)
     scope = spack.config.ImmutableConfigScope('test', str(tmpdir))
 
     data = scope.get_section('config')
-    assert data['config']['install_tree'] == 'dummy_tree_value'
+    assert data['config']['install_tree'] == {'root': 'dummy_tree_value'}
 
     with pytest.raises(spack.config.ConfigError):
         scope.write_section('config')

lib/spack/spack/test/config_values.py

@@ -6,9 +6,11 @@
 import spack.spec
 
 
-def test_set_install_hash_length(mock_packages, mutable_config, monkeypatch):
+def test_set_install_hash_length(mock_packages, mutable_config, monkeypatch,
+                                 tmpdir):
     # spack.store.layout caches initial config values, so we monkeypatch
     mutable_config.set('config:install_hash_length', 5)
+    mutable_config.set('config:install_tree', {'root': str(tmpdir)})
     monkeypatch.setattr(spack.store, 'store', spack.store._store())
 
     spec = spack.spec.Spec('libelf').concretized()
@@ -28,10 +30,14 @@ def test_set_install_hash_length(mock_packages, mutable_config, monkeypatch):
 
 
 def test_set_install_hash_length_upper_case(mock_packages, mutable_config,
-                                            monkeypatch):
+                                            monkeypatch, tmpdir):
     # spack.store.layout caches initial config values, so we monkeypatch
-    mutable_config.set('config:install_path_scheme', '{name}-{HASH}')
     mutable_config.set('config:install_hash_length', 5)
+    mutable_config.set(
+        'config:install_tree',
+        {'root': str(tmpdir),
+         'projections': {'all': '{name}-{HASH}'}}
+    )
     monkeypatch.setattr(spack.store, 'store', spack.store._store())
 
     spec = spack.spec.Spec('libelf').concretized()

lib/spack/spack/test/data/config/config.yaml

@@ -1,5 +1,6 @@
 config:
-  install_tree: $spack/opt/spack
+  install_tree:
+    root: $spack/opt/spack
   template_dirs:
   - $spack/share/spack/templates
   - $spack/lib/spack/spack/test/data/templates

lib/spack/spack/test/directory_layout.py

@@ -44,9 +44,9 @@ def test_yaml_directory_layout_parameters(tmpdir, config):
         "{name}-{version}-{hash}"))
 
     # Test hash_length parameter works correctly
-    layout_10 = YamlDirectoryLayout(str(tmpdir), hash_len=10)
+    layout_10 = YamlDirectoryLayout(str(tmpdir), hash_length=10)
     path_10 = layout_10.relative_path_for_spec(spec)
-    layout_7 = YamlDirectoryLayout(str(tmpdir), hash_len=7)
+    layout_7 = YamlDirectoryLayout(str(tmpdir), hash_length=7)
     path_7 = layout_7.relative_path_for_spec(spec)
 
     assert(len(path_default) - len(path_10) == 22)
@@ -54,32 +54,34 @@ def test_yaml_directory_layout_parameters(tmpdir, config):
 
     # Test path_scheme
     arch, compiler, package7 = path_7.split('/')
-    scheme_package7 = "{name}-{version}-{hash:7}"
+    projections_package7 = {'all': "{name}-{version}-{hash:7}"}
     layout_package7 = YamlDirectoryLayout(str(tmpdir),
-                                          path_scheme=scheme_package7)
+                                          projections=projections_package7)
     path_package7 = layout_package7.relative_path_for_spec(spec)
 
     assert(package7 == path_package7)
 
-    # Test separation of architecture
+    # Test separation of architecture or namespace
-    arch_scheme_package = "{architecture.platform}/{architecture.target}/{architecture.os}/{name}/{version}/{hash:7}"   # NOQA: ignore=E501
+    spec2 = Spec('libelf').concretized()
-    layout_arch_package = YamlDirectoryLayout(str(tmpdir),
-                                              path_scheme=arch_scheme_package)
-    arch_path_package = layout_arch_package.relative_path_for_spec(spec)
-    assert(arch_path_package == spec.format(arch_scheme_package))
 
-    # Test separation of namespace
+    arch_scheme = "{architecture.platform}/{architecture.target}/{architecture.os}/{name}/{version}/{hash:7}"   # NOQA: ignore=E501
-    ns_scheme_package = "${ARCHITECTURE}/${NAMESPACE}/${PACKAGE}-${VERSION}-${HASH:7}"   # NOQA: ignore=E501
+    ns_scheme = "${ARCHITECTURE}/${NAMESPACE}/${PACKAGE}-${VERSION}-${HASH:7}"   # NOQA: ignore=E501
-    layout_ns_package = YamlDirectoryLayout(str(tmpdir),
+    arch_ns_scheme_projections = {'all': arch_scheme,
-                                            path_scheme=ns_scheme_package)
+                                  'python': ns_scheme}
-    ns_path_package = layout_ns_package.relative_path_for_spec(spec)
+    layout_arch_ns = YamlDirectoryLayout(
-    assert(ns_path_package == spec.format(ns_scheme_package))
+        str(tmpdir), projections=arch_ns_scheme_projections)
+
+    arch_path_spec2 = layout_arch_ns.relative_path_for_spec(spec2)
+    assert(arch_path_spec2 == spec2.format(arch_scheme))
+
+    ns_path_spec = layout_arch_ns.relative_path_for_spec(spec)
+    assert(ns_path_spec == spec.format(ns_scheme))
 
     # Ensure conflicting parameters caught
     with pytest.raises(InvalidDirectoryLayoutParametersError):
         YamlDirectoryLayout(str(tmpdir),
-                            hash_len=20,
+                            hash_length=20,
-                            path_scheme=scheme_package7)
+                            projections=projections_package7)
 
 
 def test_read_and_write_spec(layout_and_dir, config, mock_packages):

lib/spack/spack/test/util/spack_yaml.py

@@ -13,7 +13,7 @@
 
 def get_config_line(pattern, lines):
     """Get a configuration line that matches a particular pattern."""
-    line = next((l for l in lines if re.search(pattern, l)), None)
+    line = next((x for x in lines if re.search(pattern, x)), None)
     assert line is not None, 'no such line!'
     return line
 
@@ -57,7 +57,7 @@ def test_config_blame_with_override(config):
     """check blame for an element from an override scope"""
     config_file = config.get_config_filename('site', 'config')
 
-    with spack.config.override('config:install_tree', 'foobar'):
+    with spack.config.override('config:install_tree', {'root': 'foobar'}):
         check_blame('install_tree', 'overrides')
 
         check_blame('source_cache', config_file, 11)