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.

3 files changed, 60 insertions, 27 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 3198e7b4ee..5a2db06424 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -28,6 +28,7 @@ import re
 import tarfile
 import yaml
 import shutil
+import platform
 
 import llnl.util.tty as tty
 from spack.util.gpg import Gpg
@@ -95,7 +96,7 @@ def read_buildinfo_file(prefix):
     return buildinfo
 
 
-def write_buildinfo_file(prefix, rel=False):
+def write_buildinfo_file(prefix, workdir, rel=False):
     """
     Create a cache file containing information
     required for the relocation
@@ -103,6 +104,7 @@ def write_buildinfo_file(prefix, rel=False):
     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):
@@ -110,7 +112,7 @@ def write_buildinfo_file(prefix, rel=False):
         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):
@@ -123,7 +125,7 @@ def write_buildinfo_file(prefix, rel=False):
     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))
 
@@ -247,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
@@ -359,7 +361,9 @@ def relocate_package(prefix):
     path_names = set()
     for filename in buildinfo['relocate_textfiles']:
         path_name = os.path.join(prefix, filename)
-        path_names.add(path_name)
+        # 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
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index 9341b55c86..1f62c70231 100644
--- a/lib/spack/spack/relocate.py
+++ b/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 "link" in filetype:
+    if "link to" in filetype:
         return False
-    if platform.system() == 'Darwin':
-        return ('Mach-O' in filetype)
-    elif platform.system() == 'Linux':
-        return ('ELF' in filetype)
+    if os_id == 'Darwin':
+        return ("Mach-O" in filetype)
+    elif os_id == 'Linux':
+        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)
 
 
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index f9c77c530f..49e5138c89 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -42,7 +42,7 @@ from spack.spec import Spec
 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 @@ echo $PATH"""
     stage.destroy()
 
 
-def test_relocate():
-    assert (needs_binary_relocation('relocatable') is False)
-    assert (needs_binary_relocation('link') is False)
-    assert (needs_text_relocation('link') is False)
+def test_relocate_text():
+    # Validate the text path replacement
+    old_dir = '/home/spack/opt/spack'
+    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'))