Control Werror by converting to Wno-error (#30882)

Using `-Werror` is good practice for development and testing, but causes us a great
deal of heartburn supporting multiple compiler versions, especially as newer compiler
versions add warnings for released packages.  This PR adds support for suppressing
`-Werror` through spack's compiler wrappers.  There are currently three modes for
the `flags:keep_werror` setting:

* `none`: (default) cancel all `-Werror`, `-Werror=*` and `-Werror-*` flags by
  converting them to `-Wno-error[=]*` flags
* `specific`: preserve explicitly selected warnings as errors, such as
  `-Werror=format-truncation`, but reverse the blanket `-Werror`
* `all`: keeps all `-Werror` flags

These can be set globally in config.yaml, through the config command-line flags, or
overridden by a particular package (some packages use Werror as a proxy for determining
support for other compiler features).  We chose to use this approach because:

1. removing `-Werror` flags entirely broke *many* build systems, especially autoconf
   based ones, because of things like checking `-Werror=feature` and making the
   assumption that if that did not error other flags related to that feature would also work
2. Attempting to preserve `-Werror` in some phases but not others caused similar issues
3. The per-package setting came about because some packages, even with all these
   protections, still use `-Werror` unsafely.  Currently there are roughly 3 such packages
   known.  

8 files changed, 184 insertions, 1 deletions

diff --git a/etc/spack/defaults/config.yaml b/etc/spack/defaults/config.yaml
index b3356428fe..0bf52a0e55 100644
--- a/etc/spack/defaults/config.yaml
+++ b/etc/spack/defaults/config.yaml
@@ -214,4 +214,9 @@ config:
 
   # Number of seconds a buildcache's index.json is cached locally before probing
   # for updates, within a single Spack invocation. Defaults to 10 minutes.
-  binary_index_ttl: 600
\ No newline at end of file
+  binary_index_ttl: 600
+
+  flags:
+    # Whether to keep -Werror flags active in package builds.
+    keep_werror: 'none'
+
diff --git a/lib/spack/env/cc b/lib/spack/env/cc
index ffdddfc0df..f54cdb36ff 100755
--- a/lib/spack/env/cc
+++ b/lib/spack/env/cc
@@ -440,6 +440,47 @@ 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
+    # the replace list is a space-separated list of pipe-separated pairs,
+    # the first in each pair is the original prefix to be matched, the
+    # second is the replacement prefix
+    if [ -n "${SPACK_COMPILER_FLAGS_REPLACE}" ] ; then
+        for rep in ${SPACK_COMPILER_FLAGS_REPLACE} ; do
+            before=${rep%|*}
+            after=${rep#*|}
+            eval "\
+            stripped=\"\${1##$before}\"
+            "
+            if [ "$stripped" = "$1" ] ; then
+                continue
+            fi
+
+            replaced="$after$stripped" 
+
+            # it matched, remove it
+            shift
+
+            if [ -z "$replaced" ] ; then
+                # completely removed, continue OUTER loop
+                continue 2
+            fi
+
+            # re-build argument list with replacement
+            set -- "$replaced" "$@"
+        done
+    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 262c2683b5..f5f1f1ccd4 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -41,6 +41,7 @@ import shutil
 import sys
 import traceback
 import types
+from typing import List, Tuple
 
 import llnl.util.tty as tty
 from llnl.util.filesystem import install, install_tree, mkdirp
@@ -284,6 +285,23 @@ def clean_environment():
     return env
 
 
+def _add_werror_handling(keep_werror, env):
+    keep_flags = set()
+    # set of pairs
+    replace_flags = []  # type: List[Tuple[str,str]]
+    if keep_werror == "all":
+        keep_flags.add("-Werror*")
+    else:
+        if keep_werror == "specific":
+            keep_flags.add("-Werror-*")
+            keep_flags.add("-Werror=*")
+        # This extra case is to handle -Werror-implicit-function-declaration
+        replace_flags.append(("-Werror-", "-Wno-error="))
+        replace_flags.append(("-Werror", "-Wno-error"))
+    env.set("SPACK_COMPILER_FLAGS_KEEP", "|".join(keep_flags))
+    env.set("SPACK_COMPILER_FLAGS_REPLACE", " ".join(["|".join(item) for item in replace_flags]))
+
+
 def set_compiler_environment_variables(pkg, env):
     assert pkg.spec.concrete
     compiler = pkg.compiler
@@ -330,6 +348,13 @@ def set_compiler_environment_variables(pkg, env):
         env.set("SPACK_DTAGS_TO_STRIP", compiler.disable_new_dtags)
         env.set("SPACK_DTAGS_TO_ADD", compiler.enable_new_dtags)
 
+    if pkg.keep_werror is not None:
+        keep_werror = pkg.keep_werror
+    else:
+        keep_werror = spack.config.get("config:flags:keep_werror")
+
+    _add_werror_handling(keep_werror, env)
+
     # Set the target parameters that the compiler will add
     # Don't set on cray platform because the targeting module handles this
     if spec.satisfies("platform=cray"):
diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py
index 736e95d63e..a4a5b321c9 100644
--- a/lib/spack/spack/package_base.py
+++ b/lib/spack/spack/package_base.py
@@ -546,6 +546,10 @@ class PackageBase(WindowsRPath, PackageViewMixin, metaclass=PackageMeta):
     #: By default do not setup mockup XCode on macOS with Clang
     use_xcode = False
 
+    #: Keep -Werror flags, matches config:flags:keep_werror to override config
+    # NOTE: should be type Optional[Literal['all', 'specific', 'none']] in 3.8+
+    keep_werror = None  # type: Optional[str]
+
     #: Most packages are NOT extendable. Set to True if you want extensions.
     extendable = False
 
diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py
index 240f99e8b0..0c0b3228cf 100644
--- a/lib/spack/spack/schema/config.py
+++ b/lib/spack/spack/schema/config.py
@@ -18,6 +18,12 @@ properties = {
         "type": "object",
         "default": {},
         "properties": {
+            "flags": {
+                "type": "object",
+                "properties": {
+                    "keep_werror": {"type": "string", "enum": ["all", "specific", "none"]},
+                },
+            },
             "shared_linking": {
                 "anyOf": [
                     {"type": "string", "enum": ["rpath", "runpath"]},
diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py
index 2dcdc533c4..68d8c8abaf 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
@@ -50,6 +53,7 @@ test_args = [
     "-llib4",
     "arg5",
     "arg6",
+    "-DGCC_ARG_WITH_PERENS=(A B C)",
     '"-DDOUBLE_QUOTED_ARG"',
     "'-DSINGLE_QUOTED_ARG'",
 ]
@@ -101,6 +105,7 @@ test_args_without_paths = [
     "-llib4",
     "arg5",
     "arg6",
+    "-DGCC_ARG_WITH_PERENS=(A B C)",
     '"-DDOUBLE_QUOTED_ARG"',
     "'-DSINGLE_QUOTED_ARG'",
 ]
@@ -167,6 +172,8 @@ def wrapper_environment():
         SPACK_LINKER_ARG="-Wl,",
         SPACK_DTAGS_TO_ADD="--disable-new-dtags",
         SPACK_DTAGS_TO_STRIP="--enable-new-dtags",
+        SPACK_COMPILER_FLAGS_KEEP="",
+        SPACK_COMPILER_FLAGS_REPLACE="-Werror*",
     ):
         yield
 
@@ -196,6 +203,22 @@ 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")
+        print(cc_modified_args)
+        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
 
@@ -690,6 +713,83 @@ def test_no_ccache_prepend_for_fc(wrapper_environment):
     )
 
 
+def test_keep_and_replace(wrapper_environment):
+    werror_specific = ["-Werror=meh"]
+    werror = ["-Werror"]
+    werror_all = werror_specific + werror
+    with set_env(
+        SPACK_COMPILER_FLAGS_KEEP="",
+        SPACK_COMPILER_FLAGS_REPLACE="-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_REPLACE="-Werror*|",
+    ):
+        check_args_contents(cc, test_args + werror_all, werror_specific, werror)
+    with set_env(
+        SPACK_COMPILER_FLAGS_KEEP="-Werror=*",
+        SPACK_COMPILER_FLAGS_REPLACE="-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", "-Wno-error", "-Wno-error=specific"],
+            ["-Werror", "-Werror=specific"],
+        ),
+        # check non-standard -Werror opts like -Werror-implicit-function-declaration
+        (
+            "config:flags:keep_werror:all",
+            ["-Werror", "-Werror-implicit-function-declaration", "-bah"],
+            ["-Werror", "-Werror-implicit-function-declaration", "-bah"],
+            [],
+        ),
+        (
+            "config:flags:keep_werror:specific",
+            ["-Werror", "-Werror-implicit-function-declaration", "-bah"],
+            ["-Werror-implicit-function-declaration", "-bah", "-Wno-error"],
+            ["-Werror"],
+        ),
+        (
+            "config:flags:keep_werror:none",
+            ["-Werror", "-Werror-implicit-function-declaration", "-bah"],
+            ["-bah", "-Wno-error=implicit-function-declaration"],
+            ["-Werror", "-Werror-implicit-function-declaration"],
+        ),
+    ],
+)
+@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()
+
+    keep_werror = spack.config.get("config:flags:keep_werror")
+    spack.build_environment._add_werror_handling(keep_werror, env)
+    env.apply_modifications()
+    check_args_contents(cc, test_args[:3] + 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"):
diff --git a/var/spack/repos/builtin/packages/gcc/package.py b/var/spack/repos/builtin/packages/gcc/package.py
index d802a47d48..ee7b415cf0 100644
--- a/var/spack/repos/builtin/packages/gcc/package.py
+++ b/var/spack/repos/builtin/packages/gcc/package.py
@@ -29,6 +29,7 @@ class Gcc(AutotoolsPackage, GNUMirrorPackage):
     git = "git://gcc.gnu.org/git/gcc.git"
     list_url = "https://ftp.gnu.org/gnu/gcc/"
     list_depth = 1
+    keep_werror = "all"
 
     maintainers = ["michaelkuhn", "alalazo"]
 
diff --git a/var/spack/repos/builtin/packages/rdma-core/package.py b/var/spack/repos/builtin/packages/rdma-core/package.py
index 017702f294..9d90b9a3bb 100644
--- a/var/spack/repos/builtin/packages/rdma-core/package.py
+++ b/var/spack/repos/builtin/packages/rdma-core/package.py
@@ -14,6 +14,7 @@ class RdmaCore(CMakePackage):
     homepage = "https://github.com/linux-rdma/rdma-core"
     url = "https://github.com/linux-rdma/rdma-core/releases/download/v17.1/rdma-core-17.1.tar.gz"
     libraries = ["librdmacm.so"]
+    keep_werror = "all"
 
     version("41.0", sha256="e0b7deb8a71f229796a0cfe0fa25192c530cd3d86b755b6b28d1a5986a77507b")
     version("40.0", sha256="8844edb71311e3212e55e28fa4bdc6e06dd6c7b839ed56ee4b606e4220d94ee8")