Put back the use of otool and install_name_tool when running on macOS. Only use machotools on linux. (#12867)

Move verbose messages to debug level

get_patchelf should return None for test platform as well because create_buildinfo invokes patchelf to get rpaths.

4 files changed, 112 insertions, 68 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 564aa3a535..40e3a7c17a 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -29,7 +29,6 @@ from spack.util.gpg import Gpg
 from spack.util.web import spider, read_from_url
 from spack.util.executable import ProcessError
 
-
 _build_cache_relative_path = 'build_cache'
 
 
@@ -155,8 +154,9 @@ def write_buildinfo_file(prefix, workdir, rel=False):
                         tty.warn(msg)
 
             if relocate.needs_binary_relocation(m_type, m_subtype):
-                rel_path_name = os.path.relpath(path_name, prefix)
-                binary_to_relocate.append(rel_path_name)
+                if not filename.endswith('.o'):
+                    rel_path_name = os.path.relpath(path_name, prefix)
+                    binary_to_relocate.append(rel_path_name)
             if relocate.needs_text_relocation(m_type, m_subtype):
                 rel_path_name = os.path.relpath(path_name, prefix)
                 text_to_relocate.append(rel_path_name)
@@ -308,6 +308,7 @@ def build_tarball(spec, outdir, force=False, rel=False, unsigned=False,
             os.remove(specfile_path)
         else:
             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))
     install_tree(spec.prefix, workdir, symlinks=True)
@@ -424,11 +425,11 @@ def make_package_relative(workdir, spec, allow_root):
         orig_path_names.append(os.path.join(prefix, filename))
         cur_path_names.append(os.path.join(workdir, filename))
     if spec.architecture.platform == 'darwin':
-        relocate.make_macho_binary_relative(cur_path_names, orig_path_names,
-                                            old_path, allow_root)
+        relocate.make_macho_binaries_relative(cur_path_names, orig_path_names,
+                                              old_path, allow_root)
     else:
-        relocate.make_elf_binary_relative(cur_path_names, orig_path_names,
-                                          old_path, allow_root)
+        relocate.make_elf_binaries_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', []):
@@ -439,8 +440,8 @@ def make_package_relative(workdir, spec, allow_root):
 
 def make_package_placeholder(workdir, spec, allow_root):
     """
-    Check if package binaries are relocatable
-    Change links to placeholder links
+    Check if package binaries are relocatable.
+    Change links to placeholder links.
     """
     prefix = spec.prefix
     buildinfo = read_buildinfo_file(workdir)
@@ -487,11 +488,11 @@ def relocate_package(workdir, spec, allow_root):
             path_name = os.path.join(workdir, filename)
             path_names.add(path_name)
         if spec.architecture.platform == 'darwin':
-            relocate.relocate_macho_binaries(path_names, old_path, new_path,
-                                             allow_root)
+            relocate.relocate_macho_binaries(path_names, old_path,
+                                             new_path, allow_root)
         else:
-            relocate.relocate_elf_binaries(path_names, old_path, new_path,
-                                           allow_root)
+            relocate.relocate_elf_binaries(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)
diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py
index b3d73053e4..5d5e693319 100644
--- a/lib/spack/spack/cmd/buildcache.py
+++ b/lib/spack/spack/cmd/buildcache.py
@@ -323,34 +323,38 @@ def createtarball(args):
     matches = find_matching_specs(pkgs, env=env)
 
     if matches:
-        tty.msg('Found at least one matching spec')
+        tty.debug('Found at least one matching spec')
 
     for match in matches:
-        tty.msg('examining match {0}'.format(match.format()))
+        tty.debug('examining match {0}'.format(match.format()))
         if match.external or match.virtual:
-            tty.msg('skipping external or virtual spec %s' %
-                    match.format())
+            tty.debug('skipping external or virtual spec %s' %
+                      match.format())
         else:
-            tty.msg('adding matching spec %s' % match.format())
+            tty.debug('adding matching spec %s' % match.format())
             specs.add(match)
-            tty.msg('recursing dependencies')
+            tty.debug('recursing dependencies')
             for d, node in match.traverse(order='post',
                                           depth=True,
                                           deptype=('link', 'run')):
                 if node.external or node.virtual:
-                    tty.msg('skipping external or virtual dependency %s' %
-                            node.format())
+                    tty.debug('skipping external or virtual dependency %s' %
+                              node.format())
                 else:
-                    tty.msg('adding dependency %s' % node.format())
+                    tty.debug('adding dependency %s' % node.format())
                     specs.add(node)
 
-    tty.msg('writing tarballs to %s/build_cache' % outdir)
+    tty.debug('writing tarballs to %s/build_cache' % outdir)
 
     for spec in specs:
         tty.msg('creating binary cache file for package %s ' % spec.format())
