From ed79d6a11b4c5baf811a126406bed1c7f623a26a Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Wed, 8 Aug 2018 22:52:19 -0700 Subject: 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. --- lib/spack/env/cc | 14 +++- lib/spack/spack/test/cc.py | 158 ++++++++++++++++++++------------------------- 2 files changed, 82 insertions(+), 90 deletions(-) (limited to 'lib') 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_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 + -- cgit v1.2.3-60-g2f50