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

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey added inline comments.


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


================
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
----------------
mehdi_amini wrote:
> 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?
Uh... somehow I read the wrong side of the diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123481