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

RESOLVED FIXED in Firefox 36

Status

()

Core
Build Config
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 wontfix, firefox36 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8512423 [details] [diff] [review]
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!
Attachment #8512423 - Flags: review?(ted)
(Assignee)

Comment 1

3 years ago
Created attachment 8512424 [details] [diff] [review]
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
Attachment #8512424 - Flags: review?(ted)
(Assignee)

Comment 2

3 years ago
Created attachment 8512425 [details] [diff] [review]
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.
Attachment #8512425 - Flags: review?(catlee)
(Assignee)

Comment 3

3 years ago
Created attachment 8512426 [details] [diff] [review]
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. :)
Attachment #8512426 - Flags: review?(n.nethercote)
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.
Attachment #8512425 - Flags: review?(catlee) → review-
(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 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.
Attachment #8512426 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 7

3 years ago
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?

http://hg.mozilla.org/build/tools/file/tip/scripts/spidermonkey_builds
Attachment #8512425 - Attachment is obsolete: true
Attachment #8512464 - Flags: review?(sphink)
(Assignee)

Updated

3 years ago
Blocks: 1090088
(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.
Attachment #8512464 - Flags: review?(sphink) → review+
(Assignee)

Comment 9

3 years ago
Landed part 3 and 4:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4917409aa902
https://hg.mozilla.org/integration/mozilla-inbound/rev/075c307bb073
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/4917409aa902
https://hg.mozilla.org/mozilla-central/rev/075c307bb073
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.
Attachment #8512423 - Flags: review?(ted) → review+
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.
Attachment #8512424 - Flags: review?(ted) → review+
(Assignee)

Comment 13

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5bdae6ae04
status-firefox35: --- → wontfix
status-firefox36: --- → fixed
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/4b5bdae6ae04
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.