Bug fix for duplicate rpath errors on macOS when creating build caches

[climbfuji]

fixes #34374

Description

Fixes duplicate rpath errors (arguments) for install_name_tool when creating build caches on macOS, reported in #34374.

This also takes care of the problem that a relocation may result in adding the same rpath twice, which install_name_tool doesn't like (commit de92afb).

[alalazo]

A minor comment. Let me know if it works.

[alalazo]

Instead of adding conditionals, I wonder if we can add tuples - similarly to what you're doing:

Suggested change

	args_to_add = ["-id", new_idpath]
	if args_to_add not in args:
	args += [args_to_add]
	args += ("-id", new_idpath)

in all three conditionals. Then we can use:

args = llnl.util.dedupe(args)
args = list(itertools.chain.from_iterable(args))

to deduplicate and flatten.

[climbfuji]

module 'llnl.util' has no attribute 'dedupe'

I fixed that one, but it's not working as expected:

 'install_name_tool' '-' 'i' 'd' '/' 'U' 's' 'e' 'r' 's' '/' 'h' 'e' 'i' 'n' 'z' 'e' 'l' 'l' '/' 'w' 'o' 'r' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '-' 'b' 'u' 'i' 'l' 'd' '-' 'a' 'n' 'd' '-' 's' 'o' 'u' 'r' 'c' 'e' '-' 'c' 'a' 'c' 'h' 'e' '/' 'e' 'n' 'v' 's' '/' 's' 'k' 'y' 'l' 'a' 'b' '-' 'd' 'e' 'v' '/' 'i' 'n' 's' 't' 'a' 'l' 'l' '/' 'a' 'p' 'p' 'l' 'e' '-' 'c' 'l' 'a' 'n' 'g' '/' '1' '3' '.' '1' '.' '6' '/' 'e' 'c' 'c' 'o' 'd' 'e' 's' '-' '2' '.' '2' '7' '.' '0' '-' '3' '4' 'm' '7' 'k' 'n' '6' '/' 'l' 'i' 'b' '/' 'l' 'i' 'b' 'e' 'c' 'c' 'o' 'd' 'e' 's' '.' 'd' 'y' 'l' 'i' 'b' '-' 'c' 'h' 'a' 'n' 'g' 'e' '@' 'l' 'o' 'a' 'd' 'e' 'r' '_' 'p' 'a' 't' 'h' '/' '.' '.' '/' '.' '.' '/' 'o' 'p' 'e' 'n' 'j' 'p' 'e' 'g' '-' '2' '.' '3' '.' '1' '-' 'l' 'y' 'n' 'j' 'k' 'b' 'm' '/' 'l' 'i' 'b' '/' 'l' 'i' 'b' 'o' 'p' 'e' 'n' 'j' 'p' '2' '.' '7' '.' 'd' 'y' 'l' 'i' 'b' '/' 'U' 's' 'e' 'r' 's' '/' 'h' 'e' 'i' 'n' 'z' 'e' 'l' 'l' '/' 'w' 'o' 'r' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '-' 'b' 'u' 'i' 'l' 'd' '-' 'a' 'n' 'd' '-' 's' 'o' 'u' 'r' 'c' 'e' '-' 'c' 'a' 'c' 'h' 'e' '/' 'e' 'n' 'v' 's' '/' 's' 'k' 'y' 'l' 'a' 'b' '-' 'd' 'e' 'v' '/' 'i' 'n' 's' 't' 'a' 'l' 'l' '/' 'a' 'p' 'p' 'l' 'e' '-' 'c' 'l' 'a' 'n' 'g' '/' '1' '3' '.' '1' '.' '6' '/' 'o' 'p' 'e' 'n' 'j' 'p' 'e' 'g' '-' '2' '.' '3' '.' '1' '-' 'l' 'y' 'n' 'j' 'k' 'b' 'm' '/' 'l' 'i' 'b' '/' 'l' 'i' 'b' 'o' 'p' 'e' 'n' 'j' 'p' '2' '.' '7' '.' 'd' 'y' 'l' 'i' 'b' '-' 'r' 'p' 'a' 't' 'h' '@' 'l' 'o' 'a' 'd' 'e' 'r' '_' 'p' 'a' 't' 'h' '/' '.' '/' 'U' 's' 'e' 'r' 's' '/' 'h' 'e' 'i' 'n' 'z' 'e' 'l' 'l' '/' 'w' 'o' 'r' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '-' 'b' 'u' 'i' 'l' 'd' '-' 'a' 'n' 'd' '-' 's' 'o' 'u' 'r' 'c' 'e' '-' 'c' 'a' 'c' 'h' 'e' '/' 'e' 'n' 'v' 's' '/' 's' 'k' 'y' 'l' 'a' 'b' '-' 'd' 'e' 'v' '/' 'i' 'n' 's' 't' 'a' 'l' 'l' '/' 'a' 'p' 'p' 'l' 'e' '-' 'c' 'l' 'a' 'n' 'g' '/' '1' '3' '.' '1' '.' '6' '/' 'e' 'c' 'c' 'o' 'd' 'e' 's' '-' '2' '.' '2' '7' '.' '0' '-' '3' '4' 'm' '7' 'k' 'n' '6' '/' 'l' 'i' 'b' '@' 'l' 'o' 'a' 'd' 'e' 'r' '_' 'p' 'a' 't' 'h' '/' '.' '.' '/' 'l' 'i' 'b' '6' '4' '/' 'U' 's' 'e' 'r' 's' '/' 'h' 'e' 'i' 'n' 'z' 'e' 'l' 'l' '/' 'w' 'o' 'r' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '-' 'b' 'u' 'i' 'l' 'd' '-' 'a' 'n' 'd' '-' 's' 'o' 'u' 'r' 'c' 'e' '-' 'c' 'a' 'c' 'h' 'e' '/' 'e' 'n' 'v' 's' '/' 's' 'k' 'y' 'l' 'a' 'b' '-' 'd' 'e' 'v' '/' 'i' 'n' 's' 't' 'a' 'l' 'l' '/' 'a' 'p' 'p' 'l' 'e' '-' 'c' 'l' 'a' 'n' 'g' '/' '1' '3' '.' '1' '.' '6' '/' 'e' 'c' 'c' 'o' 'd' 'e' 's' '-' '2' '.' '2' '7' '.' '0' '-' '3' '4' 'm' '7' 'k' 'n' '6' '/' 'l' 'i' 'b' '6' '4' '@' 'l' 'o' 'a' 'd' 'e' 'r' '_' 'p' 'a' 't' 'h' '/' '.' '.' '/' '.' '.' '/' 'o' 'p' 'e' 'n' 'j' 'p' 'e' 'g' '-' '2' '.' '3' '.' '1' '-' 'l' 'y' 'n' 'j' 'k' 'b' 'm' '/' 'l' 'i' 'b' '/' 'U' 's' 'e' 'r' 's' '/' 'h' 'e' 'i' 'n' 'z' 'e' 'l' 'l' '/' 'w' 'o' 'r' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '/' 's' 'p' 'a' 'c' 'k' '-' 's' 't' 'a' 'c' 'k' '-' 'b' 'u' 'i' 'l' 'd' '-' 'a' 'n' 'd' '-' 's' 'o' 'u' 'r' 'c' 'e' '-' 'c' 'a' 'c' 'h' 'e' '/' 'e' 'n' 'v' 's' '/' 's' 'k' 'y' 'l' 'a' 'b' '-' 'd' 'e' 'v' '/' 'i' 'n' 's' 't' 'a' 'l' 'l' '/' 'a' 'p' 'p' 'l' 'e' '-' 'c' 'l' 'a' 'n' 'g' '/' '1' '3' '.' '1' '.' '6' '/' 'o' 'p' 'e' 'n' 'j' 'p' 'e' 'g' '-' '2' '.' '3' '.' '1' '-' 'l' 'y' 'n' 'j' 'k' 'b' 'm' '/' 'l' 'i' 'b' '@' 'l' 'o' 'a' 'd' 'e' 'r' '_' 'p' 'a' 't' 'h' '/' '.' '.' '/' 'l' 'i' 'b' '/Users/heinzell/work/spack-stack/spack-stack-build-and-source-cache/envs/tst-cache/install/apple-clang/13.1.6/eccodes-2.27.0-34m7kn6/lib/libeccodes.dylib'

