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.bar...@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.ab...@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.li...@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 < > cjolivie...@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.ab...@googlemail.com> wrote: > > > > > >> +1 > > >> > > >> On Mon, Jan 15, 2018 at 6:27 PM, Pedro Larroy < > > >> pedro.larroy.li...@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. > > >> > > > >> > > > > >
