Closed Bug 646702 Opened 9 years ago Closed 9 years ago
Add a warnings-as-errors Spider
Bug 609532 turned on warnings-as-errors for all JS shell builds, but it breaks too many weird configurations. So pbiggar suggested turning it on for some well-known and well-tested configurations -- those "SM(!m !t d s)" ones on TBPL. Steps for proceed: Step 1: Add the configuration settings. Should be obvious how from looking at these commits from bug 609413: https://hg.mozilla.org/build/buildbot-configs/rev/09d71e2e4167 https://hg.mozilla.org/build/tools/rev/87edf68bff3d Step 2: Ask catlee for review (I think he'll need to commit for you too) Step 3: Add a bug asking philor to add them to tbpl.
Clarifying a bit, since from the IRC conversation what he actually meant was to add a fifth type of SM build, which to catch all flavors of warning we need to build as both opt and debug. It is a three-parter, but the parts are 1. A jseng part, to make it possible to build with WARNINGS_AS_ERRORS from something exported from a mozconfig, this bug. 2. A dependent releng bug once that exists, to add warnaserropt and warnaserrdebug to all the platform vars in build/buildbot-configs/mozilla/config.py, and add mozconfig fragments with those names to build/tools/scripts/spidermonkey_builds/. 3. A dependent tbpl bug once that's done, or has at least figured out what the build names will be, to make them visible.
Summary: Enable warnings-as-errors for "SM(!m !t d s)" shell builds on TBPL → Add a warnings-as-errors SpiderMonkey build
Version: unspecified → Trunk
Dunno if this is really the right way to go about it, but at least it seems to work, "export SM_FAIL_ON_WARNINGS && configure && make && fail" (assuming you've stuck in a warning to fail on).
Assignee: general → philringnalda
Status: NEW → ASSIGNED
Comment on attachment 523760 [details] [diff] [review] add SM_FAIL_ON_WARNINGS Oh, those aren't mozconfig fragments, they're configure args, and it looks like support for ./configure SM_FAIL_ON_WARNINGS=1 started sometime after autoconf2.13, so I don't actually know how to do this.
So, remember that configure is just a shell script, so anything exported from the environment that runs it is available as a variable. (Specifically, you can AC_SUBST(FOO), and then doing `FOO=1 ./configure` or `export FOO=1; ./configure` will just work.) You can also use exported environment variables directly as Makefile variables, as long as they're exported in the environment that runs make.
Okay, maybe I do know how, other than a typo rate nearly equal to the number of characters in the patch. The problem with exporting is that it will ugly up https://hg.mozilla.org/build/tools/file/tip/scripts/spidermonkey_builds/spidermonkey.sh, and obscure how it's built for the person who reddens it - all the other flavors of SM build are just what's in the files of configure args in https://hg.mozilla.org/build/tools/file/tip/scripts/spidermonkey_builds/, and just this one would be "read through the script to find where there's a "test $VARIANT..." that exports SM_FAIL_ON_WARNINGS." Googling made me think it should be possible to "./configure FOO=1" but apparently somewhere I'm not seeing, we decide that that means "I'm building on a FOO=1 machine, please set $host accordingly." This is a horrible --enable-foo, but we have a decade of history of that particular brand of horror, one more won't kill us.
Comment on attachment 524123 [details] [diff] [review] add SM_FAIL_ON_WARNINGS, take 2 Though you don't seem to like it, I think this is a much better approach than defining environment - I believe people understand using a --enable flag but don't really understand the interaction between configure, make and environmental vars. Also, we don't use them much otherwise. >diff --git a/js/src/Makefile.in b/js/src/Makefile.in >-#FAIL_ON_WARNINGS = 1 >+ifdef SM_FAIL_ON_WARNINGS >+FAIL_ON_WARNINGS = 1 >+endif I might be wrong, but wouldn't |SM_FAIL_ON_WARNINGS=| actually define SM_FAIL_ON_WARNINGS, causing the ifdef to succeed. Make/bash semantics not my strong-point. >+dnl ======================================================== >+dnl = Enable treating compile warnings as errors >+dnl ======================================================== >+MOZ_ARG_ENABLE_BOOL(sm-fail-on-warnings, >+[ --enable-sm-fail-on-warnings >+ Enable warnings as errors], >+ SM_FAIL_ON_WARNINGS=1, >+ SM_FAIL_ON_WARNINGS= ) >+ >+AC_SUBST(SM_FAIL_ON_WARNINGS) If there a reason you can't define FAIL_ON_WARNINGS here, and remove SM_FAIL_ON_WARNINGS altogether?
A first in my experience, the make manual (http://www.gnu.org/s/hello/manual/make/Conditional-Syntax.html) is pretty clear on that, other than a typo: "The value of that variable has a non-empty value, the text-if-true is effective; otherwise, the text-if-false, if any, is effective. Variables that have never been defined have an empty value." (Okay, clear in relative terms, but translated, "If the variable has a non-empty value, the ifdef is true; if it has an empty value or has never been defined, the ifdef is false.") No reason I can see not to just do FAIL_ON_WARNINGS in configure, if that's what you want. Bug 609532 needed to limit its scope to avoid ctypes and Android headers, but that's not a problem here. Far as I know, that'll add in editline and jsapi-tests, both of which seem to be warning-free on my compiler.
And any patch that removes lines makes me happier than one that adds them.
Just in case you don't have a warning handy for testing, I ganked this one from the fixes that came along with bug 609532.
Comment on attachment 524133 [details] [diff] [review] add SM_FAIL_ON_WARNINGS, take 3 (In reply to comment #7) > A first in my experience, the make manual > (http://www.gnu.org/s/hello/manual/make/Conditional-Syntax.html) is pretty > clear on that, other than a typo: "The value of that variable has a non-empty > value, the text-if-true is effective; otherwise, the text-if-false, if any, is > effective. Variables that have never been defined have an empty value." (Okay, > clear in relative terms, but translated, "If the variable has a non-empty > value, the ifdef is true; if it has an empty value or has never been defined, > the ifdef is false.") OK, seems good. Bad Make - have more obvious semantics! > No reason I can see not to just do FAIL_ON_WARNINGS in configure, if that's > what you want. Bug 609532 needed to limit its scope to avoid ctypes and Android > headers, but that's not a problem here. Far as I know, that'll add in editline > and jsapi-tests, both of which seem to be warning-free on my compiler. r+ on this. If editline fails on some platform, then you have automatic r+ to replace this with the previous patch (also adding FAIL_ON_WARNINGS to jsapi-tests/Makefile too), which is the second best approach.
Attachment #524133 - Flags: review?(pbiggar) → review+
Target Milestone: --- → mozilla2.2
Ah, that's a bummer, but I see what you mean about the way spidermonkey.sh is written. Maybe as a followup we could just rename that to --enable-fatal-warnings or something, and make it work in the top-level configure as well? Then at some point in the future we could enable it on tinderbox browser builds if we were so inclined.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.