[alalazo]

This is the diff for the suggestion above:

diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index 732534b402..d87fbc10b2 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import collections
+import itertools
 import multiprocessing.pool
 import os
 import re
@@ -299,29 +300,24 @@ def modify_macho_object(cur_path, rpaths, deps, idpath, paths_to_paths):
     if idpath:
         new_idpath = paths_to_paths.get(idpath, None)
         if new_idpath and not idpath == new_idpath:
-            args_to_add = ["-id", new_idpath]
-            if args_to_add not in args:
-                args += [args_to_add]
+            args += [("-id", new_idpath)]
 
     for dep in deps:
         new_dep = paths_to_paths.get(dep)
         if new_dep and dep != new_dep:
-            args_to_add = ["-change", dep, new_dep]
-            if args_to_add not in args:
-                args += [args_to_add]
+            args += [("-change", dep, new_dep)]
 
     new_rpaths = []
     for orig_rpath in rpaths:
         new_rpath = paths_to_paths.get(orig_rpath)
         if new_rpath and not orig_rpath == new_rpath:
-            args_to_add = ["-rpath", orig_rpath, new_rpath]
+            args_to_add = ("-rpath", orig_rpath, new_rpath)
             if args_to_add not in args and new_rpath not in new_rpaths:
                 args += [args_to_add]
                 new_rpaths.append(new_rpath)
 
