From 36566e406e991b8258cf9a142712d3aa617455f9 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Mon, 8 Feb 2021 18:22:34 +0000
Subject: [PATCH] build-sys: Add --enable-werror, rework compiler flags

So...at some point we somehow lost `-Wall` in our
default compiler flags which means we were missing some
potentially important warnings.  And
we used to have `-Werror` on in CI which combined
with the above was strongly opinionated about not
landing warnings in git master.

Our default stance here remains the same; we have
an opinionated set of `-Werror=` that applies in
*all* configurations.   However that set moves
into Automake - I don't think we need to do
compiler version detection anymore, we can assume
a modern compiler.

We also add back in `-Wall` by default now.

Further in CI, add `-Werror`.  The implementation
here is in our buildsystem rather than
`export CXXFLAGS=-Werror` because unfortunately
we have to fix things in libdnf too, and I don't
want to block entirely on that.
---
 Makefile.am             | 28 ++++++++++++++++++----------
 ci/build-check.sh       |  5 ++++-
 ci/clang-build-check.sh |  3 +--
 configure.ac            | 26 ++++++--------------------
 4 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 904ff6e6..7626987d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -36,20 +36,28 @@ AM_CPPFLAGS += -DDATADIR='"$(datadir)"' \
 	-DRPM_OSTREE_FEATURES='"$(RPM_OSTREE_FEATURES)"' \
 	-DRPM_OSTREE_GITREV='"$(RPM_OSTREE_GITREV)"' \
 	-DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_56 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_56
+# Compiler warnings
+warnings_skipped = sign-compare
+warnings_error = empty-body format=2 format-security format-nonliteral \
+  missing-declarations return-type switch overflow undef \
+  misleading-indentation missing-include-dirs unused-result \
+  $(NULL)
+warning_flags = -Wall \
+  $(patsubst %,-Wno-%,$(warnings_skipped)) \
+  $(patsubst %,-Werror=%,$(warnings_error)) \
+  $(NULL)
+if BUILDOPT_WERROR
+warning_flags += -Werror
+endif
 # We make some C warnings always a hard error; these should never happen in our code.
 # These flags are not valid for C++.
-warn_only_c_flags = -Werror=strict-prototypes \
-  -Werror=missing-prototypes \
-  -Werror=implicit-function-declaration \
-  -Werror=int-conversion \
-  -Werror=incompatible-pointer-types \
-  -Werror=int-conversion \
+warnings_error_only_c = strict-prototypes missing-prototypes \
+  implicit-function-declaration int-conversion incompatible-pointer-types \
   $(NULL)
-# Keep this in sync with the AM_CFLAGS in libostree; see
-# that project for more information about e.g. -fno-strict-aliasing
-AM_CFLAGS += -std=gnu11 -fno-strict-aliasing $(WARN_CFLAGS) $(warn_only_c_flags)
+# See the AM_CFLAGS in libostree for more information about -fno-strict-aliasing
+AM_CFLAGS += -std=gnu11 -fno-strict-aliasing $(warning_flags) $(patsubst %,-Werror=%,$(warnings_error_only_c))
 # Our default CXX flags
-AM_CXXFLAGS += -std=c++17 -fno-strict-aliasing $(WARN_CFLAGS)
+AM_CXXFLAGS += -std=c++17 -fno-strict-aliasing $(warning_flags)
 
 EXTRA_DIST += autogen.sh COPYING
 
diff --git a/ci/build-check.sh b/ci/build-check.sh
index 288e3dc3..9f6362bc 100755
--- a/ci/build-check.sh
+++ b/ci/build-check.sh
@@ -7,5 +7,8 @@ set -xeuo pipefail
 
 dn=$(dirname $0)
 . ${dn}/libbuild.sh
-${dn}/build.sh
+# Hard fail on compiler warnings in CI.  We control our compiler
+# version as part of the coreos-assembler buildroot and expect
+# that to be clean.
+CONFIGOPTS="--enable-werror" ${dn}/build.sh
 make check
diff --git a/ci/clang-build-check.sh b/ci/clang-build-check.sh
index 5ec44821..3eb7a4b1 100755
--- a/ci/clang-build-check.sh
+++ b/ci/clang-build-check.sh
@@ -8,5 +8,4 @@ set -xeuo pipefail
 dn=$(dirname $0)
 . ${dn}/libbuild.sh
 export CC=clang CXX=clang++
-${dn}/build.sh
-make check
+${dn}/build-check.sh
diff --git a/configure.ac b/configure.ac
index c159a775..26651f60 100644
--- a/configure.ac
+++ b/configure.ac
@@ -27,26 +27,6 @@ dnl if not set, which we definitely want; cmake doesn't do that.
 AC_PROG_CXX
 AM_PROG_CC_C_O
 
-dnl Keep this in sync with the version in ostree
-AS_IF([echo "$CFLAGS" | grep -q -E -e '-Werror($| )'], [], [
-CC_CHECK_FLAGS_APPEND([WARN_CFLAGS], [CFLAGS], [\
-  -pipe \
-  -Werror=empty-body \
-  "-Werror=format=2 -Werror=format-security -Werror=format-nonliteral" \
-  -Werror=pointer-arith -Werror=init-self \
-  -Werror=missing-declarations \
-  -Werror=return-type \
-  -Werror=switch \
-  -Werror=overflow \
-  -Werror=parenthesis \
-  -Werror=undef \
-  -Werror=misleading-indentation \
-  -Werror=missing-include-dirs \
-  -Wstrict-aliasing=2 \
-  -Werror=unused-result \
-])])
-AC_SUBST(WARN_CFLAGS)
-
 AC_MSG_CHECKING([for -fsanitize=address in CFLAGS])
 if echo $CFLAGS | grep -q -e -fsanitize=address; then
 AC_MSG_RESULT([yes])
@@ -98,6 +78,12 @@ GTK_DOC_CHECK([1.15], [--flavour no-tmpl])
 AM_CONDITIONAL([ENABLE_GTK_DOC],[false])
 ])
 
+AC_ARG_ENABLE(werror,
+              AS_HELP_STRING([--enable-werror],
+                             [Enable -Werror for C/C++ (default: no)]),,
+              [enable_werror=no])
+AM_CONDITIONAL(BUILDOPT_WERROR, [test x$enable_werror != xno])
+
 AC_ARG_ENABLE(sqlite_rpmdb_default,
               AS_HELP_STRING([--enable-sqlite-rpmdb-default],
                              [Default to sqlite rpmdb backend (default: no)]),,