spack.relocate: further coverage for ELF related functions (#16585)

* make_link_relative: added docstring

* make_elf_binaries_relative: added docstring, unit tests

* raise_if_not_relocatable: added docstring, added unit test for exceptional case

* relocate_links: removed unused arguments, added docstring and comments

Also fixed a possible bug that was issuing spurious
warning when a file was relocated successfully

* relocate_text: added docstring and comments, renamed arguments

* relocate_text_bin: added docstring and comments, renamed arguments, unit tests

4 files changed, 258 insertions, 141 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 0fb6feae02..926f4679a1 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -514,7 +514,7 @@ def make_package_relative(workdir, spec, allow_root):
             platform.system().lower() == 'linux'):
         relocate.make_elf_binaries_relative(cur_path_names, orig_path_names,
                                             old_layout_root)
-    relocate.check_files_relocatable(cur_path_names, allow_root)
+    relocate.raise_if_not_relocatable(cur_path_names, allow_root)
     orig_path_names = list()
     cur_path_names = list()
     for linkname in buildinfo.get('relocate_links', []):
@@ -532,7 +532,7 @@ def check_package_relocatable(workdir, spec, allow_root):
     cur_path_names = list()
     for filename in buildinfo['relocate_binaries']:
         cur_path_names.append(os.path.join(workdir, filename))
-    relocate.check_files_relocatable(cur_path_names, allow_root)
+    relocate.raise_if_not_relocatable(cur_path_names, allow_root)
 
 
 def relocate_package(spec, allow_root):
@@ -615,17 +615,13 @@ def relocate_package(spec, allow_root):
                                            prefix_to_prefix, rel,
                                            old_prefix,
                                            new_prefix)
