From 0182603609f289ec43baa95b9e98998cbf268d04 Mon Sep 17 00:00:00 2001 From: Tom Scogland Date: Wed, 23 Nov 2022 12:29:17 -0800 Subject: 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. --- etc/spack/defaults/config.yaml | 7 +- lib/spack/env/cc | 41 +++++++++ lib/spack/spack/build_environment.py | 25 ++++++ lib/spack/spack/package_base.py | 4 + lib/spack/spack/schema/config.py | 6 ++ lib/spack/spack/test/cc.py | 100 +++++++++++++++++++++ var/spack/repos/builtin/packages/gcc/package.py | 1 + .../repos/builtin/packages/rdma-core/package.py | 1 + 8 files changed, 184 insertions(+), 1 deletion(-) 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") -- cgit v1.2.3-70-g09d2