882859 - Move FAIL_ON_WARNINGS into moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: logic

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: automated moves

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: manual moves

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 762237 [details] [diff] [review]
Part a: logic

Review of attachment 762237 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d: disallow it in makefiles

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

CC'ing the one guy who might bitrot me here

Comment 7

[Daniel Holbert [:dholbert]]

Not actively working on any FAIL_ON_WARNINGS patches, so you're clear. :)

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 762245 [details] [diff] [review]
Part d: disallow it in makefiles

Review of attachment 762245 [details] [diff] [review]:
-----------------------------------------------------------------

Good riddance.

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 762240 [details] [diff] [review]
Part c: manual moves

Review of attachment 762240 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! Anyone know if all the _MSC_VER checks are legitimate? Or something that has just been copied/pasted around?

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Michael Shal [:mshal] from comment #9)
> Looks good to me! Anyone know if all the _MSC_VER checks are legitimate? Or
> something that has just been copied/pasted around?

Not something to be copypasted.

Those are usually there because the directory is warning-free *except* for a MSVC-specific warnings.  Ideally we should fix these MSVC build warnings and remove the exemption, but it's not always obvious what the best way to do that is. (and people tend to be slow about fixing build warnings, since they're low-priority)

Comment 11

[Daniel Holbert [:dholbert]]

(oh, I see - you were asking if it *has* been copypasted. Not sure; 2 ways to check: (a) look at hg blame to see if it was a conscious decision, and/or (b) remove it and see what happens :))

Comment 12

[Michael Shal [:mshal]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> (oh, I see - you were asking if it *has* been copypasted. Not sure; 2 ways
> to check: (a) look at hg blame to see if it was a conscious decision, and/or
> (b) remove it and see what happens :))

Yeah, that's what I meant to say :). I'm sure at least one of them has a reason for being there, but it struck me as odd that there would be so many. In any case we don't need to fix it for this bug.

Comment 13

[Masatoshi Kimura [:emk]]

Looks like all MSVC exemptions are legitimate.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Random statistic:

 bxr.html |  4271 ++++++++++++-----------------------------------------------------------------------------------------------------------------------
 1 files changed, 420 insertions(+), 3851 deletions(-)

Comment 15

[Joey Armstrong [:joey]]

Comment on attachment 762239 [details] [diff] [review]
Part b: automated moves

I tried to verify that FOW is being attention to but the build does not seem to fail with embedded warnings, at least beneath netwerk/dns/.

1) modify netwerk/dns/nameprep.c and add a few unused variables to trigger a warning condition.

2) netwerk/dns/moz.build contains 'FAIL_ON_WARNINGS = True' from the patch

3) compile, -Werror was not passed as a compiler flag and make reported 
WARNINGS_AS_ERRORS=$(null)

config/config.mk
================
# Check for normal version of flag, and add WARNINGS_AS_ERRORS if it's set to 1.
ifdef FAIL_ON_WARNINGS
CXXFLAGS += $(WARNINGS_AS_ERRORS)
CFLAGS   += $(WARNINGS_AS_ERRORS)
endif # FAIL_ON_WARNINGS




% check.sh
grep='WARN', FILE: netwerk/dns/moz.build
FAIL_ON_WARNINGS = True
grep='WARN', FILE: netwerk/dns/Makefile.in
grep='WARN', FILE: obj-x86_64-unknown-linux-gnu/netwerk/dns/backend.mk

A FOW assignment seems to be missing from backend.mk, maybe v=True was handled as something other than bool.

python/mozbuild/mozbuild/backend/recursivemake.py
+                elif isinstance(v, bool):
+                    if v:
+                        backend_file.write('%s := 1\n' % k)

Comment 16

[(not currently active) Ted Mielczarek]

You have to explicitly --enable-warnings-as-errors in your mozconfig for these to take effect.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It seems like this is the same thing from bug 883284, where we don't pick it up in config.mk.

Comment 18

