[Development] Fwd: Change in qt/qtbase[dev]: Enable -Werror for all of qtbase

[Olivier Goffart]

On Wednesday 04 September 2013 22:13:34 André Pönitz wrote:
> On Wed, Sep 04, 2013 at 04:37:54PM +0200, John Layt wrote:
> > -developer-buildOn 4 September 2013 15:21, Poenitz Andre
> > 
> > <Andre.Poenitz at digia.com> wrote:
> > > That's wasting our time, as predicted.
> > > 
> > > ../../corelib/tools/qdatetime.cpp: In function 'time_t
> > > qt_mktime(QDate*, QTime*, QDateTimePrivate::Spec*, QString*, bool*)':
> > > ../../corelib/tools/qdatetime.cpp:258:34: error: comparison between
> > > signed and unsigned integer expressions [-Werror=sign-compare] cc1plus:
> > > all warnings being treated as errors
> > > 
> > > Together with the recent insistence that it's fine to submit
> > > non-compilable chances I start wondering about the direction.
> > > 
> > > Andre'
> > 
> > Apologies, that's a change I recently merged that has been on Gerrit
> > since before Thiago's change, so snuck past.  There's a patch for it
> > already at https://codereview.qt-project.org/#change,64642 .  I suspect
> > there will be a few pain points like that until the Gerrit backlog
> > drains.
> > 
> > The real problem is not Thiago's change per se, but instead CI allowing
> > changes with warnings like mine to be merged.  Perhaps we need to revert
> > until CI has the required -developer-build instances?
> 
> Not at all.
> 
> The core of the problem _is_ enabling -Werror, _not_ the CI system.
> The CI system is absolutely innocent here, and there's no reasonable
> way to "solve" the problem on the CI side. There will always be
> configurations that are not covered by CI.
> 
> In fact, I am appalled by this yet-another-attempt-to-blame-CI-for-bad-
> alignement-of-dev-with-reality. Yes, would be nice if we lived in a perfect
> world and sources were free of warnings. In practice you do look left and
> right before crossing the street. Why? Because the world is not perfect,
> and it prevents certain problems by simply acknowledging that it isn't.
> 
> -Werror is a fine thing to switch on every now and then _temporarily_ and
> _locally_, especially when it can be done as part of scheduled "general
> cleanup and housekeeping activities". No doubt. I use like that myself.
> 
> It has absolutely no place in anything that shows up as "normal sources"
> that are used to "get things done", read: _Anywhere in the Repo_.
> 
> Any such use predictably fails, has done so in numerous projects in the
> past, and did "surprisingly" so for us today (twice). No matter how much
> diligence is put into "whitelisting". And please don't get me started about
> the usefulness of version string checks when it comes to identifying
> "supported features".


II agree with André here.
You will also have to whitelist the version of all the libraries, else a new 
version of libpng/openssl/xcb/whatever add new unavoidable warning or 
deprecate function used in Qt.  And a developer working on other part of the 
library do not want to spend time fixing warning in a unrelated location.

All that results is development time lost.
Not to mention the race everyone that someone introduce a warning not catch by 
the CI, there will be five times the same patch on gerrit to fix it because all 
the dev run into it in the same time, and duplicate the effort to fix it.

Enabling it only on the CI on the other hand has the advantage to limit the 
warnings in the real code base by preventing patch that introduce some to get 
any, without bothering everyone with compilation errors unrelated to their 
code on their machine.

-- 
Olivier

Woboq - Qt services and support - http://woboq.com - http://code.woboq.org