Closed Bug 1198334 Opened 4 years ago Closed 4 years ago

Replace opt-in FAIL_ON_WARNINGS with opt-out ALLOW_COMPILER_WARNINGS

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: cpeterson, Assigned: njn)

References

Details

Attachments

(8 files, 1 obsolete file)

In bug 1198124 comment 1, glandium recommended changing FAIL_ON_WARNINGS' default value from False to True because 486 out of ~653 mozbuild files set `FAIL_ON_WARNINGS = True`.

1. Change FAIL_ON_WARNINGS' default value from False to True.
2. Add `FAIL_ON_WARNINGS = False` to mozbuild files in directories that have warnings.
3. Remove redundant `FAIL_ON_WARNINGS = True` from mozbuild files in directories that are warning-free.
Assignee: nobody → n.nethercote
The rest of this bug's changes will have to land in a single patch, but I'm
posting it as a series of patches to make reviewing easier. I'll fold 1a, 1b,
1c, etc. together before landing.

This patch changes this:

    FAIL_ON_WARNINGS = True

to this:

    <nothing>

in 433 places.
Attachment #8653854 - Flags: review?(mh+mozilla)
This patch changes this:

    FAIL_ON_WARNINGS = not <condition>

to this:

    if <condition>:
        ALLOW_COMPILER_WARNINGS = True

in 4 places.
Attachment #8653855 - Flags: review?(mh+mozilla)
This patch changes this:

    if <condition>
        FAIL_ON_WARNINGS = True

to this

    if not <condition>
        ALLOW_COMPILER_WARNINGS = True

in 13 places.
Attachment #8653856 - Flags: review?(mh+mozilla)
This patch changes the last 5 FAIL_ON_WARNINGS occurrences in moz.build files,
none of which fit the previous patterns.
Attachment #8653857 - Flags: review?(mh+mozilla)
Comment-only changes.
Attachment #8653858 - Flags: review?(mh+mozilla)
This is the change to the actual machinery that implements the flag, and the
tests.
Attachment #8653859 - Flags: review?(mh+mozilla)
For 3rd-party code I just added it.

For Mozilla code I did likewise but also added a "XXX: We should fix these
warnings" comment to make it clear that the warnings are our problem.

I haven't quite finished this yet, but I'm posting it for completeness.
Currently there are 58 places where we set ALLOW_COMPILER_WARNINGS. There will
be a few more before I get tryserver completely green.
Attachment #8653853 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8653858 [details] [diff] [review]
(part 1e) - Update mozconfigs for FAIL_ON_WARNINGS conversion

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

::: b2g/config/mozconfigs/linux32_gecko/debug
@@ +18,5 @@
>  export MOZILLA_OFFICIAL=1
>  
>  export MOZ_TELEMETRY_REPORTING=1
>  
> +# Treat warnings as errors in directories lacking ALLOW_COMPILER_WARNINGS.

Might as well remove "in directories (...)"
Attachment #8653858 - Flags: review?(mh+mozilla) → review+
Attachment #8653860 - Attachment is obsolete: true
Comment on attachment 8653859 [details] [diff] [review]
(part 1f) - Replace the opt-in FAIL_ON_WARNINGS with the opt-out ALLOW_COMPILER_WARNINGS

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

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ -273,5 @@
>                  'EXTRA_PP_COMPONENTS += foo.pp.js',
>              ],
> -            'FAIL_ON_WARNINGS': [
> -                'FAIL_ON_WARNINGS := 1',
> -            ],

You should modify python/mozbuild/mozbuild/test/backend/data/variable_passthru/moz.build and python/mozbuild/mozbuild/test/frontend/data/variable-passthru/moz.build, which are part of those tests, to contain ALLOW_COMPILER_WARNINGS, which will check it's properly propagated, like FAIL_ON_WARNINGS was, requiring that you add ALLOW_COMPILER_WARNINGS here and in test_emitter.py.
Attachment #8653859 - Flags: review?(mh+mozilla) → review+
Attachment #8653854 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8653855 [details] [diff] [review]
(part 1b) - FAIL_ON_WARNINGS conversions (2/4)

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

::: dom/plugins/test/testplugin/moz.build
@@ +8,5 @@
>  
>  SharedLibrary('nptest')
>  
> +if CONFIG['_MSC_VER']:
> +    ALLOW_COMPILER_WARNINGS = True

