bugfix: cc handles spaces in flag variables properly

- cc cleanup caused a parsing regression in flag handling

- We added proper quoting to array expansions, but flag variables were
  never actually converted to arrays. Old code relied on this.

This commit:
- Adds reads to convert flags to arrays.
- Makes the cc test check for improper space handling to prevent future
  regressions.

2 files changed, 82 insertions, 90 deletions

diff --git a/lib/spack/env/cc b/lib/spack/env/cc
index 2dd82d7d73..826a0fe457 100755
--- a/lib/spack/env/cc
+++ b/lib/spack/env/cc
@@ -74,9 +74,18 @@ function die {
     exit 1
 }
 
-# read system directories into an array (delimited by :)
+# read input parameters into proper bash arrays.
+# SYSTEM_DIRS is delimited by :
 IFS=':' read -ra SPACK_SYSTEM_DIRS <<< "${SPACK_SYSTEM_DIRS}"
 
+# SPACK_<LANG>FLAGS and SPACK_LDLIBS are split by ' '
+IFS=' ' read -ra SPACK_FFLAGS   <<< "$SPACK_FFLAGS"
+IFS=' ' read -ra SPACK_CPPFLAGS <<< "$SPACK_CPPFLAGS"
+IFS=' ' read -ra SPACK_CFLAGS   <<< "$SPACK_CFLAGS"
+IFS=' ' read -ra SPACK_CXXFLAGS <<< "$SPACK_CXXFLAGS"
+IFS=' ' read -ra SPACK_LDFLAGS  <<< "$SPACK_LDFLAGS"
+IFS=' ' read -ra SPACK_LDLIBS   <<< "$SPACK_LDLIBS"
+
 # test whether a path is a system directory
 function system_dir {
     path="$1"
@@ -524,7 +533,8 @@ fi
 
 # dump the full command if the caller supplies SPACK_TEST_COMMAND=dump-args
 if [[ $SPACK_TEST_COMMAND == dump-args ]]; then
-    echo "${full_command[@]}"
+    IFS="
+" && echo "${full_command[*]}"
     exit
 elif [[ -n $SPACK_TEST_COMMAND ]]; then
     die "ERROR: Unknown test command"
diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py
index 1e0ef31070..c0a4cd1eed 100644
--- a/lib/spack/spack/test/cc.py
+++ b/lib/spack/spack/test/cc.py
@@ -178,77 +178,59 @@ def dep4(tmpdir_factory):
 pytestmark = pytest.mark.usefixtures('wrapper_environment')
 
 
-def check_cc(command, args, expected):
-    with set_env(SPACK_TEST_COMMAND=command):
-        assert expected == cc(*args, output=str).strip().split()
+def check_args(cc, args, expected):
+    """Check output arguments that cc produces when called with args.
 
-
-def check_cxx(command, args, expected):
-    with set_env(SPACK_TEST_COMMAND=command):
-        assert expected == cxx(*args, output=str).strip().split()
-
-
-def check_fc(command, args, expected):
-    with set_env(SPACK_TEST_COMMAND=command):
-        assert expected == fc(*args, output=str).strip().split()
-
-
-def check_ld(command, args, expected):
-    with set_env(SPACK_TEST_COMMAND=command):
-        assert expected == ld(*args, output=str).strip().split()
+    This assumes that cc will print debug command output with one element
+    per line, so that we see whether arguments that should (or shouldn't)
+    contain spaces are parsed correctly.
+    """
+    with set_env(SPACK_TEST_COMMAND='dump-args'):
+        assert expected == cc(*args, output=str).strip().split('\n')
 
 
-def check_cpp(command, args, expected):
-    with set_env(SPACK_TEST_COMMAND=command):
-        assert expected == cpp(*args, output=str).strip().split()
+def dump_mode(cc, args):
+    """Make cc dump the mode it detects, and return it."""
+    with set_env(SPACK_TEST_COMMAND='dump-mode'):
+        return cc(*args, output=str).strip()
 
 
 def test_vcheck_mode():
-    check_cc(
-        'dump-mode', ['-I/include', '--version'], ['vcheck'])
-    check_cc(
-        'dump-mode', ['-I/include', '-V'], ['vcheck'])
-    check_cc(
-        'dump-mode', ['-I/include', '-v'], ['vcheck'])
-    check_cc(
-        'dump-mode', ['-I/include', '-dumpversion'], ['vcheck'])
-    check_cc(
-        'dump-mode', ['-I/include', '--version', '-c'], ['vcheck'])
-    check_cc(
-        'dump-mode', ['-I/include', '-V', '-o', 'output'], ['vcheck'])
+    assert dump_mode(cc, ['-I/include', '--version']) == 'vcheck'
+    assert dump_mode(cc, ['-I/include', '-V']) == 'vcheck'
+    assert dump_mode(cc, ['-I/include', '-v']) == 'vcheck'
+    assert dump_mode(cc, ['-I/include', '-dumpversion']) == 'vcheck'
+    assert dump_mode(cc, ['-I/include', '--version', '-c']) == 'vcheck'
+    assert dump_mode(cc, ['-I/include', '-V', '-o', 'output']) == 'vcheck'
 
 
 def test_cpp_mode():
-    check_cc('dump-mode', ['-E'], ['cpp'])
-    check_cpp('dump-mode', [], ['cpp'])
+    assert dump_mode(cc, ['-E']) == 'cpp'
+    assert dump_mode(cxx, ['-E']) == 'cpp'
+    assert dump_mode(cpp, []) == 'cpp'
 
 
 def test_as_mode():
-    check_cc('dump-mode', ['-S'], ['as'])
+    assert dump_mode(cc, ['-S']) == 'as'
 
 
 def test_ccld_mode():
-    check_cc(
-        'dump-mode', [], ['ccld'])
-    check_cc(
-        'dump-mode', ['foo.c', '-o', 'foo'], ['ccld'])
-    check_cc(
-        'dump-mode', ['foo.c', '-o', 'foo', '-Wl,-rpath,foo'], ['ccld'])
-    check_cc(
-        'dump-mode',
-        ['foo.o', 'bar.o', 'baz.o', '-o', 'foo', '-Wl,-rpath,foo'], ['ccld'])
+    assert dump_mode(cc, []) == 'ccld'
+    assert dump_mode(cc, ['foo.c', '-o', 'foo']) == 'ccld'
+    assert dump_mode(cc, ['foo.c', '-o', 'foo', '-Wl,-rpath,foo']) == 'ccld'
+    assert dump_mode(cc, [
+        'foo.o', 'bar.o', 'baz.o', '-o', 'foo', '-Wl,-rpath,foo']) == 'ccld'
 
 
 def test_ld_mode():
-    check_ld('dump-mode', [], ['ld'])
-    check_ld(
-        'dump-mode',
-        ['foo.o', 'bar.o', 'baz.o', '-o', 'foo', '-Wl,-rpath,foo'], ['ld'])
+    assert dump_mode(ld, []) == 'ld'
+    assert dump_mode(ld, [
+        'foo.o', 'bar.o', 'baz.o', '-o', 'foo', '-Wl,-rpath,foo']) == 'ld'
 
 
 def test_ld_flags(wrapper_flags):
-    check_ld(
-        'dump-args', test_args,
+    check_args(
+        ld, test_args,
         ['ld'] +
         spack_ldflags +
         test_include_paths +
@@ -260,8 +242,8 @@ def test_ld_flags(wrapper_flags):
 
 
 def test_cpp_flags(wrapper_flags):
-    check_cpp(
-        'dump-args', test_args,
+    check_args(
+        cpp, test_args,
         ['cpp'] +
         spack_cppflags +
         test_include_paths +
@@ -270,8 +252,8 @@ def test_cpp_flags(wrapper_flags):
 
 
 def test_cc_flags(wrapper_flags):
-    check_cc(
-        'dump-args', test_args,
+    check_args(
+        cc, test_args,
         [real_cc] +
         spack_cppflags +
         spack_cflags +
@@ -285,8 +267,8 @@ def test_cc_flags(wrapper_flags):
 
 
 def test_cxx_flags(wrapper_flags):
-    check_cxx(
-        'dump-args', test_args,
+    check_args(
+        cxx, test_args,
         [real_cc] +
         spack_cppflags +
         spack_cxxflags +
@@ -300,8 +282,8 @@ def test_cxx_flags(wrapper_flags):
 
 
 def test_fc_flags(wrapper_flags):
-    check_fc(
-        'dump-args', test_args,
+    check_args(
+        fc, test_args,
         [real_cc] +
         spack_fflags +
         spack_cppflags +
@@ -316,8 +298,8 @@ def test_fc_flags(wrapper_flags):
 
 def test_dep_rpath():
     """Ensure RPATHs for root package are added."""
-    check_cc(
-        'dump-args', test_args,
+    check_args(
+        cc, test_args,
         [real_cc] +
         test_include_paths +
         test_library_paths +
@@ -331,8 +313,8 @@ def test_dep_include(dep4):
     with set_env(SPACK_DEPENDENCIES=dep4,
                  SPACK_RPATH_DEPS=dep4,
                  SPACK_LINK_DEPS=dep4):
-        check_cc(
-            'dump-args', test_args,
+        check_args(
+            cc, test_args,
             [real_cc] +
             test_include_paths +
             ['-I' + dep4 + '/include'] +
@@ -347,8 +329,8 @@ def test_dep_lib(dep2):
     with set_env(SPACK_DEPENDENCIES=dep2,
                  SPACK_RPATH_DEPS=dep2,
                  SPACK_LINK_DEPS=dep2):
-        check_cc(
-            'dump-args', test_args,
+        check_args(
+            cc, test_args,
             [real_cc] +
             test_include_paths +
             test_library_paths +
@@ -363,8 +345,8 @@ def test_dep_lib_no_rpath(dep2):
     """Ensure a single dependency link flag is added with no dep RPATH."""
     with set_env(SPACK_DEPENDENCIES=dep2,
                  SPACK_LINK_DEPS=dep2):
-        check_cc(
-            'dump-args', test_args,
+        check_args(
+            cc, test_args,
             [real_cc] +
             test_include_paths +
             test_library_paths +
@@ -378,8 +360,8 @@ def test_dep_lib_no_lib(dep2):
     """Ensure a single dependency RPATH is added with no -L."""
     with set_env(SPACK_DEPENDENCIES=dep2,
                  SPACK_RPATH_DEPS=dep2):
-        check_cc(
-            'dump-args', test_args,
+        check_args(
+            cc, test_args,
             [real_cc] +
             test_include_paths +
             test_library_paths +
@@ -395,8 +377,8 @@ def test_ccld_deps(dep1, dep2, dep3, dep4):
     with set_env(SPACK_DEPENDENCIES=deps,
                  SPACK_RPATH_DEPS=deps,
                  SPACK_LINK_DEPS=deps):
-        check_cc(
-            'dump-args', test_args,
+        check_args(
+            cc, test_args,
             [real_cc] +
             test_include_paths +
             ['-I' + dep1 + '/include',
@@ -420,8 +402,8 @@ def test_cc_deps(dep1, dep2, dep3, dep4):
     with set_env(SPACK_DEPENDENCIES=deps,
                  SPACK_RPATH_DEPS=deps,
                  SPACK_LINK_DEPS=deps):
-        check_cc(
-            'dump-args', ['-c'] + test_args,
+        check_args(
+            cc, ['-c'] + test_args,
             [real_cc] +
             test_include_paths +
             ['-I' + dep1 + '/include',
@@ -444,8 +426,8 @@ def test_ccld_with_system_dirs(dep1, dep2, dep3, dep4):
                          '-Wl,-rpath,/usr/lib64',
                          '-I/usr/local/include',
                          '-L/lib64/']
-        check_cc(
-            'dump-args', sys_path_args + test_args,
+        check_args(
+            cc, sys_path_args + test_args,
             [real_cc] +
             test_include_paths +
             ['-I' + dep1 + '/include',
@@ -474,8 +456,8 @@ def test_ld_deps(dep1, dep2, dep3, dep4):
     with set_env(SPACK_DEPENDENCIES=deps,
                  SPACK_RPATH_DEPS=deps,
                  SPACK_LINK_DEPS=deps):
-        check_ld(
-            'dump-args', test_args,
+        check_args(
+            ld, test_args,
             ['ld'] +
             test_include_paths +
             test_library_paths +
@@ -495,8 +477,8 @@ def test_ld_deps_no_rpath(dep1, dep2, dep3, dep4):
     deps = ':'.join((dep1, dep2, dep3, dep4))
     with set_env(SPACK_DEPENDENCIES=deps,
                  SPACK_LINK_DEPS=deps):
-        check_ld(
-            'dump-args', test_args,
+        check_args(
+            ld, test_args,
             ['ld'] +
             test_include_paths +
             test_library_paths +
@@ -513,8 +495,8 @@ def test_ld_deps_no_link(dep1, dep2, dep3, dep4):
     deps = ':'.join((dep1, dep2, dep3, dep4))
     with set_env(SPACK_DEPENDENCIES=deps,
                  SPACK_RPATH_DEPS=deps):
-        check_ld(
-            'dump-args', test_args,
+        check_args(
+            ld, test_args,
             ['ld'] +
             test_include_paths +
             test_library_paths +
@@ -536,8 +518,8 @@ def test_ld_deps_partial(dep1):
         # TODO: do we need to add RPATHs on other platforms like Linux?
         # TODO: Can't we treat them the same?
         os.environ['SPACK_SHORT_SPEC'] = "foo@1.2=linux-x86_64"
-        check_ld(
-            'dump-args', ['-r'] + test_args,
+        check_args(
+            ld, ['-r'] + test_args,
             ['ld'] +
             test_include_paths +
             test_library_paths +
@@ -551,8 +533,8 @@ def test_ld_deps_partial(dep1):
         # rpaths from the underlying command will still appear
         # Spack will not add its own rpaths.
         os.environ['SPACK_SHORT_SPEC'] = "foo@1.2=darwin-x86_64"
-        check_ld(
-            'dump-args', ['-r'] + test_args,
+        check_args(
+            ld, ['-r'] + test_args,
             ['ld'] +
             test_include_paths +
             test_library_paths +
@@ -564,8 +546,8 @@ def test_ld_deps_partial(dep1):
 
 def test_ccache_prepend_for_cc():
     with set_env(SPACK_CCACHE_BINARY='ccache'):
-        check_cc(
-            'dump-args', test_args,
+        check_args(
+            cc, test_args,
             ['ccache'] +  # ccache prepended in cc mode
             [real_cc] +
             test_include_paths +
@@ -576,8 +558,8 @@ def test_ccache_prepend_for_cc():
 
 
 def test_no_ccache_prepend_for_fc():
-    check_fc(
-        'dump-args', test_args,
+    check_args(
+        fc, test_args,
         # no ccache for Fortran
         [real_cc] +
         test_include_paths +