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.ab...@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.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. > > > >> > > > > >> > > > > > > > > > >
