Fix relocating MachO binary, when store projection changes (#46840)

* Remove "modify_object_macholib"

According to documentation, this function is used when installing
Mach-O binaries on linux. The implementation seems questionable at
least, and the code seems to be never hit (Spack currently doesn't
support installing Mach-O binaries on linux).

* Fix relocation on macOS, when store projection changes

---------

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

lib/spack/spack/relocate.py

@@ -205,23 +205,33 @@ def macho_find_paths(orig_rpaths, deps, idpath, old_layout_root, prefix_to_prefi
     paths_to_paths dictionary which maps all of the old paths to new paths
     """
     paths_to_paths = dict()
+    # Sort from longest path to shortest, to ensure we try /foo/bar/baz before /foo/bar
+    prefix_iteration_order = sorted(prefix_to_prefix, key=len, reverse=True)
     for orig_rpath in orig_rpaths:
         if orig_rpath.startswith(old_layout_root):
-            for old_prefix, new_prefix in prefix_to_prefix.items():
+            for old_prefix in prefix_iteration_order:
+                new_prefix = prefix_to_prefix[old_prefix]
                 if orig_rpath.startswith(old_prefix):
                     new_rpath = re.sub(re.escape(old_prefix), new_prefix, orig_rpath)
                     paths_to_paths[orig_rpath] = new_rpath
+                    break
         else:
             paths_to_paths[orig_rpath] = orig_rpath
 
     if idpath:
-        for old_prefix, new_prefix in prefix_to_prefix.items():
+        for old_prefix in prefix_iteration_order:
+            new_prefix = prefix_to_prefix[old_prefix]
             if idpath.startswith(old_prefix):
                 paths_to_paths[idpath] = re.sub(re.escape(old_prefix), new_prefix, idpath)
+                break
+
     for dep in deps:
-        for old_prefix, new_prefix in prefix_to_prefix.items():
+        for old_prefix in prefix_iteration_order:
+            new_prefix = prefix_to_prefix[old_prefix]
             if dep.startswith(old_prefix):
                 paths_to_paths[dep] = re.sub(re.escape(old_prefix), new_prefix, dep)
+                break
+
         if dep.startswith("@"):
             paths_to_paths[dep] = dep
 
@@ -270,36 +280,6 @@ def modify_macho_object(cur_path, rpaths, deps, idpath, paths_to_paths):
         install_name_tool = executable.Executable("install_name_tool")
         install_name_tool(*args)
 
-    return
-
-
-def modify_object_macholib(cur_path, paths_to_paths):
-    """
-    This function is used when install machO buildcaches on linux by
-    rewriting mach-o loader commands for dependency library paths of
-    mach-o binaries and the id path for mach-o libraries.
-    Rewritting of rpaths is handled by replace_prefix_bin.
-    Inputs
-    mach-o binary to be modified
-    dictionary mapping paths in old install layout to new install layout
-    """
-
-    dll = macholib.MachO.MachO(cur_path)
-    dll.rewriteLoadCommands(paths_to_paths.get)
-
-    try:
-        f = open(dll.filename, "rb+")
-        for header in dll.headers:
-            f.seek(0)
-            dll.write(f)
-        f.seek(0, 2)
-        f.flush()
-        f.close()
-    except Exception:
-        pass
-
-    return
-
 
 def macholib_get_paths(cur_path):
     """Get rpaths, dependent libraries, and library id of mach-o objects."""
@@ -415,10 +395,7 @@ def relocate_macho_binaries(
             # normalized paths
             rel_to_orig = macho_make_paths_normal(orig_path_name, rpaths, deps, idpath)
             # replace the relativized paths with normalized paths
-            if sys.platform == "darwin":
             modify_macho_object(path_name, rpaths, deps, idpath, rel_to_orig)
-            else:
-                modify_object_macholib(path_name, rel_to_orig)
             # get the normalized paths in the mach-o binary
             rpaths, deps, idpath = macholib_get_paths(path_name)
             # get the mapping of paths in old prefix to path in new prefix
@@ -426,10 +403,7 @@ def relocate_macho_binaries(
                 rpaths, deps, idpath, old_layout_root, prefix_to_prefix
             )
             # replace the old paths with new paths
-            if sys.platform == "darwin":
             modify_macho_object(path_name, rpaths, deps, idpath, paths_to_paths)
-            else:
-                modify_object_macholib(path_name, paths_to_paths)
             # get the new normalized path in the mach-o binary
             rpaths, deps, idpath = macholib_get_paths(path_name)
             # get the mapping of paths to relative paths in the new prefix
@@ -437,10 +411,7 @@ def relocate_macho_binaries(
                 path_name, new_layout_root, rpaths, deps, idpath
             )
             # replace the new paths with relativized paths in the new prefix
-            if sys.platform == "darwin":
             modify_macho_object(path_name, rpaths, deps, idpath, paths_to_paths)
-            else:
-                modify_object_macholib(path_name, paths_to_paths)
         else:
             # get the paths in the old prefix
             rpaths, deps, idpath = macholib_get_paths(path_name)
@@ -449,10 +420,7 @@ def relocate_macho_binaries(
                 rpaths, deps, idpath, old_layout_root, prefix_to_prefix
             )
             # replace the old paths with new paths
-            if sys.platform == "darwin":
             modify_macho_object(path_name, rpaths, deps, idpath, paths_to_paths)
-            else:
-                modify_object_macholib(path_name, paths_to_paths)
 
 
 def _transform_rpaths(orig_rpaths, orig_root, new_prefixes):

lib/spack/spack/test/packaging.py

@@ -549,3 +549,35 @@ def test_fetch_external_package_is_noop(default_mock_concretization, fetching_no
     spec.external_path = "/some/where"
     assert spec.external
     spec.package.do_fetch()
+
+
+@pytest.mark.parametrize(
+    "relocation_dict",
+    [
+        {"/foo/bar/baz": "/a/b/c", "/foo/bar": "/a/b"},
+        # Ensure correctness does not depend on the ordering of the dict
+        {"/foo/bar": "/a/b", "/foo/bar/baz": "/a/b/c"},
+    ],
+)
+def test_macho_relocation_with_changing_projection(relocation_dict):
+    """Tests that prefix relocation is computed correctly when the prefixes to be relocated
+    contain a directory and its subdirectories.
+
+    This happens when relocating to a new place AND changing the store projection. In that case we
+    might have a relocation dict like:
+
+    /foo/bar/baz/ -> /a/b/c
+    /foo/bar -> /a/b
+
+    What we need to check is that we don't end up in situations where we relocate to a mixture of
+    the two schemes, like /a/b/baz.
+    """
+    original_rpath = "/foo/bar/baz/abcdef"
+    result = macho_find_paths(
+        [original_rpath],
+        deps=[],
+        idpath=None,
+        old_layout_root="/foo",
+        prefix_to_prefix=relocation_dict,
+    )
+    assert result[original_rpath] == "/a/b/c/abcdef"