891993 - Mark toolkit/components/downloads/ as FAIL_ON_WARNINGS

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Daniel Holbert [:dholbert]]

The directory toolkit/components/downloads/ is build-warning-free on all supported platforms.

Filing this bug to mark it as FAIL_ON_WARNINGS, to keep it that way.

Here's a fully-green Try run, with the annotation added, to prove that this is safe:
 https://tbpl.mozilla.org/?tree=Try&rev=53b34881aba0

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch v1

Comment 2

[Daniel Holbert [:dholbert]]

(Aside: on my local machine, this directory does hit a few instances of "-Wdeprecated-declarations" warnings (for using deprecated GTK APIs), but those won't cause problems here because we've already excepted -Wdeprecated-declarations from FAIL_ON_WARNINGS, over in bug 833405.)

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 773420 [details] [diff] [review]
patch v1

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

I honestly trust your checks and the tryserver here. Since I don't have anything against fail on warning, it's fine.

Comment 4

[Daniel Holbert [:dholbert]]

Thanks!

Side note: I verified locally that this was being enforced -- I added an unused variable to one of the .cpp files in this directory, and it caused a build error (as you'd expect).

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Ed Morley [:emorley]]

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

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey Daniel,

I introduced a protocol buffer library as a dependency in https://bugzilla.mozilla.org/show_bug.cgi?id=837199 that is not compatible with this change. A lot of this code is automatically generated by the protocol buffer compiler and the bug to fix this has been open for more than a year: http://code.google.com/p/protobuf/issues/detail?id=361&colspec=ID%20Type%20Status%20Priority%20FixedIn%20Owner%20Summary

Is it possible to revert this?

Monica

Comment 8

[Daniel Holbert [:dholbert]]

Backed out:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/7645c1ac2ec3
so as not to block Monica from landing the 3rd-party header mentioned in comment 7 ( csd.pb.h ), which apparently has a few build warnings.

Per irc discussion, Monica's going to file a bug on those warnings, blocking this one, so that once we (hopefully) address them (or once upstream addresses them), we can re-land this annotation.

Comment 9

[Daniel Holbert [:dholbert]]

Actually, from skimming the build logs from the Try run for bug 837199, I'm not convinced that protobuf is actually triggering any build warnings in toolkit/components/downloads/ ...

The code.google.com issue flagged in comment 7 seems to be for warnings in protobuf .cc files, and some of those are shown in the Try run's build logs, but those files don't live in toolkit/components/downloads/, so they shouldn't cause any trouble here.

Maybe I'm missing something? If not, then we don't need to file a new bug (blocking this one) after all, and this never actually needed to be backed out & can be re-landed.

Comment 10

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Daniel,

Thanks for helping me :) I am recompiling with my local fixes and your original patch:

 https://tbpl.mozilla.org/?tree=Try&rev=356a26b7fa58

Monica

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

Oops, leftover mtype: https://tbpl.mozilla.org/?tree=Try&rev=5f8239555046

Comment 12

[Daniel Holbert [:dholbert]]

Per IRC discussion with Monica, she agrees that Comment 9 is probably right -- the only build warnings introduced in this dir in Bug 837199 were in Mozilla code, and Monica's fixed those locally.

So, I re-landed this:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/c434fcf6825b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c434fcf6825b