-        bindist.build_tarball(spec, outdir, args.force, args.rel,
-                              args.unsigned, args.allow_root, signkey,
-                              not args.no_rebuild_index)
+        try:
+            bindist.build_tarball(spec, outdir, args.force, args.rel,
+                                  args.unsigned, args.allow_root, signkey,
+                                  not args.no_rebuild_index)
+        except Exception as e:
+            tty.warn('%s' % e)
+            pass
 
 
 def installtarball(args):
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index f396837708..01fc913a0e 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -66,6 +66,10 @@ def get_patchelf():
     Returns the full patchelf binary path.
     """
     # as we may need patchelf, find out where it is
+    if str(spack.architecture.platform()) == 'test':
+        return None
+    if str(spack.architecture.platform()) == 'darwin':
+        return None
     patchelf = spack.util.executable.which('patchelf')
     if patchelf is None:
         patchelf_spec = spack.cmd.parse_specs("patchelf", concretize=True)[0]
@@ -255,17 +259,19 @@ def modify_macho_object(cur_path, rpaths, deps, idpath,
     if 'libgcc_' in cur_path:
         return
     install_name_tool = Executable('install_name_tool')
-    args = []
-    if new_idpath:
-        args.extend(['-id', new_idpath])
+    if new_idpath and not idpath == new_idpath:
+        install_name_tool('-id', new_idpath, str(cur_path))
 
-    for orig, new in zip(deps, new_deps):
-        args.extend(['-change', orig, new])
+    if len(deps) == len(new_deps):
+        for orig, new in zip(deps, new_deps):
+            if not orig == new:
+                install_name_tool('-change', orig, new, str(cur_path))
+
+    if len(rpaths) == len(new_rpaths):
+        for orig, new in zip(rpaths, new_rpaths):
+            if not orig == new:
+                install_name_tool('-rpath', orig, new, str(cur_path))
 
-    for orig, new in zip(rpaths, new_rpaths):
-        args.extend(['-rpath', orig, new])
-    args.append(str(cur_path))
-    install_name_tool(*args)
     return
 
 
@@ -281,17 +287,19 @@ def modify_object_machotools(cur_path, rpaths, deps, idpath,
     The old install dir in LC_RPATH is replaced with the new install dir using
     using py-machotools
     """
+    if cur_path.endswith('.o'):
+        return
     try:
         import machotools
     except ImportError as e:
         raise MissingMachotoolsException(e)
     rewriter = machotools.rewriter_factory(cur_path)
     if machotools.detect.is_dylib(cur_path):
-        rewriter.install_name = new_idpath
+        if not new_idpath == idpath:
+            rewriter.install_name = new_idpath
     for orig, new in zip(deps, new_deps):
-        rewriter.change_dependency(orig, new)
-    for orig, new in zip(rpaths, new_rpaths):
-        rewriter.append_rpath(new)
+        if not orig == new:
+            rewriter.change_dependency(orig, new)
     rewriter.commit()
     return
 
@@ -391,12 +399,21 @@ def replace_prefix_bin(path_name, old_dir, new_dir):
 
 def relocate_macho_binaries(path_names, old_dir, new_dir, allow_root):
     """
-    Change old_dir to new_dir in RPATHs of elf or mach-o files
+    Change old_dir to new_dir in LC_RPATH of mach-o files (on macOS)
+    Change old_dir to new_dir in LC_ID and LC_DEP of mach-o files
     Account for the case where old_dir is now a placeholder
     """
     placeholder = set_placeholder(old_dir)
     for path_name in path_names:
-        (rpaths, deps, idpath) = machotools_get_paths(path_name)
+        deps = set()
+        idpath = None
+        if platform.system().lower() == 'darwin':
+            if path_name.endswith('.o'):
+                continue
+            else:
+                rpaths, deps, idpath = macho_get_paths(path_name)
+        else:
+            rpaths, deps, idpath = machotools_get_paths(path_name)
         # one pass to replace placeholder
         (n_rpaths,
          n_deps,
@@ -413,16 +430,23 @@ def relocate_macho_binaries(path_names, old_dir, new_dir, allow_root):
                                            n_rpaths,
                                            n_deps,
                                            n_idpath)
-        modify_object_machotools(path_name,
-                                 rpaths, deps, idpath,
-                                 new_rpaths, new_deps, new_idpath)
-        if len(new_dir) <= len(old_dir):
-            replace_prefix_bin(path_name, old_dir, new_dir)
+        if platform.system().lower() == 'darwin':
+            modify_macho_object(path_name,
+                                rpaths, deps, idpath,
+                                new_rpaths, new_deps, new_idpath)
         else:
