Do not build with Werror by default (Bazel build)

[mehdi_amini]

Summary

This seems like an anti-pattern to have -Werror on by default:
it is hostile to user as we can't ensure that all of the supported
platforms are warning-free, and any newer compiler could break the build
for a user who does not have a clear actionable way around it.

[mehdi_amini]

Descriptin

[kuhar]

+1, I had to patch this locally in my project for exactly the reasons you mentioned

[GMNGeoffrey]

In D123481#3442718, @kuhar wrote:

+1, I had to patch this locally in my project for exactly the reasons you mentioned

I don't understand what you mean about having to patch it locally in a different project. bazelrc files are only applicable to the project itself, not anything that uses it as a dependency.

utils/bazel/.bazelrc

51

[Here for threading]

I'm pretty strongly against warnings in default build configurations. In my experience, it inevitably leads to an ever-growing pile of warning spam. If we turn it off here, I'm going to turn it on explicitly on the CI. The issue with that is that then users have things that only fail on the CI (Bazel will hide warnings from cached actions, irritatingly). I take your point about users on different platforms and compiler versions. Notably they can override bazelrc options themselves and pass --copt=-Wno-error --host_copt=-Wno-error. Have you actually run into issues or is this theoretical right now? The better workflow IMO would be for someone to switch to building locally with these options and send a patch to fix things for the new compiler/platform. Opting in to ignoring warnings seems better to me, but it depends on the frequency of this issue.

91

Why would things be different for gcc than for clang?

[kuhar]

In D123481#3505002, @GMNGeoffrey wrote:

In D123481#3442718, @kuhar wrote:

+1, I had to patch this locally in my project for exactly the reasons you mentioned

I don't understand what you mean about having to patch it locally in a different project. bazelrc files are only applicable to the project itself, not anything that uses it as a dependency.

We build and test llvm before importing because we have a small number of local patches . I don't remember what our issue was exactly, but I think it was something caused by not fully hermetic builds and picking up system headers that caused deprecation warnings in code that used mallinfo or something similar.

[mehdi_amini]

utils/bazel/.bazelrc

51

I'm pretty strongly against warnings in default build configurations.

You mean you want to remove -Wall? Disable warnings entirely by default?
I don't have a definitive opinion on this actually, why would you want this?
(it seems secondary to Werror anyway)

If we turn it off here, I'm going to turn it on explicitly on the CI. The issue with that is that then users have things that only fail on the CI

I am not sure if you're still talking about warnings in general or just the Werror option?
If it Werror then again: I don't have a definitive opinion but as long as this is a CI that isn't community maintained, you do whatever you want :)

Notably they can override bazelrc options themselves and pass --copt=-Wno-error --host_copt=-Wno-error.

That does not seem like a solution to me in any way to "what we ship to users": having the build failed in a weird way (pedantically) and have the user figure out how to manage this.

Have you actually run into issues or is this theoretical right now?

This was because of a bug report from @kuhar ; in any case this isn't theoretical: this is a quite well known aspect of shipping C++. (for example: https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend/ )

The better workflow IMO would be for someone to switch to building locally with these options and send a patch to fix things for the new compiler/platform.

No it isn't a better workflow: first users are not developers, being able to clone and build by default seems like the basic to me for our users. But also there are code pattern that will trigger a warning on some version of GCC and "fixing" the code will necessarily trigger a warning on some version of clang.

More fundamentally, in the context of LLVM it's not clear to me why the Bazel build would default to this when the CMake build does not?

91

I removed -Werror equally for gcc and clang, can you clarify what you mean?

[GMNGeoffrey]

utils/bazel/.bazelrc

51

I mean that in my experience not enforcing 0-warnings means that you end up with warnings. That blog post (plus the comments) have convinced me though. I think my experience comes from a place where toolchains were more tightly controlled. It seems to me that the right thing to do is not have Werror in the default build configuration, but to enforce it in some other way. The simplest is to do so in the CI build where you do have control over the toolchain, so I'll do that for now. Need to separately enable it for pre-merge tests so give me a bit to do so

91

Uh... somehow I read the wrong side of the diff

[GMNGeoffrey]

https://github.com/google/llvm-premerge-checks/pull/400 turns this on for the pre-merge checks and I've updated the continuous build. I want to make sure the pre-merge checks are doing what we expect. Mehdi, perhaps you could push an update to this change? That should re-run the pre-merge checks and we can confirm they're using -Werror. Alternatively, any patch that touches the bazel files or is by someone in the Bazel project would do it. I just didn't want to send draft patches because people have griped about email traffic in the past.

[mehdi_amini]

rebase

[mehdi_amini]

I rebased, can you check the premerge to see if it does what you expect? (I'm not sure what you want to look at)

[GMNGeoffrey]

LGTM

[GMNGeoffrey]

I just remembered about this. I think you can land it. -Werror is turned on explicitly on the CI. @aeubanks to confirm

[aeubanks]

In D123481#3915928, @GMNGeoffrey wrote:

I just remembered about this. I think you can land it. -Werror is turned on explicitly on the CI. @aeubanks to confirm

it should be added at the bottom of this file under build:ci

utils/bazel/.bazelrc

 ###############################################################################
 
 # Set the default compiler to the `clang` binary on the `PATH`.
 build:generic_clang --repo_env=CC=clang
 
 # C++17 standard version is required.
 build:generic_clang --cxxopt=-std=c++17 --host_cxxopt=-std=c++17
 
-# Use `-Wall` and `-Werror` for Clang.
-build:generic_clang --copt=-Wall --copt=-Werror --host_copt=-Wall --host_copt=-Werror
+# Use `-Wall` for Clang.
+build:generic_clang --copt=-Wall --host_copt=-Wall
 
 # The Clang available on MacOS has a warning that isn't clean on MLIR code. The
 # warning doesn't show up with more recent Clangs, so just disable for now.
 build:generic_clang --cxxopt=-Wno-range-loop-analysis --host_cxxopt=-Wno-range-loop-analysis
 
 # Build errors are not a helpful way to enforce deprecation in-repo and it is
 # not the point of the Bazel build to catch usage of deprecated APIs.
 build:generic_clang --copt=-Wno-deprecated --host_copt=-Wno-deprecated

 # Disable GCC warnings that are noisy and/or false positives on LLVM code.
 # These need to be global as some code triggering these is in header files.
 build:generic_gcc --copt=-Wno-unused-parameter --host_copt=-Wno-unused-parameter
 build:generic_gcc --copt=-Wno-comment --host_copt=-Wno-comment
 build:generic_gcc --cxxopt=-Wno-class-memaccess --host_cxxopt=-Wno-class-memaccess
 build:generic_gcc --copt=-Wno-maybe-uninitialized --host_copt=-Wno-maybe-uninitialized
 build:generic_gcc --copt=-Wno-misleading-indentation --host_copt=-Wno-misleading-indentation
 
-# Use `-Werror` for GCC to make sure warnings don't slip past.
-build:generic_gcc --copt=-Werror --host_copt=-Werror
-
 ###############################################################################
 # Generic Windows flags common to both MSVC and Clang.
 ###############################################################################
 
 # C++17 standard version is required.
 build:windows --cxxopt=/std:c++17 --host_cxxopt=/std:c++17
 
 # Other generic dialect flags.