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.
2022-11-23 12:29:17 -08:00
parent bf1b846f26
commit 0182603609
8 changed files with 184 additions and 1 deletions
--- a/etc/spack/defaults/config.yaml
+++ b/etc/spack/defaults/config.yaml
@@ -215,3 +215,8 @@ 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
+
+  flags:
+    # Whether to keep -Werror flags active in package builds.
+    keep_werror: 'none'
+
--- 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}"
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -41,6 +41,7 @@
 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"):
--- 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

--- a/lib/spack/spack/schema/config.py
+++ b/lib/spack/spack/schema/config.py
@@ -18,6 +18,12 @@
        "type": "object",
        "default": {},
        "properties": {
+            "flags": {
+                "type": "object",
+                "properties": {
+                    "keep_werror": {"type": "string", "enum": ["all", "specific", "none"]},
+                },
+            },
            "shared_linking": {
                "anyOf": [
                    {"type": "string", "enum": ["rpath", "runpath"]},
--- a/lib/spack/spack/test/cc.py
+++ b/lib/spack/spack/test/cc.py
@@ -12,6 +12,9 @@

 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 @@
    "-llib4",
    "arg5",
    "arg6",
+    "-DGCC_ARG_WITH_PERENS=(A B C)",
    '"-DDOUBLE_QUOTED_ARG"',
    "'-DSINGLE_QUOTED_ARG'",
 ]
@@ -101,6 +105,7 @@
    "-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"):
--- 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"]

--- 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")