-            tty.warn('Cannot do a binary string replacement'
-                     ' with padding for %s'
-                     ' because %s is longer than %s' %
-                     (path_name, new_dir, old_dir))
+            modify_object_machotools(path_name,
+                                     rpaths, deps, idpath,
+                                     new_rpaths, new_deps, new_idpath)
+
+        if not new_dir == old_dir:
+            if len(new_dir) <= len(old_dir):
+                replace_prefix_bin(path_name, old_dir, new_dir)
+            else:
+                tty.warn('Cannot do a binary string replacement'
+                         ' with padding for %s'
+                         ' because %s is longer than %s' %
+                         (path_name, new_dir, old_dir))
 
 
 def relocate_elf_binaries(path_names, old_dir, new_dir, allow_root):
@@ -441,13 +465,14 @@ def relocate_elf_binaries(path_names, old_dir, new_dir, allow_root):
             new_rpaths = substitute_rpath(n_rpaths,
                                           old_dir, new_dir)
             modify_elf_object(path_name, new_rpaths)
-            if len(new_dir) <= len(old_dir):
-                replace_prefix_bin(path_name, old_dir, new_dir)
-            else:
-                tty.warn('Cannot do a binary string replacement'
-                         ' with padding for %s'
-                         ' because %s is longer than %s.' %
-                         (path_name, new_dir, old_dir))
+            if not new_dir == old_dir:
+                if len(new_dir) <= len(old_dir):
+                    replace_prefix_bin(path_name, old_dir, new_dir)
+                else:
+                    tty.warn('Cannot do a binary string replacement'
+                             ' with padding for %s'
+                             ' because %s is longer than %s.' %
+                             (path_name, new_dir, old_dir))
 
 
 def make_link_relative(cur_path_names, orig_path_names):
@@ -462,27 +487,39 @@ def make_link_relative(cur_path_names, orig_path_names):
         os.symlink(new_src, cur_path)
 
 
-def make_macho_binary_relative(cur_path_names, orig_path_names, old_dir,
-                               allow_root):
+def make_macho_binaries_relative(cur_path_names, orig_path_names, old_dir,
+                                 allow_root):
     """
     Replace old RPATHs with paths relative to old_dir in binary files
     """
     for cur_path, orig_path in zip(cur_path_names, orig_path_names):
-        rpaths, deps, idpath = machotools_get_paths(cur_path)
+        rpaths = set()
+        deps = set()
+        idpath = None
+        if platform.system().lower() == 'darwin':
+            (rpaths, deps, idpath) = macho_get_paths(cur_path)
+        else:
+            (rpaths, deps, idpath) = machotools_get_paths(cur_path)
         (new_rpaths,
          new_deps,
          new_idpath) = macho_make_paths_relative(orig_path, old_dir,
                                                  rpaths, deps, idpath)
-        modify_object_machotools(cur_path,
-                                 rpaths, deps, idpath,
-                                 new_rpaths, new_deps, new_idpath)
+        if platform.system().lower() == 'darwin':
+            modify_macho_object(cur_path,
+                                rpaths, deps, idpath,
+                                new_rpaths, new_deps, new_idpath)
+        else:
+            modify_object_machotools(cur_path,
+                                     rpaths, deps, idpath,
+                                     new_rpaths, new_deps, new_idpath)
+
         if (not allow_root and
-                not file_is_relocatable(cur_path, old_dir)):
+                not file_is_relocatable(cur_path)):
             raise InstallRootStringException(cur_path, old_dir)
 
 
-def make_elf_binary_relative(cur_path_names, orig_path_names, old_dir,
-                             allow_root):
+def make_elf_binaries_relative(cur_path_names, orig_path_names, old_dir,
+                               allow_root):
     """
     Replace old RPATHs with paths relative to old_dir in binary files
     """
@@ -493,7 +530,7 @@ def make_elf_binary_relative(cur_path_names, orig_path_names, old_dir,
                                              orig_rpaths)
             modify_elf_object(cur_path, new_rpaths)
         if (not allow_root and
-                not file_is_relocatable(cur_path, old_dir)):
+                not file_is_relocatable(cur_path)):
             raise InstallRootStringException(cur_path, old_dir)
 
 
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index cff60e3127..e865cac32e 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 relocate_text, relocate_links
+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
@@ -364,6 +364,8 @@ def test_elf_paths():
 def test_relocate_macho(tmpdir):
     with tmpdir.as_cwd():
 
+        get_patchelf()  # this does nothing on Darwin
+
         rpaths, deps, idpath = macho_get_paths('/bin/bash')
         nrpaths, ndeps, nid = macho_make_paths_relative('/bin/bash', '/usr',
                                                         rpaths, deps, idpath)