`cc`: ensure that RPATHs passed to linker are unique

macOS Sequoia's linker will complain if RPATHs on the CLI are specified more than once.
To avoid errors due to this, make `cc` only append unique RPATHs to the final args list.

This required a few improvements to the logic in `cc`:

1. List functions in `cc` didn't have any way to append unique elements to a list. Add a
   `contains()` shell function that works like our other list functions. Use it to implement
   an optional `"unique"` argument to `append()` and an `extend_unique()`. Use that to add
   RPATHs to the `args_list`.

2. In the pure `ld` case, we weren't actually parsing `RPATH` arguments separately as we
   do for `ccld`. Fix this by adding *another* nested case statement for raw `RPATH`
   parsing. There are now 3 places where we deal with `-rpath` and friends, but I don't
   see a great way to unify them, as `-Wl,`, `-Xlinker`, and raw `-rpath` arguments are
   all ever so slightly different.

3. Fix ordering of assertions to make `pytest` diffs more intelligible. The meaning of
   `+` and `-` in diffs changed in `pytest` 6.0 and the "preferred" order for assertions
   became `assert actual == expected` instead of the other way around.

Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>

2 files changed, 114 insertions, 26 deletions

diff --git a/lib/spack/env/cc b/lib/spack/env/cc
index 44f8b9316a..45e5e26269 100755
--- a/lib/spack/env/cc
+++ b/lib/spack/env/cc
@@ -101,10 +101,9 @@ setsep() {
     esac
 }
 
-# prepend LISTNAME ELEMENT [SEP]
+# prepend LISTNAME ELEMENT
 #
-# Prepend ELEMENT to the list stored in the variable LISTNAME,
-# assuming the list is separated by SEP.
+# Prepend ELEMENT to the list stored in the variable LISTNAME.
 # Handles empty lists and single-element lists.
 prepend() {
     varname="$1"
@@ -119,18 +118,39 @@ prepend() {
     fi
 }
 
-# append LISTNAME ELEMENT [SEP]
+# contains LISTNAME ELEMENT
 #
-# Append ELEMENT to the list stored in the variable LISTNAME,
-# assuming the list is separated by SEP.
+# Test whether LISTNAME contains ELEMENT.
+# Set $? to 1 if LISTNAME does not contain ELEMENT.
+# Set $? to 0 if LISTNAME does not contain ELEMENT.
+contains() {
+    varname="$1"
+    elt="$2"
+
+    setsep "$varname"
+
+    # the list may: 1) only contain the element, 2) start with the element,
+    # 3) contain the element in the middle, or 4) end wtih the element.
+    eval "[ \"\${$varname}\" = \"$elt\" ]" \
+        || eval "[ \"\${$varname#${elt}${sep}}\" != \"\${$varname}\" ]" \
+        || eval "[ \"\${$varname#*${sep}${elt}${sep}}\" != \"\${$varname}\" ]" \
+        || eval "[ \"\${$varname%${sep}${elt}}\" != \"\${$varname}\" ]"
+}
+
+# append LISTNAME ELEMENT [unique]
+#
+# Append ELEMENT to the list stored in the variable LISTNAME.
 # Handles empty lists and single-element lists.
+#
+# If the third argument is provided and if it is the string 'unique',
+# this will not append if ELEMENT is already in the list LISTNAME.
 append() {
     varname="$1"
     elt="$2"
 
     if empty "$varname"; then
         eval "$varname=\"\${elt}\""
-    else
+    elif [ "$3" != "unique" ] || ! contains "$varname" "$elt" ; then
         # Get the appropriate separator for the list we're appending to.
         setsep "$varname"
         eval "$varname=\"\${$varname}${sep}\${elt}\""
@@ -148,10 +168,21 @@ extend() {
     if [ "$sep" != " " ]; then
         IFS="$sep"
     fi
-    eval "for elt in \${$2}; do append $1 \"$3\${elt}\"; done"
+    eval "for elt in \${$2}; do append $1 \"$3\${elt}\" ${_append_args}; done"
     unset IFS
 }
 
+# extend_unique LISTNAME1 LISTNAME2 [PREFIX]
+#
+# Append the elements stored in the variable LISTNAME2 to the list
+# stored in LISTNAME1, if they are not already present.
+# If PREFIX is provided, prepend it to each element.
+extend_unique() {
+    _append_args="unique"
+    extend "$@"
+    unset _append_args
+}
+
 # preextend LISTNAME1 LISTNAME2 [PREFIX]
 #
 # Prepend the elements stored in the list at LISTNAME2
@@ -682,7 +713,32 @@ categorize_arguments() {
             "$dtags_to_strip")
                 ;;
             *)
-                append return_other_args_list "$1"
+                # if mode is not ld, we can just add to other args
+                if [ "$mode" != "ld" ]; then
+                    append return_other_args_list "$1"
+                    shift
+                    continue
+                fi
+
+                # if we're in linker mode, we need to parse raw RPATH args
+                case "$1" in
+                    -rpath=*)
+                        arg="${1#-rpath=}"
+                        append_path_lists return_rpath_dirs_list "$arg"
+                        ;;
+                    --rpath=*)
+                        arg="${1#--rpath=}"
+                        append_path_lists return_rpath_dirs_list "$arg"
+                        ;;
+                    -rpath|--rpath)
+                        shift
+                        [ $# -eq 0 ] && break  # ignore -rpath without value
+                        append_path_lists return_rpath_dirs_list "$1"
+                        ;;
+                    *)
+                        append return_other_args_list "$1"
+                        ;;
+                esac
                 ;;
         esac
         shift
