This fixes a problem where the placeholder path was not in the first rpath entry.

* Rework of buildcache creation and install prefix checking using the functions introduced in
https://github.com/spack/spack/pull/9199

Instead of replacing rpaths with placeholder and then checking strings, make use of the functions
relocate.is_recocatable and relocate.is_file_relocatable to decide if a package needs the allow-root option.

This fixes a problem where the placeholder path was not in the first rpath entry. This was seen in c++ libraries and binaries because the compiler was outside the spack install base path and always appears first in the rpath.

Instead of checking the first rpath entry, all rpaths have the placeholder path and the old install path (if it exists) replaced with the new install path.

* flake8

3 files changed, 76 insertions, 105 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 46ac7790e4..1d9acd1af9 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -7,7 +7,6 @@ import os
 import re
 import tarfile
 import shutil
-import platform
 import tempfile
 import hashlib
 from contextlib import closing
@@ -17,7 +16,7 @@ import json
 from six.moves.urllib.error import URLError
 
 import llnl.util.tty as tty
-from llnl.util.filesystem import mkdirp, install_tree, get_filetype
+from llnl.util.filesystem import mkdirp, install_tree
 
 import spack.cmd
 import spack.fetch_strategy as fs
@@ -38,6 +37,7 @@ class NoOverwriteException(Exception):
     """
     Raised when a file exists and must be overwritten.
     """
+
     def __init__(self, file_path):
         err_msg = "\n%s\nexists\n" % file_path
         err_msg += "Use -f option to overwrite."
@@ -62,6 +62,7 @@ class PickKeyException(spack.error.SpackError):
     """
     Raised when multiple keys can be used to sign.
     """
+
     def __init__(self, keys):
         err_msg = "Multi keys available for signing\n%s\n" % keys
         err_msg += "Use spack buildcache create -k <key hash> to pick a key."
@@ -133,13 +134,13 @@ def write_buildinfo_file(prefix, workdir, rel=False):
     binary_to_relocate = []
     link_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 blacklist]
         for filename in files:
             path_name = os.path.join(root, filename)
+            m_type, m_subtype = relocate.mime_type(path_name)
             if os.path.islink(path_name):
                 link = os.readlink(path_name)
                 if os.path.isabs(link):
@@ -157,12 +158,12 @@ def write_buildinfo_file(prefix, workdir, rel=False):
             #  This cuts down on the number of files added to the list
             #  of files potentially needing relocation
             elif relocate.strings_contains_installroot(
-                    path_name, spack.store.layout.root):
-                filetype = get_filetype(path_name)
-                if relocate.needs_binary_relocation(filetype, os_id):
+                    path_name,
+                    spack.store.layout.root):
+                if relocate.needs_binary_relocation(m_type, m_subtype):
                     rel_path_name = os.path.relpath(path_name, prefix)
                     binary_to_relocate.append(rel_path_name)
-                elif relocate.needs_text_relocation(filetype):
+                elif relocate.needs_text_relocation(m_type, m_subtype):
                     rel_path_name = os.path.relpath(path_name, prefix)
                     text_to_relocate.append(rel_path_name)
 
@@ -479,8 +480,7 @@ def relocate_package(workdir, allow_root):
         for filename in buildinfo['relocate_binaries']:
             path_name = os.path.join(workdir, filename)
             path_names.add(path_name)
-        relocate.relocate_binary(path_names, old_path, new_path,
-                                 allow_root)
+        relocate.relocate_binary(path_names, old_path, new_path, allow_root)
         path_names = set()
         for filename in buildinfo.get('relocate_links', []):
             path_name = os.path.join(workdir, filename)
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index 6e508c1881..d338242766 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -36,12 +36,16 @@ def get_patchelf():
     # as we may need patchelf, find out where it is
     if platform.system() == 'Darwin':
         return None
-    patchelf_spec = spack.cmd.parse_specs("patchelf", concretize=True)[0]
-    patchelf = spack.repo.get(patchelf_spec)
-    if not patchelf.installed:
-        patchelf.do_install()
-    patchelf_executable = os.path.join(patchelf.prefix.bin, "patchelf")
-    return patchelf_executable
+    patchelf = spack.util.executable.which('patchelf')
+    if patchelf is None:
+        patchelf_spec = spack.cmd.parse_specs("patchelf", concretize=True)[0]
+        patchelf = spack.repo.get(patchelf_spec)
+        if not patchelf.installed:
+            patchelf.do_install()
+        patchelf_executable = os.path.join(patchelf.prefix.bin, "patchelf")
+        return patchelf_executable
+    else:
+        return patchelf.path
 
 
 def get_existing_elf_rpaths(path_name):
