spack buildcache: symlink and relative RPATH fixes (#6140)

* Skip rewriting files that are links (this also avoids issues with parsing
  the output of the 'file' command for symlinks)
* Fail rather than warn for several gpg signing issues (e.g. if there is no
  key available to sign with)
* Installation with 'buildcache install' only retrieves link and run
  dependencies (this matches 'buildcache create' which only creates tarballs
  of link/run dependencies)
* Don't rewrite RPATHs for a binary if the binary cached package was created
  with relative RPATHs
* Refactor relocate_binary to operate on multiple binaries (given as a
  collection of paths). This was likewise done for relocate_text and
  make_binary_relative

4 files changed, 153 insertions, 106 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 8b812db687..3198e7b4ee 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -95,7 +95,7 @@ def read_buildinfo_file(prefix):
     return buildinfo
 
 
-def write_buildinfo_file(prefix):
+def write_buildinfo_file(prefix, rel=False):
     """
     Create a cache file containing information
     required for the relocation
@@ -119,6 +119,7 @@ def write_buildinfo_file(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
@@ -132,7 +133,7 @@ def tarball_directory_name(spec):
     Return name of the tarball directory according to the convention
     <os>-<architecture>/<compiler>/<package>-<version>/
     """
-    return "%s/%s/%s-%s" % (spack.architecture.sys_type(),
+    return "%s/%s/%s-%s" % (spec.architecture,
                             str(spec.compiler).replace("@", "-"),
                             spec.name, spec.version)
 
@@ -142,7 +143,7 @@ def tarball_name(spec, ext):
     Return the name of the tarfile according to the convention
     <os>-<architecture>-<package>-<dag_hash><ext>
     """
-    return "%s-%s-%s-%s-%s%s" % (spack.architecture.sys_type(),
+    return "%s-%s-%s-%s-%s%s" % (spec.architecture,
                                  str(spec.compiler).replace("@", "-"),
                                  spec.name,
                                  spec.version,
@@ -246,7 +247,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)
+    write_buildinfo_file(workdir, rel=rel)
 
     # optinally make the paths in the binaries relative to each other
     # in the spack install tree before creating tarball
@@ -333,10 +334,13 @@ def make_package_relative(workdir, prefix):
     """
     buildinfo = read_buildinfo_file(workdir)
     old_path = buildinfo['buildpath']
+    orig_path_names = list()
+    cur_path_names = list()
     for filename in buildinfo['relocate_binaries']:
-        orig_path_name = os.path.join(prefix, filename)
-        cur_path_name = os.path.join(workdir, filename)
-        relocate.make_binary_relative(cur_path_name, orig_path_name, old_path)
+        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)
 
 
 def relocate_package(prefix):
@@ -346,18 +350,25 @@ def relocate_package(prefix):
     buildinfo = read_buildinfo_file(prefix)
     new_path = spack.store.layout.root
     old_path = buildinfo['buildpath']
-    if new_path == old_path:
+    rel = buildinfo.get('relative_rpaths', False)
+    if new_path == old_path and not rel:
         return
 
     tty.msg("Relocating package from",
             "%s to %s." % (old_path, new_path))
-    for filename in buildinfo['relocate_binaries']:
-        path_name = os.path.join(prefix, filename)
-        relocate.relocate_binary(path_name, old_path, new_path)
-
+    path_names = set()
     for filename in buildinfo['relocate_textfiles']:
         path_name = os.path.join(prefix, filename)
-        relocate.relocate_text(path_name, old_path, new_path)
+        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 buildinfo['relocate_binaries']:
+            path_name = os.path.join(prefix, filename)
+            path_names.add(path_name)
+        relocate.relocate_binary(path_names, old_path, new_path)
 
 
 def extract_tarball(spec, filename, yes_to_all=False, force=False):
@@ -391,17 +402,16 @@ def extract_tarball(spec, filename, yes_to_all=False, force=False):
     # get the sha256 checksum of the tarball
     checksum = checksum_tarball(tarfile_path)
 
-    if not yes_to_all:
-        # get the sha256 checksum recorded at creation
-        spec_dict = {}
-        with open(specfile_path, 'r') as inputfile:
-            content = inputfile.read()
-            spec_dict = yaml.load(content)
-        bchecksum = spec_dict['binary_cache_checksum']
-
-        # if the checksums don't match don't install
-        if bchecksum['hash'] != checksum:
-            raise NoChecksumException()
+    # get the sha256 checksum recorded at creation
+    spec_dict = {}
+    with open(specfile_path, 'r') as inputfile:
+        content = inputfile.read()
+        spec_dict = yaml.load(content)
+    bchecksum = spec_dict['binary_cache_checksum']
+
+    # if the checksums don't match don't install
+    if bchecksum['hash'] != checksum:
+        raise NoChecksumException()
 
     # delay creating installpath until verification is complete
     mkdirp(installpath)
diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py
index 64ac8e4d42..653c59074a 100644
--- a/lib/spack/spack/cmd/buildcache.py
+++ b/lib/spack/spack/cmd/buildcache.py
@@ -227,16 +227,16 @@ def createtarball(args):
         except NoOverwriteException as e:
             tty.warn("%s exists, use -f to force overwrite." % e)
         except NoGpgException:
-            tty.warn("gpg2 is not available,"
-                     " use -y to create unsigned build caches")
+            tty.die("gpg2 is not available,"
+                    " use -y to create unsigned build caches")
         except NoKeyException:
-            tty.warn("no default key available for signing,"
-                     " use -y to create unsigned build caches"
-                     " or spack gpg init to create a default key")
+            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.warn("multi keys available for signing,"
-                     " use -y to create unsigned build caches"
-                     " or -k <key hash> to pick a key")
+            tty.die("multi keys available for signing,"
+                    " use -y to create unsigned build caches"
+                    " or -k <key hash> to pick a key")
 
 
 def installtarball(args):
@@ -267,7 +267,7 @@ def install_tarball(spec, args):
     force = False
     if args.force:
         force = True
-    for d in s.dependencies():
+    for d in s.dependencies(deptype=('link', 'run')):
         tty.msg("Installing buildcache for dependency spec %s" % d)
         install_tarball(d, args)
     package = spack.repo.get(spec)
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index b742db2d6b..9341b55c86 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -91,7 +91,7 @@ def macho_get_paths(path_name):
     otool = Executable('otool')
     output = otool("-l", path_name, output=str, err=str)
     last_cmd = None
-    idpath = ''
+    idpath = None
     rpaths = []
     deps = []
     for line in output.split('\n'):
@@ -119,24 +119,24 @@ def macho_make_paths_relative(path_name, old_dir, rpaths, deps, idpath):
     in rpaths and deps; idpaths are replaced with @rpath/basebane(path_name);
     replacement are returned.
     """
-    id = None
-    nrpaths = []
-    ndeps = []
+    new_idpath = None
     if idpath:
-        id = '@rpath/%s' % os.path.basename(idpath)
+        new_idpath = '@rpath/%s' % os.path.basename(idpath)
+    new_rpaths = list()
+    new_deps = list()
     for rpath in rpaths:
         if re.match(old_dir, rpath):
             rel = os.path.relpath(rpath, start=os.path.dirname(path_name))
-            nrpaths.append('@loader_path/%s' % rel)
+            new_rpaths.append('@loader_path/%s' % rel)
         else:
-            nrpaths.append(rpath)
+            new_rpaths.append(rpath)
     for dep in deps:
         if re.match(old_dir, dep):
             rel = os.path.relpath(dep, start=os.path.dirname(path_name))
-            ndeps.append('@loader_path/%s' % rel)
+            new_deps.append('@loader_path/%s' % rel)
         else:
-            ndeps.append(dep)
-    return nrpaths, ndeps, id
+            new_deps.append(dep)
+    return (new_rpaths, new_deps, new_idpath)
 
 
 def macho_replace_paths(old_dir, new_dir, rpaths, deps, idpath):
@@ -144,22 +144,22 @@ def macho_replace_paths(old_dir, new_dir, rpaths, deps, idpath):
     Replace old_dir with new_dir in rpaths, deps and idpath
     and return replacements
     """
-    id = None
-    nrpaths = []
-    ndeps = []
+    new_idpath = None
     if idpath:
-        id = idpath.replace(old_dir, new_dir)
+        new_idpath = idpath.replace(old_dir, new_dir)
+    new_rpaths = list()
+    new_deps = list()
     for rpath in rpaths:
-        nrpath = rpath.replace(old_dir, new_dir)
-        nrpaths.append(nrpath)
+        new_rpath = rpath.replace(old_dir, new_dir)
+        new_rpaths.append(new_rpath)
     for dep in deps:
-        ndep = dep.replace(old_dir, new_dir)
-        ndeps.append(ndep)
-    return nrpaths, ndeps, id
+        new_dep = dep.replace(old_dir, new_dir)
+        new_deps.append(new_dep)
+    return new_rpaths, new_deps, new_idpath
 
 
-def modify_macho_object(cur_path_name, orig_path_name, old_dir, new_dir,
-                        relative):
+def modify_macho_object(cur_path, rpaths, deps, idpath,
+                        new_rpaths, new_deps, new_idpath):
     """
     Modify MachO binary path_name by replacing old_dir with new_dir
     or the relative path to spack install root.
@@ -171,28 +171,18 @@ def modify_macho_object(cur_path_name, orig_path_name, old_dir, new_dir,
     install_name_tool  -rpath old new binary
     """
     # avoid error message for libgcc_s
-    if 'libgcc_' in cur_path_name:
+    if 'libgcc_' in cur_path:
         return
-    rpaths, deps, idpath = macho_get_paths(cur_path_name)
-    id = None
-    nrpaths = []
-    ndeps = []
-    if relative:
-        nrpaths, ndeps, id = macho_make_paths_relative(orig_path_name,
-                                                       old_dir, rpaths,
-                                                       deps, idpath)
-    else:
-        nrpaths, ndeps, id = macho_replace_paths(old_dir, new_dir, rpaths,
-                                                 deps, idpath)
     install_name_tool = Executable('install_name_tool')
-    if id:
-        install_name_tool('-id', id, cur_path_name, output=str, err=str)
+    if new_idpath:
+        install_name_tool('-id', new_idpath, str(cur_path),
+                          output=str, err=str)
 
-    for orig, new in zip(deps, ndeps):
-        install_name_tool('-change', orig, new, cur_path_name)
+    for orig, new in zip(deps, new_deps):
+        install_name_tool('-change', orig, new, str(cur_path))
 
-    for orig, new in zip(rpaths, nrpaths):
-        install_name_tool('-rpath', orig, new, cur_path_name)
+    for orig, new in zip(rpaths, new_rpaths):
+        install_name_tool('-rpath', orig, new, str(cur_path))
     return
 
 
@@ -227,6 +217,8 @@ def needs_binary_relocation(filetype):
     retval = False
     if "relocatable" in filetype:
         return False
+    if "link" in filetype:
+        return False
     if platform.system() == 'Darwin':
         return ('Mach-O' in filetype)
     elif platform.system() == 'Linux':
@@ -240,44 +232,65 @@ def needs_text_relocation(filetype):
     """
     Check whether the given filetype is text that may need relocation.
     """
+    if "link" in filetype:
+        return False
     return ("text" in filetype)
 
 
-def relocate_binary(path_name, old_dir, new_dir):
+def relocate_binary(path_names, old_dir, new_dir):
     """
-    Change old_dir to new_dir in RPATHs of elf or mach-o file path_name
+    Change old_dir to new_dir in RPATHs of elf or mach-o files
     """
     if platform.system() == 'Darwin':
-        modify_macho_object(path_name, old_dir, new_dir, relative=False)
+        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)
+            modify_macho_object(path_name,
+                                rpaths, deps, idpath,
+                                new_rpaths, new_deps, new_idpath)
     elif platform.system() == 'Linux':