@@ -890,35 +946,34 @@ extend args_list system_spack_flags_lib_dirs_list "-L"
 extend args_list system_lib_dirs_list "-L"
 
 # RPATHs arguments
+rpath_prefix=""
 case "$mode" in
     ccld)
         if [ -n "$dtags_to_add" ] ; then
             append args_list "$linker_arg$dtags_to_add"
         fi
-        extend args_list spack_store_spack_flags_rpath_dirs_list "$rpath"
-        extend args_list spack_store_rpath_dirs_list "$rpath"
-
-        extend args_list spack_flags_rpath_dirs_list "$rpath"
-        extend args_list rpath_dirs_list "$rpath"
-
-        extend args_list system_spack_flags_rpath_dirs_list "$rpath"
-        extend args_list system_rpath_dirs_list "$rpath"
+        rpath_prefix="$rpath"
         ;;
     ld)
         if [ -n "$dtags_to_add" ] ; then
             append args_list "$dtags_to_add"
         fi
-        extend args_list spack_store_spack_flags_rpath_dirs_list "-rpath${lsep}"
-        extend args_list spack_store_rpath_dirs_list "-rpath${lsep}"
-
-        extend args_list spack_flags_rpath_dirs_list "-rpath${lsep}"
-        extend args_list rpath_dirs_list "-rpath${lsep}"
-
-        extend args_list system_spack_flags_rpath_dirs_list "-rpath${lsep}"
-        extend args_list system_rpath_dirs_list "-rpath${lsep}"
+        rpath_prefix="-rpath${lsep}"
         ;;
 esac
 
+# if mode is ccld or ld, extend RPATH lists with the prefix determined above
+if [ -n "$rpath_prefix" ]; then
+    extend_unique args_list spack_store_spack_flags_rpath_dirs_list "$rpath_prefix"
+    extend_unique args_list spack_store_rpath_dirs_list "$rpath_prefix"
+
+    extend_unique args_list spack_flags_rpath_dirs_list "$rpath_prefix"
+    extend_unique args_list rpath_dirs_list "$rpath_prefix"
+
+    extend_unique args_list system_spack_flags_rpath_dirs_list "$rpath_prefix"
+    extend_unique args_list system_rpath_dirs_list "$rpath_prefix"
+fi
+
 # Other arguments from the input command
 extend args_list other_args_list
 extend args_list spack_flags_other_args_list
diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py
index 4a394680f4..dc08fdfb04 100644
--- a/lib/spack/spack/test/cc.py
+++ b/lib/spack/spack/test/cc.py
@@ -199,7 +199,7 @@ def check_args(cc, args, expected):
     """
     with set_env(SPACK_TEST_COMMAND="dump-args"):
         cc_modified_args = cc(*args, output=str).strip().split("\n")
-        assert expected == cc_modified_args
+        assert cc_modified_args == expected
 
 
 def check_args_contents(cc, args, must_contain, must_not_contain):
@@ -354,6 +354,39 @@ def test_fc_flags(wrapper_environment, wrapper_flags):
     )
 
 
+def test_ld_flags_with_redundant_rpaths(wrapper_environment, wrapper_flags):
+    check_args(
+        ld,
+        test_args + test_rpaths,  # ensure thesee are made unique
+        ["ld"]
+        + test_include_paths
+        + test_library_paths
+        + ["--disable-new-dtags"]
+        + test_rpaths
+        + test_args_without_paths
+        + spack_ldlibs,
+    )
+
+
+def test_cc_flags_with_redundant_rpaths(wrapper_environment, wrapper_flags):
+    check_args(
+        cc,
+        test_args + test_wl_rpaths + test_wl_rpaths,  # ensure thesee are made unique
+        [real_cc]
+        + target_args
+        + test_include_paths
+        + ["-Lfoo"]
+        + test_library_paths
+        + ["-Wl,--disable-new-dtags"]
+        + test_wl_rpaths
+        + test_args_without_paths
+        + spack_cppflags
+        + spack_cflags
+        + ["-Wl,--gc-sections"]
+        + spack_ldlibs,
+    )
+
+
 def test_always_cflags(wrapper_environment, wrapper_flags):
     with set_env(SPACK_ALWAYS_CFLAGS="-always1 -always2"):
         check_args(