cc: refactor flag adding so that it's not in reverse order

- flags were prepended in reverse order to args, but this makes it hard
  to see what order they'll be in on the final command line.

- add them in the order they'll appear to make cc easier to maintain.

- simplify code for assembling the command line

- fix separator used in SPACK_SYSTEM_DIRS test

2 files changed, 56 insertions, 29 deletions

diff --git a/lib/spack/env/cc b/lib/spack/env/cc
index e8a0903d2a..2dd82d7d73 100755
--- a/lib/spack/env/cc
+++ b/lib/spack/env/cc
@@ -80,7 +80,7 @@ IFS=':' read -ra SPACK_SYSTEM_DIRS <<< "${SPACK_SYSTEM_DIRS}"
 # test whether a path is a system directory
 function system_dir {
     path="$1"
-    for sd in ${SPACK_SYSTEM_DIRS[@]}; do
+    for sd in "${SPACK_SYSTEM_DIRS[@]}"; do
         if [ "${path}" == "${sd}" ] || [ "${path}" == "${sd}/" ]; then
             # success if path starts with a system prefix
             return 0
@@ -89,7 +89,7 @@ function system_dir {
     return 1  # fail if path starts no system prefix
 }
 
-for param in ${parameters[@]}; do
+for param in "${parameters[@]}"; do
     if [[ -z ${!param} ]]; then
         die "Spack compiler must be run from Spack! Input '$param' is missing."
     fi
@@ -248,7 +248,6 @@ fi
 
 # Save original command for debug logging
 input_command="$*"
-args=()
 
 #
 # Parse the command line arguments.
@@ -277,7 +276,9 @@ libs=()
 other_args=()
 
 while [ -n "$1" ]; do
+    # an RPATH to be added after the case statement.
     rp=""
+
     case "$1" in
         -I*)
             arg="${1#-I}"
@@ -362,37 +363,53 @@ while [ -n "$1" ]; do
 done
 
 #
-# Prepend flags from Spack's cppflags, cflags, cxxflags, fcflags, fflags,
-# and ldflags.
+# Add flags from Spack's cppflags, cflags, cxxflags, fcflags, fflags, and
+# ldflags. We stick to the order that gmake puts the flags in by default.
+#
+# See the gmake manual on implicit rules for details:
+# https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html
 #
+flags=()
 
-# Add ldflags
+# Fortran flags come before CPPFLAGS
 case "$mode" in
-    ld|ccld)
-        args=(${SPACK_LDFLAGS[@]} "${args[@]}") ;;
+    cc|ccld)
+        case $lang_flags in
+            F)
+                flags=("${flags[@]}" "${SPACK_FFLAGS[@]}") ;;
+        esac
+        ;;
+esac
+
+# C preprocessor flags come before any C/CXX flags
+case "$mode" in
+    cpp|as|cc|ccld)
+        flags=("${flags[@]}" "${SPACK_CPPFLAGS[@]}") ;;
 esac
 
-# Add C, C++, and Fortran flags
+
+# Add C and C++ flags
 case "$mode" in
     cc|ccld)
         case $lang_flags in
             C)
-                args=(${SPACK_CFLAGS[@]} "${args[@]}") ;;
+                flags=("${flags[@]}" "${SPACK_CFLAGS[@]}") ;;
             CXX)
-                args=(${SPACK_CXXFLAGS[@]} "${args[@]}") ;;
-            F)
-                args=(${SPACK_FFLAGS[@]} "${args[@]}") ;;
+                flags=("${flags[@]}" "${SPACK_CXXFLAGS[@]}") ;;
         esac
         ;;
 esac
 
-# Add cppflags
+# Linker flags
 case "$mode" in
-    cpp|as|cc|ccld)
-        args=(${SPACK_CPPFLAGS[@]} "${args[@]}") ;;
+    ld|ccld)
+        flags=("${flags[@]}" "${SPACK_LDFLAGS[@]}") ;;
 esac
 
-# Include all -L's and prefix/whatever dirs in rpath
+
+# Include the package's prefix/lib[64] dirs in rpath. We don't know until
+# *after* installation which one's correct, so we include both lib and
+# lib64, assuming that only one will be present.
 case "$mode" in
     ld|ccld)
         $add_rpaths && rpaths+=("$SPACK_PREFIX/lib")
