Closed
Bug 557566
(FAIL_ON_WARNINGS)
Opened 14 years ago
Closed 13 years ago
Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNINGS]
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 4 obsolete files)
1.80 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
6.65 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
3.15 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
In the quest to reduce compiler warnings, it would be very nice to mark currently-warning-free files or directories as "clean" in a way that lets us notice if someone suddenly introduces warning-spammy code there. I think the easiest way to do this is with the $(WARNINGS_AS_ERRORS) build flag. Filing this bug to add $(WARNINGS_AS_ERRORS) to a smattering of directories that are known to be warning-free right now.
Assignee | ||
Comment 1•14 years ago
|
||
Note that this would mean tinderbox would turn red if anyone added warning-spammy code to a previously-clean directory. (local builds would fail too, depending on your compiler) This redness is a minor annoyance to a developer introducing any new warning-spammy code in a previously marked-as-clean directory, but I think it's a reasonable & acceptable burden. The alternative is that the committer would completely miss those build-warnings (which sometimes are indicative of real bugs), and they'll just pile on to warning-creep.
Assignee | ||
Comment 2•14 years ago
|
||
The "obvious"/brute-force fix for this would be to just add > CXXFLAGS += $(WARNINGS_AS_ERRORS) to Makefiles in directories that are known-clean. One perhaps slightly-cleaner alternative would be to define a variable in those Makefiles, e.g. > DISALLOW_NEW_WARNINGS=1 and then check that variable wherever CXXFLAGS is set up -- in config.mk or rules.mk -- and append WARNINGS_AS_ERRORS to CXXfFLAGS if the variable is set. This latter strategy would allow us to easily add a build flag for configuring this behavior, too, if we want (since there would only be one line where we'd actually be applying WARNINGS_AS_ERRORS, and we could wrap that one line in an #ifdef of some sort) (but maybe we don't want a build flag anyway, since I think you could disable this by setting WARNINGS_AS_ERRORS to be empty)
Assignee | ||
Comment 3•14 years ago
|
||
Ted, any thoughts/suggestions/vetoes on this? I'd be happy to work on a patch along the lines of either of the strategies from comment 2 (or a different strategy, if you can suggest something better).
Comment 4•14 years ago
|
||
I'm pretty sure we used to use the former (CXXFLAGS += $(WARNINGS_AS_ERRORS)) in some directories, but it's not currently used anywhere.
Comment 5•14 years ago
|
||
netwerk/cookie/src/ still does in CVS, as does camino/, though they don't count ;)
Assignee | ||
Comment 6•14 years ago
|
||
Looks like WARNINGS_AS_ERRORS got removed from network/cookie/src as part of bug 437002 / changeset af9466a5c157, due to a hunch that it was causing weirdness in PGO on windows. I don't understand the last few comments there, though, or whether the removal had any effect. Ted, that bug's assigned to you -- do you know what was going on there? (and is there anything in that bug that should make us hesitate to use WARNINGS_AS_ERRORS in more places?)
Comment 7•14 years ago
|
||
I don't think we ever determined anything more than you see on that bug. The PGO build is mysterious. Adding these back in without some sort of mitigation could certainly cause weird PGO build failures. We could try adding a flag like you described, maybe "FAIL_ON_ERRORS = 1", and then have some smarts in config.mk to only add WARNINGS_ON_ERRORS if we're not a) on windows and b) also doing a PGO build (MOZ_PROFILE_GENERATE or MOZ_PROFILE_USE is set).
Assignee | ||
Comment 8•14 years ago
|
||
This patch adds checks in config.mk for two new Makefile variables: FAIL_ON_WARNINGS FAIL_ON_WARNINGS_DEBUG The latter is a debug-only alias for the former. It's necessary because a lot of directories are warning-free for debug builds are *not* warning-free in opt builds -- largely because of variables whose only use is in optimized-away assertions, and hence get flagged with "unused" warnings. (Those warnings can & should be fixed eventually, but I don't think we want to try to fix them all before landing this.)
Assignee | ||
Updated•14 years ago
|
Attachment #439956 -
Flags: review? → review?(ted.mielczarek)
Assignee | ||
Comment 9•14 years ago
|
||
This patch applies the FAIL_ON_WARNINGS & FAIL_ON_WARNINGS_DEBUG flags in 15 currently-warning-free directories. This compiles successfully on my linux machine, using g++ 4.3, for both debug and opt builds.* I'm currently running this through tryserver (which is opt-only, sigh), to see if it trips over any compile warnings on other platforms. *Note: I'm building on top of these two simple warning-fixes, which should hopefully land soon: http://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/6903793d9902/accelerometer_warningfix.patch http://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/6903793d9902/text_control_frame_warningfix.patch
Attachment #439968 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 10•14 years ago
|
||
Note: This failed on TryServer maemo box, due to bug 557758, which has a patch that should fix the maemo issues once bug 441767 lands. (and vlad says he can land bug 441767 later today -- yay!)
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #9) > *Note: I'm building on top of these two simple warning-fixes, which should > hopefully land soon: (those warning-fixes are Bug 560271 and Bug 560274, and they've both landed)
Assignee | ||
Comment 12•14 years ago
|
||
sdwilsh points out in IRC that anyone with non-ASCII characters in their username will get compile warnings everywhere, in MSVC, due to the use of -DDEBUG_[username] So, Patch A probably needs an additional check for "if [username is non-ascii-chars]", inside the "ifeq (WINNT,$(OS_ARCH))" clause. I don't know how to write that in Makefile-speak though...
Assignee | ||
Comment 13•14 years ago
|
||
(In reply to comment #12) > needs an additional check for "if [username is non-ascii-chars]" er, s/is/contains/
(In reply to comment #12) > sdwilsh points out in IRC that anyone with non-ASCII characters in their > username will get compile warnings everywhere, in MSVC, due to the use of > -DDEBUG_[username] Can we just stop defining that? I went on a crusade eradicating all of the #ifdef DEBUG_<long gone netscape engineer> from widget/src/windows a few months back and would be more than happy to eradicate them from the rest of the tree. IMO we should be using #ifdef DEBUG_<FEATURE> rather than #ifdef DEBUG_<PERSON> e.g. DEBUG_LAYOUT, DEBUG_CC, etc.
Comment 15•14 years ago
|
||
That would be fine with me. You could ask around and see if anyone still uses that.
(In reply to comment #15) > That would be fine with me. You could ask around and see if anyone still uses > that. Asking: http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/b0bedb1c7db30f5b#
Comment 17•14 years ago
|
||
Wouldn't be interesting to add an option in configure.in to let the user disable this ? So, if an user doesn't care about warnings or is compiling on a unusual platform, he/she could prevent annoying errors. I think 'strict' is commonly used for this kind of feature.
Assignee | ||
Comment 18•14 years ago
|
||
Ok, so assuming bug 561674 lands (removing the auto-define of DEBUG_username), then we don't have to worry about comment 12 anymore. (In reply to comment #17) > Wouldn't be interesting to add an option in configure.in to let the user > disable this ? Seems reasonable. Perhaps --disable-warnings-as-errors, which could just unset WARNINGS_AS_ERRORS in configure.in. I'll update Patch A to include this.
Depends on: 561674
Assignee | ||
Updated•14 years ago
|
Attachment #439956 -
Attachment description: patch A v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG → part 1 v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG
Assignee | ||
Updated•14 years ago
|
Attachment #439968 -
Attachment description: patch B v1: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles → part 2 v1: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles
Assignee | ||
Comment 19•14 years ago
|
||
(In reply to comment #18) > Seems reasonable. Perhaps --disable-warnings-as-errors, which could just unset > WARNINGS_AS_ERRORS in configure.in. Did this as a separate patch, for simplicity. This lets people clear WARNINGS_AS_ERRORS by adding >ac_add_options --disable-warnings-as-errors to their mozconfig.
Attachment #443190 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 20•14 years ago
|
||
Updated version of part 2. Changes in this version: - No longer adds FAIL_ON_WARNINGS in widget/src/xpwidgets/Makefile.in or content/media/Makefile.in, due to some Windows- and Mac-only warnings in those directories that I didn't hit locally but uncovered on TryServer. - Fixes bitrot in content/media/ogg/Makefile.in
Attachment #443193 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•14 years ago
|
Attachment #439968 -
Attachment is obsolete: true
Attachment #439968 -
Flags: review?(ted.mielczarek)
Comment 21•14 years ago
|
||
Comment on attachment 439956 [details] [diff] [review] part 1 v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG >diff --git a/config/config.mk b/config/config.mk >--- a/config/config.mk >+++ b/config/config.mk >+# Check for FAIL_ON_WARNINGS & FAIL_ON_WARNINGS_DEBUG (Shorthand for Makefiles >+# to request that we use the 'warnings as errors' compile flags) >+ >+# NOTE: First, we clear FAIL_ON_WARNINGS[_DEBUG] if we're doing a Windows PGO >+# build, since WARNINGS_AS_ERRORS has been suspected of causing isuses in that >+# situation. (See bug 437002.) >+ifeq (WINNT,$(OS_ARCH)) >+ifneq (,$(MOZ_PROFILE_GENERATE)$(MOZ_PROFILE_USE)) You could combine these two lines into: ifeq (WINNT_1,$(OS_ARCH)_$(MOZ_PROFILE_GENERATE)$(MOZ_PROFILE_USE)) >+ifdef FAIL_ON_WARNINGS_DEBUG >+FAIL_ON_WARNINGS_DEBUG = 0 You should just set this to an empty value. We don't conventionally use a value of zero to mean anything. Also, you can just set both of these to empty without checking their values first, it won't hurt anything. >+# Now, check for debug version of flag; it turns on normal flag in debug builds. >+ifdef FAIL_ON_WARNINGS_DEBUG >+ifeq (1,$(FAIL_ON_WARNINGS_DEBUG)) Just the ifdef should be fine, our boolean makefile variables are "unset = false, any value = true". >+# Check for normal version of flag, and add WARNINGS_AS_ERRORS if it's set to 1. >+ifdef FAIL_ON_WARNINGS >+ifeq (1,$(FAIL_ON_WARNINGS)) Same here. >+CXXFLAGS += $(WARNINGS_AS_ERRORS) Do you want to set CFLAGS too? r=me with those changes.
Attachment #439956 -
Flags: review?(ted.mielczarek) → review+
Updated•14 years ago
|
Attachment #443190 -
Flags: review?(ted.mielczarek) → review+
Comment 22•14 years ago
|
||
Comment on attachment 443193 [details] [diff] [review] part 2 v2: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles I think you should file a followup on making the places where you need _DEBUG not warn in release builds. Do we have some sort of UNUSED macro? Having warnings in our release builds due to stray debug variables seems wrong.
Attachment #443193 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 23•14 years ago
|
||
(In reply to comment #22) > (From update of attachment 443193 [details] [diff] [review]) > I think you should file a followup on making the places where you need _DEBUG > not warn in release builds. Will do. > Do we have some sort of UNUSED macro? GCC's convention to suppress the warning is: |(void)variableName;| I think our preferred solution is to wrap the variable declaration in |#ifdef DEBUG/#endif|, though. > Having > warnings in our release builds due to stray debug variables seems wrong. Yup, though sadly it happens a lot, I think. :) Also, a good number of the warnings are for "variable may be used uninitialized". For some reason, these warnings only get triggered in optimized builds. I've filed bugs on a number of those: bug 560266, bug 557742, bug 557690, bug 534171, bug 534162, bug 534156, bug 534150 Often this "may be used uninitialized" warning actually isn't a problem -- e.g. int foo; if (myFlag) { foo = [something]; } // other stuff, that doesn't touch myFlag or foo if (myFlag) { someFunction(foo); } As long as that "// other stuff" clause doesn't touch myFlag, then this isn't a problem. Still, it may be better to take the (probably-negligible) perf hit and initialize foo to some sentinel value up-front, for the sake of sanity & safety.
Assignee | ||
Comment 24•14 years ago
|
||
Here's part 1 again, with ted's review feedback from comment 21 addressed. Carrying forward r=ted.
Attachment #439956 -
Attachment is obsolete: true
Attachment #443716 -
Flags: review+
Assignee | ||
Comment 25•14 years ago
|
||
sorry, that last version had an accidental space character introduced at the top of the file. fixed here.
Attachment #443716 -
Attachment is obsolete: true
Attachment #443717 -
Flags: review+
Assignee | ||
Comment 26•14 years ago
|
||
One more update to part 1: this now applies the changes to "js/src/config/config.mk", too, so that "make check" doesn't complain about it being out of sync with config/config.mk.
Attachment #443717 -
Attachment is obsolete: true
Attachment #443776 -
Flags: review+
Assignee | ||
Comment 27•14 years ago
|
||
Landed part 0 & part 1: http://hg.mozilla.org/mozilla-central/rev/a0b935912fa2 http://hg.mozilla.org/mozilla-central/rev/86af015da3fc (This just means FAIL_ON_WARNINGS is now *available* for use in Makefiles, but it isn't actually actually used anywhere yet -- we need to wait on bug 561674 before we can do that, per comment 12 / comment 14).
Assignee | ||
Comment 28•14 years ago
|
||
Landed a tweaked version of "part 2" - I removed the _DEBUG chunks -- I intend to land those in a separate patch, with a few new additions. - Added two new warning-free directories: uriloader/base and docshell/shistory/src/ http://hg.mozilla.org/mozilla-central/rev/5da9fc2be835
Assignee | ||
Comment 29•14 years ago
|
||
Pushed a followup to disable FAIL_ON_WARNINGS[_DEBUG] on Android for now, since android adds some global-ish #included files that have build warnings, causing failures like this: http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1277588954.1277589437.28511.gz Followup is: http://hg.mozilla.org/mozilla-central/rev/19f3fce93339
Assignee | ||
Comment 30•14 years ago
|
||
Pushed another followup to remove FAIL_ON_WARNINGS from "canvas" & "wave" directories, since GCC 4.4 suffers from a version of bug 573895 in those directories. (being tracked on that bug) http://hg.mozilla.org/mozilla-central/rev/a65d962718b0
Assignee | ||
Comment 31•14 years ago
|
||
Backed out the FAIL_ON_WARNINGS additions for now, per m.d.platform thread: https://groups.google.com/group/mozilla.dev.platform/msg/db86e555e7e40a9e Backout is: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=1934562edc1b Hoping to change this to be opt-in instead of opt-out and get the tinderbox farm opted in, so we can still catch new warnings in previously-clean directories without causing pain in local builds for people with non-"standard" compilers.
Comment 32•14 years ago
|
||
I can think of two ways to do this: 1) Have a configure option to enable the behavior 2) Have a whitelist of supported compilers in configure, enable only for those compilers The problem with #1 is that if developers don't enable it locally, but we have it enabled on tinderbox, they can wind up pushing something that built fine locally but errors on tinderbox.
Assignee | ||
Comment 33•13 years ago
|
||
I'm not actively working on the remaining parts of this anymore, so I'm reshaping this into just Comment 27, and resolving as fixed. If we end up wanting to build with warnings as errors in certain directories by default, that should probably be done in followup bugs (like Bug 646702 -- woohoo!)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Summary: Build with WARNINGS_AS_ERRORS in currently-warning-free directories → FAIL_ON_WARNINGS: Allow building with WARNINGS_AS_ERRORS in specific directories
Assignee | ||
Updated•13 years ago
|
Alias: FAIL_ON_WARNINGS
Summary: FAIL_ON_WARNINGS: Allow building with WARNINGS_AS_ERRORS in specific directories → Allow building with WARNINGS_AS_ERRORS in specific directories
Updated•12 years ago
|
Summary: Allow building with WARNINGS_AS_ERRORS in specific directories → Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNINGS]
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Updated•9 years ago
|
Blocks: Winconsistent-missing-override
Updated•9 years ago
|
No longer blocks: Winconsistent-missing-override
Depends on: Winconsistent-missing-override
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•