Move FAIL_ON_WARNINGS into moz.build

RESOLVED FIXED in mozilla26

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

No description provided.
Assignee

Comment 1

6 years ago
Posted patch Part a: logic (obsolete) — Splinter Review
Attachment #762237 - Flags: review?(gps)
Assignee

Comment 2

6 years ago
Posted patch Part b: automated moves (obsolete) — Splinter Review
Attachment #762239 - Flags: review?(joey)
Assignee

Comment 3

6 years ago
Posted patch Part c: manual moves (obsolete) — Splinter Review
Attachment #762240 - Flags: review?(mshal)
Comment on attachment 762237 [details] [diff] [review]
Part a: logic

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

\o/
Attachment #762237 - Flags: review?(gps) → review+
Assignee

Comment 5

6 years ago
Attachment #762245 - Flags: review?(gps)
Assignee

Comment 6

6 years ago
CC'ing the one guy who might bitrot me here
Not actively working on any FAIL_ON_WARNINGS patches, so you're clear. :)
Comment on attachment 762245 [details] [diff] [review]
Part d: disallow it in makefiles

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

Good riddance.
Attachment #762245 - Flags: review?(gps) → review+
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?
Attachment #762240 - Flags: review?(mshal) → review+
(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)
(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 :))
(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.
Looks like all MSVC exemptions are legitimate.
Random statistic:

 bxr.html |  4271 ++++++++++++-----------------------------------------------------------------------------------------------------------------------
 1 files changed, 420 insertions(+), 3851 deletions(-)
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)
Attachment #762239 - Flags: review?(joey) → review-
You have to explicitly --enable-warnings-as-errors in your mozconfig for these to take effect.
It seems like this is the same thing from bug 883284, where we don't pick it up in config.mk.
(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+.
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.
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.
(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)
(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)
(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)
Yeah, I'll catch them then.
Attachment #762237 - Attachment is obsolete: true
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.
Attachment #762239 - Attachment is obsolete: true
Attachment #791729 - Flags: review?(joey)
Attachment #762240 - Attachment is obsolete: true
Attachment #791730 - Flags: review?(joey)
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.
Attachment #762245 - Attachment is obsolete: true
Attachment #791731 - Flags: review?(khuey)
Comment on attachment 791729 [details] [diff] [review]
Part b (1): automatic moves

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

Patch looks good.
Attachment #791729 - Flags: review?(joey) → review+
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 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
Attachment #791730 - Flags: review?(joey) → review-
(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.
Assignee

Updated

6 years ago
Attachment #791730 - Flags: review- → review?(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
Attachment #791730 - Flags: review?(joey)
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.
Attachment #791731 - Flags: review?(mshal) → review+
Blocks: 883284
Assignee

Updated

6 years ago
Blocks: 907337
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.
ah, thus the removal of config.mk includes.
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
Attachment #791730 - Flags: review+
(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 ?
Assignee

Updated

6 years ago
Blocks: 908142
https://hg.mozilla.org/mozilla-central/rev/db29093b48d2
https://hg.mozilla.org/mozilla-central/rev/9fb5b1f56c21
https://hg.mozilla.org/mozilla-central/rev/ea6d4b1744f5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.