1090016 - SpiderMonkey: neither --enable-sm-fail-on-warnings nor --enable-warnings-as-errors enable warnings as errors

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: part-1-enable-warnings-as-errors.patch

Part 1: SpiderMonkey: --disable-warnings-as-errors should not enable warnings as errors.

Bug 703121 intended to make warnings-as-errors opt-in instead of opt-out. However, the patch that landed did not s/MOZ_ARG_DISABLE_BOOL/MOZ_ARG_ENABLE_BOOL/ in js/src/configure.in:

https://hg.mozilla.org/mozilla-central/rev/08d24f493dc1

Thus, the meanings of --enable-warnings-as-errors and --disable-warnings-as-errors seem to have been reversed for SpiderMonkey since 2012!

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-2-remove-enable-sm-fail-on-warnings.patch

Part 2: Replace SpiderMonkey's --enable-sm-fail-on-warnings flag with explicit moz.build FAIL_ON_WARNINGS.

Bug 646702 added a --enable-sm-fail-on-warnings flag for SpiderMonkey shell builds. However, --enable-sm-fail-on-warnings (inadvertently?) requires --enable-warnings-as-errors to work. Bug 647927 added dedicated "warnaserr" shell builds, but did not include --enable-warnings-as-errors (and even if it had, SpiderMonkey's --enable-warnings-as-errors logic was reversed in bug 703121 above).

Thus, AFAICT, neither Try's --enable-warnings-as-errors Firefox builds nor SpiderMonkey's "warnaserr" shell builds have ever treated SpiderMonkey warnings as errors. This patch removes the --enable-sm-fail-on-warnings indirection and uses the same --enable-warnings-as-errors and FAIL_ON_WARNINGS flags as the rest of Firefox. (I have a follow up patch to switch the "warnaserr" shell build configs to use --enable-warnings-as-errors.)

https://tbpl.mozilla.org/?tree=Try&rev=1fc669872c59

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-3-update-warnaserr-build-configs.patch

Part 3: Update SpiderMonkey's "warnaserr" shell build configs to use --enable-warnings-as-errors instead of --enable-sm-fail-on-warnings.

Patch 2 above removed --enable-sm-fail-on-warnings so SpiderMonkey's build can use the same --enable-warnings-as-errors as the rest of Firefox.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-4-fix-shell-warnings.patch

Part 4: Fix last gcc/clang warning in js/src/shell and mark as FAIL_ON_WARNINGS (for gcc/clang). The shell code still has some non-trivial MSVC warnings about DLL linkage, but we can at least "lock in" our gcc/clang warning winnings. :)

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8512425 [details] [diff] [review]
part-3-update-warnaserr-build-configs.patch

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

Tricked you! These files aren't used anymore. Or rather, they are, but only for older branches where you probably don't want to turn this on because then you'll have to backport the warning fixes too.

Unless you want to backport? In which case you can take my r- to mean an r+. :-)

But anyway, the files you want to modify are now in js/src/devtools/automation. Same filenames.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Steve Fink [:sfink, :s:] from comment #4)
> Tricked you! These files aren't used anymore.

Why are they still in the tree, then?

> Or rather, they are, but only for older branches

No, the answer is not that, since those older branches would be using those files from their own checkout.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8512426 [details] [diff] [review]
part-4-fix-shell-warnings.patch

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

Good catch on the enable/disable switch!

::: js/src/shell/moz.build
@@ +34,5 @@
>  OS_LIBS += CONFIG['EDITLINE_LIBS']
>  OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
> +
> +if CONFIG['GNU_CXX']:
> +    FAIL_ON_WARNINGS = True

I'm not an expert on moz.build files but I see |if CONFIG['GNU_CXX']| in several other files so I guess it's ok.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-3-update-warnaserr-build-configs.patch

Part 3, take 2: Update js/src/devtools/automation/warnaserr* build configs to use --enable-warnings-as-errors instead of --enable-sm-fail-on-warnings.

(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Steve Fink [:sfink, :s:] from comment #4)
> > Tricked you! These files aren't used anymore.
> 
> Why are they still in the tree, then?

These files are in the build/tools repo, not mozilla-central. Must build/tools repo's tip support all older branches in perpetuity?

http://hg.mozilla.org/build/tools/file/tip/scripts/spidermonkey_builds

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Steve Fink [:sfink, :s:] from comment #4)
> > Tricked you! These files aren't used anymore.
> 
> Why are they still in the tree, then?
> 
> > Or rather, they are, but only for older branches
> 
> No, the answer is not that, since those older branches would be using those
> files from their own checkout.

Those files don't exist in the tree in older checkouts. I just added them to the tree a week or two ago, in order to move the spidermonkey build/test logic into the tree (which you have requested in the past, and I was dragging my heels on because I intended to switch these jobs to mozharness first.) Bug 1087361.

(In reply to Chris Peterson (needinfo? :cpeterson) from comment #7)
> Created attachment 8512464 [details] [diff] [review]
> part-3-update-warnaserr-build-configs.patch
> 
> Part 3, take 2: Update js/src/devtools/automation/warnaserr* build configs
> to use --enable-warnings-as-errors instead of --enable-sm-fail-on-warnings.
> 
> (In reply to Mike Hommey [:glandium] from comment #5)
> > (In reply to Steve Fink [:sfink, :s:] from comment #4)
> > > Tricked you! These files aren't used anymore.
> > 
> > Why are they still in the tree, then?
> 
> These files are in the build/tools repo, not mozilla-central. Must
> build/tools repo's tip support all older branches in perpetuity?

The tip is what is used for all mozilla-central branches. build/tools must support the oldest branch that we're running spidermonkey tests on. So in perpetuity, no, but for a really long time, yes.

Comment 9

[Chris Peterson [:cpeterson]]

Landed part 3 and 4:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4917409aa902
https://hg.mozilla.org/integration/mozilla-inbound/rev/075c307bb073

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4917409aa902
https://hg.mozilla.org/mozilla-central/rev/075c307bb073

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 8512423 [details] [diff] [review]
part-1-enable-warnings-as-errors.patch

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

Wow, that's dumb.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8512424 [details] [diff] [review]
part-2-remove-enable-sm-fail-on-warnings.patch

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

::: js/xpconnect/shell/moz.build
@@ +42,5 @@
>      'xpcomglue_s',
>      'xul',
>  ]
> +
> +FAIL_ON_WARNINGS = True

This is xpcshell's moz.build, not the js shell, which is js/src/shell, FYI.

Comment 13

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5bdae6ae04

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b5bdae6ae04