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.

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'):