Relocate links using prefix to prefix map (#33636)

Previously symlinks were not relocated when they pointed across packages

lib/spack/spack/binary_distribution.py

@@ -1646,8 +1646,8 @@ def is_backup_file(file):
             )
 
         # Relocate links to the new install prefix
-        links = [link for link in buildinfo.get("relocate_links", [])]
-        relocate.relocate_links(links, old_layout_root, old_prefix, new_prefix)
+        links = [os.path.join(workdir, f) for f in buildinfo.get("relocate_links", [])]
+        relocate.relocate_links(links, prefix_to_prefix_bin)
 
         # For all buildcaches
         # relocate the install prefixes in text files including dependencies

lib/spack/spack/relocate.py

@@ -702,41 +702,27 @@ def raise_if_not_relocatable(binaries, allow_root):
             raise InstallRootStringError(binary, spack.store.layout.root)
 
 
-def relocate_links(links, orig_layout_root, orig_install_prefix, new_install_prefix):
-    """Relocate links to a new install prefix.
+def warn_if_link_cant_be_relocated(link, target):
+    if not os.path.isabs(target):
+        return
+    tty.warn('Symbolic link at "{}" to "{}" cannot be relocated'.format(link, target))
 
-    The symbolic links are relative to the original installation prefix.
-    The old link target is read and the placeholder is replaced by the old
-    layout root. If the old link target is in the old install prefix, the new
-    link target is create by replacing the old install prefix with the new
-    install prefix.
 
-    Args:
-        links (list): list of links to be relocated
-        orig_layout_root (str): original layout root
-        orig_install_prefix (str): install prefix of the original installation
-        new_install_prefix (str): install prefix where we want to relocate
-    """
-    placeholder = _placeholder(orig_layout_root)
-    abs_links = [os.path.join(new_install_prefix, link) for link in links]
-    for abs_link in abs_links:
-        link_target = os.readlink(abs_link)
-        link_target = re.sub(placeholder, orig_layout_root, link_target)
-        # If the link points to a file in the original install prefix,
-        # compute the corresponding target in the new prefix and relink
-        if link_target.startswith(orig_install_prefix):
-            link_target = re.sub(orig_install_prefix, new_install_prefix, link_target)
-            os.unlink(abs_link)
-            symlink(link_target, abs_link)
+def relocate_links(links, prefix_to_prefix):
+    """Relocate links to a new install prefix."""
+    regex = re.compile("|".join(re.escape(p) for p in prefix_to_prefix.keys()))
+    for link in links:
+        old_target = os.readlink(link)
+        match = regex.match(old_target)
 
-        # If the link is absolute and has not been relocated then
-        # warn the user about that
-        if os.path.isabs(link_target) and not link_target.startswith(new_install_prefix):
-            msg = (
-                'Link target "{0}" for symbolic link "{1}" is outside'
-                " of the new install prefix {2}"
-            )
-            tty.warn(msg.format(link_target, abs_link, new_install_prefix))
+        # No match.
+        if match is None:
+            warn_if_link_cant_be_relocated(link, old_target)
+            continue
+
+        new_target = prefix_to_prefix[match.group()] + old_target[match.end() :]
+        os.unlink(link)
+        symlink(new_target, link)
 
 
 def utf8_path_to_binary_regex(prefix):

lib/spack/spack/test/packaging.py

@@ -9,10 +9,10 @@
 import argparse
 import os
 import platform
-import re
 import shutil
 import stat
 import sys
+from collections import OrderedDict
 
 import pytest
 
@@ -28,7 +28,6 @@
 from spack.fetch_strategy import FetchStrategyComposite, URLFetchStrategy
 from spack.paths import mock_gpg_keys_path
 from spack.relocate import (
-    _placeholder,
     file_is_relocatable,
     macho_find_paths,
     macho_make_paths_normal,
@@ -213,27 +212,42 @@ def test_unsafe_relocate_text(tmpdir):
 
 
 def test_relocate_links(tmpdir):
-    with tmpdir.as_cwd():
-        old_layout_root = os.path.join("%s" % tmpdir, "home", "spack", "opt", "spack")
-        old_install_prefix = os.path.join("%s" % old_layout_root, "debian6", "test")
-        old_binname = os.path.join(old_install_prefix, "binfile")
-        placeholder = _placeholder(old_layout_root)
-        re.sub(old_layout_root, placeholder, old_binname)
-        filenames = ["link.ln", "outsideprefix.ln"]
-        new_layout_root = os.path.join("%s" % tmpdir, "opt", "rh", "devtoolset")
-        new_install_prefix = os.path.join("%s" % new_layout_root, "test", "debian6")
-        new_linkname = os.path.join(new_install_prefix, "link.ln")
-        new_linkname2 = os.path.join(new_install_prefix, "outsideprefix.ln")
-        new_binname = os.path.join(new_install_prefix, "binfile")
-        mkdirp(new_install_prefix)
-        with open(new_binname, "w") as f:
-            f.write("\n")
-        os.utime(new_binname, None)
-        symlink(old_binname, new_linkname)
-        symlink("/usr/lib/libc.so", new_linkname2)
-        relocate_links(filenames, old_layout_root, old_install_prefix, new_install_prefix)
-        assert os.readlink(new_linkname) == new_binname
-        assert os.readlink(new_linkname2) == "/usr/lib/libc.so"
+    tmpdir.ensure("new_prefix_a", dir=True)
+
+    own_prefix_path = str(tmpdir.join("prefix_a", "file"))
+    dep_prefix_path = str(tmpdir.join("prefix_b", "file"))
+    system_path = os.path.join(os.path.sep, "system", "path")
+
+    # Old prefixes to new prefixes
+    prefix_to_prefix = OrderedDict(
+        [
+            # map <tmpdir>/prefix_a -> <tmpdir>/new_prefix_a
+            (str(tmpdir.join("prefix_a")), str(tmpdir.join("new_prefix_a"))),
+            # map <tmpdir>/prefix_b -> <tmpdir>/new_prefix_b
+            (str(tmpdir.join("prefix_b")), str(tmpdir.join("new_prefix_b"))),
+            # map <tmpdir> -> /fallback/path -- this is just to see we respect order.
+            (str(tmpdir), os.path.join(os.path.sep, "fallback", "path")),
+        ]
+    )
+
+    with tmpdir.join("new_prefix_a").as_cwd():
+        # To be relocated
+        os.symlink(own_prefix_path, "to_self")
+        os.symlink(dep_prefix_path, "to_dependency")
+
+        # To be ignored
+        os.symlink(system_path, "to_system")
+        os.symlink("relative", "to_self_but_relative")
+
+        relocate_links(["to_self", "to_dependency", "to_system"], prefix_to_prefix)
+
+        # These two are relocated
+        assert os.readlink("to_self") == str(tmpdir.join("new_prefix_a", "file"))
+        assert os.readlink("to_dependency") == str(tmpdir.join("new_prefix_b", "file"))
+
+        # These two are not.
+        assert os.readlink("to_system") == system_path
+        assert os.readlink("to_self_but_relative") == "relative"
 
 
 def test_needs_relocation():