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

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 is_backup_file(file):
                                            prefix_to_prefix, rel,
                                            old_prefix,
                                            new_prefix)
-        # Relocate links to the new install prefix
+            # Relocate links to the new install prefix
-            link_names = [linkname
+            links = [link for link in buildinfo.get('relocate_links', [])]
-                          for linkname in buildinfo.get('relocate_links', [])]
+            relocate.relocate_links(
-            relocate.relocate_links(link_names,
+                links, old_layout_root, old_prefix, new_prefix
-                                    old_layout_root,
+            )
-                                    new_layout_root,
-                                    old_prefix,
-                                    new_prefix,
-                                    prefix_to_prefix)
 
-    # For all buildcaches
+        # 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 is_backup_file(file):
 
         # 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,

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):
+def make_link_relative(new_links, orig_links):
-    """
+    """Compute the relative target from the original link and
-    Change absolute links to relative links.
+    make the new link relative.
-    """
-    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))
 
-        os.unlink(cur_path)
+    Args:
-        os.symlink(relative_target, cur_path)
+        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,
+def make_elf_binaries_relative(new_binaries, orig_binaries, orig_layout_root):
-                               old_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
     """
-    Replace old RPATHs with paths relative to old_dir in binary files
+    for new_binary, orig_binary in zip(new_binaries, orig_binaries):
-    """
+        orig_rpaths = _elf_rpaths_for(new_binary)
-    for cur_path, orig_path in zip(cur_path_names, orig_path_names):
-        orig_rpaths = _elf_rpaths_for(cur_path)
         if orig_rpaths:
-            new_rpaths = _make_relative(orig_path, old_layout_root,
+            new_rpaths = _make_relative(
-                                        orig_rpaths)
+                orig_binary, orig_layout_root, orig_rpaths
-            _set_elf_rpaths(cur_path, new_rpaths)
+            )
+            _set_elf_rpaths(new_binary, new_rpaths)
 
 
-def check_files_relocatable(cur_path_names, allow_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
+
+    Raises:
+        InstallRootStringError: if the file is not relocatable
     """
-    Check binary files for the current install root
+    for binary in binaries:
-    """
+        if not (allow_root or file_is_relocatable(binary)):
-    for cur_path in cur_path_names:
+            raise InstallRootStringError(binary, spack.store.layout.root)
-        if (not allow_root and
-                not file_is_relocatable(cur_path)):
-            raise InstallRootStringError(
-                cur_path, spack.store.layout.root)
 
 
-def relocate_links(linknames, old_layout_root, new_layout_root,
+def relocate_links(links, orig_layout_root,
-                   old_install_prefix, new_install_prefix, prefix_to_prefix):
+                   orig_install_prefix, new_install_prefix):
-    """
+    """Relocate links to a new install prefix.
-    The symbolic links in filenames are absolute links or placeholder links.
+
+    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.
+
+    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(old_layout_root)
+    placeholder = _placeholder(orig_layout_root)
-    link_names = [os.path.join(new_install_prefix, linkname)
+    abs_links = [os.path.join(new_install_prefix, link) for link in links]
-                  for linkname in linknames]
+    for abs_link in abs_links:
-    for link_name in link_names:
+        link_target = os.readlink(abs_link)
-        link_target = os.readlink(link_name)
+        link_target = re.sub(placeholder, orig_layout_root, link_target)
-        link_target = re.sub(placeholder, old_layout_root, link_target)
+        # If the link points to a file in the original install prefix,
-        if link_target.startswith(old_install_prefix):
+        # compute the corresponding target in the new prefix and relink
-            new_link_target = re.sub(
+        if link_target.startswith(orig_install_prefix):
-                old_install_prefix, new_install_prefix, link_target)
+            link_target = re.sub(
-            os.unlink(link_name)
+                orig_install_prefix, new_install_prefix, link_target
-            os.symlink(new_link_target, link_name)
+            )
+            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 = ('Link target "{0}" for symbolic link "{1}" is outside'
-            msg += ' for symbolic link %s is outside' % link_name
+                   ' of the new install prefix {2}')
-            msg += ' of the newinstall prefix %s.\n' % new_install_prefix
+            tty.warn(msg.format(link_target, abs_link, new_install_prefix))
-            tty.warn(msg)
 
 
-def relocate_text(path_names, old_layout_root, new_layout_root,
+def relocate_text(
-                  old_install_prefix, new_install_prefix,
+        files, orig_layout_root, new_layout_root, orig_install_prefix,
-                  old_spack_prefix, new_spack_prefix,
+        new_install_prefix, orig_spack, new_spack, new_prefixes
-                  prefix_to_prefix):
+):
+    """Relocate text file from the original installation prefix to the
+    new 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
+    # TODO: reduce the number of arguments (8 seems too much)
-    including the path the the spack sbang script
+    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
+    # Raise if the new install prefix is longer than the
-    sbangnew = '#!/bin/bash %s/bin/sbang' % new_spack_prefix
+    # 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:
+    for binary in binaries:
-        _replace_prefix_text(path_name, old_install_prefix, new_install_prefix)
+        for old_dep_prefix, new_dep_prefix in new_prefixes.items():
-        for orig_dep_prefix, new_dep_prefix in prefix_to_prefix.items():
+            if len(new_dep_prefix) <= len(old_dep_prefix):
-            _replace_prefix_text(path_name, orig_dep_prefix, new_dep_prefix)
+                _replace_prefix_bin(binary, old_dep_prefix, new_dep_prefix)
-        _replace_prefix_text(path_name, old_layout_root, new_layout_root)
+        _replace_prefix_bin(binary, orig_spack, new_spack)
-        _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)
 
 
 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:])

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,
+        relocate_links(filenames, old_layout_root,
-                       old_install_prefix, new_install_prefix,
+                       old_install_prefix, new_install_prefix)
-                       {old_install_prefix: new_install_prefix})
         assert os.readlink(new_linkname) == new_binname
         assert os.readlink(new_linkname2) == '/usr/lib/libc.so'
 

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.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 _factory(output):
 
 @pytest.fixture()
 def hello_world(tmpdir):
-    source = tmpdir.join('main.c')
+    """Factory fixture that compiles an ELF binary setting its RPATH. Relative
-    source.write("""
+    paths are encoded with `$ORIGIN` prepended.
-#include <stdio.h>
+    """
-int main(){
+    def _factory(rpaths, message="Hello world!"):
-    printf("Hello world!");
+        source = tmpdir.join('main.c')
-}
+        source.write("""
-""")
+        #include <stdio.h>
-
+        int main(){{
-    def _factory(rpaths):
+            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 @@ def _factory(rpaths):
     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')
+    new_binary = copy_binary(orig_binary)
-    shutil.copy(str(orig_binary), str(new_root))
 
-    # 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(
+    orig_binary = hello_world(rpaths=['lib', 'lib64', '/opt/local/lib'])
-        rpaths=['$ORIGIN/lib', '$ORIGIN/lib64', '/opt/local/lib']
+    new_binary = copy_binary(orig_binary)
-    )
-    new_root = tmpdir.mkdir('another_dir')
-    shutil.copy(str(orig_binary), str(new_root))
 
-    # 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_root=str(new_binary.dirpath()),
-        new_prefixes={str(tmpdir): '/foo'},
+        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
+        )