Improved binary relocation: error checking and reporting (#7314)

Fixes #7237
Fixes #6404
Fixes #6418
Fixes #6369

Identify when binary relocation fails to remove all instances of the
source prefix (and report an error to the user unless they specify
-a to allow the old root to appear). This check occurs at two stages:
during "bincache create" all instances of the root are replaced with
a special placeholder string ("@@@@..."), and a failure occurs if the
root is detected at this point; when the binary package is extracted
there is a second check. This addresses #7237 and #6418.

This is intended to be compatible with previously-created binary
packages.

This also adds:

* Better error messages for "spack install --use-cache" (#6404)
* Faster relocation on Mac OS (using a single call to
  install_name_tool for all files rather than a call for each file)
* Clean up when "buildcache create" fails (addresses #6369)
* Explicit error message when the spack instance extracting the binary
  package uses a different install layout than the spack instance that
  created the binary package (since this is currently not supported)
* Remove the option to create unsigned binary packages with -y

lib/spack/spack/binary_distribution.py

@@ -29,6 +29,7 @@
 import yaml
 import shutil
 import platform
+import tempfile
 
 import llnl.util.tty as tty
 from spack.util.gpg import Gpg
@@ -46,26 +47,57 @@
 
 
 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."
+        super(NoOverwriteException, self).__init__(err_msg)
+
+
+class NoGpgException(spack.error.SpackError):
+    """
+    Raised when gpg2 is not in PATH
+    """
     pass
 
 
-class NoGpgException(Exception):
+class NoKeyException(spack.error.SpackError):
+    """
+    Raised when gpg has no default key added.
+    """
     pass
 
 
-class PickKeyException(Exception):
+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."
+        super(PickKeyException, self).__init__(err_msg)
+
+
+class NoVerifyException(spack.error.SpackError):
+    """
+    Raised if file fails signature verification.
+    """
     pass
 
 
-class NoKeyException(Exception):
+class NoChecksumException(spack.error.SpackError):
+    """
+    Raised if file fails checksum verification.
+    """
     pass
 
 
-class NoVerifyException(Exception):
-    pass
-
-
-class NoChecksumException(Exception):
+class NewLayoutException(spack.error.SpackError):
+    """
+    Raised if directory layout is different from buildcache.
+    """
     pass
 
 
@@ -114,7 +146,8 @@ def write_buildinfo_file(prefix, workdir, rel=False):
             #  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):
+            if relocate.strings_contains_installroot(path_name,
+                                                     spack.store.layout.root):
                 filetype = relocate.get_filetype(path_name)
                 if relocate.needs_binary_relocation(filetype, os_id):
                     rel_path_name = os.path.relpath(path_name, prefix)
@@ -127,6 +160,8 @@ def write_buildinfo_file(prefix, workdir, rel=False):
     buildinfo = {}
     buildinfo['relative_rpaths'] = rel
     buildinfo['buildpath'] = spack.store.layout.root
+    buildinfo['relative_prefix'] = os.path.relpath(prefix,
+                                                   spack.store.layout.root)
     buildinfo['relocate_textfiles'] = text_to_relocate
     buildinfo['relocate_binaries'] = binary_to_relocate
     filename = buildinfo_file_name(workdir)
@@ -178,19 +213,24 @@ def checksum_tarball(file):
     return hasher.hexdigest()
 
 
-def sign_tarball(yes_to_all, key, force, specfile_path):
+def sign_tarball(key, force, specfile_path):
     # Sign the packages if keys available
     if not has_gnupg2():
-        raise NoGpgException()
+        raise NoGpgException(
+            "gpg2 is not available in $PATH .\n"
+            "Use spack install gnupg and spack load gnupg.")
     else:
         if key is None:
             keys = Gpg.signing_keys()
             if len(keys) == 1:
                 key = keys[0]
             if len(keys) > 1:
-                raise PickKeyException()
+                raise PickKeyException(str(keys))
             if len(keys) == 0:
-                raise NoKeyException()
+                msg = "No default key available for signing.\n"
+                msg += "Use spack gpg init and spack gpg create"
+                msg += " to create a default key."
+                raise NoKeyException(msg)
     if os.path.exists('%s.asc' % specfile_path):
         if force:
             os.remove('%s.asc' % specfile_path)
@@ -214,7 +254,7 @@ def generate_index(outdir, indexfile_path):
     f.close()
 
 
-def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False,
+def build_tarball(spec, outdir, force=False, rel=False, allow_root=False,
                   key=None):
     """
     Build a tarball from given spec and put it into the directory structure
@@ -247,9 +287,7 @@ def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False,
         else:
             raise NoOverwriteException(str(specfile_path))
     # make a copy of the install directory to work with
-    workdir = join_path(outdir, os.path.basename(spec.prefix))
-    if os.path.exists(workdir):
-        shutil.rmtree(workdir)
+    workdir = join_path(tempfile.mkdtemp(), os.path.basename(spec.prefix))
     install_tree(spec.prefix, workdir, symlinks=True)
 
     # create info for later relocation and create tar
@@ -258,11 +296,23 @@ def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False,
     # optinally make the paths in the binaries relative to each other
     # in the spack install tree before creating tarball
     if rel:
-        make_package_relative(workdir, spec.prefix)
+        try:
+            make_package_relative(workdir, spec.prefix, allow_root)
+        except Exception as e:
+            shutil.rmtree(workdir)
+            shutil.rmtree(tarfile_dir)
+            tty.die(str(e))
+    else:
+        try:
+            make_package_placeholder(workdir, allow_root)
+        except Exception as e:
+            shutil.rmtree(workdir)
+            shutil.rmtree(tarfile_dir)
+            tty.die(str(e))
     # create compressed tarball of the install prefix
     with closing(tarfile.open(tarfile_path, 'w:gz')) as tar:
         tar.add(name='%s' % workdir,
-                arcname='%s' % os.path.basename(workdir))
+                arcname='%s' % os.path.basename(spec.prefix))
     # remove copy of install directory
     shutil.rmtree(workdir)
 
@@ -278,32 +328,26 @@ def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False,
     bchecksum['hash_algorithm'] = 'sha256'
     bchecksum['hash'] = checksum
     spec_dict['binary_cache_checksum'] = bchecksum
+    # Add original install prefix relative to layout root to spec.yaml.
+    # This will be used to determine is the directory layout has changed.
+    buildinfo = {}
+    buildinfo['relative_prefix'] = os.path.relpath(spec.prefix,
+                                                   spack.store.layout.root)
+    spec_dict['buildinfo'] = buildinfo
     with open(specfile_path, 'w') as outfile:
         outfile.write(yaml.dump(spec_dict))
-    signed = False
-    if not yes_to_all:
-        # sign the tarball and spec file with gpg
-        try:
-            sign_tarball(yes_to_all, key, force, specfile_path)
-            signed = True
-        except NoGpgException:
-            raise NoGpgException()
-        except PickKeyException:
-            raise PickKeyException()
-        except NoKeyException():
-            raise NoKeyException()
+    # sign the tarball and spec file with gpg
+    sign_tarball(key, force, specfile_path)
     # put tarball, spec and signature files in .spack archive
     with closing(tarfile.open(spackfile_path, 'w')) as tar:
         tar.add(name='%s' % tarfile_path, arcname='%s' % tarfile_name)
         tar.add(name='%s' % specfile_path, arcname='%s' % specfile_name)
-        if signed:
-            tar.add(name='%s.asc' % specfile_path,
-                    arcname='%s.asc' % specfile_name)
+        tar.add(name='%s.asc' % specfile_path,
+                arcname='%s.asc' % specfile_name)
 
     # cleanup file moved to archive
     os.remove(tarfile_path)
-    if signed:
-        os.remove('%s.asc' % specfile_path)
+    os.remove('%s.asc' % specfile_path)
 
     # create an index.html for the build_cache directory so specs can be found
     if os.path.exists(indexfile_path):
@@ -334,7 +378,7 @@ def download_tarball(spec):
     return None
 
 
-def make_package_relative(workdir, prefix):
+def make_package_relative(workdir, prefix, allow_root):
     """
     Change paths in binaries to relative paths
     """
@@ -345,15 +389,26 @@ def make_package_relative(workdir, prefix):
     for filename in buildinfo['relocate_binaries']:
         orig_path_names.append(os.path.join(prefix, filename))
         cur_path_names.append(os.path.join(workdir, filename))
-        relocate.make_binary_relative(cur_path_names, orig_path_names,
-                                      old_path)
+    relocate.make_binary_relative(cur_path_names, orig_path_names,
+                                  old_path, allow_root)
 
 
-def relocate_package(prefix):
+def make_package_placeholder(workdir, allow_root):
+    """
+    Change paths in binaries to placeholder paths
+    """
+    buildinfo = read_buildinfo_file(workdir)
+    cur_path_names = list()
+    for filename in buildinfo['relocate_binaries']:
+        cur_path_names.append(os.path.join(workdir, filename))
+    relocate.make_binary_placeholder(cur_path_names, allow_root)
+
+
+def relocate_package(workdir, allow_root):
     """
     Relocate the given package
     """
-    buildinfo = read_buildinfo_file(prefix)
+    buildinfo = read_buildinfo_file(workdir)
     new_path = spack.store.layout.root
     old_path = buildinfo['buildpath']
     rel = buildinfo.get('relative_rpaths', False)
@@ -364,7 +419,7 @@ def relocate_package(prefix):
             "%s to %s." % (old_path, new_path))
     path_names = set()
     for filename in buildinfo['relocate_textfiles']:
-        path_name = os.path.join(prefix, filename)
+        path_name = os.path.join(workdir, filename)
         # Don't add backup files generated by filter_file during install step.
         if not path_name.endswith('~'):
             path_names.add(path_name)
@@ -374,39 +429,46 @@ def relocate_package(prefix):
     if not rel:
         path_names = set()
         for filename in buildinfo['relocate_binaries']:
-            path_name = os.path.join(prefix, filename)
+            path_name = os.path.join(workdir, filename)
             path_names.add(path_name)
-        relocate.relocate_binary(path_names, old_path, new_path)
+        relocate.relocate_binary(path_names, old_path, new_path,
+                                 allow_root)
 
 
-def extract_tarball(spec, filename, yes_to_all=False, force=False):
+def extract_tarball(spec, filename, allow_root=False, force=False):
     """
     extract binary tarball for given package into install area
     """
-    installpath = spec.prefix
-    if os.path.exists(installpath):
+    if os.path.exists(spec.prefix):
         if force:
-            shutil.rmtree(installpath)
+            shutil.rmtree(spec.prefix)
         else:
-            raise NoOverwriteException(str(installpath))
+            raise NoOverwriteException(str(spec.prefix))
+
+    tmpdir = tempfile.mkdtemp()
     stagepath = os.path.dirname(filename)
     spackfile_name = tarball_name(spec, '.spack')
     spackfile_path = os.path.join(stagepath, spackfile_name)
     tarfile_name = tarball_name(spec, '.tar.gz')
-    tarfile_path = os.path.join(stagepath, tarfile_name)
+    tarfile_path = os.path.join(tmpdir, tarfile_name)
     specfile_name = tarball_name(spec, '.spec.yaml')
-    specfile_path = os.path.join(stagepath, specfile_name)
+    specfile_path = os.path.join(tmpdir, specfile_name)
 
     with closing(tarfile.open(spackfile_path, 'r')) as tar:
-        tar.extractall(stagepath)
+        tar.extractall(tmpdir)
 
-    if not yes_to_all:
-        if os.path.exists('%s.asc' % specfile_path):
+    if os.path.exists('%s.asc' % specfile_path):
+        try:
             Gpg.verify('%s.asc' % specfile_path, specfile_path)
-            os.remove(specfile_path + '.asc')
-        else:
-            raise NoVerifyException()
-
+        except Exception as e:
+            shutil.rmtree(tmpdir)
+            tty.die(str(e))
+    else:
+        shutil.rmtree(tmpdir)
+        raise NoVerifyException(
+            "Package spec file failed signature verification.\n"
+            "Use spack buildcache keys to download "
+            "and install a key for verification from the mirror.")
     # get the sha256 checksum of the tarball
     checksum = checksum_tarball(tarfile_path)
 
@@ -419,16 +481,48 @@ def extract_tarball(spec, filename, yes_to_all=False, force=False):
 
     # if the checksums don't match don't install
     if bchecksum['hash'] != checksum:
-        raise NoChecksumException()
+        shutil.rmtree(tmpdir)
+        raise NoChecksumException(
+            "Package tarball failed checksum verification.\n"
+            "It cannot be installed.")
 
-    # delay creating installpath until verification is complete
-    mkdirp(installpath)
+    new_relative_prefix = str(os.path.relpath(spec.prefix,
+                                              spack.store.layout.root))
+    # if the original relative prefix is in the spec file use it
+    buildinfo = spec_dict.get('buildinfo', {})
+    old_relative_prefix = buildinfo.get('relative_prefix', new_relative_prefix)
+    # if the original relative prefix and new relative prefix differ the
+    # directory layout has changed and the  buildcache cannot be installed
+    if old_relative_prefix != new_relative_prefix:
+        shutil.rmtree(tmpdir)
+        msg = "Package tarball was created from an install "
+        msg += "prefix with a different directory layout.\n"
+        msg += "It cannot be relocated."
+        raise NewLayoutException(msg)
+
+    # extract the tarball in a temp directory
     with closing(tarfile.open(tarfile_path, 'r')) as tar:
-        tar.extractall(path=join_path(installpath, '..'))
+        tar.extractall(path=tmpdir)
+    # the base of the install prefix is used when creating the tarball
+    # so the pathname should be the same now that the directory layout
+    # is confirmed
+    workdir = join_path(tmpdir, os.path.basename(spec.prefix))
 
+    # cleanup
     os.remove(tarfile_path)
     os.remove(specfile_path)
-    relocate_package(installpath)
+
+    try:
+        relocate_package(workdir, allow_root)
+    except Exception as e:
+        shutil.rmtree(workdir)
+        tty.die(str(e))
+    # Delay creating spec.prefix until verification is complete
+    # and any relocation has been done.
+    else:
+        install_tree(workdir, spec.prefix, symlinks=True)
+    finally:
+        shutil.rmtree(tmpdir)
 
 
 def get_specs(force=False):

lib/spack/spack/cmd/buildcache.py

@@ -24,15 +24,11 @@
 ##############################################################################
 import argparse
 
-import os
 import llnl.util.tty as tty
 
 import spack
 import spack.cmd
 import spack.binary_distribution as bindist
-from spack.binary_distribution import NoOverwriteException, NoGpgException
-from spack.binary_distribution import NoKeyException, PickKeyException
-from spack.binary_distribution import NoVerifyException, NoChecksumException
 
 description = "create, download and install binary packages"
 section = "packaging"
@@ -49,9 +45,9 @@ def setup_parser(subparser):
                              " before creating tarballs.")
     create.add_argument('-f', '--force', action='store_true',
                         help="overwrite tarball if it exists.")
-    create.add_argument('-y', '--yes-to-all', action='store_true',
-                        help="answer yes to all create unsigned " +
-                             "buildcache questions")
+    create.add_argument('-a', '--allow_root', action='store_true',
+                        help="allow install root string in binary files " +
+                             "after RPATH substitution")
     create.add_argument('-k', '--key', metavar='key',
                         type=str, default=None,
                         help="Key for signing.")
@@ -66,9 +62,11 @@ def setup_parser(subparser):
     install = subparsers.add_parser('install', help=installtarball.__doc__)
     install.add_argument('-f', '--force', action='store_true',
                          help="overwrite install directory if it exists.")
-    install.add_argument('-y', '--yes-to-all', action='store_true',
-                         help="answer yes to all install unsigned " +
-                              "buildcache questions")
+    install.add_argument('-m', '--multiple', action='store_true',
+                         help="allow all matching packages ")
+    install.add_argument('-a', '--allow_root', action='store_true',
+                         help="allow install root string in binary files " +
+                              "after RPATH substitution")
     install.add_argument(
         'packages', nargs=argparse.REMAINDER,
         help="specs of packages to install biuldache for")
@@ -87,7 +85,7 @@ def setup_parser(subparser):
         '-i', '--install', action='store_true',
         help="install Keys pulled from mirror")
     dlkeys.add_argument(
-        '-y', '--yes-to-all', action='store_true',
+        '-y', '--yes_to_all', action='store_true',
         help="answer yes to all trust questions")
     dlkeys.add_argument('-f', '--force', action='store_true',
                         help="force new download of keys")
@@ -185,17 +183,17 @@ def createtarball(args):
                 " installed package argument")
     pkgs = set(args.packages)
     specs = set()
-    outdir = os.getcwd()
+    outdir = '.'
     if args.directory:
         outdir = args.directory
     signkey = None
     if args.key:
         signkey = args.key
-    yes_to_all = False
+    allow_root = False
     force = False
     relative = False
-    if args.yes_to_all:
-        yes_to_all = True
+    if args.allow_root:
+        allow_root = True
     if args.force:
         force = True
     if args.rel:
@@ -220,24 +218,12 @@ def createtarball(args):
                     tty.msg('adding dependency %s' % node.format())
                     specs.add(node)
 
+    tty.msg('writing tarballs to %s/build_cache' % outdir)
+
     for spec in specs:
         tty.msg('creating binary cache file for package %s ' % spec.format())
-        try:
-            bindist.build_tarball(spec, outdir, force,
-                                  relative, yes_to_all, signkey)
-        except NoOverwriteException as e:
-            tty.warn("%s exists, use -f to force overwrite." % e)
-        except NoGpgException:
-            tty.die("gpg2 is not available,"
-                    " use -y to create unsigned build caches")
-        except NoKeyException:
-            tty.die("no default key available for signing,"
-                    " use -y to create unsigned build caches"
-                    " or spack gpg init to create a default key")
-        except PickKeyException:
-            tty.die("multi keys available for signing,"
-                    " use -y to create unsigned build caches"
-                    " or -k <key hash> to pick a key")
+        bindist.build_tarball(spec, outdir, force,
+                              relative, allow_root, signkey)
 
 
 def installtarball(args):
@@ -246,13 +232,13 @@ def installtarball(args):
         tty.die("build cache file installation requires" +
                 " at least one package spec argument")
     pkgs = set(args.packages)
-    yes_to_all = False
-    if args.yes_to_all:
-        yes_to_all = True
+    multiple = False
+    if args.multiple:
+        multiple = True
     force = False
     if args.force:
         force = True
-    matches = match_downloaded_specs(pkgs, yes_to_all, force)
+    matches = match_downloaded_specs(pkgs, multiple, force)
 
     for match in matches:
         install_tarball(match, args)
@@ -263,9 +249,9 @@ def install_tarball(spec, args):
     if s.external or s.virtual:
         tty.warn("Skipping external or virtual package %s" % spec.format())
         return
-    yes_to_all = False
-    if args.yes_to_all:
-        yes_to_all = True
+    allow_root = False
+    if args.allow_root:
+        allow_root = True
     force = False
     if args.force:
         force = True
@@ -274,24 +260,13 @@ def install_tarball(spec, args):
         install_tarball(d, args)
     package = spack.repo.get(spec)
     if s.concrete and package.installed and not force:
-        tty.warn("Package for spec %s already installed." % spec.format(),
-                 " Use -f flag to overwrite.")
+        tty.warn("Package for spec %s already installed." % spec.format())
     else:
         tarball = bindist.download_tarball(spec)
         if tarball:
             tty.msg('Installing buildcache for spec %s' % spec.format())
-            try:
-                bindist.extract_tarball(spec, tarball, yes_to_all, force)
-            except NoOverwriteException as e:
-                tty.warn("%s exists. use -f to force overwrite." % e.args)
-            except NoVerifyException:
-                tty.die("Package spec file failed signature verification,"
-                        " use -y flag to install build cache")
-            except NoChecksumException:
-                tty.die("Package tarball failed checksum verification,"
-                        " use -y flag to install build cache")
-            finally:
-                spack.store.db.reindex(spack.store.layout)
+            bindist.extract_tarball(spec, tarball, allow_root, force)
+            spack.store.db.reindex(spack.store.layout)
         else:
             tty.die('Download of binary cache file for spec %s failed.' %
                     spec.format())

lib/spack/spack/package.py

@@ -1288,7 +1288,7 @@ def try_install_from_binary_cache(self, explicit):
         tty.msg('Installing %s from binary cache' % self.name)
         tarball = binary_distribution.download_tarball(binary_spec)
         binary_distribution.extract_tarball(
-            binary_spec, tarball, yes_to_all=False, force=False)
+            binary_spec, tarball, allow_root=False, force=False)
         spack.store.db.add(self.spec, spack.store.layout, explicit=explicit)
         return True
 

lib/spack/spack/relocate.py

@@ -33,6 +33,18 @@
 import llnl.util.tty as tty
 
 
+class InstallRootStringException(spack.error.SpackError):
+    """
+    Raised when the relocated binary still has the install root string.
+    """
+    def __init__(self, file_path, root_path):
+        super(InstallRootStringException, self).__init__(
+            "\n %s \ncontains string\n %s \n"
+            "after replacing it in rpaths.\n"
+            "Package should not be relocated.\n Use -a to override." %
+            (file_path, root_path))
+
+
 def get_patchelf():
     """
     Builds and installs spack patchelf package on linux platforms
@@ -86,6 +98,29 @@ def get_relative_rpaths(path_name, orig_dir, orig_rpaths):
     return rel_rpaths
 
 
+def set_placeholder(dirname):
+    """
+    return string of @'s with same length
+    """
+    return '@' * len(dirname)
+
+
+def get_placeholder_rpaths(path_name, orig_rpaths):
+    """
+    Replaces original layout root dir with a placeholder string in all rpaths.
+    """
+    rel_rpaths = []
+    orig_dir = spack.store.layout.root
+    for rpath in orig_rpaths:
+        if re.match(orig_dir, rpath):
+            placeholder = set_placeholder(orig_dir)
+            rel = re.sub(orig_dir, placeholder, rpath)
+            rel_rpaths.append('%s' % rel)
+        else:
+            rel_rpaths.append(rpath)
+    return rel_rpaths
+
+
 def macho_get_paths(path_name):
     """
     Examines the output of otool -l path_name for these three fields:
@@ -121,7 +156,7 @@ def macho_get_paths(path_name):
 def macho_make_paths_relative(path_name, old_dir, rpaths, deps, idpath):
     """
     Replace old_dir with relative path from dirname(path_name)
-    in rpaths and deps; idpaths are replaced with @rpath/basebane(path_name);
+    in rpaths and deps; idpaths are replaced with @rpath/libname as needed;
     replacement are returned.
     """
     new_idpath = None
@@ -144,6 +179,34 @@ def macho_make_paths_relative(path_name, old_dir, rpaths, deps, idpath):
     return (new_rpaths, new_deps, new_idpath)
 
 
+def macho_make_paths_placeholder(rpaths, deps, idpath):
+    """
+    Replace old_dir with a placeholder of the same length
+    in rpaths and deps and idpaths is needed.
+    replacement are returned.
+    """
+    new_idpath = None
+    old_dir = spack.store.layout.root
+    placeholder = set_placeholder(old_dir)
+    if idpath:
+        new_idpath = re.sub(old_dir, placeholder, idpath)
+    new_rpaths = list()
+    new_deps = list()
+    for rpath in rpaths:
+        if re.match(old_dir, rpath):
+            ph = re.sub(old_dir, placeholder, rpath)
+            new_rpaths.append('%s' % ph)
+        else:
+            new_rpaths.append(rpath)
+    for dep in deps:
+        if re.match(old_dir, dep):
+            ph = re.sub(old_dir, placeholder, dep)
+            new_deps.append('%s' % ph)
+        else:
+            new_deps.append(dep)
+    return (new_rpaths, new_deps, new_idpath)
+
+
 def macho_replace_paths(old_dir, new_dir, rpaths, deps, idpath):
     """
     Replace old_dir with new_dir in rpaths, deps and idpath
@@ -179,15 +242,17 @@ def modify_macho_object(cur_path, rpaths, deps, idpath,
     if 'libgcc_' in cur_path:
         return
     install_name_tool = Executable('install_name_tool')
+    args = []
     if new_idpath:
-        install_name_tool('-id', new_idpath, str(cur_path),
-                          output=str, err=str)
+        args.extend(['-id', new_idpath])
 
     for orig, new in zip(deps, new_deps):
-        install_name_tool('-change', orig, new, str(cur_path))
+        args.extend(['-change', orig, new])
 
     for orig, new in zip(rpaths, new_rpaths):
-        install_name_tool('-rpath', orig, new, str(cur_path))
+        args.extend(['-rpath', orig, new])
+    args.append(str(cur_path))
+    install_name_tool(*args)
     return
 
 
@@ -202,14 +267,14 @@ def get_filetype(path_name):
     return output.strip()
 
 
-def strings_contains_installroot(path_name):
+def strings_contains_installroot(path_name, root_dir):
     """
     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)
+    return (root_dir in output)
 
 
 def modify_elf_object(path_name, new_rpaths):
@@ -257,34 +322,66 @@ def needs_text_relocation(filetype):
     return ("text" in filetype)
 
 
-def relocate_binary(path_names, old_dir, new_dir):
+def relocate_binary(path_names, old_dir, new_dir, allow_root):
     """
     Change old_dir to new_dir in RPATHs of elf or mach-o files
+    Account for the case where old_dir is now a placeholder
     """
+    placeholder = set_placeholder(old_dir)
     if platform.system() == 'Darwin':
         for path_name in path_names:
-            rpaths, deps, idpath = macho_get_paths(path_name)
-            new_rpaths, new_deps, new_idpath = macho_replace_paths(old_dir,
-                                                                   new_dir,
-                                                                   rpaths,
-                                                                   deps,
-                                                                   idpath)
+            (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)
             modify_macho_object(path_name,
                                 rpaths, deps, idpath,
                                 new_rpaths, new_deps, new_idpath)
+            if (not allow_root and
+                strings_contains_installroot(path_name, old_dir)):
+                    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:
-                new_rpaths = substitute_rpath(orig_rpaths, old_dir, new_dir)
+                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)
                 modify_elf_object(path_name, new_rpaths)
+                if (not allow_root and
+                    strings_contains_installroot(path_name, old_dir)):
+                        raise InstallRootStringException(path_name, old_dir)
     else:
         tty.die("Relocation not implemented for %s" % platform.system())
 
 
-def make_binary_relative(cur_path_names, orig_path_names, old_dir):
+def make_binary_relative(cur_path_names, orig_path_names, old_dir, allow_root):
     """
-    Make RPATHs relative to old_dir in given elf or mach-o files
+    Replace old RPATHs with paths relative to old_dir in binary files
     """
     if platform.system() == 'Darwin':
         for cur_path, orig_path in zip(cur_path_names, orig_path_names):
@@ -296,6 +393,9 @@ def make_binary_relative(cur_path_names, orig_path_names, old_dir):
             modify_macho_object(cur_path,
                                 rpaths, deps, idpath,
                                 new_rpaths, new_deps, new_idpath)
+            if (not allow_root and
+                strings_contains_installroot(cur_path)):
+                    raise InstallRootStringException(cur_path)
     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)
@@ -303,10 +403,46 @@ def make_binary_relative(cur_path_names, orig_path_names, old_dir):
                 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)
     else:
         tty.die("Prelocation not implemented for %s" % platform.system())
 
 
+def make_binary_placeholder(cur_path_names, allow_root):
+    """
+    Replace old install root in RPATHs with placeholder in binary files
+    """
+    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)):
+                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)
+    else:
+        tty.die("Placeholder not implemented for %s" % platform.system())
+
+
 def relocate_text(path_names, old_dir, new_dir):
     """
     Replace old path with new path in text file path_name

lib/spack/spack/test/packaging.py

@@ -73,7 +73,7 @@ def fake_fetchify(url, pkg):
 
 
 @pytest.mark.usefixtures('install_mockery', 'testing_gpg_directory')
-def test_packaging(mock_archive, tmpdir):
+def test_buildcache(mock_archive, tmpdir):
     # tweak patchelf to only do a download
     spec = Spec("patchelf")
     spec.concretize()
@@ -218,22 +218,20 @@ def test_packaging(mock_archive, tmpdir):
 
 
 def test_relocate_text(tmpdir):
-    # Validate the text path replacement
-    old_dir = '/home/spack/opt/spack'
-    filename = str(tmpdir) + '/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)
-
-    assert(strings_contains_installroot(filename) is False)
-
-    with open(filename, "r") as script:
-        for line in script:
-            assert(new_dir in line)
+    with tmpdir.as_cwd():
+        # 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)
+        assert(strings_contains_installroot(filename, old_dir) is False)
 
 
 def test_needs_relocation():