-    # Flatten list of lists
-    args = [item for subargs in args for item in subargs]
-
+    # Deduplicate and flatten
+    args = list(itertools.chain.from_iterable(llnl.util.lang.dedupe(args)))
     if args:
         args.append(str(cur_path))
         install_name_tool = executable.Executable("install_name_tool")

Can you check if it works for you too?

[climbfuji]

This works for me. I get weird errors like

find: {}: No such file or directory
find: {}: No such file or directory
find: {}: No such file or directory

one for every package that it puts in the buildcache, but that's unrelated. And then it aborts with

==> Error: Unknown Mach-O header: 0x7f454c46 in <_io.BufferedReader name='/var/folders/gb/w8lys0xn3c35rbw_5vfq4mkxxgj2gv/T/tmp5kdy8n02/crtm-v2.4_jedi-53ivn27/share/crtm/src/src/Build/CMakeFiles/3.16.0/CompilerIdFortran/a.out'>

which is again unrelated to this PR/your suggestion. I'll push an update momentarily.

[alalazo]

What did you try? I was able to reproduce #34374 on an Intel based macOS (ventura, apple-clang@14.0.0), and this modification was fixing the issue with libtiff.

[climbfuji]

@alalazo Your suggestion to use tuples didn't work as expected, see comment above (#34375 (comment)). I reverted to my original approach. I also had to add commit de92afb to avoid situations where install_name_tool would try to add the same rpath twice, and it doesn't like that.

[climbfuji]

@haampie @alalazo A gentle reminder about this PR that addresses issues with duplicate rpath in the install_name_tool command on macOS ... thanks!

[alalazo]

@spackbot run pipeline

[spackbot-app]

I've started that pipeline for you!

[haampie]

Optimally we wouldn't end up in a situation where duplicate paths are passed to that function, but I think the solution is fine for now.