Binary caching bugfix: symlink relocation (#10073)

Binary caches of packages with absolute symlinks had broken symlinks.
As a stopgap measure, #9747 addressed this by replacing symlinks with
copies of files when creating binary cached packages.

This reverts #9747 and instead, either relative-izes the symlink or
rewrites the target. If the binary cache is created using '--rel' (as
in "spack buildcache create --rel...") then absolute symlinks will be
replaced with relative symlinks (in addition to making RPATHs relative
as before); otherwise they are rewritten (when the binary cache is
unpacked and installed).

3 files changed, 131 insertions, 9 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 4a5522340b..7bceb1189e 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -116,6 +116,7 @@ def write_buildinfo_file(prefix, workdir, rel=False):
     """
     text_to_relocate = []
     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.
@@ -124,10 +125,23 @@ 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)
+            if os.path.islink(path_name):
+                link = os.readlink(path_name)
+                if os.path.isabs(link):
+                    # Relocate absolute links into the spack tree
+                    if link.startswith(spack.store.layout.root):
+                        rel_path_name = os.path.relpath(path_name, prefix)
+                        link_to_relocate.append(rel_path_name)
+                    else:
+                        msg = 'Absolute link %s to %s ' % (path_name, link)
+                        msg += 'outside of stage %s ' % prefix
+                        msg += 'cannot be relocated.'
+                        tty.warn(msg)
+
             #  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(
+            elif relocate.strings_contains_installroot(
                     path_name, spack.store.layout.root):
                 filetype = get_filetype(path_name)
                 if relocate.needs_binary_relocation(filetype, os_id):
@@ -145,6 +159,7 @@ def write_buildinfo_file(prefix, workdir, rel=False):
         prefix, spack.store.layout.root)
     buildinfo['relocate_textfiles'] = text_to_relocate
     buildinfo['relocate_binaries'] = binary_to_relocate
+    buildinfo['relocate_links'] = link_to_relocate
     filename = buildinfo_file_name(workdir)
     with open(filename, 'w') as outfile:
         outfile.write(yaml.dump(buildinfo, default_flow_style=True))
@@ -269,9 +284,7 @@ def build_tarball(spec, outdir, force=False, rel=False, unsigned=False,
             raise NoOverwriteException(str(specfile_path))
     # make a copy of the install directory to work with
     workdir = os.path.join(tempfile.mkdtemp(), os.path.basename(spec.prefix))
-    # set symlinks=False here to avoid broken symlinks when archiving and
-    # moving the package
-    install_tree(spec.prefix, workdir, symlinks=False)
+    install_tree(spec.prefix, workdir, symlinks=True)
 
     # create info for later relocation and create tar
     write_buildinfo_file(spec.prefix, workdir, rel=rel)
@@ -287,7 +300,7 @@ def build_tarball(spec, outdir, force=False, rel=False, unsigned=False,
             tty.die(str(e))
     else:
         try:
-            make_package_placeholder(workdir, allow_root)
+            make_package_placeholder(workdir, spec.prefix, allow_root)
         except Exception as e:
             shutil.rmtree(workdir)
             shutil.rmtree(tarfile_dir)
@@ -366,7 +379,8 @@ def download_tarball(spec):
 
 def make_package_relative(workdir, prefix, allow_root):
     """
-    Change paths in binaries to relative paths
+    Change paths in binaries to relative paths. Change absolute symlinks
+    to relative symlinks.
     """
     buildinfo = read_buildinfo_file(workdir)
     old_path = buildinfo['buildpath']
@@ -377,9 +391,15 @@ def make_package_relative(workdir, prefix, allow_root):
         cur_path_names.append(os.path.join(workdir, filename))
     relocate.make_binary_relative(cur_path_names, orig_path_names,
                                   old_path, allow_root)
+    orig_path_names = list()
+    cur_path_names = list()
+    for filename in buildinfo.get('relocate_links', []):
+        orig_path_names.append(os.path.join(prefix, filename))
+        cur_path_names.append(os.path.join(workdir, filename))
+    relocate.make_link_relative(cur_path_names, orig_path_names)
 
 
-def make_package_placeholder(workdir, allow_root):
+def make_package_placeholder(workdir, prefix, allow_root):
     """
     Change paths in binaries to placeholder paths
     """
@@ -389,6 +409,11 @@ def make_package_placeholder(workdir, allow_root):
         cur_path_names.append(os.path.join(workdir, filename))
     relocate.make_binary_placeholder(cur_path_names, allow_root)
 
+    cur_path_names = list()
+    for filename in buildinfo.get('relocate_links', []):
+        cur_path_names.append(os.path.join(workdir, filename))
+    relocate.make_link_placeholder(cur_path_names, workdir, prefix)
+
 
 def relocate_package(workdir, allow_root):
     """
@@ -419,6 +444,11 @@ def relocate_package(workdir, allow_root):
             path_names.add(path_name)
         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)