-        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)
+        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)
     else:
         tty.die("Relocation not implemented for %s" % platform.system())
 
 
-def make_binary_relative(cur_path_name, orig_path_name, old_dir):
+def make_binary_relative(cur_path_names, orig_path_names, old_dir):
     """
-    Make RPATHs relative to old_dir in given elf or mach-o file path_name
+    Make RPATHs relative to old_dir in given elf or mach-o files
     """
     if platform.system() == 'Darwin':
-        new_dir = ''
-        modify_macho_object(cur_path_name, orig_path_name, old_dir, new_dir,
-                            relative=True)
+        for cur_path, orig_path in zip(cur_path_names, orig_path_names):
+            rpaths, deps, idpath = macho_get_paths(cur_path)
+            (new_rpaths,
+             new_deps,
+             new_idpath) = macho_make_paths_relative(orig_path, old_dir,
+                                                     rpaths, deps, idpath)
+            modify_macho_object(cur_path,
+                                rpaths, deps, idpath,
+                                new_rpaths, new_deps, new_idpath)
     elif platform.system() == 'Linux':
-        orig_rpaths = get_existing_elf_rpaths(cur_path_name)
-        new_rpaths = get_relative_rpaths(orig_path_name, old_dir, orig_rpaths)
-        modify_elf_object(cur_path_name, orig_rpaths, new_rpaths)
+        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)
     else:
         tty.die("Prelocation not implemented for %s" % platform.system())
 
 