ALLOW_COMPILER_WARNINGS = CONFIG['_MSC_VER'] should work the same, but whichever you prefer.
Attachment #8653855 - Flags: review?(mh+mozilla) → review+
Attachment #8653856 - Flags: review?(mh+mozilla) → review+
Attachment #8653857 - Flags: review?(mh+mozilla) → review+
BTW, thank you for the level at which you did you patch split, it made review so much easier.
Comment on attachment 8653904 [details] [diff] [review]
(part 1g) - Add |ALLOW_COMPILER_WARNINGS = True| where necessary

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

::: config/external/nss/moz.build
@@ +45,5 @@
>          '/security/nss/lib/util/nssutil3',
>          'sqlite',
>      ]
> +
> +# njn: for the NSS B2G bustages...

s/the//

::: db/sqlite3/src/moz.build
@@ +8,5 @@
>  EXPORTS += [
>      'sqlite3.h',
>  ]
>  
> +ALLOW_COMPILER_WARNINGS = True

We still should fix that malloc_usable_size warning, although I agree it doesn't make sense to block sqlite3 updates on warnings it might get when building with our compilers.

::: extensions/spellcheck/hunspell/src/moz.build
@@ +40,5 @@
>  ]
>  
> +# XXX: This directory is a mix of Mozilla code and third-party code. We should
> +# put the Mozilla code in a separate directory and disallow compiler warnings
> +# there. Until then, allow warnings for all of the code.

File a followup?

::: gfx/angle/moz.build
@@ +133,5 @@
>  # which we need to get these symbols exported from gkmedias
>  DEFINES['COMPONENT_BUILD'] = True
>  DEFINES['ANGLE_TRANSLATOR_IMPLEMENTATION'] = True
>  
> +ALLOW_COMPILER_WARNINGS = True

Please update generate_mozbuild.py on https://github.com/mozilla/angle

::: gfx/qcms/moz.build
@@ +16,5 @@
>      'transform.c',
>      'transform_util.c',
>  ]
>  
> +ALLOW_COMPILER_WARNINGS = True

I think we are actually upstream for this.

::: ipc/chromium/moz.build
@@ +253,5 @@
>      ]
>  
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
> +ALLOW_COMPILER_WARNINGS = True

We may or may not want to address those. We're probably never going to actually pull a new upstream here.

::: media/mtransport/third_party/moz.build
@@ +68,5 @@
>  GYP_DIRS['nICEr'].non_unified_sources += nICEr_non_unified_sources
>  
>  GYP_DIRS['nrappkit'].input = 'nrappkit/nrappkit.gyp'
>  GYP_DIRS['nrappkit'].variables = gyp_vars
> +GYP_DIRS['nrappkit'].sandbox_vars['ALLOW_COMPILER_WARNINGS'] = True

Might be worth checking with jesup, but I know there are parts of webrtc where we are upstream. I don't know if those are.
Attachment #8653904 - Flags: review?(mh+mozilla) → review+
Summary: Change FAIL_ON_WARNINGS' default value from False to True → Replace opt-in FAIL_ON_WARNINGS with opt-out ALLOW_COMPILER_WARNINGS
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9411723e0e18

I had to land this follow-up because SM(e) builds (a) don't show up on try pushes, and (b) apparently work differently to all the other jobs on try. I think SM(e) builds can actually be removed now, and I filed bug 1200072 about this.


> We still should fix that malloc_usable_size warning, although I agree it
> doesn't make sense to block sqlite3 updates on warnings it might get when
> building with our compilers.

I filed bug 1200075.


> > +# XXX: This directory is a mix of Mozilla code and third-party code. We should
> > +# put the Mozilla code in a separate directory and disallow compiler warnings
> > +# there. Until then, allow warnings for all of the code.
> 
> File a followup?

I filed bug 1200065.


> > +ALLOW_COMPILER_WARNINGS = True
> 
> Please update generate_mozbuild.py on https://github.com/mozilla/angle

https://github.com/mozilla/angle/pull/7
(In reply to Nicholas Nethercote [:njn] from comment #19)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/9411723e0e18
> 
> I had to land this follow-up because SM(e) builds (a) don't show up on try
> pushes, and (b) apparently work differently to all the other jobs on try. I
> think SM(e) builds can actually be removed now, and I filed bug 1200072
> about this.

It's not that they don't show up, it's that they aren't scheduled at all apparently. Whoopsie. I guess we don't need to worry about it if you intend to kill the job, but we should otherwise get a bug on file for getting them enabled for Try pushes.
Duplicate of this bug: 977977
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.