+            path_names.add(path_name)
+        relocate.relocate_links(path_names, old_path, new_path)
 
 
 def extract_tarball(spec, filename, allow_root=False, unsigned=False,
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index a559ae6495..2d3bc6f837 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -351,6 +351,18 @@ def relocate_binary(path_names, old_dir, new_dir, allow_root):
         tty.die("Relocation not implemented for %s" % platform.system())
 
 
+def make_link_relative(cur_path_names, orig_path_names):
+    """
+    Change absolute links to be relative.
+    """
+    for cur_path, orig_path in zip(cur_path_names, orig_path_names):
+        old_src = os.readlink(orig_path)
+        new_src = os.path.relpath(old_src, orig_path)
+
+        os.unlink(cur_path)
+        os.symlink(new_src, cur_path)
+
+
 def make_binary_relative(cur_path_names, orig_path_names, old_dir, allow_root):
     """
     Replace old RPATHs with paths relative to old_dir in binary files
@@ -415,6 +427,41 @@ def make_binary_placeholder(cur_path_names, allow_root):
         tty.die("Placeholder not implemented for %s" % platform.system())
 
 
+def make_link_placeholder(cur_path_names, cur_dir, old_dir):
+    """
+    Replace old install path with placeholder in absolute links.
+
+    Links in ``cur_path_names`` must link to absolute paths.
+    """
+    for cur_path in cur_path_names:
+        placeholder = set_placeholder(spack.store.layout.root)
+        placeholder_prefix = old_dir.replace(spack.store.layout.root,
+                                             placeholder)
+        cur_src = os.readlink(cur_path)
+        rel_src = os.path.relpath(cur_src, cur_dir)
+        new_src = os.path.join(placeholder_prefix, rel_src)
+
+        os.unlink(cur_path)
+        os.symlink(new_src, cur_path)
+
+
+def relocate_links(path_names, old_dir, new_dir):
+    """
+    Replace old path with new path in link sources.
+
+    Links in ``path_names`` must link to absolute paths or placeholders.
+    """
+    placeholder = set_placeholder(old_dir)
+    for path_name in path_names:
+        old_src = os.readlink(path_name)
+        # replace either placeholder or old_dir
+        new_src = old_src.replace(placeholder, new_dir, 1)
+        new_src = new_src.replace(old_dir, new_dir, 1)
+
+        os.unlink(path_name)
+        os.symlink(new_src, path_name)
+
+
 def relocate_text(path_names, old_dir, new_dir):
     """
     Replace old path with new path in text file path_name
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 096d257a3e..bb086f20c7 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -25,7 +25,7 @@ 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 get_patchelf, relocate_text, relocate_links
 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
@@ -85,9 +85,12 @@ echo $PATH"""
     with open(filename, "w") as script:
         script.write(spec.prefix)
 
+    # Create an absolute symlink
+    linkname = os.path.join(spec.prefix, "link_to_dummy.txt")
+    os.symlink(filename, linkname)
+
     # Create the build cache  and
     # put it directly into the mirror
-
     mirror_path = os.path.join(str(tmpdir), 'test-mirror')
     spack.mirror.create(
         mirror_path, specs=[], no_checksum=True
@@ -100,6 +103,7 @@ echo $PATH"""
     stage = spack.stage.Stage(
         mirrors['spack-mirror-test'], name="build_cache", keep=True)
     stage.create()
+
     # setup argument parser
     parser = argparse.ArgumentParser()
     buildcache.setup_parser(parser)
@@ -120,6 +124,13 @@ echo $PATH"""
         args = parser.parse_args(['install', '-f', str(pkghash)])
         buildcache.buildcache(parser, args)
 
+        files = os.listdir(spec.prefix)
+        assert 'link_to_dummy.txt' in files
+        assert 'dummy.txt' in files
+        assert os.path.realpath(
+            os.path.join(spec.prefix, 'link_to_dummy.txt')
+        ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
         # create build cache with relative path and signing
         args = parser.parse_args(
             ['create', '-d', mirror_path, '-f', '-r', str(spec)])
@@ -136,6 +147,13 @@ echo $PATH"""
         args = parser.parse_args(['install', '-f', str(pkghash)])
         buildcache.buildcache(parser, args)
 
+        files = os.listdir(spec.prefix)
+        assert 'link_to_dummy.txt' in files
+        assert 'dummy.txt' in files
+        assert os.path.realpath(
+            os.path.join(spec.prefix, 'link_to_dummy.txt')
+        ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
     else:
         # create build cache without signing
         args = parser.parse_args(
@@ -149,6 +167,13 @@ echo $PATH"""
         args = parser.parse_args(['install', '-u', str(spec)])
         buildcache.install_tarball(spec, args)
 
+        files = os.listdir(spec.prefix)
+        assert 'link_to_dummy.txt' in files
+        assert 'dummy.txt' in files
+        assert os.path.realpath(
+            os.path.join(spec.prefix, 'link_to_dummy.txt')
+        ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
         # test overwrite install without verification
         args = parser.parse_args(['install', '-f', '-u', str(pkghash)])
         buildcache.buildcache(parser, args)
@@ -169,9 +194,17 @@ echo $PATH"""
         args = parser.parse_args(['install', '-f', '-u', str(pkghash)])
         buildcache.buildcache(parser, args)
 
+        files = os.listdir(spec.prefix)
+        assert 'link_to_dummy.txt' in files
+        assert 'dummy.txt' in files
+        assert os.path.realpath(
+            os.path.join(spec.prefix, 'link_to_dummy.txt')
+        ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
     # Validate the relocation information
     buildinfo = bindist.read_buildinfo_file(spec.prefix)
     assert(buildinfo['relocate_textfiles'] == ['dummy.txt'])
+    assert(buildinfo['relocate_links'] == ['link_to_dummy.txt'])
 
     args = parser.parse_args(['list'])
     buildcache.buildcache(parser, args)
@@ -219,6 +252,18 @@ def test_relocate_text(tmpdir):
         assert(strings_contains_installroot(filename, old_dir) is False)
 
 
+def test_relocate_links(tmpdir):
+    with tmpdir.as_cwd():
+        old_dir = '/home/spack/opt/spack'
+        filename = 'link.ln'
+        old_src = os.path.join(old_dir, filename)
+        os.symlink(old_src, filename)
+        filenames = [filename]
+        new_dir = '/opt/rh/devtoolset/'
+        relocate_links(filenames, old_dir, new_dir)
+        assert os.path.realpath(filename) == os.path.join(new_dir, filename)
+
+
 def test_needs_relocation():
     binary_type = (
         'ELF 64-bit LSB executable, x86-64, version 1 (SYSV),'