Closed Bug 1090016 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox35 wontfix, firefox36 fixed)

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(4 files, 1 obsolete file)

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)
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)
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)
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+
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)
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+
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+
https://hg.mozilla.org/mozilla-central/rev/4b5bdae6ae04
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.