elf relocation fix: cherry-picked from develop branch (#6889)

* Revert "Quick fix for relocation issues."

This reverts commit 57608a6dc4.

* Buildcache: relocate fixes (#6512)

* Updated function which checks if a binary file needs relocation.
  Previously this was incorrectly identifying ELF binaries as symbolic
  links (so they were being excluded from relocation). Added test to
  check that ELF binaries are not considered symlinks.

* relocate_text was not replacing paths in text files. Added test to
  check that text files are relocated properly (i.e. paths in the file
  are converted to the new prefix).

* Exclude backup files created by filter_file when installing from
  binary cache.

* Update write_buildinfo_file method signature to distinguish between
  the spec prefix and the working directory for the binary cache
  package.

lib/spack/spack/binary_distribution.py

@@ -28,6 +28,7 @@
 import tarfile
 import yaml
 import shutil
+import platform
 
 import llnl.util.tty as tty
 from spack.util.gpg import Gpg
@@ -44,9 +45,6 @@
 import spack.relocate as relocate
 
 
-_relocation_blacklist = (".spack", "man")
-
-
 class NoOverwriteException(Exception):
     pass
 
@@ -98,41 +96,36 @@ def read_buildinfo_file(prefix):
     return buildinfo
 
 
-def _find_relocations(prefix):
+def write_buildinfo_file(prefix, workdir, rel=False):
+    """
+    Create a cache file containing information
+    required for the relocation
+    """
     text_to_relocate = []
     binary_to_relocate = []
+    blacklist = (".spack", "man")
+    os_id = platform.system()
     # Do this at during tarball creation to save time when tarball unpacked.
     # Used by make_package_relative to determine binaries to change.
     for root, dirs, files in os.walk(prefix, topdown=True):
-        dirs[:] = [d for d in dirs if d not in _relocation_blacklist]
+        dirs[:] = [d for d in dirs if d not in blacklist]
-
         for filename in files:
             path_name = os.path.join(root, filename)
             filetype = relocate.get_filetype(path_name)
-            if relocate.needs_binary_relocation(filetype):
+            if relocate.needs_binary_relocation(filetype, os_id):
                 rel_path_name = os.path.relpath(path_name, prefix)
                 binary_to_relocate.append(rel_path_name)
             elif relocate.needs_text_relocation(filetype):
                 rel_path_name = os.path.relpath(path_name, prefix)
                 text_to_relocate.append(rel_path_name)
 
-    return text_to_relocate, binary_to_relocate
-
-
-def write_buildinfo_file(prefix, rel=False):
-    """
-    Create a cache file containing information
-    required for the relocation
-    """
-    text_to_relocate, binary_to_relocate = _find_relocations(prefix)
-
     # Create buildinfo data and write it to disk
     buildinfo = {}
     buildinfo['relative_rpaths'] = rel
     buildinfo['buildpath'] = spack.store.layout.root
     buildinfo['relocate_textfiles'] = text_to_relocate
     buildinfo['relocate_binaries'] = binary_to_relocate
-    filename = buildinfo_file_name(prefix)
+    filename = buildinfo_file_name(workdir)
     with open(filename, 'w') as outfile:
         outfile.write(yaml.dump(buildinfo, default_flow_style=True))
 
@@ -256,7 +249,7 @@ def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False,
     install_tree(spec.prefix, workdir, symlinks=True)
 
     # create info for later relocation and create tar
-    write_buildinfo_file(workdir, rel=rel)
+    write_buildinfo_file(spec.prefix, workdir, rel=rel)
 
     # optinally make the paths in the binaries relative to each other
     # in the spack install tree before creating tarball
@@ -363,28 +356,20 @@ def relocate_package(prefix):
     if new_path == old_path and not rel:
         return
 
-    text_relocs = buildinfo['relocate_textfiles']
-    binary_relocs = buildinfo['relocate_binaries']
-
-    # if there are no relocations, search for them instead
-    # TODO: revisit this in a 0.11 point release
-    if not text_relocs or not binary_relocs:
-        text_relocs, binary_relocs = _find_relocations(prefix)
-        rel = False
-
     tty.msg("Relocating package from",
             "%s to %s." % (old_path, new_path))
     path_names = set()
-    for filename in text_relocs:
+    for filename in buildinfo['relocate_textfiles']:
         path_name = os.path.join(prefix, filename)
+        # Don't add backup files generated by filter_file during install step.
+        if not path_name.endswith('~'):
             path_names.add(path_name)
     relocate.relocate_text(path_names, old_path, new_path)
-
     # If the binary files in the package were not edited to use
     # relative RPATHs, then the RPATHs need to be relocated
     if not rel:
         path_names = set()
-        for filename in binary_relocs:
+        for filename in buildinfo['relocate_binaries']:
             path_name = os.path.join(prefix, filename)
             path_names.add(path_name)
         relocate.relocate_binary(path_names, old_path, new_path)

lib/spack/spack/relocate.py

@@ -210,21 +210,21 @@ def modify_elf_object(path_name, orig_rpath, new_rpath):
         tty.die('relocation not supported for this platform')
 
 
-def needs_binary_relocation(filetype):
+def needs_binary_relocation(filetype, os_id=None):
     """
     Check whether the given filetype is a binary that may need relocation.
     """
     retval = False
     if "relocatable" in filetype:
         return False
-    if "symbolic link" in filetype:
+    if "link to" in filetype:
         return False
-    if platform.system() == 'Darwin':
+    if os_id == 'Darwin':
-        return ('Mach-O' in filetype)
+        return ("Mach-O" in filetype)
-    elif platform.system() == 'Linux':
+    elif os_id == 'Linux':
-        return ('ELF' in filetype)
+        return ("ELF" in filetype)
     else:
-        tty.die("Relocation not implemented for %s" % platform.system())
+        tty.die("Relocation not implemented for %s" % os_id)
     return retval
 
 
@@ -232,7 +232,7 @@ def needs_text_relocation(filetype):
     """
     Check whether the given filetype is text that may need relocation.
     """
-    if "link" in filetype:
+    if "link to" in filetype:
         return False
     return ("text" in filetype)
 
@@ -289,7 +289,7 @@ def relocate_text(path_names, old_dir, new_dir):
     """
     Replace old path with new path in text file path_name
     """
-    filter_file("r'%s'" % old_dir, "r'%s'" % new_dir,
+    filter_file('%s' % old_dir, '%s' % new_dir,
                 *path_names, backup=False)
 
 

lib/spack/spack/test/packaging.py

@@ -42,7 +42,7 @@
 from spack.fetch_strategy import URLFetchStrategy, FetchStrategyComposite
 from spack.util.executable import ProcessError
 from spack.relocate import needs_binary_relocation, needs_text_relocation
-from spack.relocate import get_patchelf
+from spack.relocate import get_patchelf, relocate_text
 from spack.relocate import substitute_rpath, get_relative_rpaths
 from spack.relocate import macho_replace_paths, macho_make_paths_relative
 from spack.relocate import modify_macho_object, macho_get_paths
@@ -217,10 +217,43 @@ def test_packaging(mock_archive, tmpdir):
     stage.destroy()
 
 
-def test_relocate():
+def test_relocate_text():
-    assert (needs_binary_relocation('relocatable') is False)
+    # Validate the text path replacement
-    assert (needs_binary_relocation('link') is False)
+    old_dir = '/home/spack/opt/spack'
-    assert (needs_text_relocation('link') is False)
+    filename = 'dummy.txt'
+    with open(filename, "w") as script:
+        script.write(old_dir)
+        script.close()
+
+    filenames = [filename]
+    new_dir = '/opt/rh/devtoolset/'
+    relocate_text(filenames, old_dir, new_dir)
+
+    with open(filename, "r")as script:
+        for line in script:
+            assert(new_dir in line)
+
+
+def test_needs_relocation():
+    binary_type = (
+        'ELF 64-bit LSB executable, x86-64, version 1 (SYSV),'
+        ' dynamically linked (uses shared libs),'
+        ' for GNU/Linux x.y.z, stripped')
+
+    assert needs_binary_relocation(binary_type, os_id='Linux')
+    assert not needs_binary_relocation('relocatable',
+                                       os_id='Linux')
+    assert not needs_binary_relocation('symbolic link to `foo\'',
+                                       os_id='Linux')
+
+    assert needs_text_relocation('ASCII text')
+    assert not needs_text_relocation('symbolic link to `foo.text\'')
+
+    macho_type = 'Mach-O 64-bit executable x86_64'
+    assert needs_binary_relocation(macho_type, os_id='Darwin')
+
+
+def test_macho_paths():
 
     out = macho_make_paths_relative('/Users/Shares/spack/pkgC/lib/libC.dylib',
                                     '/Users/Shared/spack',
@@ -289,6 +322,8 @@ def test_relocate():
                     '/usr/local/lib/libloco.dylib'],
                    None)
 
+
+def test_elf_paths():
     out = get_relative_rpaths(
         '/usr/bin/test', '/usr',
         ('/usr/lib', '/usr/lib64', '/opt/local/lib'))
@@ -303,8 +338,8 @@ def test_relocate():
                     reason="only works with Mach-o objects")
 def test_relocate_macho(tmpdir):
     with tmpdir.as_cwd():
-        get_patchelf()
+
-        assert (needs_binary_relocation('Mach-O'))
+        get_patchelf()  # this does nothing on Darwin
 
         rpaths, deps, idpath = macho_get_paths('/bin/bash')
         nrpaths, ndeps, nid = macho_make_paths_relative('/bin/bash', '/usr',
@@ -341,9 +376,3 @@ def test_relocate_macho(tmpdir):
             'libncurses.5.4.dylib',
             rpaths, deps, idpath,
             nrpaths, ndeps, nid)
-
-
-@pytest.mark.skipif(sys.platform != 'linux2',
-                    reason="only works with Elf objects")
-def test_relocate_elf():
-    assert (needs_binary_relocation('ELF'))