binary caching: handle files misidentified as needing relocation (#6679)

* Only specify a file as needing relocation if it contains the spack
  root as a text string (this constraint also applies to binaries)
* Don't fail if there is an error retrieving RPATH information from a
  binary (even if it is specified as requiring relocation)

3 files changed, 54 insertions, 25 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 5a2db06424..e62e9aaf14 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -111,13 +111,17 @@ def write_buildinfo_file(prefix, workdir, rel=False):
         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, 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)
+            #  Check if the file contains a string with the installroot.
+            #  This cuts down on the number of files added to the list
+            #  of files potentially needing relocation
+            if relocate.strings_contains_installroot(path_name):
+                filetype = relocate.get_filetype(path_name)
+                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)
 
     # Create buildinfo data and write it to disk
     buildinfo = {}
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index 1f62c70231..325ec22509 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -28,7 +28,7 @@ import platform
 import re
 import spack
 import spack.cmd
-from spack.util.executable import Executable
+from spack.util.executable import Executable, ProcessError
 from llnl.util.filesystem import filter_file
 import llnl.util.tty as tty
 
@@ -56,10 +56,15 @@ def get_existing_elf_rpaths(path_name):
     as a list of strings.
     """
     if platform.system() == 'Linux':
-        command = Executable(get_patchelf())
-        output = command('--print-rpath', '%s' %
-                         path_name, output=str, err=str)
-        return output.rstrip('\n').split(':')
+        patchelf = Executable(get_patchelf())
+        try:
+            output = patchelf('--print-rpath', '%s' %
+                              path_name, output=str, error=str)
+            return output.rstrip('\n').split(':')
+        except ProcessError as e:
+            tty.debug('patchelf --print-rpath produced an error on %s' %
+                      path_name, e)
+            return []
     else:
         tty.die('relocation not supported for this platform')
     return
@@ -193,19 +198,34 @@ def get_filetype(path_name):
     file = Executable('file')
     file.add_default_env('LC_ALL', 'C')
     output = file('-b', '-h', '%s' % path_name,
-                  output=str, err=str)
+                  output=str, error=str)
     return output.strip()
 
 
-def modify_elf_object(path_name, orig_rpath, new_rpath):
+def strings_contains_installroot(path_name):
+    """
+    Check if the file contain the install root string.
+    """
+    strings = Executable('strings')
+    output = strings('%s' % path_name,
+                     output=str, err=str)
+    return (spack.store.layout.root in output)
+
+
+def modify_elf_object(path_name, new_rpaths):
     """
     Replace orig_rpath with new_rpath in RPATH of elf object path_name
     """
     if platform.system() == 'Linux':
-        new_joined = ':'.join(new_rpath)
+        new_joined = ':'.join(new_rpaths)
         patchelf = Executable(get_patchelf())
-        patchelf('--force-rpath', '--set-rpath', '%s' % new_joined,
-                 '%s' % path_name, output=str, cmd=str)
+        try:
+            patchelf('--force-rpath', '--set-rpath', '%s' % new_joined,
+                     '%s' % path_name, output=str, error=str)
+        except ProcessError as e:
+            tty.die('patchelf --set-rpath %s failed' %
+                    path_name, e)
+            pass
     else:
         tty.die('relocation not supported for this platform')
 
@@ -255,8 +275,9 @@ def relocate_binary(path_names, old_dir, new_dir):
     elif platform.system() == 'Linux':
         for path_name in path_names:
             orig_rpaths = get_existing_elf_rpaths(path_name)
-            new_rpaths = substitute_rpath(orig_rpaths, old_dir, new_dir)
-            modify_elf_object(path_name, orig_rpaths, new_rpaths)
+            if orig_rpaths:
+                new_rpaths = substitute_rpath(orig_rpaths, old_dir, new_dir)
+                modify_elf_object(path_name, new_rpaths)
     else:
         tty.die("Relocation not implemented for %s" % platform.system())
 
@@ -278,9 +299,10 @@ def make_binary_relative(cur_path_names, orig_path_names, old_dir):
     elif platform.system() == 'Linux':
         for cur_path, orig_path in zip(cur_path_names, orig_path_names):
             orig_rpaths = get_existing_elf_rpaths(cur_path)
-            new_rpaths = get_relative_rpaths(orig_path, old_dir,
-                                             orig_rpaths)
-            modify_elf_object(cur_path, orig_rpaths, new_rpaths)
+            if orig_rpaths:
+                new_rpaths = get_relative_rpaths(orig_path, old_dir,
+                                                 orig_rpaths)
+                modify_elf_object(cur_path, new_rpaths)
     else:
         tty.die("Prelocation not implemented for %s" % platform.system())
 
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 49e5138c89..7cb8dfc1fe 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -42,6 +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 strings_contains_installroot
 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
@@ -217,10 +218,10 @@ echo $PATH"""
     stage.destroy()
 
 
-def test_relocate_text():
+def test_relocate_text(tmpdir):
     # Validate the text path replacement
     old_dir = '/home/spack/opt/spack'
-    filename = 'dummy.txt'
+    filename = str(tmpdir) + '/dummy.txt'
     with open(filename, "w") as script:
         script.write(old_dir)
         script.close()
@@ -229,7 +230,9 @@ def test_relocate_text():
     new_dir = '/opt/rh/devtoolset/'
     relocate_text(filenames, old_dir, new_dir)
 
-    with open(filename, "r")as script:
+    assert(strings_contains_installroot(filename) is False)
+
+    with open(filename, "r") as script:
         for line in script:
             assert(new_dir in line)