move sbang to unpadded install tree root (#19640)

Since #11598 sbang has been installed within the install_tree. This doesn’t play
nicely with install_tree padding, since sbang can’t do its job if it is installed in a
long path (this is the whole point of sbang).

This PR changes the padding specification.  Instead of $padding inside paths,
we now have a separate `padding:` field in the `install_tree` configuration.

Previously, the `install_tree` looked like this:

```
    /path/to/opt/spack_padding_padding_padding_padding_padding/
        bin/
            sbang
        .spack-db/
            ...
        linux-rhel7-x86_64/
            ...
```

```
This PR updates things to look like this:

    /path/to/opt/
        bin/
            sbang
        spack_padding_padding_padding_padding_padding/
            .spack-db/
                ...
            linux-rhel7-x86_64/
                ...

So padding is added at the start of all install prefixes *within* the unpadded
root.  The database and all installations still go under the padded root.

This ensures that `sbang` is in the shorted possible path while also allowing
us to make long paths for relocatable binaries.

etc/spack/defaults/config.yaml

@@ -20,6 +20,13 @@ config:
     root: $spack/opt/spack
     projections:
       all: "${ARCHITECTURE}/${COMPILERNAME}-${COMPILERVER}/${PACKAGE}-${VERSION}-${HASH}"
+    # install_tree can include an optional padded length (int or boolean)
+    # default is False (do not pad)
+    # if padded_length is True, Spack will pad as close to the system max path
+    # length as possible
+    # if padded_length is an integer, Spack will pad to that many characters,
+    # assuming it is higher than the length of the install_tree root.
+    # padded_length: 128
 
 
   # Locations where templates should be found

lib/spack/spack/hooks/sbang.py

@@ -16,7 +16,6 @@
 import spack.paths
 import spack.store
 
-
 #: OS-imposed character limit for shebang line: 127 for Linux; 511 for Mac.
 #: Different Linux distributions have different limits, but 127 is the
 #: smallest among all modern versions.
@@ -28,7 +27,12 @@
 
 def sbang_install_path():
     """Location sbang should be installed within Spack's ``install_tree``."""
-    return os.path.join(spack.store.layout.root, "bin", "sbang")
+    sbang_root = str(spack.store.unpadded_root)
+    install_path = os.path.join(sbang_root, "bin", "sbang")
+    if len(install_path) > shebang_limit:
+        raise SbangPathError(
+            'Install tree root is too long. Spack cannot patch shebang lines.')
+    return install_path
 
 
 def sbang_shebang_line():
@@ -164,3 +168,7 @@ def post_install(spec):
 
     for directory, _, filenames in os.walk(spec.prefix):
         filter_shebangs_in_directory(directory, filenames)
+
+
+class SbangPathError(spack.error.SpackError):
+    """Raised when the install tree root is too long for sbang to work."""

lib/spack/spack/schema/config.py

@@ -28,6 +28,9 @@
                         'type': 'object',
                         'properties': union_dicts(
                             {'root': {'type': 'string'}},
+                            {'padded_length': {'oneOf': [
+                                {'type': 'integer', 'minimum': 0},
+                                {'type': 'boolean'}]}},
                             spack.schema.projections.properties,
                         ),
                     },

lib/spack/spack/store.py

@@ -24,6 +24,7 @@
 
 """
 import os
+import re
 import six
 
 import llnl.util.lang
@@ -33,10 +34,100 @@
 import spack.config
 import spack.util.path
 import spack.database
-import spack.directory_layout as dir_layout
+import spack.directory_layout
+
 
 #: default installation root, relative to the Spack install path
-default_root = os.path.join(spack.paths.opt_path, 'spack')
+default_install_tree_root = os.path.join(spack.paths.opt_path, 'spack')
+
+
+def parse_install_tree(config_dict):
+    """Parse config settings and return values relevant to the store object.
+
+    Arguments:
+        config_dict (dict): dictionary of config values, as returned from
+            spack.config.get('config')
+
+    Returns:
+        (tuple): triple of the install tree root, the unpadded install tree
+            root (before padding was applied), and the projections for the
+            install tree
+
+    Encapsulate backwards compatibility capabilities for install_tree
+    and deprecated values that are now parsed as part of install_tree.
+    """
+    # The following two configs are equivalent, the first being the old format
+    # and the second the new format. The new format is also more flexible.
+
+    # config:
+    #   install_tree: /path/to/root$padding:128
+    #   install_path_scheme: '{name}-{version}'
+
+    # config:
+    #   install_tree:
+    #     root: /path/to/root
+    #     padding: 128
+    #     projections:
+    #       all: '{name}-{version}'
+
+    install_tree = config_dict.get('install_tree', {})
+
+    padded_length = False
+    if isinstance(install_tree, six.string_types):
+        tty.warn("Using deprecated format for configuring install_tree")
+        unpadded_root = install_tree
+        unpadded_root = spack.util.path.canonicalize_path(unpadded_root)
+        # construct projection from previous values for backwards compatibility
+        all_projection = config_dict.get(
+            'install_path_scheme',
+            spack.directory_layout.default_projections['all'])
+
+        projections = {'all': all_projection}
+    else:
+        unpadded_root = install_tree.get('root', default_install_tree_root)
+        unpadded_root = spack.util.path.canonicalize_path(unpadded_root)
+
+        padded_length = install_tree.get('padded_length', False)
+        if padded_length is True:
+            padded_length = spack.util.path.get_system_path_max()
+            padded_length -= spack.util.path.SPACK_MAX_INSTALL_PATH_LENGTH
+
+        projections = install_tree.get(
+            'projections', spack.directory_layout.default_projections)
+
+        path_scheme = config_dict.get('install_path_scheme', None)
+        if path_scheme:
+            tty.warn("Deprecated config value 'install_path_scheme' ignored"
+                     " when using new install_tree syntax")
+
+    # Handle backwards compatibility for padding
+    old_pad = re.search(r'\$padding(:\d+)?|\${padding(:\d+)?}', unpadded_root)
+    if old_pad:
+        if padded_length:
+            msg = "Ignoring deprecated padding option in install_tree root "
+            msg += "because new syntax padding is present."
+            tty.warn(msg)
+        else:
+            unpadded_root = unpadded_root.replace(old_pad.group(0), '')
+            if old_pad.group(1) or old_pad.group(2):
+                length_group = 2 if '{' in old_pad.group(0) else 1
+                padded_length = int(old_pad.group(length_group)[1:])
+            else:
+                padded_length = spack.util.path.get_system_path_max()
+                padded_length -= spack.util.path.SPACK_MAX_INSTALL_PATH_LENGTH
+
+    unpadded_root = unpadded_root.rstrip(os.path.sep)
+
+    if padded_length:
+        root = spack.util.path.add_padding(unpadded_root, padded_length)
+        if len(root) != padded_length:
+            msg = "Cannot pad %s to %s characters." % (root, padded_length)
+            msg += " It is already %s characters long" % len(root)
+            tty.warn(msg)
+    else:
+        root = unpadded_root
+
+    return (root, unpadded_root, projections)
 
 
 class Store(object):
@@ -52,17 +143,23 @@ class Store(object):
 
     Args:
         root (str): path to the root of the install tree
+        unpadded_root (str): path to the root of the install tree without
+            padding; the sbang script has to be installed here to work with
+            padded roots
         path_scheme (str): expression according to guidelines in
             ``spack.util.path`` that describes how to construct a path to
             a package prefix in this store
         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, projections=None, hash_length=None):
+    def __init__(
+            self, root, unpadded_root=None, projections=None, hash_length=None
+    ):
         self.root = root
+        self.unpadded_root = unpadded_root or root
         self.db = spack.database.Database(
             root, upstream_dbs=retrieve_upstream_dbs())
-        self.layout = dir_layout.YamlDirectoryLayout(
+        self.layout = spack.directory_layout.YamlDirectoryLayout(
             root, projections=projections, hash_length=hash_length)
 
     def reindex(self):
@@ -72,32 +169,13 @@ def reindex(self):
 
 def _store():
     """Get the singleton store instance."""
-    install_tree = spack.config.get('config:install_tree', {})
+    config_dict = spack.config.get('config')
-
+    root, unpadded_root, projections = parse_install_tree(config_dict)
-    if isinstance(install_tree, six.string_types):
+    hash_length = spack.config.get('config:install_hash_length')
-        tty.warn("Using deprecated format for configuring install_tree")
+    return Store(root=root,
-        root = install_tree
+                 unpadded_root=unpadded_root,
-
+                 projections=projections,
-        # construct projection from previous values for backwards compatibility
+                 hash_length=hash_length)
-        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'))
 
 
 #: Singleton store instance
@@ -108,6 +186,10 @@ def _store_root():
     return store.root
 
 
+def _store_unpadded_root():
+    return store.unpadded_root
+
+
 def _store_db():
     return store.db
 
@@ -118,6 +200,7 @@ def _store_layout():
 
 # convenience accessors for parts of the singleton store
 root = llnl.util.lang.LazyReference(_store_root)
+unpadded_root = llnl.util.lang.LazyReference(_store_unpadded_root)
 db = llnl.util.lang.LazyReference(_store_db)
 layout = llnl.util.lang.LazyReference(_store_layout)
 

lib/spack/spack/test/config.py

@@ -358,28 +358,56 @@ def test_substitute_tempdir(mock_low_high_config):
     )
 
 
-def test_substitute_padding(mock_low_high_config):
+PAD_STRING = spack.util.path.SPACK_PATH_PADDING_CHARS
-    max_system_path = spack_path.get_system_path_max()
+MAX_PATH_LEN = spack.util.path.get_system_path_max()
-    expected_length = (max_system_path -
+MAX_PADDED_LEN = MAX_PATH_LEN - spack.util.path.SPACK_MAX_INSTALL_PATH_LENGTH
-                       spack_path.SPACK_MAX_INSTALL_PATH_LENGTH)
+reps = [PAD_STRING for _ in range((MAX_PADDED_LEN // len(PAD_STRING) + 1) + 2)]
+full_padded_string = os.path.join(
+    '/path', os.path.sep.join(reps))[:MAX_PADDED_LEN]
 
-    install_path = spack_path.canonicalize_path('/foo/bar/${padding}/baz')
 
-    assert spack_path.SPACK_PATH_PADDING_CHARS in install_path
+@pytest.mark.parametrize('config_settings,expected', [
-    assert len(install_path) == expected_length
+    ([], [None, None, None]),
+    ([['config:install_tree:root', '/path']], ['/path', None, None]),
+    ([['config:install_tree', '/path']], ['/path', None, None]),
+    ([['config:install_tree:projections', {'all': '{name}'}]],
+     [None, None, {'all': '{name}'}]),
+    ([['config:install_path_scheme', '{name}']],
+     [None, None, {'all': '{name}'}]),
+    ([['config:install_tree:root', '/path'],
+      ['config:install_tree:padded_length', 11]],
+     [os.path.join('/path', PAD_STRING[:5]), '/path', None]),
+    ([['config:install_tree:root', '/path/$padding:11']],
+     [os.path.join('/path', PAD_STRING[:5]), '/path', None]),
+    ([['config:install_tree', '/path/${padding:11}']],
+     [os.path.join('/path', PAD_STRING[:5]), '/path', None]),
+    ([['config:install_tree:padded_length', False]], [None, None, None]),
+    ([['config:install_tree:padded_length', True],
+      ['config:install_tree:root', '/path']],
+     [full_padded_string, '/path', None]),
+    ([['config:install_tree:', '/path$padding']],
+     [full_padded_string, '/path', None]),
+    ([['config:install_tree:', '/path/${padding}']],
+     [full_padded_string, '/path', None]),
+])
+def test_parse_install_tree(config_settings, expected, mutable_config):
+    expected_root = expected[0] or spack.store.default_install_tree_root
+    expected_unpadded_root = expected[1] or expected_root
+    expected_proj = expected[2] or spack.directory_layout.default_projections
 
-    install_path = spack_path.canonicalize_path('/foo/bar/baz/gah/$padding')
+    # config settings is a list of 2-element lists, [path, value]
+    # where path is a config path and value is the value to set at that path
+    # these can be "splatted" in as the arguments to config.set
+    for config_setting in config_settings:
+        mutable_config.set(*config_setting)
 
-    assert spack_path.SPACK_PATH_PADDING_CHARS in install_path
+    config_dict = mutable_config.get('config')
-    assert len(install_path) == expected_length
+    root, unpadded_root, projections = spack.store.parse_install_tree(
+        config_dict)
 
-    i_path = spack_path.canonicalize_path('/foo/$padding:10')
+    assert root == expected_root
-    i_expect = os.path.join('/foo', spack_path.SPACK_PATH_PADDING_CHARS[:10])
+    assert unpadded_root == expected_unpadded_root
-    assert i_path == i_expect
+    assert projections == expected_proj
-
-    i_path = spack_path.canonicalize_path('/foo/${padding:20}')
-    i_expect = os.path.join('/foo', spack_path.SPACK_PATH_PADDING_CHARS[:20])
-    assert i_path == i_expect
 
 
 def test_read_config(mock_low_high_config, write_config_file):

lib/spack/spack/test/sbang.py

@@ -40,6 +40,7 @@
 php_line_patched = "<?php #!/this/" + ('x' * too_long) + "/is/php\n"
 php_line_patched2 = "?>\n"
 
+sbang_line = '#!/bin/sh %s/bin/sbang\n' % spack.store.store.unpadded_root
 last_line  = "last!\n"
 
 
@@ -191,7 +192,7 @@ def test_shebang_handles_non_writable_files(script_dir, sbang_line):
 def check_sbang_installation():
     sbang_path = sbang.sbang_install_path()
     sbang_bin_dir = os.path.dirname(sbang_path)
-    assert sbang_path.startswith(spack.store.layout.root)
+    assert sbang_path.startswith(spack.store.store.unpadded_root)
 
     assert os.path.exists(sbang_path)
     assert fs.is_exe(sbang_path)
@@ -207,7 +208,7 @@ def test_install_sbang(install_mockery):
     sbang_path = sbang.sbang_install_path()
     sbang_bin_dir = os.path.dirname(sbang_path)
 
-    assert sbang_path.startswith(spack.store.layout.root)
+    assert sbang_path.startswith(spack.store.store.unpadded_root)
     assert not os.path.exists(sbang_bin_dir)
 
     sbang.install_sbang()

lib/spack/spack/util/path.py

@@ -103,36 +103,36 @@ def _get_padding_string(length):
     return os.path.sep.join(reps_list)
 
 
-def _add_computed_padding(path):
+def add_padding(path, length):
-    """Subtitute in padding of os-specific length.  The intent is to leave
+    """Add padding subdirectories to path until total is length characters
-    SPACK_MAX_INSTALL_PATH_LENGTH characters available for parts of the
+
-    path generated by spack.  This is to allow for not-completely-known
+    Returns the padded path. If path is length - 1 or more characters long,
-    lengths of things like os/arch, compiler, package name, hash length,
+    returns path. If path is length - 1 characters, warns that it is not
-    etc.
+    padding to length
-    """
+
-    padding_regex = re.compile(r'(\$[\w\d\:]+\b|\$\{[\w\d\:]+\})')
+    Assumes path does not have a trailing path separator"""
-    m = padding_regex.search(path)
+    padding_length = length - len(path)
-    if m and m.group(0).strip('${}').startswith('padding'):
+    if padding_length == 1:
-        padding_part = m.group(0)
+        # The only 1 character addition we can make to a path is `/`
-        len_pad_part = len(m.group(0))
+        # Spack internally runs normpath, so `foo/` will be reduced to `foo`
-        p_match = re.search(r'\:(\d+)', padding_part)
+        # Even if we removed this behavior from Spack, the user could normalize
-        if p_match:
+        # the path, removing the additional `/`.
-            computed_padding = _get_padding_string(int(p_match.group(1)))
+        # Because we can't expect one character of padding to show up in the
-        else:
+        # resulting binaries, we warn the user and do not pad by a single char
-            # Take whatever has been computed/substituted so far and add some
+        tty.warn("Cannot pad path by exactly one character.")
-            # room
+    if padding_length <= 0:
-            path_len = len(path) - len_pad_part + SPACK_MAX_INSTALL_PATH_LENGTH
-            system_max_path = get_system_path_max()
-            needed_pad_len = system_max_path - path_len
-            computed_padding = _get_padding_string(needed_pad_len)
-        return padding_regex.sub(computed_padding, path)
         return path
 
+    # we subtract 1 from the padding_length to account for the path separator
+    # coming from os.path.join below
+    padding = _get_padding_string(padding_length - 1)
+
+    return os.path.join(path, padding)
+
 
 def canonicalize_path(path):
     """Same as substitute_path_variables, but also take absolute path."""
     path = substitute_path_variables(path)
     path = os.path.abspath(path)
-    path = _add_computed_padding(path)
 
     return path