[Joey Armstrong [:joey]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> You have to explicitly --enable-warnings-as-errors in your mozconfig for
> these to take effect.

This would be good to mention in the var docs.  If that was the only problem the patch can have an r+.

Comment 19

[(not currently active) Ted Mielczarek]

If it's not making it into backend.mk then there might be another issue. If it's making it there but not taking effect then it might be a combination of the configure flag and possibly what Ms2ger mentioned above.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I checked manually for content/events/src; the backend.mk correctly has "FAIL_ON_WARNINGS := 1", and the Makefile has

include $(topsrcdir)/config/config.mk
include $(topsrcdir)/ipc/chromium/chromium-config.mk
include $(topsrcdir)/config/rules.mk

so I'm going to blame that. Which means that we need to deal with it somehow.

Comment 21

[Daniel Holbert [:dholbert]]

(I don't know how close this is to landing, given comment 20 & friends, but just as a heads-up: bug 885598 adds a few more FAIL_ON_WARNINGS annotations)

Comment 22

[Daniel Holbert [:dholbert]]

(bug 889136 adds another new annotation, too.)

(I won't mention any further new annotations here; not sure how soon we'll sort out the backend.mk issue, but when we do, it'd probably be worth doing a 'hg log -p | grep FAIL_ON_WARNINGS' before landing to catch any annotations that've been added since the last patch was posted here on June 13)

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> (bug 889136 adds another new annotation, too.)
(sorry, I meant bug 889099)
(I'll shut up now, I promise)

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yeah, I'll catch them then.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: logic (r=gps)

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b (1): automatic moves

This should only contain changes to makefiles that don't include config.mk directly. Any removed comments and now-empty makefiles are dealt with in the next patch.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b (2): manual moves

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Move in dom/plugins/ipc/hangui

I haven't yet done the following:

* dom/plugins/test/testplugin/Makefile.in
* js/xpconnect/shell/Makefile.in
* netwerk/test/Makefile.in
* security/manager/ssl/tests/unit/test_ocsp_stapling/Makefile.in
* uriloader/exthandler/tests/Makefile.in
* xpcom/tests/Makefile.in

and I'll probably do those in a followup.

Comment 29

[Joey Armstrong [:joey]]

Comment on attachment 791729 [details] [diff] [review]
Part b (1): automatic moves

Review of attachment 791729 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good.

Comment 30

[Joey Armstrong [:joey]]

Comment on attachment 791730 [details] [diff] [review]
Part b (2): manual moves

Review of attachment 791730 [details] [diff] [review]:
-----------------------------------------------------------------

Nits:
=================================================================
Source Error: tools/profiler/moz.build
line 9: Trailing whitespace detected - or remove extra blank line.

Source Error: netwerk/system/maemo/Makefile.in
line 17: Trailing whitespace detected

Source Error: netwerk/system/android/Makefile.in
line 16: Trailing whitespace detected

Source Error: embedding/browser/webBrowser/Makefile.in
line 18: Trailing whitespace detected

Comment 31

[Joey Armstrong [:joey]]

Comment on attachment 791730 [details] [diff] [review]
Part b (2): manual moves

Review of attachment 791730 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good for the most part but there are several stray edits that should be submitted in a separate patch.
can r+ once the comments are relocated.



content/xslt/src/xslt/Makefile.in
=================================
+
+# For nsDependentJSString

dom/bindings/Makefile.in
========================
+
+# Need this to find all our DOM source files.

dom/system/windows/Makefile.in
==============================
+
+# We fire the nsDOMDeviceAcceleration

embedding/components/windowwatcher/src/Makefile.in
==================================================
-LOCAL_INCLUDES += -I$(topsrcdir)/dom/base \
+
+# For nsJSUtils
+LOCAL_INCLUDES += \
+  -I$(topsrcdir)/dom/base \
+  $(NULL)

image/encoders/ico/Makefile.in
==============================
+
+# Decoders need RasterImage.h

image/src/Makefile.in
=====================
+
+# We need to instantiate the decoder

netwerk/dns/Makefile.in
=======================
+
+# need to include etld_data.inc

netwerk/streamconv/src/moz.build
================================
-if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa' and CONFIG['OS_TEST'] != 'x86_64':




dom/plugins/test/testplugin/moz.build 
gfx/thebes/moz.build b/gfx/thebes/moz.build
js/xpconnect/wrappers/moz.build
media/mtransport/build/moz.build
[...]
==================================================
  * Is _MSC_VER guaranteed to be defined on all platforms ?

+FAIL_ON_WARNINGS = not CONFIG['_MSC_VER']

  gfx/thebes/Makefile.in b/gfx/thebes/Makefile.in
    -ifndef _MSC_VER
    -FAIL_ON_WARNINGS = 1
    -endif # !_MSC_VER

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Joey Armstrong [:joey] from comment #31)
> Comment on attachment 791730 [details] [diff] [review]
> Part b (2): manual moves
> 
> Review of attachment 791730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good for the most part but there are several stray edits that
> should be submitted in a separate patch.
> can r+ once the comments are relocated.

These comments are those that were removed in the automated patch; I'll merge both on landing.

Comment 33

[Joey Armstrong [:joey]]

(In reply to :Ms2ger from comment #32)
> (In reply to Joey Armstrong [:joey] from comment #31)
> > Comment on attachment 791730 [details] [diff] [review]
> > Part b (2): manual moves
> > 
> > Review of attachment 791730 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Patch looks good for the most part but there are several stray edits that
> > should be submitted in a separate patch.
> > can r+ once the comments are relocated.
> 
> These comments are those that were removed in the automated patch; I'll
> merge both on landing.


If the bug is handling migration of 'FAIL_ON_WARNINGS' -- adding comments/reformatting LOCAL_INCLUDE and adding platform checks around MOZ_WIDGET_TOOLKIT should be handled in a separate patch since they are different logic.

Also are try results available to validate the results ?  Esp CONFIG['_MSC_VER'] ?


netwerk/streamconv/src/moz.build
================================
-if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa' and CONFIG['OS_TEST'] != 'x86_64':



dom/plugins/test/testplugin/moz.build 
gfx/thebes/moz.build b/gfx/thebes/moz.build
js/xpconnect/wrappers/moz.build
media/mtransport/build/moz.build
[...]
==================================================
  * Is _MSC_VER guaranteed to be defined on all platforms ?

+FAIL_ON_WARNINGS = not CONFIG['_MSC_VER']

  gfx/thebes/Makefile.in b/gfx/thebes/Makefile.in
    -ifndef _MSC_VER
    -FAIL_ON_WARNINGS = 1
    -endif # !_MSC_VER

Comment 34

[Michael Shal [:mshal]]

Comment on attachment 791731 [details] [diff] [review]
Part c: Move in dom/plugins/ipc/hangui

Bit confusing to review since FAIL_ON_WARNINGS is used in config.mk, and MOZ_GLUE_LDFLAGS is supposed to override the setting in autoconf.mk, but I think all the changes are good.

Comment 35

[Mike Hommey [:glandium]]

Good point, there's a ifdef FAIL_ON_WARNINGS in config.mk (in fact, that's what actually enables it), and backend.mk is included *after* than.

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Almost, but not quite:

http://mxr.mozilla.org/mozilla-central/source/config/rules.mk?mark=63-63,77-77#58

Comment 37

[Mike Hommey [:glandium]]

ah, thus the removal of config.mk includes.

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 791730 [details] [diff] [review]
Part b (2): manual moves

Review of attachment 791730 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I'm not too worried about FAIL_ON_WARNINGS causing anything too severe to lose sleep over.

::: tools/profiler/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  if CONFIG['MOZ_ENABLE_PROFILER_SPS']:
> +    FAIL_ON_WARNINGS = not CONFIG['_MSC_VER']
> +    

Nit: whitespace

Comment 39

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #38)
> Comment on attachment 791730 [details] [diff] [review]
> Part b (2): manual moves
> 
> Review of attachment 791730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. I'm not too worried about FAIL_ON_WARNINGS causing
> anything too severe to lose sleep over.
> 
> ::: tools/profiler/moz.build
> @@ +5,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  if CONFIG['MOZ_ENABLE_PROFILER_SPS']:
> > +    FAIL_ON_WARNINGS = not CONFIG['_MSC_VER']
> > +    
> 
> Nit: whitespace

I've had some of the conversion patches rejected over stray content and comments like these.  Any particular reason they are ok in this patch ?

Comment 40

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/db29093b48d2
https://hg.mozilla.org/mozilla-central/rev/9fb5b1f56c21
https://hg.mozilla.org/mozilla-central/rev/ea6d4b1744f5