-        # Relocate links to the new install prefix
-            link_names = [linkname
-                          for linkname in buildinfo.get('relocate_links', [])]
-            relocate.relocate_links(link_names,
-                                    old_layout_root,
-                                    new_layout_root,
-                                    old_prefix,
-                                    new_prefix,
-                                    prefix_to_prefix)
-
-    # For all buildcaches
+            # Relocate links to the new install prefix
+            links = [link for link in buildinfo.get('relocate_links', [])]
+            relocate.relocate_links(
+                links, old_layout_root, old_prefix, new_prefix
+            )
+
+        # For all buildcaches
         # relocate the install prefixes in text files including dependencies
         relocate.relocate_text(text_names,
                                old_layout_root, new_layout_root,
@@ -636,7 +632,6 @@ def relocate_package(spec, allow_root):
 
         # relocate the install prefixes in binary files including dependencies
         relocate.relocate_text_bin(files_to_relocate,
-                                   old_layout_root, new_layout_root,
                                    old_prefix, new_prefix,
                                    old_spack_prefix,
                                    new_spack_prefix,
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index 94fd2b58bf..01a60db6ba 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -458,6 +458,7 @@ def _replace_prefix_text(filename, old_dir, new_dir):
         old_dir (str): directory to be searched in the file
         new_dir (str): substitute for the old directory
     """
+    # TODO: cache regexes globally to speedup computation
     with open(filename, 'rb+') as f:
         data = f.read()
         f.seek(0)
@@ -675,16 +676,19 @@ def relocate_elf_binaries(binaries, orig_root, new_root,
             _set_elf_rpaths(new_binary, new_rpaths)
 
 
-def make_link_relative(cur_path_names, orig_path_names):
-    """
-    Change absolute links to relative links.
-    """
-    for cur_path, orig_path in zip(cur_path_names, orig_path_names):
-        target = os.readlink(orig_path)
-        relative_target = os.path.relpath(target, os.path.dirname(orig_path))
+def make_link_relative(new_links, orig_links):
+    """Compute the relative target from the original link and
+    make the new link relative.
 
-        os.unlink(cur_path)
-        os.symlink(relative_target, cur_path)
+    Args:
+        new_links (list): new links to be made relative
+        orig_links (list): original links
+    """
+    for new_link, orig_link in zip(new_links, orig_links):
+        target = os.readlink(orig_link)
+        relative_target = os.path.relpath(target, os.path.dirname(orig_link))
+        os.unlink(new_link)
+        os.symlink(relative_target, new_link)
 
 
 def make_macho_binaries_relative(cur_path_names, orig_path_names,
@@ -706,97 +710,143 @@ def make_macho_binaries_relative(cur_path_names, orig_path_names,
                                 paths_to_paths)
 
 
-def make_elf_binaries_relative(cur_path_names, orig_path_names,
-                               old_layout_root):
-    """
-    Replace old RPATHs with paths relative to old_dir in binary files
+def make_elf_binaries_relative(new_binaries, orig_binaries, orig_layout_root):
+    """Replace the original RPATHs in the new binaries making them
+    relative to the original layout root.
+
+    Args:
+        new_binaries (list): new binaries whose RPATHs is to be made relative
+        orig_binaries (list): original binaries
+        orig_layout_root (str): path to be used as a base for making
+            RPATHs relative
     """
-    for cur_path, orig_path in zip(cur_path_names, orig_path_names):
-        orig_rpaths = _elf_rpaths_for(cur_path)
+    for new_binary, orig_binary in zip(new_binaries, orig_binaries):
+        orig_rpaths = _elf_rpaths_for(new_binary)
         if orig_rpaths:
-            new_rpaths = _make_relative(orig_path, old_layout_root,
-                                        orig_rpaths)
-            _set_elf_rpaths(cur_path, new_rpaths)
+            new_rpaths = _make_relative(
+                orig_binary, orig_layout_root, orig_rpaths
+            )
+            _set_elf_rpaths(new_binary, new_rpaths)
 
 
-def check_files_relocatable(cur_path_names, allow_root):
-    """
-    Check binary files for the current install root
-    """
-    for cur_path in cur_path_names:
-        if (not allow_root and
-                not file_is_relocatable(cur_path)):
-            raise InstallRootStringError(
-                cur_path, spack.store.layout.root)
+def raise_if_not_relocatable(binaries, allow_root):
+    """Raise an error if any binary in the list is not relocatable.
 
+    Args:
+        binaries (list): list of binaries to check
+        allow_root (bool): whether root dir is allowed or not in a binary
 
-def relocate_links(linknames, old_layout_root, new_layout_root,
-                   old_install_prefix, new_install_prefix, prefix_to_prefix):
+    Raises:
+        InstallRootStringError: if the file is not relocatable
     """
-    The symbolic links in filenames are absolute links or placeholder links.
+    for binary in binaries:
+        if not (allow_root or file_is_relocatable(binary)):
+            raise InstallRootStringError(binary, spack.store.layout.root)
+
+
+def relocate_links(links, orig_layout_root,
+                   orig_install_prefix, new_install_prefix):
+    """Relocate links to a new install prefix.
+
+    The symbolic links are relative to the original installation prefix.
     The old link target is read and the placeholder is replaced by the old
     layout root. If the old link target is in the old install prefix, the new
     link target is create by replacing the old install prefix with the new
     install prefix.
-    """
-    placeholder = _placeholder(old_layout_root)
-    link_names = [os.path.join(new_install_prefix, linkname)
-                  for linkname in linknames]
-    for link_name in link_names:
-        link_target = os.readlink(link_name)
-        link_target = re.sub(placeholder, old_layout_root, link_target)
-        if link_target.startswith(old_install_prefix):
-            new_link_target = re.sub(
-                old_install_prefix, new_install_prefix, link_target)
-            os.unlink(link_name)
-            os.symlink(new_link_target, link_name)
+
+    Args:
+        links (list): list of links to be relocated
+        orig_layout_root (str): original layout root
+        orig_install_prefix (str): install prefix of the original installation
+        new_install_prefix (str): install prefix where we want to relocate
+    """
+    placeholder = _placeholder(orig_layout_root)
+    abs_links = [os.path.join(new_install_prefix, link) for link in links]
+    for abs_link in abs_links:
+        link_target = os.readlink(abs_link)
+        link_target = re.sub(placeholder, orig_layout_root, link_target)
+        # If the link points to a file in the original install prefix,
+        # compute the corresponding target in the new prefix and relink
+        if link_target.startswith(orig_install_prefix):
+            link_target = re.sub(
+                orig_install_prefix, new_install_prefix, link_target
+            )
+            os.unlink(abs_link)
+            os.symlink(link_target, abs_link)
+
+        # If the link is absolute and has not been relocated then
+        # warn the user about that
         if (os.path.isabs(link_target) and
             not link_target.startswith(new_install_prefix)):
-            msg = 'Link target %s' % link_target
-            msg += ' for symbolic link %s is outside' % link_name
-            msg += ' of the newinstall prefix %s.\n' % new_install_prefix
-            tty.warn(msg)
+            msg = ('Link target "{0}" for symbolic link "{1}" is outside'
+                   ' of the new install prefix {2}')
+            tty.warn(msg.format(link_target, abs_link, new_install_prefix))
+
 
+def relocate_text(
+        files, orig_layout_root, new_layout_root, orig_install_prefix,
+        new_install_prefix, orig_spack, new_spack, new_prefixes
+):
+    """Relocate text file from the original installation prefix to the
+    new prefix.
 
-def relocate_text(path_names, old_layout_root, new_layout_root,
-                  old_install_prefix, new_install_prefix,
-                  old_spack_prefix, new_spack_prefix,
-                  prefix_to_prefix):
+    Relocation also affects the the path in Spack's sbang script.
+
+    Args:
+        files (list): text files to be relocated
+        orig_layout_root (str): original layout root
+        new_layout_root (str): new layout root
+        orig_install_prefix (str): install prefix of the original installation
+        new_install_prefix (str): install prefix where we want to relocate
+        orig_spack (str): path to the original Spack
+        new_spack (str): path to the new Spack
+        new_prefixes (dict): dictionary that maps the original prefixes to
+            where they should be relocated
     """
-    Replace old paths with new paths in text files
-    including the path the the spack sbang script
+    # TODO: reduce the number of arguments (8 seems too much)
+    sbang_regex = r'#!/bin/bash {0}/bin/sbang'.format(orig_spack)
+    new_sbang = r'#!/bin/bash {0}/bin/sbang'.format(new_spack)
+
+    for file in files:
+        _replace_prefix_text(file, orig_install_prefix, new_install_prefix)
+        for orig_dep_prefix, new_dep_prefix in new_prefixes.items():
+            _replace_prefix_text(file, orig_dep_prefix, new_dep_prefix)
+        _replace_prefix_text(file, orig_layout_root, new_layout_root)
+        _replace_prefix_text(file, sbang_regex, new_sbang)
+
+
+def relocate_text_bin(
+        binaries, orig_install_prefix, new_install_prefix,
+        orig_spack, new_spack, new_prefixes
+):
+    """Replace null terminated path strings hard coded into binaries.
+
+    The new install prefix must be shorter than the original one.
+
+    Args:
+        binaries (list): binaries to be relocated
+        orig_install_prefix (str): install prefix of the original installation
+        new_install_prefix (str): install prefix where we want to relocate
+        orig_spack (str): path to the original Spack
+        new_spack (str): path to the new Spack
+        new_prefixes (dict): dictionary that maps the original prefixes to
+            where they should be relocated
+
+    Raises:
+      BinaryTextReplaceError: when the new path in longer than the old path
     """
-    sbangre = '#!/bin/bash %s/bin/sbang' % old_spack_prefix
-    sbangnew = '#!/bin/bash %s/bin/sbang' % new_spack_prefix
+    # Raise if the new install prefix is longer than the
+    # original one, since it means we can't change the original
+    # binary to relocate it
+    new_prefix_is_shorter = len(new_install_prefix) <= len(orig_install_prefix)
+    if not new_prefix_is_shorter and len(binaries) > 0:
+        raise BinaryTextReplaceError(orig_install_prefix, new_install_prefix)
 
-    for path_name in path_names:
-        _replace_prefix_text(path_name, old_install_prefix, new_install_prefix)
-        for orig_dep_prefix, new_dep_prefix in prefix_to_prefix.items():
-            _replace_prefix_text(path_name, orig_dep_prefix, new_dep_prefix)
-        _replace_prefix_text(path_name, old_layout_root, new_layout_root)
-        _replace_prefix_text(path_name, sbangre, sbangnew)
-
-
-def relocate_text_bin(path_names, old_layout_root, new_layout_root,
-                      old_install_prefix, new_install_prefix,
-                      old_spack_prefix, new_spack_prefix,
-                      prefix_to_prefix):
-    """
-      Replace null terminated path strings hard coded into binaries.
-      Raise an exception when the new path in longer than the old path
-      because this breaks the binary.
-      """
-    if len(new_install_prefix) <= len(old_install_prefix):
-        for path_name in path_names:
-            for old_dep_prefix, new_dep_prefix in prefix_to_prefix.items():
-                if len(new_dep_prefix) <= len(old_dep_prefix):
-                    _replace_prefix_bin(
-                        path_name, old_dep_prefix, new_dep_prefix)
-            _replace_prefix_bin(path_name, old_spack_prefix, new_spack_prefix)
-    else:
-        if len(path_names) > 0:
-            raise BinaryTextReplaceError(
-                old_install_prefix, new_install_prefix)
+    for binary in binaries:
+        for old_dep_prefix, new_dep_prefix in new_prefixes.items():
+            if len(new_dep_prefix) <= len(old_dep_prefix):
+                _replace_prefix_bin(binary, old_dep_prefix, new_dep_prefix)
+        _replace_prefix_bin(binary, orig_spack, new_spack)
 
 
 def is_relocatable(spec):
@@ -929,4 +979,4 @@ def mime_type(file):
     if '/' not in output:
         output += '/'
     split_by_slash = output.strip().split('/')
-    return (split_by_slash[0], "/".join(split_by_slash[1:]))
+    return split_by_slash[0], "/".join(split_by_slash[1:])
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 67c9e52875..7d4adfe975 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -242,9 +242,8 @@ def test_relocate_links(tmpdir):
         os.utime(new_binname, None)
         os.symlink(old_binname, new_linkname)
         os.symlink('/usr/lib/libc.so', new_linkname2)
-        relocate_links(filenames, old_layout_root, new_layout_root,
-                       old_install_prefix, new_install_prefix,
-                       {old_install_prefix: new_install_prefix})
+        relocate_links(filenames, old_layout_root,
+                       old_install_prefix, new_install_prefix)
         assert os.readlink(new_linkname) == new_binname
         assert os.readlink(new_linkname2) == '/usr/lib/libc.so'
 
diff --git a/lib/spack/spack/test/relocate.py b/lib/spack/spack/test/relocate.py
index d9becebe7b..6669b25eb4 100644
--- a/lib/spack/spack/test/relocate.py
+++ b/lib/spack/spack/test/relocate.py
@@ -2,10 +2,10 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
 import collections
 import os.path
 import platform
+import re
 import shutil
 
 import llnl.util.filesystem
@@ -20,6 +20,23 @@ import spack.tengine
 import spack.util.executable
 
 
+def rpaths_for(new_binary):
+    """Return the RPATHs or RUNPATHs of a binary."""
+    patchelf = spack.util.executable.which('patchelf')
+    output = patchelf('--print-rpath', str(new_binary), output=str)
+    return output.strip()
+
+
+def text_in_bin(text, binary):
+    with open(str(binary), "rb") as f:
+        data = f.read()
+        f.seek(0)
+        pat = re.compile(text.encode('utf-8'))
+        if not pat.search(data):
+            return False
+        return True
+
+
 @pytest.fixture(params=[True, False])
 def is_relocatable(request):
     return request.param
@@ -105,17 +122,22 @@ def mock_patchelf(tmpdir):
 
 @pytest.fixture()
 def hello_world(tmpdir):
-    source = tmpdir.join('main.c')
-    source.write("""
-#include <stdio.h>
-int main(){
-    printf("Hello world!");
-}
-""")
-
-    def _factory(rpaths):
+    """Factory fixture that compiles an ELF binary setting its RPATH. Relative
+    paths are encoded with `$ORIGIN` prepended.
+    """
+    def _factory(rpaths, message="Hello world!"):
+        source = tmpdir.join('main.c')
+        source.write("""
+        #include <stdio.h>
+        int main(){{
+            printf("{0}");
+        }}
+        """.format(message))
         gcc = spack.util.executable.which('gcc')
         executable = source.dirpath('main.x')
+        # Encode relative RPATHs using `$ORIGIN` as the root prefix
+        rpaths = [x if os.path.isabs(x) else os.path.join('$ORIGIN', x)
+                  for x in rpaths]
         rpath_str = ':'.join(rpaths)
         opts = [
             '-Wl,--disable-new-dtags',
@@ -128,6 +150,19 @@ int main(){
     return _factory
 
 
+@pytest.fixture()
+def copy_binary():
+    """Returns a function that copies a binary somewhere and
+    returns the new location.
+    """
+    def _copy_somewhere(orig_binary):
+        new_root = orig_binary.mkdtemp()
+        new_binary = new_root.join('main.x')
+        shutil.copy(str(orig_binary), str(new_binary))
+        return new_binary
+    return _copy_somewhere
+
+
 @pytest.mark.requires_executables(
     '/usr/bin/gcc', 'patchelf', 'strings', 'file'
 )
@@ -250,23 +285,18 @@ def test_replace_prefix_bin(hello_world):
     # Relocate the RPATHs
     spack.relocate._replace_prefix_bin(str(executable), '/usr', '/foo')
 
-    # Check that the RPATHs changed
-    patchelf = spack.util.executable.which('patchelf')
-    output = patchelf('--print-rpath', str(executable), output=str)
-
     # Some compilers add rpaths so ensure changes included in final result
-    assert '/foo/lib:/foo/lib64' in output
+    assert '/foo/lib:/foo/lib64' in rpaths_for(executable)
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
-def test_relocate_elf_binaries_absolute_paths(hello_world, tmpdir):
+def test_relocate_elf_binaries_absolute_paths(
+        hello_world, copy_binary, tmpdir
+):
     # Create an executable, set some RPATHs, copy it to another location
     orig_binary = hello_world(rpaths=[str(tmpdir.mkdir('lib')), '/usr/lib64'])
-    new_root = tmpdir.mkdir('another_dir')
-    shutil.copy(str(orig_binary), str(new_root))
+    new_binary = copy_binary(orig_binary)
 
-    # Relocate the binary
-    new_binary = new_root.join('main.x')
     spack.relocate.relocate_elf_binaries(
         binaries=[str(new_binary)],
         orig_root=str(orig_binary.dirpath()),
@@ -279,38 +309,81 @@ def test_relocate_elf_binaries_absolute_paths(hello_world, tmpdir):
         orig_prefix=None, new_prefix=None
     )
 
-    # Check that the RPATHs changed
-    patchelf = spack.util.executable.which('patchelf')
-    output = patchelf('--print-rpath', str(new_binary), output=str)
-
     # Some compilers add rpaths so ensure changes included in final result
-    assert '/foo/lib:/usr/lib64' in output
+    assert '/foo/lib:/usr/lib64' in rpaths_for(new_binary)
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
-def test_relocate_elf_binaries_relative_paths(hello_world, tmpdir):
+def test_relocate_elf_binaries_relative_paths(hello_world, copy_binary):
     # Create an executable, set some RPATHs, copy it to another location
-    orig_binary = hello_world(
-        rpaths=['$ORIGIN/lib', '$ORIGIN/lib64', '/opt/local/lib']
-    )
-    new_root = tmpdir.mkdir('another_dir')
-    shutil.copy(str(orig_binary), str(new_root))
+    orig_binary = hello_world(rpaths=['lib', 'lib64', '/opt/local/lib'])
+    new_binary = copy_binary(orig_binary)
 
-    # Relocate the binary
-    new_binary = new_root.join('main.x')
     spack.relocate.relocate_elf_binaries(
         binaries=[str(new_binary)],
         orig_root=str(orig_binary.dirpath()),
-        new_root=str(new_root),
-        new_prefixes={str(tmpdir): '/foo'},
+        new_root=str(new_binary.dirpath()),
+        new_prefixes={str(orig_binary.dirpath()): '/foo'},
         rel=True,
         orig_prefix=str(orig_binary.dirpath()),
-        new_prefix=str(new_root)
+        new_prefix=str(new_binary.dirpath())
     )
 
-    # Check that the RPATHs changed
-    patchelf = spack.util.executable.which('patchelf')
-    output = patchelf('--print-rpath', str(new_binary), output=str)
-
     # Some compilers add rpaths so ensure changes included in final result
-    assert '/foo/lib:/foo/lib64:/opt/local/lib' in output
+    assert '/foo/lib:/foo/lib64:/opt/local/lib' in rpaths_for(new_binary)
+
+
+@pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
+def test_make_elf_binaries_relative(hello_world, copy_binary, tmpdir):
+    orig_binary = hello_world(rpaths=[
+        str(tmpdir.mkdir('lib')), str(tmpdir.mkdir('lib64')), '/opt/local/lib'
+    ])
+    new_binary = copy_binary(orig_binary)
+
+    spack.relocate.make_elf_binaries_relative(
+        [str(new_binary)], [str(orig_binary)], str(orig_binary.dirpath())
+    )
+
+    assert rpaths_for(new_binary) == '$ORIGIN/lib:$ORIGIN/lib64:/opt/local/lib'
+
+
+def test_raise_if_not_relocatable(monkeypatch):
+    monkeypatch.setattr(spack.relocate, 'file_is_relocatable', lambda x: False)
+    with pytest.raises(spack.relocate.InstallRootStringError):
+        spack.relocate.raise_if_not_relocatable(
+            ['an_executable'], allow_root=False
+        )
+
+
+@pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
+def test_relocate_text_bin(hello_world, copy_binary, tmpdir):
+    orig_binary = hello_world(rpaths=[
+        str(tmpdir.mkdir('lib')), str(tmpdir.mkdir('lib64')), '/opt/local/lib'
+    ], message=str(tmpdir))
+    new_binary = copy_binary(orig_binary)
+
+    # Check original directory is in the executabel and the new one is not
+    assert text_in_bin(str(tmpdir), new_binary)
+    assert not text_in_bin(str(new_binary.dirpath()), new_binary)
+
+    # Check this call succeed
+    spack.relocate.relocate_text_bin(
+        [str(new_binary)],
+        str(orig_binary.dirpath()), str(new_binary.dirpath()),
+        spack.paths.spack_root, spack.paths.spack_root,
+        {str(orig_binary.dirpath()): str(new_binary.dirpath())}
+    )
+
+    # Check original directory is not there anymore and it was
+    # substituted with the new one
+    assert not text_in_bin(str(tmpdir), new_binary)
+    assert text_in_bin(str(new_binary.dirpath()), new_binary)
+
+
+def test_relocate_text_bin_raise_if_new_prefix_is_longer():
+    short_prefix = '/short'
+    long_prefix = '/much/longer'
+    with pytest.raises(spack.relocate.BinaryTextReplaceError):
+        spack.relocate.relocate_text_bin(
+            ['item'], short_prefix, long_prefix, None, None, None
+        )