Improve the coverage of spack.relocate (#15654)

This PR introduces trivial refactoring in:
- `get_existing_elf_rpaths`
- `get_relative_elf_rpaths`
- `get_normalized_elf_rpaths`
- `set_placeholder`

mainly to be more consistent with practices used in other 
parts of the code and to simplify functions locally. It also 
adds or reworks unit tests for these functions and extends 
their docstrings.

Co-authored-by: Patrick Gartung <gartung@fnal.gov>
Co-authored-by: Peter J. Scheibel <scheibel1@llnl.gov>

lib/spack/spack/relocate.py

@@ -97,70 +97,102 @@ def _patchelf():
     return exe_path if os.path.exists(exe_path) else None
 
 
-def get_existing_elf_rpaths(path_name):
+def _elf_rpaths_for(path):
-    """
+    """Return the RPATHs for an executable or a library.
-    Return the RPATHS returned by patchelf --print-rpath path_name
+
-    as a list of strings.
+    The RPATHs are obtained by ``patchelf --print-rpath PATH``.
+
+    Args:
+        path (str): full path to the executable or library
+
+    Return:
+        RPATHs as a list of strings.
     """
+    # If we're relocating patchelf itself, use it
+    patchelf_path = path if path.endswith("/bin/patchelf") else _patchelf()
+    patchelf = executable.Executable(patchelf_path)
 
-    # if we're relocating patchelf itself, use it
+    output = ''
-
-    if path_name.endswith("/bin/patchelf"):
-        patchelf = executable.Executable(path_name)
-    else:
-        patchelf = executable.Executable(_patchelf())
-
-    rpaths = list()
     try:
-        output = patchelf('--print-rpath', '%s' %
+        output = patchelf('--print-rpath', path, output=str, error=str)
-                          path_name, output=str, error=str)
+        output = output.strip('\n')
-        rpaths = output.rstrip('\n').split(':')
     except executable.ProcessError as e:
-        msg = 'patchelf --print-rpath %s produced an error %s' % (path_name, e)
+        msg = 'patchelf --print-rpath {0} produced an error [{1}]'
-        tty.warn(msg)
+        tty.warn(msg.format(path, str(e)))
-    return rpaths
+
+    return output.split(':') if output else []
 
 
-def get_relative_elf_rpaths(path_name, orig_layout_root, orig_rpaths):
+def _make_relative(reference_file, path_root, paths):
+    """Return a list where any path in ``paths`` that starts with
+    ``path_root`` is made relative to the directory in which the
+    reference file is stored.
+
+    After a path is made relative it is prefixed with the ``$ORIGIN``
+    string.
+
+    Args:
+        reference_file (str): file from which the reference directory
+            is computed
+        path_root (str): root of the relative paths
+        paths: paths to be examined
+
+    Returns:
+        List of relative paths
     """
-    Replaces orig rpath with relative path from dirname(path_name) if an rpath
+    # Check prerequisites of the function
-    in orig_rpaths contains orig_layout_root. Prefixes $ORIGIN
+    msg = "{0} is not a file".format(reference_file)
-    to relative paths and returns replacement rpaths.
+    assert os.path.isfile(reference_file), msg
-    """
+
-    rel_rpaths = []
+    start_directory = os.path.dirname(reference_file)
-    for rpath in orig_rpaths:
+    pattern = re.compile(path_root)
-        if re.match(orig_layout_root, rpath):
+    relative_paths = []
-            rel = os.path.relpath(rpath, start=os.path.dirname(path_name))
+
-            rel_rpaths.append(os.path.join('$ORIGIN', '%s' % rel))
+    for path in paths:
-        else:
+        if pattern.match(path):
-            rel_rpaths.append(rpath)
+            rel = os.path.relpath(path, start=start_directory)
-    return rel_rpaths
+            path = os.path.join('$ORIGIN', rel)
+
+        relative_paths.append(path)
+
+    return relative_paths
 
 
-def get_normalized_elf_rpaths(orig_path_name, rel_rpaths):
+def _normalize_relative_paths(start_path, relative_paths):
+    """Normalize the relative paths with respect to the original path name
+    of the file (``start_path``).
+
+    The paths that are passed to this function existed or were relevant
+    on another filesystem, so os.path.abspath cannot be used.
+
+    A relative path may contain the signifier $ORIGIN. Assuming that
+    ``start_path`` is absolute, this implies that the relative path
+    (relative to start_path) should be replaced with an absolute path.
+
+    Args:
+        start_path (str): path from which the starting directory
+            is extracted
+        relative_paths (str): list of relative paths as obtained by a
+            call to :ref:`_make_relative`
+
+    Returns:
+        List of normalized paths
     """
-    Normalize the relative rpaths with respect to the original path name
+    normalized_paths = []
-    of the file. If the rpath starts with $ORIGIN replace $ORIGIN with the
+    pattern = re.compile(re.escape('$ORIGIN'))
-    dirname of the original path name and then normalize the rpath.
+    start_directory = os.path.dirname(start_path)
-    A dictionary mapping relativized rpaths to normalized rpaths is returned.
+
-    """
+    for path in relative_paths:
-    norm_rpaths = list()
+        if path.startswith('$ORIGIN'):
-    for rpath in rel_rpaths:
+            sub = pattern.sub(start_directory, path)
-        if rpath.startswith('$ORIGIN'):
+            path = os.path.normpath(sub)
-            sub = re.sub(re.escape('$ORIGIN'),
+        normalized_paths.append(path)
-                         os.path.dirname(orig_path_name),
+
-                         rpath)
+    return normalized_paths
-            norm = os.path.normpath(sub)
-            norm_rpaths.append(norm)
-        else:
-            norm_rpaths.append(rpath)
-    return norm_rpaths
 
 
-def set_placeholder(dirname):
+def _placeholder(dirname):
-    """
+    """String of  of @'s with same length of the argument"""
-    return string of @'s with same length
-    """
     return '@' * len(dirname)
 
 
@@ -592,7 +624,7 @@ def relocate_elf_binaries(path_names, old_layout_root, new_layout_root,
     rpath was in the old layout root, i.e. system paths are not replaced.
     """
     for path_name in path_names:
-        orig_rpaths = get_existing_elf_rpaths(path_name)
+        orig_rpaths = _elf_rpaths_for(path_name)
         new_rpaths = list()
         if rel:
             # get the file path in the old_prefix
@@ -600,13 +632,13 @@ def relocate_elf_binaries(path_names, old_layout_root, new_layout_root,
                                     path_name)
             # get the normalized rpaths in the old prefix using the file path
             # in the orig prefix
-            orig_norm_rpaths = get_normalized_elf_rpaths(orig_path_name,
+            orig_norm_rpaths = _normalize_relative_paths(orig_path_name,
                                                          orig_rpaths)
             # get the normalize rpaths in the new prefix
             norm_rpaths = elf_find_paths(orig_norm_rpaths, old_layout_root,
                                          prefix_to_prefix)
             # get the relativized rpaths in the new prefix
-            new_rpaths = get_relative_elf_rpaths(path_name, new_layout_root,
+            new_rpaths = _make_relative(path_name, new_layout_root,
                                         norm_rpaths)
             modify_elf_object(path_name, new_rpaths)
         else:
@@ -652,9 +684,9 @@ def make_elf_binaries_relative(cur_path_names, orig_path_names,
     Replace old RPATHs with paths relative to old_dir in binary files
     """
     for cur_path, orig_path in zip(cur_path_names, orig_path_names):
-        orig_rpaths = get_existing_elf_rpaths(cur_path)
+        orig_rpaths = _elf_rpaths_for(cur_path)
         if orig_rpaths:
-            new_rpaths = get_relative_elf_rpaths(orig_path, old_layout_root,
+            new_rpaths = _make_relative(orig_path, old_layout_root,
                                         orig_rpaths)
             modify_elf_object(cur_path, new_rpaths)
 
@@ -679,7 +711,7 @@ def relocate_links(linknames, old_layout_root, new_layout_root,
     link target is create by replacing the old install prefix with the new
     install prefix.
     """
-    placeholder = set_placeholder(old_layout_root)
+    placeholder = _placeholder(old_layout_root)
     link_names = [os.path.join(new_install_prefix, linkname)
                   for linkname in linknames]
     for link_name in link_names:
@@ -810,7 +842,7 @@ def file_is_relocatable(file, paths_to_relocate=None):
 
     if platform.system().lower() == 'linux':
         if m_subtype == 'x-executable' or m_subtype == 'x-sharedlib':
-            rpaths = ':'.join(get_existing_elf_rpaths(file))
+            rpaths = ':'.join(_elf_rpaths_for(file))
             set_of_strings.discard(rpaths)
     if platform.system().lower() == 'darwin':
         if m_subtype == 'x-mach-binary':

lib/spack/spack/test/packaging.py

@@ -25,11 +25,9 @@
 from spack.fetch_strategy import URLFetchStrategy, FetchStrategyComposite
 from spack.relocate import needs_binary_relocation, needs_text_relocation
 from spack.relocate import relocate_text, relocate_links
-from spack.relocate import get_relative_elf_rpaths
-from spack.relocate import get_normalized_elf_rpaths
 from spack.relocate import macho_make_paths_relative
 from spack.relocate import macho_make_paths_normal
-from spack.relocate import set_placeholder, macho_find_paths
+from spack.relocate import _placeholder, macho_find_paths
 from spack.relocate import file_is_relocatable
 
 
@@ -228,7 +226,7 @@ def test_relocate_links(tmpdir):
         old_install_prefix = os.path.join(
             '%s' % old_layout_root, 'debian6', 'test')
         old_binname = os.path.join(old_install_prefix, 'binfile')
-        placeholder = set_placeholder(old_layout_root)
+        placeholder = _placeholder(old_layout_root)
         re.sub(old_layout_root, placeholder, old_binname)
         filenames = ['link.ln', 'outsideprefix.ln']
         new_layout_root = os.path.join(
@@ -561,15 +559,3 @@ def test_macho_make_paths():
                    '/Users/Shared/spack/pkgB/libB.dylib',
                    '/usr/local/lib/libloco.dylib':
                    '/usr/local/lib/libloco.dylib'}
-
-
-def test_elf_paths():
-    out = get_relative_elf_rpaths(
-        '/usr/bin/test', '/usr',
-        ('/usr/lib', '/usr/lib64', '/opt/local/lib'))
-    assert out == ['$ORIGIN/../lib', '$ORIGIN/../lib64', '/opt/local/lib']
-
-    out = get_normalized_elf_rpaths(
-        '/usr/bin/test',
-        ['$ORIGIN/../lib', '$ORIGIN/../lib64', '/opt/local/lib'])
-    assert out == ['/usr/lib', '/usr/lib64', '/opt/local/lib']

lib/spack/spack/test/relocate.py

@@ -89,6 +89,20 @@ def do_install_mock(self, **kwargs):
     return expected_path
 
 
+@pytest.fixture()
+def mock_patchelf(tmpdir):
+    import jinja2
+
+    def _factory(output):
+        f = tmpdir.mkdir('bin').join('patchelf')
+        t = jinja2.Template('#!/bin/bash\n{{ output }}\n')
+        f.write(t.render(output=output))
+        f.chmod(0o755)
+        return str(f)
+
+    return _factory
+
+
 @pytest.mark.requires_executables(
     '/usr/bin/gcc', 'patchelf', 'strings', 'file'
 )
@@ -140,3 +154,43 @@ def test_file_is_relocatable_errors(tmpdir):
 def test_search_patchelf(expected_patchelf_path):
     current = spack.relocate._patchelf()
     assert current == expected_patchelf_path
+
+
+@pytest.mark.parametrize('patchelf_behavior,expected', [
+    ('echo ', []),
+    ('echo /opt/foo/lib:/opt/foo/lib64', ['/opt/foo/lib', '/opt/foo/lib64']),
+    ('exit 1', [])
+])
+def test_existing_rpaths(patchelf_behavior, expected, mock_patchelf):
+    # Here we are mocking an executable that is always called "patchelf"
+    # because that will skip the part where we try to build patchelf
+    # by ourselves. The executable will output some rpaths like
+    # `patchelf --print-rpath` would.
+    path = mock_patchelf(patchelf_behavior)
+    rpaths = spack.relocate._elf_rpaths_for(path)
+    assert rpaths == expected
+
+
+@pytest.mark.parametrize('start_path,path_root,paths,expected', [
+    ('/usr/bin/test', '/usr', ['/usr/lib', '/usr/lib64', '/opt/local/lib'],
+     ['$ORIGIN/../lib', '$ORIGIN/../lib64', '/opt/local/lib'])
+])
+def test_make_relative_paths(start_path, path_root, paths, expected):
+    relatives = spack.relocate._make_relative(start_path, path_root, paths)
+    assert relatives == expected
+
+
+@pytest.mark.parametrize('start_path,relative_paths,expected', [
+    # $ORIGIN will be replaced with os.path.dirname('usr/bin/test')
+    # and then normalized
+    ('/usr/bin/test',
+     ['$ORIGIN/../lib', '$ORIGIN/../lib64', '/opt/local/lib'],
+     ['/usr/lib', '/usr/lib64', '/opt/local/lib']),
+    # Relative path without $ORIGIN
+    ('/usr/bin/test', ['../local/lib'], ['../local/lib']),
+])
+def test_normalize_relative_paths(start_path, relative_paths, expected):
+    normalized = spack.relocate._normalize_relative_paths(
+        start_path, relative_paths
+    )
+    assert normalized == expected