Make macOS installed libraries more relocatable (#26608)

* relocate: call install_name_tool less

* zstd: fix race condition

Multiple times on my mac, trying to install in parallel led to failures
from multiple tasks trying to simultaneously create `$PREFIX/lib`.

* PackageMeta: simplify callback flush

* Relocate: use spack.platforms instead of platform

* Relocate: code improvements

* fix zstd

* Automatically fix rpaths for packages on macOS

* Only change library IDs when the path is already in the rpath

This restores the hardcoded library path for GCC.

* Delete nonexistent rpaths and add more testing

* Relocate: Allow @executable_path and @loader_path

6 files changed, 392 insertions, 112 deletions

diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py
index f6ac0b3821..f19d5fa54b 100644
--- a/lib/spack/spack/build_systems/autotools.py
+++ b/lib/spack/spack/build_systems/autotools.py
@@ -645,3 +645,6 @@ To resolve this problem, please try the following:
             fs.mkdirp(os.path.dirname(self._removed_la_files_log))
             with open(self._removed_la_files_log, mode='w') as f:
                 f.write('\n'.join(libtool_files))
+
+    # On macOS, force rpaths for shared library IDs and remove duplicate rpaths
+    run_after('install')(PackageBase.apply_macos_rpath_fixups)
diff --git a/lib/spack/spack/build_systems/makefile.py b/lib/spack/spack/build_systems/makefile.py
index 45c1432dd4..ef046d5782 100644
--- a/lib/spack/spack/build_systems/makefile.py
+++ b/lib/spack/spack/build_systems/makefile.py
@@ -110,3 +110,6 @@ class MakefilePackage(PackageBase):
 
     # Check that self.prefix is there after installation
     run_after('install')(PackageBase.sanity_check_prefix)
+
+    # On macOS, force rpaths for shared library IDs and remove duplicate rpaths
+    run_after('install')(PackageBase.apply_macos_rpath_fixups)
diff --git a/lib/spack/spack/filesystem_view.py b/lib/spack/spack/filesystem_view.py
index 26457c0f60..27a8708596 100644
--- a/lib/spack/spack/filesystem_view.py
+++ b/lib/spack/spack/filesystem_view.py
@@ -19,7 +19,6 @@ from llnl.util.tty.color import colorize
 
 import spack.config
 import spack.projections
-import spack.relocate
 import spack.schema.projections
 import spack.spec
 import spack.store
@@ -74,6 +73,9 @@ def view_copy(src, dst, view, spec=None):
         # TODO: Not sure which one to use...
         import spack.hooks.sbang as sbang
 
+        # Break a package include cycle
+        import spack.relocate
+
         orig_sbang = '#!/bin/bash {0}/bin/sbang'.format(spack.paths.spack_root)
         new_sbang = sbang.sbang_shebang_line()
 
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 0e9e07f52e..4fbf24c56e 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -286,34 +286,27 @@ class PackageMeta(
 
         def _flush_callbacks(check_name):
             # Name of the attribute I am going to check it exists
-            attr_name = PackageMeta.phase_fmt.format(check_name)
-            checks = getattr(cls, attr_name)
+            check_attr = PackageMeta.phase_fmt.format(check_name)
+            checks = getattr(cls, check_attr)
             if checks:
                 for phase_name, funcs in checks.items():
+                    phase_attr = PackageMeta.phase_fmt.format(phase_name)
                     try:
                         # Search for the phase in the attribute dictionary
-                        phase = attr_dict[
-                            PackageMeta.phase_fmt.format(phase_name)]
+                        phase = attr_dict[phase_attr]
                     except KeyError:
                         # If it is not there it's in the bases
                         # and we added a check. We need to copy
                         # and extend
                         for base in bases:
-                            phase = getattr(
-                                base,
-                                PackageMeta.phase_fmt.format(phase_name),
-                                None
-                            )
+                            phase = getattr(base, phase_attr, None)
                             if phase is not None:
                                 break
 
-                        attr_dict[PackageMeta.phase_fmt.format(
-                            phase_name)] = phase.copy()
-                        phase = attr_dict[
-                            PackageMeta.phase_fmt.format(phase_name)]
+                        phase = attr_dict[phase_attr] = phase.copy()
                     getattr(phase, check_name).extend(funcs)
                 # Clear the attribute for the next class
-                setattr(cls, attr_name, {})
+                setattr(cls, check_attr, {})
 
         _flush_callbacks('run_before')
         _flush_callbacks('run_after')
@@ -1962,6 +1955,25 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
             raise InstallError(
                 "Install failed for %s.  Nothing was installed!" % self.name)
 
+    def apply_macos_rpath_fixups(self):
+        """On Darwin, make installed libraries more easily relocatable.
+
+        Some build systems (handrolled, autotools, makefiles) can set their own
+        rpaths that are duplicated by spack's compiler wrapper. Additionally,
+        many simpler build systems do not link using ``-install_name
+        @rpath/foo.dylib``, which propagates the library's hardcoded
+        absolute path into downstream dependencies. This fixup interrogates,
+        and postprocesses if necessary, all libraries installed by the code.
+
+        It should be added as a @run_after to packaging systems (or individual
+        packages) that do not install relocatable libraries by default.
+        """
+        if 'platform=darwin' not in self.spec:
+            return
+
+        from spack.relocate import fixup_macos_rpaths
+        fixup_macos_rpaths(self.spec)
+
     @property
     def build_log_path(self):
         """
@@ -2702,6 +2714,8 @@ class Package(PackageBase):
     # This will be used as a registration decorator in user
     # packages, if need be
     run_after('install')(PackageBase.sanity_check_prefix)
+    # On macOS, force rpaths for shared library IDs and remove duplicate rpaths
+    run_after('install')(PackageBase.apply_macos_rpath_fixups)
 
 
 def install_dependency_symlinks(pkg, spec, prefix):
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index bbfc4bd52f..212223e7e0 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -4,9 +4,9 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import multiprocessing.pool
 import os
-import platform
 import re
 import shutil
+from collections import defaultdict
 
 import macholib.mach_o
 import macholib.MachO
@@ -20,6 +20,8 @@ import spack.repo
 import spack.spec
 import spack.util.executable as executable
 
+is_macos = (str(spack.platforms.real_host()) == 'darwin')
+
 
 class InstallRootStringError(spack.error.SpackError):
     def __init__(self, file_path, root_path):
@@ -82,7 +84,7 @@ def _patchelf():
     Return None on Darwin or if patchelf cannot be found.
     """
     # Check if patchelf is already in the PATH
-    patchelf = spack.util.executable.which('patchelf')
+    patchelf = executable.which('patchelf')
     if patchelf is not None:
         return patchelf.path
 
@@ -94,7 +96,7 @@ def _patchelf():
         return exe_path
 
     # Skip darwin
-    if str(spack.platforms.host()) == 'darwin':
+    if is_macos:
         return None
 
     # Install the spec and return its path
@@ -197,6 +199,10 @@ def _placeholder(dirname):
     return '@' * len(dirname)
 
 
+def _decode_macho_data(bytestring):
+    return bytestring.rstrip(b'\x00').decode('ascii')
+
+
 def macho_make_paths_relative(path_name, old_layout_root,
                               rpaths, deps, idpath):
     """
@@ -309,21 +315,27 @@ def modify_macho_object(cur_path, rpaths, deps, idpath,
     # avoid error message for libgcc_s
     if 'libgcc_' in cur_path:
         return
-    install_name_tool = executable.Executable('install_name_tool')
+    args = []
 
     if idpath:
         new_idpath = paths_to_paths.get(idpath, None)
         if new_idpath and not idpath == new_idpath:
-            install_name_tool('-id', new_idpath, str(cur_path))
+            args += ['-id', new_idpath]
     for dep in deps:
         new_dep = paths_to_paths.get(dep)
         if new_dep and dep != new_dep:
-            install_name_tool('-change', dep, new_dep, str(cur_path))
+            args += ['-change', dep, new_dep]
 
     for orig_rpath in rpaths:
         new_rpath = paths_to_paths.get(orig_rpath)
         if new_rpath and not orig_rpath == new_rpath:
-            install_name_tool('-rpath', orig_rpath, new_rpath, str(cur_path))
+            args += ['-rpath', orig_rpath, new_rpath]
+
+    if args:
+        args.append(str(cur_path))
+        install_name_tool = executable.Executable('install_name_tool')
+        install_name_tool(*args)
+
     return
 
 
@@ -339,14 +351,7 @@ def modify_object_macholib(cur_path, paths_to_paths):
     """
 
     dll = macholib.MachO.MachO(cur_path)
-
-    changedict = paths_to_paths
-
-    def changefunc(path):
-        npath = changedict.get(path, None)
-        return npath
-
-    dll.rewriteLoadCommands(changefunc)
+    dll.rewriteLoadCommands(paths_to_paths.get)
 
     try:
         f = open(dll.filename, 'rb+')
@@ -363,30 +368,35 @@ def modify_object_macholib(cur_path, paths_to_paths):
 
 
 def macholib_get_paths(cur_path):
+    """Get rpaths, dependent libraries, and library id of mach-o objects.
     """
-    Get rpaths, dependencies and id of mach-o objects
-    using python macholib package
-    """
-    dll = macholib.MachO.MachO(cur_path)
+    headers = macholib.MachO.MachO(cur_path).headers
+    if not headers:
+        tty.warn("Failed to read Mach-O headers: {0}".format(cur_path))
+        commands = []
+    else:
+        if len(headers) > 1:
+            # Reproduce original behavior of only returning the last mach-O
+            # header section
+            tty.warn("Encountered fat binary: {0}".format(cur_path))
+        commands = headers[-1].commands
+
+    LC_ID_DYLIB = macholib.mach_o.LC_ID_DYLIB
+    LC_LOAD_DYLIB = macholib.mach_o.LC_LOAD_DYLIB
+    LC_RPATH = macholib.mach_o.LC_RPATH
 
     ident = None
-    rpaths = list()
-    deps = list()
-    for header in dll.headers:
-        rpaths = [data.rstrip(b'\0').decode('utf-8')
-                  for load_command, dylib_command, data in header.commands if
-                  load_command.cmd == macholib.mach_o.LC_RPATH]
-        deps = [data.rstrip(b'\0').decode('utf-8')
-                for load_command, dylib_command, data in header.commands if
-                load_command.cmd == macholib.mach_o.LC_LOAD_DYLIB]
-        idents = [data.rstrip(b'\0').decode('utf-8')
-                  for load_command, dylib_command, data in header.commands if
-                  load_command.cmd == macholib.mach_o.LC_ID_DYLIB]
-        if len(idents) == 1:
-            ident = idents[0]
-    tty.debug('ident: %s' % ident)
-    tty.debug('deps: %s' % deps)
-    tty.debug('rpaths: %s' % rpaths)
+    rpaths = []
+    deps = []
+    for load_command, dylib_command, data in commands:
+        cmd = load_command.cmd
+        if cmd == LC_RPATH:
+            rpaths.append(_decode_macho_data(data))
+        elif cmd == LC_LOAD_DYLIB:
+            deps.append(_decode_macho_data(data))
+        elif cmd == LC_ID_DYLIB:
+            ident = _decode_macho_data(data)
+
     return (rpaths, deps, ident)
 
 
@@ -539,7 +549,7 @@ def relocate_macho_binaries(path_names, old_layout_root, new_layout_root,
                                                   rpaths, deps,
                                                   idpath)
             # replace the relativized paths with normalized paths
-            if platform.system().lower() == 'darwin':
+            if is_macos:
                 modify_macho_object(path_name, rpaths, deps,
                                     idpath, rel_to_orig)
             else:
@@ -552,7 +562,7 @@ def relocate_macho_binaries(path_names, old_layout_root, new_layout_root,
                                               old_layout_root,
                                               prefix_to_prefix)
             # replace the old paths with new paths
-            if platform.system().lower() == 'darwin':
+            if is_macos:
                 modify_macho_object(path_name, rpaths, deps,
                                     idpath, paths_to_paths)
             else:
@@ -565,7 +575,7 @@ def relocate_macho_binaries(path_names, old_layout_root, new_layout_root,
                                                        new_layout_root,
                                                        rpaths, deps, idpath)
             # replace the new paths with relativized paths in the new prefix
-            if platform.system().lower() == 'darwin':
+            if is_macos:
                 modify_macho_object(path_name, rpaths, deps,
                                     idpath, paths_to_paths)
             else:
@@ -579,7 +589,7 @@ def relocate_macho_binaries(path_names, old_layout_root, new_layout_root,
                                               old_layout_root,
                                               prefix_to_prefix)
             # replace the old paths with new paths
-            if platform.system().lower() == 'darwin':
+            if is_macos:
                 modify_macho_object(path_name, rpaths, deps,
                                     idpath, paths_to_paths)
             else:
@@ -695,18 +705,15 @@ def make_macho_binaries_relative(cur_path_names, orig_path_names,
     """
     Replace old RPATHs with paths relative to old_dir in binary files
     """
+    if not is_macos:
+        return
+
     for cur_path, orig_path in zip(cur_path_names, orig_path_names):
-        rpaths = set()
-        deps = set()
-        idpath = None
-        if platform.system().lower() == 'darwin':
-            (rpaths, deps, idpath) = macholib_get_paths(cur_path)
-            paths_to_paths = macho_make_paths_relative(orig_path,
-                                                       old_layout_root,
-                                                       rpaths, deps, idpath)
-            modify_macho_object(cur_path,
-                                rpaths, deps, idpath,
-                                paths_to_paths)
+        (rpaths, deps, idpath) = macholib_get_paths(cur_path)
+        paths_to_paths = macho_make_paths_relative(
+            orig_path, old_layout_root, rpaths, deps, idpath
+        )
+        modify_macho_object(cur_path, rpaths, deps, idpath, paths_to_paths)
 
 
 def make_elf_binaries_relative(new_binaries, orig_binaries, orig_layout_root):
@@ -915,11 +922,6 @@ def file_is_relocatable(filename, paths_to_relocate=None):
     default_paths_to_relocate = [spack.store.layout.root, spack.paths.prefix]
     paths_to_relocate = paths_to_relocate or default_paths_to_relocate
 
-    if not (platform.system().lower() == 'darwin'
-            or platform.system().lower() == 'linux'):
-        msg = 'function currently implemented only for linux and macOS'
-        raise NotImplementedError(msg)
-
     if not os.path.exists(filename):
         raise ValueError('{0} does not exist'.format(filename))
 
@@ -935,11 +937,11 @@ def file_is_relocatable(filename, paths_to_relocate=None):
     if m_type == 'application':
         tty.debug('{0},{1}'.format(m_type, m_subtype))
 
-    if platform.system().lower() == 'linux':
+    if not is_macos:
         if m_subtype == 'x-executable' or m_subtype == 'x-sharedlib':
             rpaths = ':'.join(_elf_rpaths_for(filename))
             set_of_strings.discard(rpaths)
-    if platform.system().lower() == 'darwin':
+    else:
         if m_subtype == 'x-mach-binary':
             rpaths, deps, idpath = macholib_get_paths(filename)
             set_of_strings.discard(set(rpaths))
@@ -979,6 +981,14 @@ def is_binary(filename):
 
 
 @llnl.util.lang.memoized
+def _get_mime_type():
+    file_cmd = executable.which('file')
+    for arg in ['-b', '-h', '--mime-type']:
+        file_cmd.add_default_arg(arg)
+    return file_cmd
+
+
+@llnl.util.lang.memoized
 def mime_type(filename):
     """Returns the mime type and subtype of a file.
 
@@ -988,13 +998,159 @@ def mime_type(filename):
     Returns:
         Tuple containing the MIME type and subtype
     """
-    file_cmd = executable.Executable('file')
-    output = file_cmd(
-        '-b', '-h', '--mime-type', filename, output=str, error=str)
-    tty.debug('[MIME_TYPE] {0} -> {1}'.format(filename, output.strip()))
-    # In corner cases the output does not contain a subtype prefixed with a /
-    # In those cases add the / so the tuple can be formed.
-    if '/' not in output:
-        output += '/'
-    split_by_slash = output.strip().split('/')
-    return split_by_slash[0], "/".join(split_by_slash[1:])
+    output = _get_mime_type()(filename, output=str, error=str).strip()
+    tty.debug('==> ' + output)
+    type, _, subtype = output.partition('/')
+    return type, subtype
+
+
+# Memoize this due to repeated calls to libraries in the same directory.
+@llnl.util.lang.memoized
+def _exists_dir(dirname):
+    return os.path.isdir(dirname)
+
+
+def fixup_macos_rpath(root, filename):
+    """Apply rpath fixups to the given file.
+
+    Args:
+        root: absolute path to the parent directory
+        filename: relative path to the library or binary
+
+    Returns:
+        True if fixups were applied, else False
+    """
+    abspath = os.path.join(root, filename)
+    if mime_type(abspath) != ('application', 'x-mach-binary'):
+        return False
+
+    # Get Mach-O header commands
+    (rpath_list, deps, id_dylib) = macholib_get_paths(abspath)
+
+    # Convert rpaths list to (name -> number of occurrences)
+    add_rpaths = set()
+    del_rpaths = set()
+    rpaths = defaultdict(int)
+    for rpath in rpath_list:
+        rpaths[rpath] += 1
+
+    args = []
+
+    # Check dependencies for non-rpath entries
+    spack_root = spack.store.layout.root
+    for name in deps:
+        if name.startswith(spack_root):
+            tty.debug("Spack-installed dependency for {0}: {1}"
+                      .format(abspath, name))
+            (dirname, basename) = os.path.split(name)
+            if dirname != root or dirname in rpaths:
+                # Only change the rpath if it's a dependency *or* if the root
+                # rpath was already added to the library (this is to prevent
+                # GCC or similar getting rpaths when they weren't at all
+                # configured)
+                args += ['-change', name, '@rpath/' + basename]
+                add_rpaths.add(dirname.rstrip('/'))
+
+    # Check for nonexistent rpaths (often added by spack linker overzealousness
+    # with both lib/ and lib64/) and duplicate rpaths
+    for (rpath, count) in rpaths.items():
+        if (rpath.startswith('@loader_path')
+                or rpath.startswith('@executable_path')):
+            # Allowable relative paths
+            pass
+        elif not _exists_dir(rpath):
+            tty.debug("Nonexistent rpath in {0}: {1}".format(abspath, rpath))
+            del_rpaths.add(rpath)
+        elif count > 1:
+            # Rpath should only be there once, but it can sometimes be
+            # duplicated between Spack's compiler and libtool. If there are
+            # more copies of the same one, something is very odd....
+            tty_debug = tty.debug if count == 2 else tty.warn
+            tty_debug("Rpath appears {0} times in {1}: {2}".format(
+                count, abspath, rpath
+            ))
+            del_rpaths.add(rpath)
+
+    # Check for relocatable ID
+    if id_dylib is None:
+        tty.debug("No dylib ID is set for {0}".format(abspath))
+    elif not id_dylib.startswith('@'):
+        tty.debug("Non-relocatable dylib ID for {0}: {1}"
+                  .format(abspath, id_dylib))
+        if root in rpaths or root in add_rpaths:
+            args += ['-id', '@rpath/' + filename]
+        else:
+            tty.debug("Allowing hardcoded dylib ID because its rpath "
+                      "is *not* in the library already")
+
+    # Delete bad rpaths
+    for rpath in del_rpaths:
+        args += ['-delete_rpath', rpath]
+
+    # Add missing rpaths that are not set for deletion
+    for rpath in add_rpaths - del_rpaths - set(rpaths):
+        args += ['-add_rpath', rpath]
+
+    if not args:
+        # No fixes needed
+        return False
+
+    args.append(abspath)
+    executable.Executable('install_name_tool')(*args)
+    return True
+
+
+def fixup_macos_rpaths(spec):
+    """Remove duplicate rpaths and make shared library IDs relocatable.
+
+    Some autotools packages write their own ``-rpath`` entries in addition to
+    those implicitly added by the Spack compiler wrappers. On Linux these
+    duplicate rpaths are eliminated, but on macOS they result in multiple
+    entries which makes it harder to adjust with ``install_name_tool
+    -delete_rpath``.
+
+    Furthermore, many autotools programs (on macOS) set a library's install
+    paths to use absolute paths rather than relative paths.
+    """
+    if spec.external or spec.virtual:
+        tty.warn('external or virtual package cannot be fixed up: {0!s}'
+                 .format(spec))
+        return False
+
+    if 'platform=darwin' not in spec:
+        raise NotImplementedError('fixup_macos_rpaths requires macOS')
+
+    applied = 0
+
+    libs = frozenset(['lib', 'lib64', 'libexec', 'plugins',
+                      'Library', 'Frameworks'])
+    prefix = spec.prefix
+
+    if not os.path.exists(prefix):
+        raise RuntimeError(
+            'Could not fix up install prefix spec {0} because it does '
+            'not exist: {1!s}'.format(prefix, spec.name)
+        )
+
+    # Explore the installation prefix of the spec
+    for root, dirs, files in os.walk(prefix, topdown=True):
+        dirs[:] = set(dirs) & libs
+        for name in files:
+            try:
+                needed_fix = fixup_macos_rpath(root, name)
+            except Exception as e:
+                tty.warn("Failed to apply library fixups to: {0}/{1}: {2!s}"
+                         .format(root, name, e))
+                needed_fix = False
+            if needed_fix:
+                applied += 1
+
+    specname = spec.format('{name}{/hash:7}')
+    if applied:
+        tty.info('Fixed rpaths for {0:d} {1} installed to {2}'.format(
+            applied,
+            "binary" if applied == 1 else "binaries",
+            specname
+        ))
+    else:
+        tty.debug('No rpath fixup needed for ' + specname)
diff --git a/lib/spack/spack/test/relocate.py b/lib/spack/spack/test/relocate.py
index 2743235174..386ca88d2d 100644
--- a/lib/spack/spack/test/relocate.py
+++ b/lib/spack/spack/test/relocate.py
@@ -4,7 +4,6 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import collections
 import os.path
-import platform
 import re
 import shutil
 
@@ -14,6 +13,7 @@ import llnl.util.filesystem
 
 import spack.concretize
 import spack.paths
+import spack.platforms
 import spack.relocate
 import spack.spec
 import spack.store
@@ -21,6 +21,13 @@ import spack.tengine
 import spack.util.executable
 
 
+def skip_unless_linux(f):
+    return pytest.mark.skipif(
+        str(spack.platforms.real_host()) != 'linux',
+        reason='implementation currently requires linux'
+    )(f)
+
+
 def rpaths_for(new_binary):
     """Return the RPATHs or RUNPATHs of a binary."""
     patchelf = spack.util.executable.which('patchelf')
@@ -145,6 +152,66 @@ def hello_world(tmpdir):
 
 
 @pytest.fixture()
+def make_dylib(tmpdir_factory):
+    """Create a shared library with unfriendly qualities.
+
+    - Writes the same rpath twice
+    - Writes its install path as an absolute path
+    """
+    cc = spack.util.executable.which('cc')
+
+    def _factory(abs_install_name="abs", extra_rpaths=[]):
+        assert all(extra_rpaths)
+
+        tmpdir = tmpdir_factory.mktemp(
+            abs_install_name + '-'.join(extra_rpaths).replace('/', '')
+        )
+        src = tmpdir.join('foo.c')
+        src.write("int foo() { return 1; }\n")
+
+        filename = 'foo.dylib'
+        lib = tmpdir.join(filename)
+
+        args = ['-shared', str(src), '-o', str(lib)]
+        rpaths = list(extra_rpaths)
+        if abs_install_name.startswith('abs'):
+            args += ['-install_name', str(lib)]
+        else:
+            args += ['-install_name', '@rpath/' + filename]
+
+        if abs_install_name.endswith('rpath'):
+            rpaths.append(str(tmpdir))
+
+        args.extend('-Wl,-rpath,' + s for s in rpaths)
+
+        cc(*args)
+
+        return (str(tmpdir), filename)
+
+    return _factory
+
+
+@pytest.fixture()
+def make_object_file(tmpdir):
+    cc = spack.util.executable.which('cc')
+
+    def _factory():
+        src = tmpdir.join('bar.c')
+        src.write("int bar() { return 2; }\n")
+
+        filename = 'bar.o'
+        lib = tmpdir.join(filename)
+
+        args = ['-c', str(src), '-o', str(lib)]
+
+        cc(*args)
+
+        return (str(tmpdir), filename)
+
+    return _factory
+
+
+@pytest.fixture()
 def copy_binary():
     """Returns a function that copies a binary somewhere and
     returns the new location.
@@ -179,10 +246,7 @@ def test_patchelf_is_relocatable():
     assert spack.relocate.file_is_relocatable(patchelf)
 
 
-@pytest.mark.skipif(
-    platform.system().lower() != 'linux',
-    reason='implementation for MacOS still missing'
-)
+@skip_unless_linux
 def test_file_is_relocatable_errors(tmpdir):
     # The file passed in as argument must exist...
     with pytest.raises(ValueError) as exc_info:
@@ -199,10 +263,7 @@ def test_file_is_relocatable_errors(tmpdir):
         assert 'is not an absolute path' in str(exc_info.value)
 
 
-@pytest.mark.skipif(
-    platform.system().lower() != 'linux',
-    reason='implementation for MacOS still missing'
-)
+@skip_unless_linux
 def test_search_patchelf(expected_patchelf_path):
     current = spack.relocate._patchelf()
     assert current == expected_patchelf_path
@@ -272,10 +333,7 @@ def test_set_elf_rpaths_warning(mock_patchelf):
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
-@pytest.mark.skipif(
-    platform.system().lower() != 'linux',
-    reason='implementation for MacOS still missing'
-)
+@skip_unless_linux
 def test_replace_prefix_bin(hello_world):
     # Compile an "Hello world!" executable and set RPATHs
     executable = hello_world(rpaths=['/usr/lib', '/usr/lib64'])
@@ -288,10 +346,7 @@ def test_replace_prefix_bin(hello_world):
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
-@pytest.mark.skipif(
-    platform.system().lower() != 'linux',
-    reason='implementation for MacOS still missing'
-)
+@skip_unless_linux
 def test_relocate_elf_binaries_absolute_paths(
         hello_world, copy_binary, tmpdir
 ):
@@ -316,10 +371,7 @@ def test_relocate_elf_binaries_absolute_paths(
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
-@pytest.mark.skipif(
-    platform.system().lower() != 'linux',
-    reason='implementation for MacOS still missing'
-)
+@skip_unless_linux
 def test_relocate_elf_binaries_relative_paths(hello_world, copy_binary):
     # Create an executable, set some RPATHs, copy it to another location
     orig_binary = hello_world(rpaths=['lib', 'lib64', '/opt/local/lib'])
@@ -340,10 +392,7 @@ def test_relocate_elf_binaries_relative_paths(hello_world, copy_binary):
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
-@pytest.mark.skipif(
-    platform.system().lower() != 'linux',
-    reason='implementation for MacOS still missing'
-)
+@skip_unless_linux
 def test_make_elf_binaries_relative(hello_world, copy_binary, tmpdir):
     orig_binary = hello_world(rpaths=[
         str(tmpdir.mkdir('lib')), str(tmpdir.mkdir('lib64')), '/opt/local/lib'
@@ -367,10 +416,7 @@ def test_raise_if_not_relocatable(monkeypatch):
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
-@pytest.mark.skipif(
-    platform.system().lower() != 'linux',
-    reason='implementation for MacOS still missing'
-)
+@skip_unless_linux
 def test_relocate_text_bin(hello_world, copy_binary, tmpdir):
     orig_binary = hello_world(rpaths=[
         str(tmpdir.mkdir('lib')), str(tmpdir.mkdir('lib64')), '/opt/local/lib'
@@ -406,3 +452,59 @@ def test_relocate_text_bin_raise_if_new_prefix_is_longer(tmpdir):
         spack.relocate.relocate_text_bin(
             [fpath], {short_prefix: long_prefix}
         )
+
+
+@pytest.mark.requires_executables('install_name_tool', 'file', 'cc')
+def test_fixup_macos_rpaths(make_dylib, make_object_file):
+    # For each of these tests except for the "correct" case, the first fixup
+    # should make changes, and the second fixup should be a null-op.
+    fixup_rpath = spack.relocate.fixup_macos_rpath
+
+    no_rpath = []
+    duplicate_rpaths = ['/usr', '/usr']
+    bad_rpath = ['/nonexistent/path']
+
+    # Non-relocatable library id and duplicate rpaths
+    (root, filename) = make_dylib("abs", duplicate_rpaths)
+    assert fixup_rpath(root, filename)
+    assert not fixup_rpath(root, filename)
+
+    # Bad but relocatable library id
+    (root, filename) = make_dylib("abs_with_rpath", no_rpath)
+    assert fixup_rpath(root, filename)
+    assert not fixup_rpath(root, filename)
+
+    # Library id uses rpath but there are extra duplicate rpaths
+    (root, filename) = make_dylib("rpath", duplicate_rpaths)
+    assert fixup_rpath(root, filename)
+    assert not fixup_rpath(root, filename)
+
+    # Shared library was constructed with relocatable id from the get-go
+    (root, filename) = make_dylib("rpath", no_rpath)
+    assert not fixup_rpath(root, filename)
+
+    # Non-relocatable library id
+    (root, filename) = make_dylib("abs", no_rpath)
+    assert not fixup_rpath(root, filename)
+
+    # Relocatable with executable paths and loader paths
+    (root, filename) = make_dylib("rpath", ['@executable_path/../lib',
+                                            '@loader_path'])
+    assert not fixup_rpath(root, filename)
+
+    # Non-relocatable library id but nonexistent rpath
+    (root, filename) = make_dylib("abs", bad_rpath)
+    assert fixup_rpath(root, filename)
+    assert not fixup_rpath(root, filename)
+
+    # Duplicate nonexistent rpath will need *two* passes
+    (root, filename) = make_dylib("rpath", bad_rpath * 2)
+    assert fixup_rpath(root, filename)
+    assert fixup_rpath(root, filename)
+    assert not fixup_rpath(root, filename)
+
+    # Test on an object file, which *also* has type 'application/x-mach-binary'
+    # but should be ignored (no ID headers, no RPATH)
+    # (this is a corner case for GCC installation)
+    (root, filename) = make_object_file()
+    assert not fixup_rpath(root, filename)