mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Olivier <cjolivie...@gmail.com>
Subject Re: Proposal for treating warnings as errors in Linux & Clang builds (-Werror)
Date Tue, 16 Jan 2018 15:55:09 GMT
Pedro,

i don’t know if you’ve ever done much development or not, but during
development, it’s quite common to comment out arbitrary lines of code,
create a variable only for debug inspection, or other things that will
generate warnings, but are actually intentional.  causing a compile error i
this case would not be acceptable, in my opinion.

as for the any compiler issue, if someone is using a newer gcc or clang,
and while it only has 2 new warning, they appear in 200 places, are you
saying it’s the responsibility of this poor community developer to fix all
of those warnings? or they can open up a JIRA to your team and you will fix
them?


On Tue, Jan 16, 2018 at 7:48 AM Marco de Abreu <marco.g.abreu@googlemail.com>
wrote:

> 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