Honor werror variable for untrusted builds.

Apply -Werror for untrusted code in the Gyp build in more cases

This change moves the setting of -Werror from build_nexe
out into untrusted.gypi.

Also, move the pnacl_compile_flag so that they follow the
default compile_flags so that they can be used to suppress
warnings that are enabled by -Wall.

BUG=nativeclient:3108
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=10297

[jvoung (off chromium), 2012-11-09 23:22:11]

lgtm

[Sam Clegg, 2012-11-10 00:18:48]

Had to move pnacl_compile_flags after regular compile_flags to that individual
errors can be overridden with -Wno-XXX. (For some errors -Wno-XXX seems to need
to follow -Wall on
the command line).

[jvoung - send to chromium..., 2012-11-10 00:53:56]

Looks okay to me, but Derek can you double check that the order of the pnacl
flags vs compile flags can be re-arranged?

[Mark Seaborn, 2012-11-10 02:24:19]

Are these flags only needed for compiling Chrome-side code and not NaCl-side
code?  If so, this is the wrong place to put them.  Please add them to the
Chrome-side Gyp files instead.

BTW, I've set BUG=https://code.google.com/p/nativeclient/issues/detail?id=3108
in the commit message for context.

http://codereview.chromium.org/11363175/diff/1002/build/untrusted.gypi
File build/untrusted.gypi (right):

http://codereview.chromium.org/11363175/diff/1002/build/untrusted.gypi#newcod...
build/untrusted.gypi:678: # Copyied from common.gypi.  These flags are needed to
compile
'Copied'

[Derek Schuff, 2012-11-12 16:48:07]

Since all the pnacl-specific flags are warning flags, it should be fine to put
them at the end, although it begs the question of whether the existing ones were
serving any purpose.

Mark is probably right that if the warnings are triggered by chrome-side code,
they should go on the chrome side. (You can at least take a look if there's a
good place to put them; if not then they can probably go here anyway).

On 2012/11/10 02:24:19, Mark Seaborn wrote:
> Are these flags only needed for compiling Chrome-side code and not NaCl-side
> code?  If so, this is the wrong place to put them.  Please add them to the
> Chrome-side Gyp files instead.
> 
> BTW, I've set BUG=https://code.google.com/p/nativeclient/issues/detail?id=3108
> in the commit message for context.
> 
> http://codereview.chromium.org/11363175/diff/1002/build/untrusted.gypi
> File build/untrusted.gypi (right):
> 
>
http://codereview.chromium.org/11363175/diff/1002/build/untrusted.gypi#newcod...
> build/untrusted.gypi:678: # Copyied from common.gypi.  These flags are needed
to
> compile
> 'Copied'

[Sam Clegg, 2012-11-12 17:49:45]

On 2012/11/12 16:48:07, Derek Schuff wrote:
> Since all the pnacl-specific flags are warning flags, it should be fine to put
> them at the end, although it begs the question of whether the existing ones
were
> serving any purpose.
> 
> Mark is probably right that if the warnings are triggered by chrome-side code,
> they should go on the chrome side. (You can at least take a look if there's a
> good place to put them; if not then they can probably go here anyway).
> 
> On 2012/11/10 02:24:19, Mark Seaborn wrote:
> > Are these flags only needed for compiling Chrome-side code and not NaCl-side
> > code?  If so, this is the wrong place to put them.  Please add them to the
> > Chrome-side Gyp files instead.
> > 
> > BTW, I've set
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3108
> > in the commit message for context.
> > 
> > http://codereview.chromium.org/11363175/diff/1002/build/untrusted.gypi
> > File build/untrusted.gypi (right):
> > 
> >
>
http://codereview.chromium.org/11363175/diff/1002/build/untrusted.gypi#newcod...
> > build/untrusted.gypi:678: # Copyied from common.gypi.  These flags are
needed
> to
> > compile
> > 'Copied'

OK, I will look into moving the warning suppressions higher up (most likely into
chrome tree).  Will also look at how useful the pre-existing suppressions were.

I'll also look into enabling Werror for arm nexes, although I think it might
make more sense to add Werror in untrusted.gypi and have it added according the
werror variable in gyp (then turning of werror globally would also effect nexe
building, which seems like the correct behaviour).

[Sam Clegg, 2012-11-13 22:39:49]

This change is dependent on a corresponding change
to chrome landing first:

http://codereview.chromium.org/11360238/

[Mark Seaborn, 2012-11-13 22:55:23]

Can you address the TODO about -Werror in build/standalone_flags.gypi too,
please?

[Sam Clegg, 2012-11-13 23:14:11]