@@ -400,10 +417,10 @@ case "$mode" in
         ;;
 esac
 
-# Read spack dependencies from the path environment variable
+# Read spack dependencies from the environment. This is a list of prefixes.
 IFS=':' read -ra deps <<< "$SPACK_DEPENDENCIES"
 for dep in "${deps[@]}"; do
-    # Append include directories
+    # Append include directories in any compilation mode
     case "$mode" in
         cpp|cc|as|ccld)
             if [[ -d $dep/include ]]; then
@@ -412,7 +429,7 @@ for dep in "${deps[@]}"; do
             ;;
     esac
 
-    # Append lib/lib64 and RPATH directories
+    # Append lib/lib64 and RPATH directories, but only if we're linking
     case "$mode" in
         ld|ccld)
             if [[ -d $dep/lib ]]; then
@@ -436,6 +453,7 @@ for dep in "${deps[@]}"; do
     esac
 done
 
+# add RPATHs if we're in in any linking mode
 case "$mode" in
     ld|ccld)
         # Set extra RPATHs
@@ -446,14 +464,23 @@ case "$mode" in
         done
 
         # Add SPACK_LDLIBS to args
-        for lib in ${SPACK_LDLIBS[@]}; do
+        for lib in "${SPACK_LDLIBS[@]}"; do
             libs+=("${lib#-l}")
         done
         ;;
 esac
 
-# Put the arguments back together in one list
+#
+# Finally, reassemble the command line.
+#
+
 # Includes and system includes first
+args=()
+
+# flags assembled earlier
+args+=("${flags[@]}")
+
+# include directory search paths
 for dir in "${includes[@]}";         do args+=("-I$dir"); done
 for dir in "${system_includes[@]}";  do args+=("-I$dir"); done
 
diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py
index 517b7f1dd3..1e0ef31070 100644
--- a/lib/spack/spack/test/cc.py
+++ b/lib/spack/spack/test/cc.py
@@ -124,7 +124,7 @@ def wrapper_environment():
             SPACK_DEBUG_LOG_ID='foo-hashabc',
             SPACK_COMPILER_SPEC='gcc@4.4.7',
             SPACK_SHORT_SPEC='foo@1.2 arch=linux-rhel6-x86_64 /hashabc',
-            SPACK_SYSTEM_DIRS=' '.join(system_dirs),
+            SPACK_SYSTEM_DIRS=':'.join(system_dirs),
             SPACK_CC_RPATH_ARG='-Wl,-rpath,',
             SPACK_CXX_RPATH_ARG='-Wl,-rpath,',
             SPACK_F77_RPATH_ARG='-Wl,-rpath,',
@@ -180,27 +180,27 @@ pytestmark = pytest.mark.usefixtures('wrapper_environment')
 
 def check_cc(command, args, expected):
     with set_env(SPACK_TEST_COMMAND=command):
-        assert cc(*args, output=str).strip().split() == expected
+        assert expected == cc(*args, output=str).strip().split()
 
 
 def check_cxx(command, args, expected):
     with set_env(SPACK_TEST_COMMAND=command):
-        assert cxx(*args, output=str).strip().split() == expected
+        assert expected == cxx(*args, output=str).strip().split()
 
 
 def check_fc(command, args, expected):
     with set_env(SPACK_TEST_COMMAND=command):
-        assert fc(*args, output=str).strip().split() == expected
+        assert expected == fc(*args, output=str).strip().split()
 
 
 def check_ld(command, args, expected):
     with set_env(SPACK_TEST_COMMAND=command):
-        assert ld(*args, output=str).strip().split() == expected
+        assert expected == ld(*args, output=str).strip().split()
 
 
 def check_cpp(command, args, expected):
     with set_env(SPACK_TEST_COMMAND=command):
-        assert cpp(*args, output=str).strip().split() == expected
+        assert expected == cpp(*args, output=str).strip().split()
 
 
 def test_vcheck_mode():
@@ -303,8 +303,8 @@ def test_fc_flags(wrapper_flags):
     check_fc(
         'dump-args', test_args,
         [real_cc] +
-        spack_cppflags +
         spack_fflags +
+        spack_cppflags +
         spack_ldflags +
         test_include_paths +
         test_library_paths +