@@ -266,31 +270,22 @@ def modify_elf_object(path_name, new_rpaths):
         tty.die('relocation not supported for this platform')
 
 
-def needs_binary_relocation(filetype, os_id=None):
+def needs_binary_relocation(m_type, m_subtype):
     """
     Check whether the given filetype is a binary that may need relocation.
     """
-    retval = False
-    if "relocatable" in filetype:
-        return False
-    if "link to" in filetype:
-        return False
-    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" % os_id)
-    return retval
+    if m_type == 'application':
+        if (m_subtype == 'x-executable' or m_subtype == 'x-sharedlib' or
+           m_subtype == 'x-mach-binary'):
+            return True
+    return False
 
 
-def needs_text_relocation(filetype):
+def needs_text_relocation(m_type, m_subtype):
     """
     Check whether the given filetype is text that may need relocation.
     """
-    if "link to" in filetype:
-        return False
-    return ("text" in filetype)
+    return (m_type == "text")
 
 
 def relocate_binary(path_names, old_dir, new_dir, allow_root):
@@ -302,51 +297,44 @@ def relocate_binary(path_names, old_dir, new_dir, allow_root):
     if platform.system() == 'Darwin':
         for path_name in path_names:
             (rpaths, deps, idpath) = macho_get_paths(path_name)
-            # new style buildaches with placeholder in binaries
-            if (deps[0].startswith(placeholder) or
-                rpaths[0].startswith(placeholder) or
-                (idpath and idpath.startswith(placeholder))):
-                (new_rpaths,
-                 new_deps,
-                 new_idpath) = macho_replace_paths(placeholder,
-                                                   new_dir,
-                                                   rpaths,
-                                                   deps,
-                                                   idpath)
-            # old style buildcaches with original install root in binaries
-            else:
-                (new_rpaths,
-                 new_deps,
-                 new_idpath) = macho_replace_paths(old_dir,
-                                                   new_dir,
-                                                   rpaths,
-                                                   deps,
-                                                   idpath)
+            # one pass to replace placeholder
+            (n_rpaths,
+             n_deps,
+             n_idpath) = macho_replace_paths(placeholder,
+                                             new_dir,
+                                             rpaths,
+                                             deps,
+                                             idpath)
+            # another pass to replace old_dir
+            (new_rpaths,
+             new_deps,
+             new_idpath) = macho_replace_paths(old_dir,
+                                               new_dir,
+                                               n_rpaths,
+                                               n_deps,
+                                               n_idpath)
             modify_macho_object(path_name,
                                 rpaths, deps, idpath,
                                 new_rpaths, new_deps, new_idpath)
             if (not allow_root and
                 old_dir != new_dir and
-                strings_contains_installroot(path_name, old_dir)):
+                    not file_is_relocatable(path_name)):
                 raise InstallRootStringException(path_name, old_dir)
 
     elif platform.system() == 'Linux':
         for path_name in path_names:
             orig_rpaths = get_existing_elf_rpaths(path_name)
             if orig_rpaths:
-                if orig_rpaths[0].startswith(placeholder):
-                    # new style buildaches with placeholder in binaries
-                    new_rpaths = substitute_rpath(orig_rpaths,
-                                                  placeholder, new_dir)
-                else:
-                    # old style buildcaches with original install
-                    # root in binaries
-                    new_rpaths = substitute_rpath(orig_rpaths,
-                                                  old_dir, new_dir)
+                # one pass to replace placeholder
+                n_rpaths = substitute_rpath(orig_rpaths,
+                                            placeholder, new_dir)
+                # one pass to replace old_dir
+                new_rpaths = substitute_rpath(n_rpaths,
+                                              old_dir, new_dir)
                 modify_elf_object(path_name, new_rpaths)
                 if (not allow_root and
                     old_dir != new_dir and
-                    strings_contains_installroot(path_name, old_dir)):
+                    not file_is_relocatable(path_name)):
                     raise InstallRootStringException(path_name, old_dir)
     else:
         tty.die("Relocation not implemented for %s" % platform.system())