On 2012/11/13 22:55:23, Mark Seaborn wrote:
> Can you address the TODO about -Werror in build/standalone_flags.gypi too,
> please?

Done.

[Sam Clegg, 2012-11-14 19:44:17]

PTAL

[Mark Seaborn, 2012-11-14 19:50:05]

You have typos in your commit message: "pnacl_comile_flagso"

I don't see any trybot results for this change.  Also, can you do trybot runs
against ARM Chromium (cros_daisy) too?

http://codereview.chromium.org/11363175/diff/6003/build/untrusted.gypi
File build/untrusted.gypi (right):

http://codereview.chromium.org/11363175/diff/6003/build/untrusted.gypi#newcod...
build/untrusted.gypi:655: # be disabled in ~/.gyp/include.gypi on the valgrind
builders.
Nit: 'valgrind' -> 'Valgrind'.  Why would -Werror need to be disabled on the
Valgrind builders?  This shouldn't really be done via ~/.gyp/include.gypi --
that's very hacky.

[Sam Clegg, 2012-11-15 00:19:16]

bots are now passing.

http://codereview.chromium.org/11363175/diff/6003/build/untrusted.gypi
File build/untrusted.gypi (right):

http://codereview.chromium.org/11363175/diff/6003/build/untrusted.gypi#newcod...
build/untrusted.gypi:655: # be disabled in ~/.gyp/include.gypi on the valgrind
builders.
On 2012/11/14 19:50:05, Mark Seaborn wrote:
> Nit: 'valgrind' -> 'Valgrind'.  Why would -Werror need to be disabled on the
> Valgrind builders?  This shouldn't really be done via ~/.gyp/include.gypi --
> that's very hacky.

Fixed.

This was just a direct copy from chromium/src/build/common.gypi.

[Mark Seaborn, 2012-11-15 00:26:01]

> > You have typos in your commit message: "pnacl_comile_flagso"

Please fix that.

> > I don't see any trybot results for this change.  Also, can you do
> > trybot runs against ARM Chromium (cros_daisy) too?
>
> bots are now passing.

Yes, the NaCl bots are passing, but can you also test against the Chromium bots,
especially cros_daisy?  See
https://groups.google.com/group/native-client-dev/browse_thread/thread/808e77...
for how to do that.

http://codereview.chromium.org/11363175/diff/11/build/untrusted.gypi
File build/untrusted.gypi (right):

http://codereview.chromium.org/11363175/diff/11/build/untrusted.gypi#newcode656
build/untrusted.gypi:656: 'werror%': '-Werror',
Hmm, this is probably redundant because build/common.gypi has the same variable
definition.  Does -Werror still get passed with this removed?

[Sam Clegg, 2012-11-15 19:21:05]

This change depends on both:

http://codereview.chromium.org/11360238/

and

http://codereview.chromium.org/11417011/

So I'm currently try'ing it with both of those combined
into the patch.

http://codereview.chromium.org/11363175/diff/7009/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/11363175/diff/7009/build/common.gypi#newcode107
build/common.gypi:107: 

Moving werror out of linux only block as it is now needed on all platforms (or
at least on all platforms that build nexes).

[Sam Clegg, 2012-11-15 23:08:42]

PTAL.

All bots are now passing, but we need to wait for these
two changes to land in chome before it is Werror clean:

http://codereview.chromium.org/11365242/
http://codereview.chromium.org/11360238/

They are both in the CQ right now.

[Mark Seaborn, 2012-11-15 23:38:48]

LGTM.  I've fixed your commit message.

http://codereview.chromium.org/11363175/diff/20001/build/untrusted.gypi
File build/untrusted.gypi (right):

http://codereview.chromium.org/11363175/diff/20001/build/untrusted.gypi#newco...
build/untrusted.gypi:26: '-Werror',
Nit: Can you put this next to '-Wall' so that the warning flags are together?

[Sam Clegg, 2012-11-16 00:13:18]

I'll wait until the two chrome side fixes land then try once again on the chrome
bots before committing this.

http://codereview.chromium.org/11363175/diff/20001/build/untrusted.gypi
File build/untrusted.gypi (right):

http://codereview.chromium.org/11363175/diff/20001/build/untrusted.gypi#newco...
build/untrusted.gypi:26: '-Werror',
On 2012/11/15 23:38:48, Mark Seaborn wrote:
> Nit: Can you put this next to '-Wall' so that the warning flags are together?

Done.

[commit-bot: I haz the power, 2012-11-17 01:09:54]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/sbc@chromium.org/11363175/21004

[commit-bot: I haz the power, 2012-11-17 14:12:21]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/sbc@chromium.org/11363175/21004

[commit-bot: I haz the power, 2012-11-17 19:19:16]

Change committed as 10297