1436336 - --enable-warnings-as-errors should have the same default for developer builds as for automation

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Mark Banner (:standard8)]

I just pushed a patch to autoland that didn't build when it hit the builders. It had built fine locally.

I did some minor changes to bug 1423896 to complete it, which built & tested fine locally, but then it got backed out due to:

/builds/worker/workspace/build/src/toolkit/components/places/nsNavHistoryResult.cpp:3657:27: error: unused variable 'node' [-Werror=unused-variable]

If the builders are failing due to this, then my local builds should be failing in the same way.

I am using a standard type of mozconfig file on OSX:

-----
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/objdir-ff-debug
mk_add_options AUTOCLOBBER=1

ac_add_options --enable-application=browser
ac_add_options --enable-debug
ac_add_options --enable-optimize

ac_add_options --with-ccache=sccache

export CC=clang
export CXX=clang++
-----

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Hard to fix ... between gcc & clang and their different versions, they all have a different set of warnings available & enabled...

Comment 2

[Nathan Froyd [:froydnj]]

The real fix here is to have everybody using the exact compiler that's being used in automation.  That would fix things for single-platform developers.  But the original bug got backed out because of problems on Linux and the patch compiled fine on OS X.  That's much harder to avoid.

Maybe in a future world where we use clang on every platform this becomes tractable.  But right now I think this is wontfix.

Comment 3

[Mark Banner (:standard8)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> The real fix here is to have everybody using the exact compiler that's being
> used in automation.  That would fix things for single-platform developers. 
> But the original bug got backed out because of problems on Linux and the
> patch compiled fine on OS X.  That's much harder to avoid.

By the original bug, do you mean bug 1423896? That got failed on treeherder on Mac & Linux.

I've just turned on --enable-warnings-as-errors locally, and now my build fails as well.

> Maybe in a future world where we use clang on every platform this becomes
> tractable.  But right now I think this is wontfix.

Is it possible we could turn it on by default for those using the same type of compiler as automation? or is version really important here?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Mark Banner (:standard8) from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > The real fix here is to have everybody using the exact compiler that's being
> > used in automation.  That would fix things for single-platform developers. 
> > But the original bug got backed out because of problems on Linux and the
> > patch compiled fine on OS X.  That's much harder to avoid.
> 
> By the original bug, do you mean bug 1423896? That got failed on treeherder
> on Mac & Linux.

Yes.  Specifically, I was looking at bug 1423896 comment 81 for the backout, which references this push:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4012bc74e9001ece2b50f5d02a9c60106c0aad55

Ah, I see Mac builds were failing on that too; I didn't notice that before.  My mistake!

> > Maybe in a future world where we use clang on every platform this becomes
> > tractable.  But right now I think this is wontfix.
> 
> Is it possible we could turn it on by default for those using the same type
> of compiler as automation? or is version really important here?

The version matters very much; newer versions of gcc/clang than what we're using in automation, especially, tend to turn on new warnings.  (People are generally good about submitting patches for such issues, so they tend to get fixed quickly.)  But older versions than what we use in automation might warn under different circumstances, too.

You are welcome to turn on --enable-warnings-as-errors, of course.  But you might spend some of your development time fixing your build because somebody committed code that warned for your compiler and not for automation.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

> The version matters very much; newer versions of gcc/clang than what we're using in automation, especially, tend to turn on new warnings.

FYI, I have automation to test with --enable-warnings-as-errors using clang trunk (https://apt.llvm.org/) and gcc-8 Debian packages (which are more or less trunk).
I have been reporting them in bug 1427842 & bug 1409283.

Comment 6

[Gregory Szorc [:gps]]

If we are using the toolchains that are used by automation, we should enable warnings by default (and other settings) to match automation's config.

What we don't yet do very well - and a vision we're working towards - is making it easier for local builds to use the toolchains used by automation. I'd like to see an e.g. --use-managed-toolchains configure flag (or similar) that has configure install and use toolchains produced by CI. If you update your revision, the toolchain in use follows what was pinned to that revision. I would eventually like to see this mode the default. Package builds and others who wish to test non-official toolchains can opt into doing so. But the default would be to approximate CI as closely as possible.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Don't we basically use explicit toolchains now?

Comment 8

[Mike Hommey [:glandium]]

There are a ton of complications from enabling warnings by default even with the bootstrapped toolchains. Because we have tons of configure options that people do use and that cause warnings. So it wouldn't be enough to enable warnings as errors automatically, they would have to only be enabled in very specific cases. Which would then be a surprise for people expecting them but not getting them because of their build options.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Ok, though I fear that this does cause us to burn Try resources and backouts.