1200072 - Replace SM(e) builds with SM(p) builds

Status: RESOLVED DUPLICATE of bug 1248502

(Firefox Build System :: General, defect)

Description

[Nicholas Nethercote [inactive]]

TL;DR: I don't think we need SM(e) builds (SpiderMonkey fail-on-warnings builds) any more because vanilla SM builds have the same amount of checking.

Details...

SpiderMonkey is now built with warnings-as-errors on for GCC in vanilla builds, courtesy of these lines in js/src/moz.build:

> if not CONFIG['GNU_CXX']:
>     ALLOW_COMPILER_WARNINGS = True

(This means we *don't* allow compiler warnings when building with GCC, i.e. we enable warnings-as-errors with GCC.)

js/src/shell/moz.build and js/src/jsapi-tests/moz.build have the same lines, and the other moz.build files under js/src/ never set ALLOW_COMPILER_WARNINGS.

And the SM(e) builds are only done on Linux, which uses GCC anyway.

So I think the SM(e) builds can be removed. This will get rid of a build configuration, which reduces complexity and machine load.

It will also avoid tripping up people who unluckily stumble across the odd corners of SM(e) builds and the fact that SM(e) builds don't run on try pushes. E.g. in bug 1198334 I just hit this, where some warnings occurred not in js/src/ files but in .c files in config/. I guess those files aren't built in normal builds but are built in SM(e) builds? Not sure, but it'll all be moot if we can remove SM(e) builds.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

You OK with this, Steve?

Comment 2

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #1)
> You OK with this, Steve?

Not exactly. The SM(e) builds are used as "normal" shell builds, without any special gczeal flags or environment settings or anything. They don't have tests suppressed like eg SM(cgc) builds do. If we were to autopackage an SM source release, it would probably come from these builds.

That said, I am definitely in favor of renaming these away from SM(e) if the warnings-as-errors is now redundant. Specifically, these should be switched to SM(p) (for "plain"). There are already SM(p) builds for Windows since we didn't want the warnings-as-errors stuff there.

One wrinkle is that we now have taskcluster-based shell builds. I'm not clear on whether those are ready to replace the buildbot-based ones and if not, what's left to be done. But we can do an SM(e) -> SM(p) switcheroo here regardless.

Comment 3

[Nicholas Nethercote [inactive]]

> It will also avoid tripping up people who unluckily stumble across the odd
> corners of SM(e) builds and the fact that SM(e) builds don't run on try
> pushes. E.g. in bug 1198334 I just hit this, where some warnings occurred
> not in js/src/ files but in .c files in config/.

I just hit this again in bug 1232219.

Comment 4

[Nicholas Nethercote [inactive]]

Will remove the --enable-warnings-as-errors lines in js/src/devtools/automation/variants/warnaserr{,debug} be enough to disable warnings-as-errors for these builds?

And then I guess the strings used by Treeherder will need to be updated, wherever they are stored.

Comment 5

[Nicholas Nethercote [inactive]]

> SpiderMonkey is now built with warnings-as-errors on for GCC in vanilla
> builds, courtesy of these lines in js/src/moz.build:
> 
> > if not CONFIG['GNU_CXX']:
> >     ALLOW_COMPILER_WARNINGS = True

BTW, these lines have been removed and fatal warnings are enabled for normal Treeherder builds on all platforms now. Which may not be relevant, other than one previous distinction between platforms has now gone.

Comment 6

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

Just stumbled across this bug. I did this in bug 1248502.

Comment 7

[Nicholas Nethercote [inactive]]

Yay! Thank you.