@@ -379,8 +367,8 @@ def make_binary_relative(cur_path_names, orig_path_names, old_dir, allow_root):
                                 rpaths, deps, idpath,
                                 new_rpaths, new_deps, new_idpath)
             if (not allow_root and
-                strings_contains_installroot(cur_path)):
-                raise InstallRootStringException(cur_path)
+                not file_is_relocatable(cur_path, old_dir)):
+                raise InstallRootStringException(cur_path, 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)
@@ -388,9 +376,9 @@ def make_binary_relative(cur_path_names, orig_path_names, old_dir, allow_root):
                 new_rpaths = get_relative_rpaths(orig_path, old_dir,
                                                  orig_rpaths)
                 modify_elf_object(cur_path, new_rpaths)
-                if (not allow_root and
-                    strings_contains_installroot(cur_path, old_dir)):
-                    raise InstallRootStringException(cur_path, old_dir)
+            if (not allow_root and
+                    not file_is_relocatable(cur_path, old_dir)):
+                raise InstallRootStringException(cur_path, old_dir)
     else:
         tty.die("Prelocation not implemented for %s" % platform.system())
 
@@ -401,29 +389,16 @@ def make_binary_placeholder(cur_path_names, allow_root):
     """
     if platform.system() == 'Darwin':
         for cur_path in cur_path_names:
-            rpaths, deps, idpath = macho_get_paths(cur_path)
-            (new_rpaths,
-             new_deps,
-             new_idpath) = macho_make_paths_placeholder(rpaths, deps, idpath)
-            modify_macho_object(cur_path,
-                                rpaths, deps, idpath,
-                                new_rpaths, new_deps, new_idpath)
             if (not allow_root and
-                strings_contains_installroot(cur_path,
-                                             spack.store.layout.root)):
+                not file_is_relocatable(cur_path)):
                 raise InstallRootStringException(
                     cur_path, spack.store.layout.root)
     elif platform.system() == 'Linux':
         for cur_path in cur_path_names:
-            orig_rpaths = get_existing_elf_rpaths(cur_path)
-            if orig_rpaths:
-                new_rpaths = get_placeholder_rpaths(cur_path, orig_rpaths)
-                modify_elf_object(cur_path, new_rpaths)
-                if (not allow_root and
-                    strings_contains_installroot(
-                        cur_path, spack.store.layout.root)):
-                    raise InstallRootStringException(
-                        cur_path, spack.store.layout.root)
+            if (not allow_root and
+                not file_is_relocatable(cur_path)):
+                raise InstallRootStringException(
+                    cur_path, spack.store.layout.root)
     else:
         tty.die("Placeholder not implemented for %s" % platform.system())
 
@@ -498,6 +473,8 @@ def is_relocatable(spec):
         raise ValueError('spec is not installed [{0}]'.format(str(spec)))
 
     if spec.external or spec.virtual:
+        tty.warn('external or virtual package %s is not relocatable' %
+                 spec.name)
         return False
 
     # Explore the installation prefix of the spec
@@ -538,7 +515,7 @@ def file_is_relocatable(file):
         raise ValueError('{0} is not an absolute path'.format(file))
 
     strings = Executable('strings')
-    patchelf = Executable('patchelf')
+    patchelf = Executable(get_patchelf())
 
     # Remove the RPATHS from the strings in the executable
     set_of_strings = set(strings(file, output=str).split())
@@ -600,6 +577,6 @@ def mime_type(file):
         Tuple containing the MIME type and subtype
     """
     file_cmd = Executable('file')
-    output = file_cmd('-b', '--mime-type', file, output=str, error=str)
+    output = file_cmd('-b', '-h', '--mime-type', file, output=str, error=str)
     tty.debug('[MIME_TYPE] {0} -> {1}'.format(file, output.strip()))
     return tuple(output.strip().split('/'))
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index bb086f20c7..c83ebc30ea 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -157,7 +157,7 @@ echo $PATH"""
     else:
         # create build cache without signing
         args = parser.parse_args(
-            ['create', '-d', mirror_path, '-u', str(spec)])
+            ['create', '-d', mirror_path, '-f', '-u', str(spec)])
         buildcache.buildcache(parser, args)
 
         # Uninstall the package
@@ -265,22 +265,16 @@ def test_relocate_links(tmpdir):
 
 
 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')
+
+    assert needs_binary_relocation('application', 'x-sharedlib')
+    assert needs_binary_relocation('application', 'x-executable')
+    assert not needs_binary_relocation('application', 'x-octet-stream')
+    assert not needs_binary_relocation('text', 'x-')
+
+    assert needs_text_relocation('text', 'x-')
+    assert not needs_text_relocation('symbolic link to', 'x-')
+
+    assert needs_binary_relocation('application', 'x-mach-binary')
 
 
 def test_macho_paths():