From 330832c22cfa59554f6681a570bdec24ca46e79b Mon Sep 17 00:00:00 2001 From: Tom Scogland Date: Mon, 23 May 2022 21:57:09 -0700 Subject: strip -Werror: all specific or none (#30284) Add a config option to strip `-Werror*` or `-Werror=*` from compile lines everywhere. ```yaml config: keep_werror: false ``` By default, we strip all `-Werror` arguments out of compile lines, to avoid unwanted failures when upgrading compilers. You can re-enable `-Werror` in your builds if you really want to, with either: ```yaml config: keep_werror: all ``` or to keep *just* specific `-Werror=XXX` args: ```yaml config: keep_werror: specific ``` This should make swapping in newer versions of compilers much smoother when maintainers have decided to enable `-Werror` by default. --- lib/spack/env/cc | 26 +++++++++++- lib/spack/spack/build_environment.py | 11 +++++ lib/spack/spack/config.py | 3 ++ lib/spack/spack/schema/config.py | 11 ++++- lib/spack/spack/test/cc.py | 79 +++++++++++++++++++++++++++++++++++- 5 files changed, 127 insertions(+), 3 deletions(-) diff --git a/lib/spack/env/cc b/lib/spack/env/cc index bef7209bfa..6ce60a8730 100755 --- a/lib/spack/env/cc +++ b/lib/spack/env/cc @@ -401,7 +401,8 @@ input_command="$*" # command line and recombine them with Spack arguments later. We # parse these out so that we can make sure that system paths come # last, that package arguments come first, and that Spack arguments -# are injected properly. +# are injected properly. Based on configuration, we also strip -Werror +# arguments. # # All other arguments, including -l arguments, are treated as # 'other_args' and left in their original order. This ensures that @@ -440,6 +441,29 @@ while [ $# -ne 0 ]; do continue fi + if [ -n "${SPACK_COMPILER_FLAGS_KEEP}" ] ; then + # NOTE: the eval is required to allow `|` alternatives inside the variable + eval "\ + case '$1' in + $SPACK_COMPILER_FLAGS_KEEP) + append other_args_list "$1" + shift + continue + ;; + esac + " + fi + if [ -n "${SPACK_COMPILER_FLAGS_REMOVE}" ] ; then + eval "\ + case '$1' in + $SPACK_COMPILER_FLAGS_REMOVE) + shift + continue + ;; + esac + " + fi + case "$1" in -isystem*) arg="${1#-isystem}" diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py index 6b461cf672..c21514091d 100644 --- a/lib/spack/spack/build_environment.py +++ b/lib/spack/spack/build_environment.py @@ -242,6 +242,17 @@ def clean_environment(): # show useful matches. env.set('LC_ALL', build_lang) + remove_flags = set() + keep_flags = set() + if spack.config.get('config:flags:keep_werror') == 'all': + keep_flags.add('-Werror*') + else: + if spack.config.get('config:flags:keep_werror') == 'specific': + keep_flags.add('-Werror=*') + remove_flags.add('-Werror*') + env.set('SPACK_COMPILER_FLAGS_KEEP', '|'.join(keep_flags)) + env.set('SPACK_COMPILER_FLAGS_REMOVE', '|'.join(remove_flags)) + # Remove any macports installs from the PATH. The macports ld can # cause conflicts with the built-in linker on el capitan. Solves # assembler issues, e.g.: diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index ce90ed231e..92735c4301 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -105,6 +105,9 @@ config_defaults = { 'build_stage': '$tempdir/spack-stage', 'concretizer': 'clingo', 'license_dir': spack.paths.default_license_dir, + 'flags': { + 'keep_werror': 'none', + }, } } diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py index 2de54a6179..bdaa584914 100644 --- a/lib/spack/spack/schema/config.py +++ b/lib/spack/spack/schema/config.py @@ -91,7 +91,16 @@ properties = { 'additional_external_search_paths': { 'type': 'array', 'items': {'type': 'string'} - } + }, + 'flags': { + 'type': 'object', + 'properties': { + 'keep_werror': { + 'type': 'string', + 'enum': ['all', 'specific', 'none'], + }, + }, + }, }, 'deprecatedProperties': { 'properties': ['module_roots'], diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py index 36890c2eb2..5c2ec7fad2 100644 --- a/lib/spack/spack/test/cc.py +++ b/lib/spack/spack/test/cc.py @@ -12,6 +12,9 @@ import sys import pytest +import spack.build_environment +import spack.config +import spack.spec from spack.paths import build_env_path from spack.util.environment import set_env, system_dirs from spack.util.executable import Executable, ProcessError @@ -129,7 +132,9 @@ def wrapper_environment(): SPACK_TARGET_ARGS="-march=znver2 -mtune=znver2", SPACK_LINKER_ARG='-Wl,', SPACK_DTAGS_TO_ADD='--disable-new-dtags', - SPACK_DTAGS_TO_STRIP='--enable-new-dtags'): + SPACK_DTAGS_TO_STRIP='--enable-new-dtags', + SPACK_COMPILER_FLAGS_KEEP='', + SPACK_COMPILER_FLAGS_REMOVE='-Werror*',): yield @@ -157,6 +162,21 @@ def check_args(cc, args, expected): assert expected == cc_modified_args +def check_args_contents(cc, args, must_contain, must_not_contain): + """Check output arguments that cc produces when called with args. + + 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'): + cc_modified_args = cc(*args, output=str).strip().split('\n') + for a in must_contain: + assert a in cc_modified_args + for a in must_not_contain: + assert a not in cc_modified_args + + def check_env_var(executable, var, expected): """Check environment variables updated by the passed compiler wrapper @@ -642,6 +662,63 @@ def test_no_ccache_prepend_for_fc(wrapper_environment): common_compile_args) +def test_keep_and_remove(wrapper_environment): + werror_specific = ['-Werror=meh'] + werror = ['-Werror'] + werror_all = werror_specific + werror + with set_env( + SPACK_COMPILER_FLAGS_KEEP='', + SPACK_COMPILER_FLAGS_REMOVE='-Werror*', + ): + check_args_contents(cc, test_args + werror_all, ['-Wl,--end-group'], werror_all) + with set_env( + SPACK_COMPILER_FLAGS_KEEP='-Werror=*', + SPACK_COMPILER_FLAGS_REMOVE='-Werror*', + ): + check_args_contents(cc, test_args + werror_all, werror_specific, werror) + with set_env( + SPACK_COMPILER_FLAGS_KEEP='-Werror=*', + SPACK_COMPILER_FLAGS_REMOVE='-Werror*|-llib1|-Wl*', + ): + check_args_contents( + cc, + test_args + werror_all, + werror_specific, + werror + ["-llib1", "-Wl,--rpath"] + ) + + +@pytest.mark.parametrize('cfg_override,initial,expected,must_be_gone', [ + # Set and unset variables + ('config:flags:keep_werror:all', + ['-Werror', '-Werror=specific', '-bah'], + ['-Werror', '-Werror=specific', '-bah'], + [], + ), + ('config:flags:keep_werror:specific', + ['-Werror', '-Werror=specific', '-bah'], + ['-Werror=specific', '-bah'], + ['-Werror'], + ), + ('config:flags:keep_werror:none', + ['-Werror', '-Werror=specific', '-bah'], + ['-bah'], + ['-Werror', '-Werror=specific'], + ), +]) +@pytest.mark.usefixtures('wrapper_environment', 'mutable_config') +def test_flag_modification(cfg_override, initial, expected, must_be_gone): + spack.config.add(cfg_override) + env = spack.build_environment.clean_environment() + env.apply_modifications() + check_args_contents( + cc, + test_args + initial, + expected, + must_be_gone + ) + + @pytest.mark.regression('9160') def test_disable_new_dtags(wrapper_environment, wrapper_flags): with set_env(SPACK_TEST_COMMAND='dump-args'): -- cgit v1.2.3-60-g2f50