Use -Werror when running configure

[kinkie]

Configure currently sets -Werror in SQUID_C{,XX}FLAGS.
This makes it so that these flags are not used for configure tests,
but are used for building squid, and this can cause different
behaviours during build (e.g. with Apple's LDAP on MacOS).

This is a simple change, but the consequences can be profound
and tricky to figure out, as this may cause flips in detected
system features.

Opening as a draft to support discussion

[rousskov]

The following research may help us make an informed decision:

1. Why was SQUID_CFLAGS (and SQUID_CXXFLAGS) variables added in the first place? Is that (or similar) use case still valid? Knowing the corresponding commit SHA(s) would be a start, but I hope we can figure out the actual answer.

2. Why compiler warning management flags were originally added to SQUID_CFLAGS (or SQUID_CXXFLAGS)? Knowing the corresponding commit SHA(s) would be a start, but I hope we can figure out the actual answer.

3. Do the arguments for dropping SQUID_CXXFLAGS use for compiler warning management (i.e. -Werror and friends) apply to other SQUID_CXXFLAGS use cases? Knowing at least one use case where SQUID_CXXFLAGS are needed may help validate whether they are needed for compiler warning management.

[yadij]

Ideal World:

• Those CFLAGS etc names are reserved for users and correct configure.ac logic should not be touching them.

• The correct variable to be modifying is AM_CXXFLAGS. However, I have not yet tested to confirm that using it here will have the desired effect on the ./configure checks.

"Ouch, but Good Enough" World:

• Setting CXXFLAGS works, but
• avoid whenever possible, and
• wrap logic playing with them in SQUID_STATE_SAVE/SQUID_STATE_RESTORE

@kinkie, if you want to put some time into a useful bit of research we do need to know whether setting the AM_* variable(s) will work the same as if we had set *FLAGS. If it does, then please set that one here instead.

[rousskov]

Automake's AM_CXXFLAGS variable itself seems like a red herring to me (because we are discussing flags effects on ./configure rather than make), but if SQUID_CFLAGS were added to preserve admin-set CFLAGS, then we would know the answer to the first and possibly second question asked in this change request. I would still like to know the relevant commit SHA(s).

[kinkie]

I confirm that AM_CFLAGS is only used for building.
The automake documentation says that implicitly, as AM_CFLAGS is used in Makefile.am (not configure.ac):

If a target has a target_CFLAGS variable set in Makefile.am, then the flags used for it are $(target_CFLAGS) $(CFLAGS); otherwise it's $(AM_CFLAGS) $(CFLAGS).

Looking at the generated configure sources, we see

ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext

I think the error we are making here is to conflate automake and autoconf. We should never override CFLAGS, CXXFLAGS, LDFLAGS in Makefile.am, but that doesn't imply we should not do it in configure.ac .

The existence of AX_APPEND_FLAG in autoconf-archive, and it defaulting to current language's CFLAGS is implicit evidence (there might be more explicit evidence as well) that modifying CFLAGS in configure.ac is not a problem.

We should be careful to not ignore user-provided CFLAGS, but that's where we should stop.

Opinions?

[yadij]

The issue as I have seen it described by others is that user may run make CXXFLAGS=Y which will break anything done by us to setup CXXFLAGS in ./configure even if we handle ./configure CXXFLAGS=FOO correctly.
The best way to avoid that is to not assume nor rely on particular CXXFLAGS values inside configure.ac.

[rousskov]

The correct variable to be modifying is AM_CXXFLAGS

I think the error we are making here is to conflate automake and autoconf. ... modifying CFLAGS in configure.ac is not a problem. ... Opinions?

[Use -Werror when running configure] approach is unfeasible and it will break fundamental autoconf macros such as AC_SEARCH_LIBS.

I do not yet see where I am conflating anything. So far, the PR/discussion feels like "Let's try X (or Y) and see what would happen" experiment(s). It is easy to lose the sight of the actual problem and/or reject the right solution bits that way IMHO. In hope to refactor this discussion, I would like to ask: What exact problem should this PR fix?

Also, I think it may be useful to step back a little: Why are we using compiler warnings in the first place? The primary goal is not to check that some system/library code is well-written, of course. The primary goal is to check that Squid code is well-written. With that goal in mind:

• We do not need any warnings and, hence, must not use -Werror when compiling autoconf-generated test snippets in ./configure, including AC_SEARCH_LIBS() code snippets. That code is not our responsibility, and it is definitely not compatible with some of the compiler warnings!
• We need some warnings when compiling Squid code (including, ideally, ./configure test code snippets) that we write because (we believe that) those warnings help us maintain higher quality of our code.
• We do not need other warnings (e.g., excessively pedantic warnings or warnings not yet compatible with our existing code).

It is a lot easier to (eventually and gradually) correctly turn useful warnings and -Werror for individual ./configure snippets of our code than to enable -Werror globally and then disable it for each autoconf macro that might contain a code snippet. Thus, we do not enable -Werror globally for ./configure. If we find a test code snippet that benefits from it, and we want to do the necessary legwork, we enable it for that snippet.

---

The -Wno-deprecated-declarations warning is tricky. It is kind of about our code, but it is triggered by markings in system/library code. We do want to know when we are using deprecated interfaces, but using them is often the lesser evil. Until we have a specific use case where Squid must avoid a library or API specifically because it has been marked as deprecated, I suggest disabling that warning unconditionally, in all environments. This is the simplest solution to "default-enable" support for LDAP on MacOS (and other usable libraries in other environments!) that I can think of.

Another viable alternative is to allow admins to disable any specific warning that Squid enables by default (without fully switching to custom compiler flags). This would allow us to enable -Wno-deprecated-declarations by default and then bug admins with instructions to use something like ./configure --disable-Wno-deprecated-declarations to disable it (and only it) when that warning stands in the way. This alternative requires a lot more work, of course, but it preserves potentially useful signal about Squid code using deprecated interfaces.

[kinkie]

I ran a quick experiment, and TL;DR: this approach is unfeasible and it will break fundamental autoconf macros such as AC_SEARCH_LIBS.

Comparing the configure script output between master and this branch, here's a telling difference:

-checking for library containing log... -lm
+checking for library containing log... no

Which is due to (from config.log):

configure:52533: gcc -o conftest -Wall -g -O2 -Werror      -I/usr/include/p11-kit-1  -I/usr/include/libxml2    -g conftest.c -lm  -lnsl -ldl  >&5
conftest.c:366:6: error: conflicting types for built-in function 'log'; expected 'double(double)' [-Werror=builtin-declaration-mismatch]
  366 | char log ();
      |      ^~~
conftest.c:1:1: note: 'log' is declared in header '<math.h>'
    1 | /* confdefs.h */
cc1: all warnings being treated as errors

This also tells us that the approach in PR #1694 may be the only feasible one

[kinkie]

Following up to myself: on gcc, adding -Werror -Wno-builtin-declaration-mismatch restores ./configure's ability to detect. On clang 14, this option is not supported and, in fact, not needed, configure works with -Werror.

I am however a bit concerned by adding all this tricky and necessary magic to configure, and I'd still prefer to stick to not adding -Werror during configure

[kinkie]

Closing this PR as it has achieved its goal.
Having -Werror in CFLAGS is a bad idea and we shouldn't do it. I have drafted a post for the squid wiki, there doesn't seem to be any easily-found literature online about this and it could be helpful for others to know