mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com>
Subject Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)
Date Tue, 16 Jan 2018 15:47:40 GMT
So you're proposing to have a stage AFTER test execution which would report
warnings as errors? While this is a good idea, I'm afraid that a fail-fast
would also have its benefits - especially considering that compilation only
takes a few minutes and consumes few resources while test execution takes
up most of the time and is very costly.

-Marco

On Tue, Jan 16, 2018 at 4:11 PM, Barber, Christopher <
Christopher.Barber@analog.com> wrote:

> Personally, I don’t like treating warnings as errors because it prevents
> compilation from completing and causes you to lose any ability to test the
> code and get any other information. Killing the build because of a failed
> warning for something that might not matter means that you may not find out
> about other important test failures until much later. Better to add a test
> that grovels the build logs for warning messages and treat it as a test
> failure.
>
> I also prefer to only enable exactly those warnings that truly matter.
>
> On 1/16/18, 8:23 AM, "Marco de Abreu" <marco.g.abreu@googlemail.com>
> wrote:
>
>     I'd vote for having warnings as errors only for CI but not in general
>     builds which are getting executed by users on their local machine.
> Just in
>     case CI misses a warning due to a different version, this could block a
>     developer from compiling MXNet locally even though it might just be a
>     warning which is not critical enough (otherwise it would be an error)
> to
>     justify blocking the compilation. In my opinion, it would be good if
> we can
>     filter most warnings during PR-stage and risk that some are getting
> into
>     the master branch due to a different compiler version. A reduction of
> (for
>     example) 95% without risking to break the master branch on different
>     compilers is way better in my opinion than having a 100% coverage which
>     could block compilation - especially because we would only notice if a
> user
>     tells us afterwards.
>
>     -Marco
>
>     On Tue, Jan 16, 2018 at 1:32 PM, Pedro Larroy <
> pedro.larroy.lists@gmail.com>
>     wrote:
>
>     > Hi Chris
>     >
>     > I get the rationale of the point you raise, but In my opinion, and
>     > considering the complexity of C++ and the potential for difficult and
>     > expensive to track bugs, I think this should be enabled by default
> for
>     > both release and debug. A developer is free to disable warnings in
> his
>     > own private branch, but I don't see what would be the benefit of
> this.
>     >
>     > Regarding your second point, I think this is a minor issue which is
>     > outweighed by the benefits. In the case you propose, the author of a
>     > PR can easily fix a bunch of warnings when CI fails as usual. For
>     > example in case he gets one or two warnings that his version of the
>     > compiler didn't catch, or if she has an additional warning of some
>     > type with a different version of GCC / Clang.
>     >
>     > This has the objective to prevent warning inflation. In practice, a
>     > different version of GCC might produce just a couple of new warning
>     > types that will be easily fixable once we upgrade the compiler in CI.
>     > We also get the benefit of preventing warnings on the gcc versions
>     > that the author is using, in the case he has a different one. Another
>     > option is to enable warnings as errors only on CI. I would prefer to
>     > have it enabled by default, for correctness. As first time users are
>     > not likely to compile MXNet by themselves, and also considering the
>     > significant complexity of compiling MXNet from scratch for newcomers.
>     >
>     > In general, the compilers that we have running on CI should be our
>     > reference compilers. And for practical purposes, having no warnings
> in
>     > those versions of Clang and GCC would be a positive step towards more
>     > code quality, clean compilation and a more mantainable code base.
>     > Once we have CI stable we can build a matrix of supported compilers
> in
>     > the docs, as for example there are versions of GCC which are not
>     > supported by the nvidia tools.
>     >
>     > Pedro.
>     >
>     >
>     >
>     >
>     > On Mon, Jan 15, 2018 at 7:27 PM, Chris Olivier <
> cjolivier01@gmail.com>
>     > wrote:
>     > > If enabled, it should only cause errors in Release builds, since
> having
>     > > warnings in WIP code is not unusual.
>     > >
>     > > In addition, different developers use different gcc/clang
> versions. Some
>     > > gcc versions, for instance, generate warnings where others do
> not.  It
>     > > would not be fair to render unbuildable a developer who is using a
> newer
>     > > (or older) gcc version is different from CI.  Can this argument be
> tied
>     > to
>     > > a particular compiler/platform/version?
>     > >
>     > > On Mon, Jan 15, 2018 at 9:43 AM, Marco de Abreu <
>     > > marco.g.abreu@googlemail.com> wrote:
>     > >
>     > >> +1
>     > >>
>     > >> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy <
>     > >> pedro.larroy.lists@gmail.com>
>     > >> wrote:
>     > >>
>     > >> > Hi
>     > >> >
>     > >> > I would like to propose to compile in CI with warnings as
> errors for
>     > >> > increased code quality. This has a dual purpose:
>     > >> >
>     > >> > 1. Enforce a clean compilation output. Warnings often indicate
>     > >> > deficiencies in the code and hide new warnings which can be an
>     > >> > indicator of problems.
>     > >> >
>     > >> > 2. Warnings can surface bugs as has happened before.
>     > >> >
>     > >> > While this might be impractical in all architectures, I would
> propose
>     > >> > having the Linux and Clang build run without warnings in CI.
>     > >> >
>     > >> > I think we are very close to this as I personally have been
> fixing
>     > >> > warnings in Linux and OSX / Clang.
>     > >> >
>     > >> > References:
>     > >> >
>     > >> > https://github.com/apache/incubator-mxnet/pull/9398
>     > >> >
>     > >> > http://jenkins.mxnet-ci.amazon-ml.com/blue/
> organizations/jenkins/
>     > >> > incubator-mxnet/detail/PR-9398/1/pipeline
>     > >> >
>     > >> > Pedro.
>     > >> >
>     > >>
>     >
>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message