zhangyiss/spack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

bugfix: cc handles spaces in flag variables properly

Browse Source 
- 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.
This commit is contained in:
Todd Gamblin 2018-08-08 22:52:19 -07:00
parent 393d3c64fc
commit ed79d6a11b
2 changed files with 83 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
lib/spack/env/cc vendored
View File

@ -74,9 +74,18 @@ function die {
exit 1 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}" 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 # test whether a path is a system directory
function system_dir { function system_dir {
path="$1" path="$1"
@ -524,7 +533,8 @@ fi
# dump the full command if the caller supplies SPACK_TEST_COMMAND=dump-args # dump the full command if the caller supplies SPACK_TEST_COMMAND=dump-args
if [[ $SPACK_TEST_COMMAND == dump-args ]]; then if [[ $SPACK_TEST_COMMAND == dump-args ]]; then
echo "${full_command[@]}" IFS="
" && echo "${full_command[*]}"
exit exit
elif [[ -n $SPACK_TEST_COMMAND ]]; then elif [[ -n $SPACK_TEST_COMMAND ]]; then
die "ERROR: Unknown test command" die "ERROR: Unknown test command"

160
lib/spack/spack/test/cc.py
View File

@ -178,77 +178,59 @@ def dep4(tmpdir_factory):
pytestmark = pytest.mark.usefixtures('wrapper_environment') pytestmark = pytest.mark.usefixtures('wrapper_environment')
def check_cc(command, args, expected): def check_args(cc, args, expected):
with set_env(SPACK_TEST_COMMAND=command): """Check output arguments that cc produces when called with args.
assert expected == cc(*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_cxx(command, args, expected): def dump_mode(cc, args):
with set_env(SPACK_TEST_COMMAND=command): """Make cc dump the mode it detects, and return it."""
assert expected == cxx(*args, output=str).strip().split() with set_env(SPACK_TEST_COMMAND='dump-mode'):
return cc(*args, output=str).strip()
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()
def check_cpp(command, args, expected):
with set_env(SPACK_TEST_COMMAND=command):
assert expected == cpp(*args, output=str).strip().split()
def test_vcheck_mode(): def test_vcheck_mode():
check_cc( assert dump_mode(cc, ['-I/include', '--version']) == 'vcheck'
'dump-mode', ['-I/include', '--version'], ['vcheck']) assert dump_mode(cc, ['-I/include', '-V']) == 'vcheck'
check_cc( assert dump_mode(cc, ['-I/include', '-v']) == 'vcheck'
'dump-mode', ['-I/include', '-V'], ['vcheck']) assert dump_mode(cc, ['-I/include', '-dumpversion']) == 'vcheck'
check_cc( assert dump_mode(cc, ['-I/include', '--version', '-c']) == 'vcheck'
'dump-mode', ['-I/include', '-v'], ['vcheck']) assert dump_mode(cc, ['-I/include', '-V', '-o', 'output']) == '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'])
def test_cpp_mode(): def test_cpp_mode():
check_cc('dump-mode', ['-E'], ['cpp']) assert dump_mode(cc, ['-E']) == 'cpp'
check_cpp('dump-mode', [], ['cpp']) assert dump_mode(cxx, ['-E']) == 'cpp'
assert dump_mode(cpp, []) == 'cpp'
def test_as_mode(): def test_as_mode():
check_cc('dump-mode', ['-S'], ['as']) assert dump_mode(cc, ['-S']) == 'as'
def test_ccld_mode(): def test_ccld_mode():
check_cc( assert dump_mode(cc, []) == 'ccld'
'dump-mode', [], ['ccld']) assert dump_mode(cc, ['foo.c', '-o', 'foo']) == 'ccld'
check_cc( assert dump_mode(cc, ['foo.c', '-o', 'foo', '-Wl,-rpath,foo']) == 'ccld'
'dump-mode', ['foo.c', '-o', 'foo'], ['ccld']) assert dump_mode(cc, [
check_cc( 'foo.o', 'bar.o', 'baz.o', '-o', 'foo', '-Wl,-rpath,foo']) == 'ccld'
'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'])
def test_ld_mode(): def test_ld_mode():
check_ld('dump-mode', [], ['ld']) assert dump_mode(ld, []) == 'ld'
check_ld( assert dump_mode(ld, [
'dump-mode', 'foo.o', 'bar.o', 'baz.o', '-o', 'foo', '-Wl,-rpath,foo']) == 'ld'
['foo.o', 'bar.o', 'baz.o', '-o', 'foo', '-Wl,-rpath,foo'], ['ld'])
def test_ld_flags(wrapper_flags): def test_ld_flags(wrapper_flags):
check_ld( check_args(
'dump-args', test_args, ld, test_args,
['ld'] + ['ld'] +
spack_ldflags + spack_ldflags +
test_include_paths + test_include_paths +
@ -260,8 +242,8 @@ def test_ld_flags(wrapper_flags):
def test_cpp_flags(wrapper_flags): def test_cpp_flags(wrapper_flags):
check_cpp( check_args(
'dump-args', test_args, cpp, test_args,
['cpp'] + ['cpp'] +
spack_cppflags + spack_cppflags +
test_include_paths + test_include_paths +
@ -270,8 +252,8 @@ def test_cpp_flags(wrapper_flags):
def test_cc_flags(wrapper_flags): def test_cc_flags(wrapper_flags):
check_cc( check_args(
'dump-args', test_args, cc, test_args,
[real_cc] + [real_cc] +
spack_cppflags + spack_cppflags +
spack_cflags + spack_cflags +
@ -285,8 +267,8 @@ def test_cc_flags(wrapper_flags):
def test_cxx_flags(wrapper_flags): def test_cxx_flags(wrapper_flags):
check_cxx( check_args(
'dump-args', test_args, cxx, test_args,
[real_cc] + [real_cc] +
spack_cppflags + spack_cppflags +
spack_cxxflags + spack_cxxflags +
@ -300,8 +282,8 @@ def test_cxx_flags(wrapper_flags):
def test_fc_flags(wrapper_flags): def test_fc_flags(wrapper_flags):
check_fc( check_args(
'dump-args', test_args, fc, test_args,
[real_cc] + [real_cc] +
spack_fflags + spack_fflags +
spack_cppflags + spack_cppflags +
@ -316,8 +298,8 @@ def test_fc_flags(wrapper_flags):
def test_dep_rpath(): def test_dep_rpath():
"""Ensure RPATHs for root package are added.""" """Ensure RPATHs for root package are added."""
check_cc( check_args(
'dump-args', test_args, cc, test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
test_library_paths + test_library_paths +
@ -331,8 +313,8 @@ def test_dep_include(dep4):
with set_env(SPACK_DEPENDENCIES=dep4, with set_env(SPACK_DEPENDENCIES=dep4,
SPACK_RPATH_DEPS=dep4, SPACK_RPATH_DEPS=dep4,
SPACK_LINK_DEPS=dep4): SPACK_LINK_DEPS=dep4):
check_cc( check_args(
'dump-args', test_args, cc, test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
['-I' + dep4 + '/include'] + ['-I' + dep4 + '/include'] +
@ -347,8 +329,8 @@ def test_dep_lib(dep2):
with set_env(SPACK_DEPENDENCIES=dep2, with set_env(SPACK_DEPENDENCIES=dep2,
SPACK_RPATH_DEPS=dep2, SPACK_RPATH_DEPS=dep2,
SPACK_LINK_DEPS=dep2): SPACK_LINK_DEPS=dep2):
check_cc( check_args(
'dump-args', test_args, cc, test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
test_library_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.""" """Ensure a single dependency link flag is added with no dep RPATH."""
with set_env(SPACK_DEPENDENCIES=dep2, with set_env(SPACK_DEPENDENCIES=dep2,
SPACK_LINK_DEPS=dep2): SPACK_LINK_DEPS=dep2):
check_cc( check_args(
'dump-args', test_args, cc, test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
test_library_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.""" """Ensure a single dependency RPATH is added with no -L."""
with set_env(SPACK_DEPENDENCIES=dep2, with set_env(SPACK_DEPENDENCIES=dep2,
SPACK_RPATH_DEPS=dep2): SPACK_RPATH_DEPS=dep2):
check_cc( check_args(
'dump-args', test_args, cc, test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
test_library_paths + test_library_paths +
@ -395,8 +377,8 @@ def test_ccld_deps(dep1, dep2, dep3, dep4):
with set_env(SPACK_DEPENDENCIES=deps, with set_env(SPACK_DEPENDENCIES=deps,
SPACK_RPATH_DEPS=deps, SPACK_RPATH_DEPS=deps,
SPACK_LINK_DEPS=deps): SPACK_LINK_DEPS=deps):
check_cc( check_args(
'dump-args', test_args, cc, test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
['-I' + dep1 + '/include', ['-I' + dep1 + '/include',
@ -420,8 +402,8 @@ def test_cc_deps(dep1, dep2, dep3, dep4):
with set_env(SPACK_DEPENDENCIES=deps, with set_env(SPACK_DEPENDENCIES=deps,
SPACK_RPATH_DEPS=deps, SPACK_RPATH_DEPS=deps,
SPACK_LINK_DEPS=deps): SPACK_LINK_DEPS=deps):
check_cc( check_args(
'dump-args', ['-c'] + test_args, cc, ['-c'] + test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
['-I' + dep1 + '/include', ['-I' + dep1 + '/include',
@ -444,8 +426,8 @@ def test_ccld_with_system_dirs(dep1, dep2, dep3, dep4):
'-Wl,-rpath,/usr/lib64', '-Wl,-rpath,/usr/lib64',
'-I/usr/local/include', '-I/usr/local/include',
'-L/lib64/'] '-L/lib64/']
check_cc( check_args(
'dump-args', sys_path_args + test_args, cc, sys_path_args + test_args,
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
['-I' + dep1 + '/include', ['-I' + dep1 + '/include',
@ -474,8 +456,8 @@ def test_ld_deps(dep1, dep2, dep3, dep4):
with set_env(SPACK_DEPENDENCIES=deps, with set_env(SPACK_DEPENDENCIES=deps,
SPACK_RPATH_DEPS=deps, SPACK_RPATH_DEPS=deps,
SPACK_LINK_DEPS=deps): SPACK_LINK_DEPS=deps):
check_ld( check_args(
'dump-args', test_args, ld, test_args,
['ld'] + ['ld'] +
test_include_paths + test_include_paths +
test_library_paths + test_library_paths +
@ -495,8 +477,8 @@ def test_ld_deps_no_rpath(dep1, dep2, dep3, dep4):
deps = ':'.join((dep1, dep2, dep3, dep4)) deps = ':'.join((dep1, dep2, dep3, dep4))
with set_env(SPACK_DEPENDENCIES=deps, with set_env(SPACK_DEPENDENCIES=deps,
SPACK_LINK_DEPS=deps): SPACK_LINK_DEPS=deps):
check_ld( check_args(
'dump-args', test_args, ld, test_args,
['ld'] + ['ld'] +
test_include_paths + test_include_paths +
test_library_paths + test_library_paths +
@ -513,8 +495,8 @@ def test_ld_deps_no_link(dep1, dep2, dep3, dep4):
deps = ':'.join((dep1, dep2, dep3, dep4)) deps = ':'.join((dep1, dep2, dep3, dep4))
with set_env(SPACK_DEPENDENCIES=deps, with set_env(SPACK_DEPENDENCIES=deps,
SPACK_RPATH_DEPS=deps): SPACK_RPATH_DEPS=deps):
check_ld( check_args(
'dump-args', test_args, ld, test_args,
['ld'] + ['ld'] +
test_include_paths + test_include_paths +
test_library_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: do we need to add RPATHs on other platforms like Linux?
# TODO: Can't we treat them the same? # TODO: Can't we treat them the same?
os.environ['SPACK_SHORT_SPEC'] = "foo@1.2=linux-x86_64" os.environ['SPACK_SHORT_SPEC'] = "foo@1.2=linux-x86_64"
check_ld( check_args(
'dump-args', ['-r'] + test_args, ld, ['-r'] + test_args,
['ld'] + ['ld'] +
test_include_paths + test_include_paths +
test_library_paths + test_library_paths +
@ -551,8 +533,8 @@ def test_ld_deps_partial(dep1):
# rpaths from the underlying command will still appear # rpaths from the underlying command will still appear
# Spack will not add its own rpaths. # Spack will not add its own rpaths.
os.environ['SPACK_SHORT_SPEC'] = "foo@1.2=darwin-x86_64" os.environ['SPACK_SHORT_SPEC'] = "foo@1.2=darwin-x86_64"
check_ld( check_args(
'dump-args', ['-r'] + test_args, ld, ['-r'] + test_args,
['ld'] + ['ld'] +
test_include_paths + test_include_paths +
test_library_paths + test_library_paths +
@ -564,8 +546,8 @@ def test_ld_deps_partial(dep1):
def test_ccache_prepend_for_cc(): def test_ccache_prepend_for_cc():
with set_env(SPACK_CCACHE_BINARY='ccache'): with set_env(SPACK_CCACHE_BINARY='ccache'):
check_cc( check_args(
'dump-args', test_args, cc, test_args,
['ccache'] + # ccache prepended in cc mode ['ccache'] + # ccache prepended in cc mode
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +
@ -576,8 +558,8 @@ def test_ccache_prepend_for_cc():
def test_no_ccache_prepend_for_fc(): def test_no_ccache_prepend_for_fc():
check_fc( check_args(
'dump-args', test_args, fc, test_args,
# no ccache for Fortran # no ccache for Fortran
[real_cc] + [real_cc] +
test_include_paths + test_include_paths +