From fafff0c6c0142e62e0f6b65b1d53ea58feb7fc7a Mon Sep 17 00:00:00 2001
From: Greg Becker <becker33@llnl.gov>
Date: Thu, 12 Nov 2020 16:08:55 -0800
Subject: move sbang to unpadded install tree root (#19640)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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 | 7 ++
lib/spack/spack/hooks/sbang.py | 12 +++-
lib/spack/spack/schema/config.py | 3 +
lib/spack/spack/store.py | 143 +++++++++++++++++++++++++++++++--------
lib/spack/spack/test/config.py | 72 ++++++++++++++------
lib/spack/spack/test/sbang.py | 7 +-
lib/spack/spack/util/path.py | 50 +++++++-------
7 files changed, 212 insertions(+), 82 deletions(-)
diff --git a/etc/spack/defaults/config.yaml b/etc/spack/defaults/config.yaml
index aa4ee2bbc6..6c5e8697b1 100644
--- a/etc/spack/defaults/config.yaml
+++ b/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
diff --git a/lib/spack/spack/hooks/sbang.py b/lib/spack/spack/hooks/sbang.py
index d3a6174948..687cf651f4 100644
--- a/lib/spack/spack/hooks/sbang.py
+++ b/lib/spack/spack/hooks/sbang.py
@@ -16,7 +16,6 @@ import spack.modules
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 @@ else:
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."""
diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py
index a21db3d048..7e422eb5b3 100644
--- a/lib/spack/spack/schema/config.py
+++ b/lib/spack/spack/schema/config.py
@@ -28,6 +28,9 @@ properties = {
'type': 'object',
'properties': union_dicts(
{'root': {'type': 'string'}},
+ {'padded_length': {'oneOf': [
+ {'type': 'integer', 'minimum': 0},
+ {'type': 'boolean'}]}},
spack.schema.projections.properties,
),
},
diff --git a/lib/spack/spack/store.py b/lib/spack/spack/store.py
index 75c3969362..779a5531a4 100644
--- a/lib/spack/spack/store.py
+++ b/lib/spack/spack/store.py
@@ -24,6 +24,7 @@ configuration.
"""
import os
+import re
import six
import llnl.util.lang
@@ -33,10 +34,100 @@ import spack.paths
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 @@ class Store(object):
def _store():
"""Get the singleton store instance."""
- install_tree = spack.config.get('config:install_tree', {})
-
- if isinstance(install_tree, six.string_types):
- 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'))
+ config_dict = spack.config.get('config')
+ root, unpadded_root, projections = parse_install_tree(config_dict)
+ hash_length = spack.config.get('config:install_hash_length')
+ return Store(root=root,
+ unpadded_root=unpadded_root,
+ projections=projections,
+ hash_length=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)
diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py
index b852f4777e..f1081be737 100644
--- a/lib/spack/spack/test/config.py
+++ b/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):
- max_system_path = spack_path.get_system_path_max()
- expected_length = (max_system_path -
- spack_path.SPACK_MAX_INSTALL_PATH_LENGTH)
-
- install_path = spack_path.canonicalize_path('/foo/bar/${padding}/baz')
-
- assert spack_path.SPACK_PATH_PADDING_CHARS in install_path
- assert len(install_path) == expected_length
-
- install_path = spack_path.canonicalize_path('/foo/bar/baz/gah/$padding')
-
- assert spack_path.SPACK_PATH_PADDING_CHARS in install_path
- assert len(install_path) == expected_length
-
- i_path = spack_path.canonicalize_path('/foo/$padding:10')
- i_expect = os.path.join('/foo', spack_path.SPACK_PATH_PADDING_CHARS[:10])
- assert i_path == i_expect
-
- 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
+PAD_STRING = spack.util.path.SPACK_PATH_PADDING_CHARS
+MAX_PATH_LEN = spack.util.path.get_system_path_max()
+MAX_PADDED_LEN = MAX_PATH_LEN - spack.util.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]
+
+
+@pytest.mark.parametrize('config_settings,expected', [
+ ([], [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
+
+ # 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)
+
+ config_dict = mutable_config.get('config')
+ root, unpadded_root, projections = spack.store.parse_install_tree(
+ config_dict)
+
+ assert root == expected_root
+ assert unpadded_root == expected_unpadded_root
+ assert projections == expected_proj
def test_read_config(mock_low_high_config, write_config_file):
diff --git a/lib/spack/spack/test/sbang.py b/lib/spack/spack/test/sbang.py
index ca7d2a6fe6..9307d2bc4b 100644
--- a/lib/spack/spack/test/sbang.py
+++ b/lib/spack/spack/test/sbang.py
@@ -40,7 +40,8 @@ php_in_text = ("line\n") * 100 + "php\n" + ("line\n" * 100)
php_line_patched = "<?php #!/this/" + ('x' * too_long) + "/is/php\n"
php_line_patched2 = "?>\n"
-last_line = "last!\n"
+sbang_line = '#!/bin/sh %s/bin/sbang\n' % spack.store.store.unpadded_root
+last_line = "last!\n"
@pytest.fixture
@@ -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()
diff --git a/lib/spack/spack/util/path.py b/lib/spack/spack/util/path.py
index 8bcf598882..182f93c585 100644
--- a/lib/spack/spack/util/path.py
+++ b/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):
- """Subtitute in padding of os-specific length. The intent is to leave
- SPACK_MAX_INSTALL_PATH_LENGTH characters available for parts of the
- path generated by spack. This is to allow for not-completely-known
- lengths of things like os/arch, compiler, package name, hash length,
- etc.
- """
- padding_regex = re.compile(r'(\$[\w\d\:]+\b|\$\{[\w\d\:]+\})')
- m = padding_regex.search(path)
- if m and m.group(0).strip('${}').startswith('padding'):
- padding_part = m.group(0)
- len_pad_part = len(m.group(0))
- p_match = re.search(r'\:(\d+)', padding_part)
- if p_match:
- computed_padding = _get_padding_string(int(p_match.group(1)))
- else:
- # Take whatever has been computed/substituted so far and add some
- # room
- 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
+def add_padding(path, length):
+ """Add padding subdirectories to path until total is length characters
+
+ Returns the padded path. If path is length - 1 or more characters long,
+ returns path. If path is length - 1 characters, warns that it is not
+ padding to length
+
+ Assumes path does not have a trailing path separator"""
+ padding_length = length - len(path)
+ if padding_length == 1:
+ # The only 1 character addition we can make to a path is `/`
+ # Spack internally runs normpath, so `foo/` will be reduced to `foo`
+ # Even if we removed this behavior from Spack, the user could normalize
+ # the path, removing the additional `/`.
+ # Because we can't expect one character of padding to show up in the
+ # resulting binaries, we warn the user and do not pad by a single char
+ tty.warn("Cannot pad path by exactly one character.")
+ if padding_length <= 0:
+ 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
--
cgit v1.2.3-70-g09d2