646702 - Add a warnings-as-errors SpiderMonkey build

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Phil Ringnalda (:philor)]

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.

Comment 2

[Phil Ringnalda (:philor)]

Attachment: add SM_FAIL_ON_WARNINGS

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).

Comment 3

[Phil Ringnalda (:philor)]

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.

Comment 4

[(not currently active) Ted Mielczarek]

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.

Comment 5

[Phil Ringnalda (:philor)]

Attachment: add SM_FAIL_ON_WARNINGS, take 2

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 6

[Paul Biggar]

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?

Comment 7

[Phil Ringnalda (:philor)]

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.

Comment 8

[Phil Ringnalda (:philor)]

Attachment: add SM_FAIL_ON_WARNINGS, take 3

And any patch that removes lines makes me happier than one that adds them.

Comment 9

[Phil Ringnalda (:philor)]

Attachment: A handy warning

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 10

[Paul Biggar]

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.

Comment 11

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/tracemonkey/rev/d030c439c7cb

Comment 12

[(not currently active) Ted Mielczarek]

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.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/d030c439c7cb