[PATCH] D123481: Do not build with Werror by default (Bazel build)

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: utils/bazel/.bazelrc:38
+# Use `-Wall` for Clang.
+build:generic_clang --copt=-Wall --host_copt=-Wall
 
----------------
GMNGeoffrey wrote:
> [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.
> 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?


================
Comment at: utils/bazel/.bazelrc:74
 
-# Use `-Werror` for GCC to make sure warnings don't slip past.
-build:generic_gcc --copt=-Werror --host_copt=-Werror
----------------
GMNGeoffrey wrote:
> Why would things be different for gcc than for clang?
I removed -Werror equally for gcc and clang, can you clarify what you mean?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123481/new/

https://reviews.llvm.org/D123481