-def relocate_text(path_name, old_dir, new_dir):
+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, path_name)
+    filter_file("r'%s'" % old_dir, "r'%s'" % new_dir,
+                *path_names, backup=False)
 
 
 def substitute_rpath(orig_rpath, topdir, new_root_path):
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 65cbdeaecc..f9c77c530f 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -41,7 +41,8 @@ import spack.cmd.buildcache as buildcache
 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, get_patchelf
+from spack.relocate import needs_binary_relocation, needs_text_relocation
+from spack.relocate import get_patchelf
 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
@@ -218,6 +219,8 @@ echo $PATH"""
 
 def test_relocate():
     assert (needs_binary_relocation('relocatable') is False)
+    assert (needs_binary_relocation('link') is False)
+    assert (needs_text_relocation('link') is False)
 
     out = macho_make_paths_relative('/Users/Shares/spack/pkgC/lib/libC.dylib',
                                     '/Users/Shared/spack',
@@ -225,8 +228,8 @@ def test_relocate():
                                      '/Users/Shared/spack/pkgB/lib',
                                      '/usr/local/lib'),
                                     ('/Users/Shared/spack/pkgA/libA.dylib',
-                                        '/Users/Shared/spack/pkgB/libB.dylib',
-                                        '/usr/local/lib/libloco.dylib'),
+                                     '/Users/Shared/spack/pkgB/libB.dylib',
+                                     '/usr/local/lib/libloco.dylib'),
                                     '/Users/Shared/spack/pkgC/lib/libC.dylib')
     assert out == (['@loader_path/../../../../Shared/spack/pkgA/lib',
                     '@loader_path/../../../../Shared/spack/pkgB/lib',
@@ -242,8 +245,8 @@ def test_relocate():
                                      '/Users/Shared/spack/pkgB/lib',
                                      '/usr/local/lib'),
                                     ('/Users/Shared/spack/pkgA/libA.dylib',
-                                        '/Users/Shared/spack/pkgB/libB.dylib',
-                                        '/usr/local/lib/libloco.dylib'), None)
+                                     '/Users/Shared/spack/pkgB/libB.dylib',
+                                     '/usr/local/lib/libloco.dylib'), None)
 
     assert out == (['@loader_path/../../pkgA/lib',
                     '@loader_path/../../pkgB/lib',
@@ -258,8 +261,8 @@ def test_relocate():
                                '/Users/Shared/spack/pkgB/lib',
                                '/usr/local/lib'),
                               ('/Users/Shared/spack/pkgA/libA.dylib',
-                                  '/Users/Shared/spack/pkgB/libB.dylib',
-                                  '/usr/local/lib/libloco.dylib'),
+                               '/Users/Shared/spack/pkgB/libB.dylib',
+                               '/usr/local/lib/libloco.dylib'),
                               '/Users/Shared/spack/pkgC/lib/libC.dylib')
     assert out == (['/Applications/spack/pkgA/lib',
                     '/Applications/spack/pkgB/lib',
@@ -275,8 +278,8 @@ def test_relocate():
                                '/Users/Shared/spack/pkgB/lib',
                                '/usr/local/lib'),
                               ('/Users/Shared/spack/pkgA/libA.dylib',
-                                  '/Users/Shared/spack/pkgB/libB.dylib',
-                                  '/usr/local/lib/libloco.dylib'),
+                               '/Users/Shared/spack/pkgB/libB.dylib',
+                               '/usr/local/lib/libloco.dylib'),
                               None)
     assert out == (['/Applications/spack/pkgA/lib',
                     '/Applications/spack/pkgB/lib',
@@ -301,25 +304,46 @@ def test_relocate():
 def test_relocate_macho(tmpdir):
     with tmpdir.as_cwd():
         get_patchelf()
-        assert (needs_binary_relocation('Mach-O') is True)
+        assert (needs_binary_relocation('Mach-O'))
 
-        macho_get_paths('/bin/bash')
+        rpaths, deps, idpath = macho_get_paths('/bin/bash')
+        nrpaths, ndeps, nid = macho_make_paths_relative('/bin/bash', '/usr',
+                                                        rpaths, deps, idpath)
         shutil.copyfile('/bin/bash', 'bash')
+        modify_macho_object('bash',
+                            rpaths, deps, idpath,
+                            nrpaths, ndeps, nid)
 
-        modify_macho_object('bash', '/bin/bash', '/usr', '/opt', False)
-        modify_macho_object('bash', '/bin/bash', '/usr', '/opt', True)
+        rpaths, deps, idpath = macho_get_paths('/bin/bash')
+        nrpaths, ndeps, nid = macho_replace_paths('/usr', '/opt',
+                                                  rpaths, deps, idpath)
+        shutil.copyfile('/bin/bash', 'bash')
+        modify_macho_object('bash',
+                            rpaths, deps, idpath,
+                            nrpaths, ndeps, nid)
+
+        path = '/usr/lib/libncurses.5.4.dylib'
+        rpaths, deps, idpath = macho_get_paths(path)
+        nrpaths, ndeps, nid = macho_make_paths_relative(path, '/usr',
+                                                        rpaths, deps, idpath)
+        shutil.copyfile(
+            '/usr/lib/libncurses.5.4.dylib', 'libncurses.5.4.dylib')
+        modify_macho_object('libncurses.5.4.dylib',
+                            rpaths, deps, idpath,
+                            nrpaths, ndeps, nid)
 
+        rpaths, deps, idpath = macho_get_paths(path)
+        nrpaths, ndeps, nid = macho_replace_paths('/usr', '/opt',
+                                                  rpaths, deps, idpath)
         shutil.copyfile(
             '/usr/lib/libncurses.5.4.dylib', 'libncurses.5.4.dylib')
         modify_macho_object(
             'libncurses.5.4.dylib',
-            '/usr/lib/libncurses.5.4.dylib', '/usr', '/opt', False)
-        modify_macho_object(
-            'libncurses.5.4.dylib',
-            '/usr/lib/libncurses.5.4.dylib', '/usr', '/opt', True)
+            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') is True)
+    assert (needs_binary_relocation('ELF'))