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>

3 files changed, 148 insertions, 76 deletions

diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index 9f8669f3d4..41770103b0 100644
--- a/lib/spack/spack/relocate.py
+++ b/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):
-    """
-    Return the RPATHS returned by patchelf --print-rpath path_name
-    as a list of strings.
-    """
+def _elf_rpaths_for(path):
+    """Return the RPATHs for an executable or a library.
 
-    # if we're relocating patchelf itself, use it
+    The RPATHs are obtained by ``patchelf --print-rpath PATH``.
 
-    if path_name.endswith("/bin/patchelf"):
-        patchelf = executable.Executable(path_name)
-    else:
-        patchelf = executable.Executable(_patchelf())
+    Args:
+        path (str): full path to the executable or library
 
-    rpaths = list()
+    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)
+
+    output = ''
     try:
-        output = patchelf('--print-rpath', '%s' %
-                          path_name, output=str, error=str)
-        rpaths = output.rstrip('\n').split(':')
+        output = patchelf('--print-rpath', path, output=str, error=str)
+        output = output.strip('\n')
     except executable.ProcessError as e:
-        msg = 'patchelf --print-rpath %s produced an error %s' % (path_name, e)
-        tty.warn(msg)
-    return rpaths
+        msg = 'patchelf --print-rpath {0} produced an error [{1}]'
+        tty.warn(msg.format(path, str(e)))
 
+    return output.split(':') if output else []
 
-def get_relative_elf_rpaths(path_name, orig_layout_root, orig_rpaths):
-    """
-    Replaces orig rpath with relative path from dirname(path_name) if an rpath
-    in orig_rpaths contains orig_layout_root. Prefixes $ORIGIN
-    to relative paths and returns replacement rpaths.
-    """
-    rel_rpaths = []
-    for rpath in orig_rpaths:
-        if re.match(orig_layout_root, rpath):
-            rel = os.path.relpath(rpath, start=os.path.dirname(path_name))
-            rel_rpaths.append(os.path.join('$ORIGIN', '%s' % rel))
-        else:
-            rel_rpaths.append(rpath)
-    return rel_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.
 
-def get_normalized_elf_rpaths(orig_path_name, rel_rpaths):
-    """
-    Normalize the relative rpaths with respect to the original path name
-    of the file. If the rpath starts with $ORIGIN replace $ORIGIN with the
-    dirname of the original path name and then normalize the rpath.
-    A dictionary mapping relativized rpaths to normalized rpaths is returned.
-    """
-    norm_rpaths = list()
-    for rpath in rel_rpaths:
-        if rpath.startswith('$ORIGIN'):
-            sub = re.sub(re.escape('$ORIGIN'),
-                         os.path.dirname(orig_path_name),
-                         rpath)
-            norm = os.path.normpath(sub)
-            norm_rpaths.append(norm)
-        else:
-            norm_rpaths.append(rpath)
-    return norm_rpaths
+    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
 
-def set_placeholder(dirname):
+    Returns:
+        List of relative paths
     """
-    return string of @'s with same length
+    # Check prerequisites of the function
+    msg = "{0} is not a file".format(reference_file)
+    assert os.path.isfile(reference_file), msg
+
+    start_directory = os.path.dirname(reference_file)
+    pattern = re.compile(path_root)
+    relative_paths = []
+
+    for path in paths:
+        if pattern.match(path):
+            rel = os.path.relpath(path, start=start_directory)
+            path = os.path.join('$ORIGIN', rel)
+
+        relative_paths.append(path)
+
+    return relative_paths
+
+
+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
     """
+    normalized_paths = []
+    pattern = re.compile(re.escape('$ORIGIN'))
+    start_directory = os.path.dirname(start_path)
+
+    for path in relative_paths:
+        if path.startswith('$ORIGIN'):
+            sub = pattern.sub(start_directory, path)
+            path = os.path.normpath(sub)
+        normalized_paths.append(path)
+
+    return normalized_paths
+
+
+def _placeholder(dirname):
+    """String of  of @'s with same length of the argument"""
     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,14 +632,14 @@ 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,
-                                                 norm_rpaths)
+            new_rpaths = _make_relative(path_name, new_layout_root,
+                                        norm_rpaths)
             modify_elf_object(path_name, new_rpaths)
         else:
             new_rpaths = elf_find_paths(orig_rpaths, old_layout_root,
@@ -652,10 +684,10 @@ 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,
-                                                 orig_rpaths)
+            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':
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 39da7c3ae5..67c9e52875 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -25,11 +25,9 @@ from spack.paths import mock_gpg_keys_path
 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']
diff --git a/lib/spack/spack/test/relocate.py b/lib/spack/spack/test/relocate.py
index 0a9e9f7f0a..a2bba52f40 100644
--- a/lib/spack/spack/test/relocate.py
+++ b/lib/spack/spack/test/relocate.py
@@ -89,6 +89,20 @@ def expected_patchelf_path(request, mutable_database, monkeypatch):
     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