Closed Bug 1119980 Opened 10 years ago Closed 10 years ago

Fix Werrors in Lollipop-based b2g build

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.5+, tracking-b2g:backlog, firefox41 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S13 (29may)
blocking-b2g 2.5+
tracking-b2g backlog
Tracking Status
firefox41 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 8 obsolete files)

822 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
5.87 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
99.30 KB, patch
botond
: review+
Details | Diff | Splinter Review
818 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
Warnings-as-errors was enabled for b2g in bug 1073003, and it was clean on TBPL, but the Lollipop-based b2g build, which is not currently tested on TBPL, was broken.
Assignee: nobody → botond
Blocks: 1073003
Attachment #8546914 - Flags: review?(waychen)
This wasn't actually a *warning*, but I needed it to get a successful build.
Attachment #8546915 - Flags: review?(kinetik)
Ehsan, I officially owe you a beer (or several)!
Attachment #8546916 - Flags: review?(ehsan)
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=71e661a9b25d This doesn't actually test Lollipop, since that doesn't run on TBPL; it's to help make sure I didn't break other things.
Attachment #8546914 - Flags: review?(waychen) → review+
(In reply to Botond Ballo [:botond] from comment #4) > Try run: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=71e661a9b25d Seems like Windows doesn't have 'snprintf'...
Comment on attachment 8546916 [details] [diff] [review] Part 3 - Use 'snprintf' instead of 'sprintf' Clearly this patch is no good as is. Any suggestions for how to deal with not having snprintf on Windows? Do we have a suitable polyfill for it?
Flags: needinfo?(ehsan)
Attachment #8546916 - Flags: review?(ehsan) → review-
(Meanwhile, if anyone working with Lollipop builds is blocked by this, please say so, and we can disable warnings-as-errors for Lollipop builds until the warnings are fixed.)
Comment on attachment 8546915 [details] [diff] [review] Part 2 - Add a missing include to MediaSource.cpp Review of attachment 8546915 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but note this was already fixed in bug 1119691.
Attachment #8546915 - Flags: review?(kinetik) → review+
Comment on attachment 8546915 [details] [diff] [review] Part 2 - Add a missing include to MediaSource.cpp (In reply to Matthew Gregan [:kinetik] from comment #8) > Looks good, but note this was already fixed in bug 1119691. Ah, thanks - no need for it here then.
Attachment #8546915 - Attachment is obsolete: true
(In reply to Botond Ballo [:botond] from comment #6) > Comment on attachment 8546916 [details] [diff] [review] > Part 3 - Use 'snprintf' instead of 'sprintf' > > Clearly this patch is no good as is. > > Any suggestions for how to deal with not having snprintf on Windows? Do we > have a suitable polyfill for it? Use PR_snprintf with the pattern used in bug 1119776, since MSVC 2015 has added support for it (finally!)
Flags: needinfo?(ehsan)
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → 2.2+
(In reply to Botond Ballo [:botond] from comment #7) > (Meanwhile, if anyone working with Lollipop builds is blocked by this, > please say so, and we can disable warnings-as-errors for Lollipop builds > until the warnings are fixed.) Botond, I think this is a good idea. AIK, some people working on gonk-l are blocked by this compile error, each of them need to work-around it locally. Could you help to disable warnings-as-errors for Lollipop builds until all warnings are fixed. Thanks!
Flags: needinfo?(botond)
Flags: needinfo?(botond)
Attachment #8550615 - Flags: review?(mh+mozilla)
Comment on attachment 8550615 [details] [diff] [review] [Temporary solution] disable warnings-as-errors in Lollipop-based builds Review of attachment 8550615 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/confvars.sh @@ +62,5 @@ > # Warnings-as-errors cannot be enabled on gcc <= 4.4 due to bug 915555. > if test "$GCC_MAJOR_VERSION" -gt 4 -o \ > "$GCC_MAJOR_VERSION" -eq 4 -a "$GCC_MINOR_VERSION" -gt 4; then > +# Nor can they be enabled on Lollipop until bug 1119980 is fixed. > +if test "$PLATFORM_SDK_VERSION" -lt 21; then Your need to update your tree first. The gcc 4.4 check is gone.
Attachment #8550615 - Flags: review?(mh+mozilla) → feedback+
Rebased.
Attachment #8550615 - Attachment is obsolete: true
Attachment #8551644 - Flags: review?(mh+mozilla)
Attachment #8551644 - Flags: review?(mh+mozilla) → review+
Landed patch to disable warnings-as-errors temporarily for Lollipop: https://hg.mozilla.org/integration/mozilla-inbound/rev/311bc10282b2 Marking as 'leave-open' until the proper fix lands.
Keywords: leave-open
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18) > https://hg.mozilla.org/mozilla-central/rev/311bc10282b2 Is it going to land to v2.2 branch?
(In reply to Hermes Cheng[:hermescheng] from comment #19) > (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18) > > https://hg.mozilla.org/mozilla-central/rev/311bc10282b2 > > Is it going to land to v2.2 branch? I was going to request uplift of the proper fix once it's in place. If you think the 2.2 branch needs a fix sooner, I can request uplift for the temporary fix.
Uplifting the temporary fix to v2.2 would be very nice, as we're working on v2.2 not master.
Comment on attachment 8551644 [details] [diff] [review] [2.2 solution] disable warnings-as-errors in Lollipop-based builds NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1073003. User impact if declined: Lollipop-based b2g builds are broken by warnings-as-errors. Testing completed: Confirmed locally that patch fixes the build (by not treating the warnings as errors). Patch on m-c since 2015-01-20. Risk to taking this patch (and alternatives if risky): Very low. The patch just reverts, for Lollipop-based builds only, a very recent change to treat warnings as errors. String or UUID changes made by this patch: None.
Attachment #8551644 - Flags: approval-mozilla-b2g37?
Attachment #8551644 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Whiteboard: [CR 786383]
Whiteboard: [CR 786383] → [caf priority: p2][CR 786383]
I just tried building B2G for Nexus 5 from scratch with: ./config.sh nexus-5-l ./build.sh Botond's workaround patch is in B2G/gecko/b2g/confvars.sh but the build is still failing because warnings are treated as errors.
Flags: needinfo?(botond)
(In reply to David Flanagan [:djf] from comment #23) > I just tried building B2G for Nexus 5 from scratch with: > > ./config.sh nexus-5-l > ./build.sh > > Botond's workaround patch is in B2G/gecko/b2g/confvars.sh but the build is > still failing because warnings are treated as errors. While doing a clean Lollipop build to try to repro this, I ran into a (non-Werror) build error. Filed bug 1126499 for it.
(In reply to Botond Ballo [:botond] from comment #24) > While doing a clean Lollipop build to try to repro this, I ran into a > (non-Werror) build error. Filed bug 1126499 for it. After reverting the commit causing this bustage (the proper solution is still being worked on), I was able to get a successful build. David, is it possible you were seeing this bustage rather than a Werror bustage? If not, i.e. if you actually were seeing a Werror bustage, could you please post the compiler output? Thanks!
Flags: needinfo?(botond) → needinfo?(dflanagan)
Botond, I did see that MP4Stream.h error on one of my build attempts. On prior attempts, I was also seeing a lot of warnings and I thought I saw something about those being treated as errors. Even when I ran with -j1 I was seeing a confusing mass of warnings. Maybe the root cause of the build failure was always the missing MP4Stream.h and I just didn't understand the output. I no longer have a copy of that output to post. Sorry. Let's assume that it was the MP4Stream.h error plus possibly operator error on my part.
Flags: needinfo?(dflanagan)
Ok, thanks. Do let me know if you encounter Werror-related bustage in the future!
(In reply to [Away: 1/29-2/20] from comment #10) > > Any suggestions for how to deal with not having snprintf on Windows? Do we > > have a suitable polyfill for it? > > Use PR_snprintf with the pattern used in bug 1119776, since MSVC 2015 has > added support for it (finally!) Implemented this. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13c550fa0a65
(In reply to Hermes Cheng[:hermescheng] from comment #19) > Is it going to land to v2.2 branch? https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9210eb2c60f7
:djf, fyi : the errors/patch is in bug 1126499 which landed.
Flags: needinfo?(dflanagan)
No longer blocks: 1111890
Flags: needinfo?(dflanagan)
(In reply to Botond Ballo [:botond] from comment #28) > (In reply to [Away: 1/29-2/20] from comment #10) > > > Any suggestions for how to deal with not having snprintf on Windows? Do we > > > have a suitable polyfill for it? > > > > Use PR_snprintf with the pattern used in bug 1119776, since MSVC 2015 has > > added support for it (finally!) > > Implemented this. Try push: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=13c550fa0a65 Had to make a few adjustments to get the patch to build successfully. Green build-only Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c23c6ef8fd5 Daniel, would you be able to review this? I usually ask Ehsan for things like this, but he's on vacation.
Attachment #8546916 - Attachment is obsolete: true
Attachment #8558311 - Flags: review?(dholbert)
Comment on attachment 8558311 [details] [diff] [review] Part 2 - Use 'snprintf' instead of 'sprintf' Thanks for doing this! Hooray for switching to buffer-length-safe functions. >diff --git a/dom/ipc/CrashReporterParent.cpp b/dom/ipc/CrashReporterParent.cpp >--- a/dom/ipc/CrashReporterParent.cpp >+++ b/dom/ipc/CrashReporterParent.cpp > char startTime[32]; >- sprintf(startTime, "%lld", static_cast<long long>(mStartTime)); >+ // need to disambiguate between libc's snprintf and base::snprintf >+ base::snprintf(startTime, 32, "%lld", static_cast<long long>(mStartTime)); [...] >diff --git a/dom/media/AudioStream.cpp b/dom/media/AudioStream.cpp >--- a/dom/media/AudioStream.cpp >+++ b/dom/media/AudioStream.cpp > char buf[100]; >- sprintf(buf, "dumped-audio-%d.wav", gDumpedAudioCount); >+ snprintf(buf, 100, "dumped-audio-%d.wav", gDumpedAudioCount); Hmm... repeating the hardcoded magic number in each case like this seems fragile. I'd much prefer we use "ArrayLength(startTime)" & "ArrayLength(buf)" in the snprintf invocation. ArrayLength is provided by mfbt/ArrayUtils.h: http://mxr.mozilla.org/mozilla-central/source/mfbt/ArrayUtils.h#47 Would you mind rewriting to use that? (I might punt on doing a thorough review until then, because it looks like this review-comment applies to most of the patch.)
Comment on attachment 8558311 [details] [diff] [review] Part 2 - Use 'snprintf' instead of 'sprintf' (In reply to Daniel Holbert [:dholbert] from comment #32) > Hmm... repeating the hardcoded magic number in each case like this seems > fragile. I'd much prefer we use "ArrayLength(startTime)" & > "ArrayLength(buf)" in the snprintf invocation. > > ArrayLength is provided by mfbt/ArrayUtils.h: > http://mxr.mozilla.org/mozilla-central/source/mfbt/ArrayUtils.h#47 > > Would you mind rewriting to use that? (I might punt on doing a thorough > review until then, because it looks like this review-comment applies to most > of the patch.) Will do, thanks! Dropping review flag until I have an updated patch.
Attachment #8558311 - Flags: review?(dholbert)
This fix accidentally snuck into my snprintf patch. Splitting out for separate review.
Attachment #8558311 - Attachment is obsolete: true
Attachment #8562896 - Flags: review?(sotaro.ikeda.g)
And here's the updated snprintf patch, with much ArrayLength goodness. Green build-only Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57d85f00860
Attachment #8562897 - Flags: review?(dholbert)
Comment on attachment 8562897 [details] [diff] [review] Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds >+++ b/dom/ipc/CrashReporterParent.cpp [...] >- sprintf(startTime, "%lld", static_cast<long long>(mStartTime)); >+ // need to disambiguate between libc's snprintf and base::snprintf >+ base::snprintf(startTime, ArrayLength(startTime), "%lld", static_cast<long long>(mStartTime)); As discussed in IRC -- here, it looks like we're using the "base::" version simply because "snprintf" on its own causes a compile error in this spot, and there's no way (?) to resolve the ambiguity in favor of the libc version (as long as we have "using namespace base"). This feels like a bad reason to choose the "base::" version. (Maybe the base:: version is fine, but I'm not comfortable using it without auditing it more thoroughly -- and I'm not sure it's worth doing that, if we don't have to use it.) Looks like you filed bug 1132153 to do away with the "using namespace base", which hopefully means we can use the standard version here and drop this "base::" -- nice. >diff --git a/dom/media/AudioStream.cpp b/dom/media/AudioStream.cpp >+#if defined(_MSC_VER) && _MSC_VER < 1900 >+#include <nspr/prprf.h> >+#define snprintf PR_snprintf >+#endif Per tromey/froydnj's notes in IRC, I'm not sure it's a good idea to apply this #define across the board. As tromey says: <tromey> like for nspr, "%l" means "take a 64 bit int" <tromey> but for ordinary printf, "%l" means "take a long" ...and this could result in subtle bugs, where we'd be implicitly saying something different for win32-MSVC vs. win64-MSVC vs. other platforms. Given that this is a mass-conversion, it seems possible/likely that this patch could introduce such a bug. So, to avoid that risk, we probably need to use a more standard-printf-flavored polyfill, if at all possible... (Maybe base::snprintf could even be a solution :) though I'm not sure without having audited it.) Also: given that I was initially completely unaware of the MSVC/nspr compat issues with snprintf, I don't really feel comfortable being the person to r+ this. I'd defer to froydnj or tromey (or someone else of your choice), if they're up for reviewing.
Attachment #8562897 - Flags: review?(dholbert) → feedback+
(In reply to Daniel Holbert [:dholbert] from comment #36) > Comment on attachment 8562897 [details] [diff] [review] > Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on > Lollipop-based builds > > >+++ b/dom/ipc/CrashReporterParent.cpp > [...] > >- sprintf(startTime, "%lld", static_cast<long long>(mStartTime)); > >+ // need to disambiguate between libc's snprintf and base::snprintf > >+ base::snprintf(startTime, ArrayLength(startTime), "%lld", static_cast<long long>(mStartTime)); > > As discussed in IRC -- here, it looks like we're using the "base::" version > simply because "snprintf" on its own causes a compile error in this spot, > and there's no way (?) to resolve the ambiguity in favor of the libc version > (as long as we have "using namespace base"). This feels like a bad reason to > choose the "base::" version. (Maybe the base:: version is fine, but I'm not > comfortable using it without auditing it more thoroughly -- and I'm not sure > it's worth doing that, if we don't have to use it.) You should be able to use ::snprintf to favor libc's.
Attachment #8562896 - Flags: review?(sotaro.ikeda.g) → review+
Botond, Now that this is r+'d what else is needed to land this for 2.2? Thanks, Mike
Flags: needinfo?(botond)
(In reply to Mike Lee [:mlee] from comment #38) > Now that this is r+'d what else is needed to land this for 2.2? The temporary fix (disable -Werror on Lollipop) has already landed on 2.2 (see comment 29). For the proper fix (re-enable -Werror and fix the warnings on Lollipop), I still need to address review comments for the Part 3 patch. I don't think I'll be uplifting that to 2.2 - I don't see much value in it. (I'm open to being convinced otherwise.)
Flags: needinfo?(botond)
All right, after a lot of fighting with MSVC, here's a version that, on MSVC versions that don't provide 'snprintf', delegates to 'vsnprintf', which they do provide. The nice thing about this approach is that it doesn't require using a macro to do the wrapping. Nathan, would you be able to review this?
Attachment #8562897 - Attachment is obsolete: true
Attachment #8571150 - Flags: review?(nfroyd)
(bump to v2.2? as the temp fix provided in comment 29 is sufficient over here, and comment 39 questions Moz's fortitude for an uplift)
No longer blocks: CAF-v2.2-metabug
blocking-b2g: 2.2+ → 2.2?
Whiteboard: [caf priority: p2][CR 786383]
triage: per comment 41, it's working fine so no longer blocking.
blocking-b2g: 2.2? → backlog
Calling v2.2 as fixed as it's likely every going to be.
Comment on attachment 8571150 [details] [diff] [review] Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds Review of attachment 8571150 [details] [diff] [review]: ----------------------------------------------------------------- If we're going to polyfill snprintf, we should add in a template overload that automagically computes the |const char[]| buffer length, so we can dispense with all the ArrayLength boilerplate. WDYT? (I guess that would have to be mozilla::snprintf, which could get confusing...) Could you file a followup for adding |#pragma GCC poison sprintf| to mozilla-config.h.in? (I'm not entirely sure we can get away with this, but it'd be excellent if we could.) ::: dom/media/AudioStream.cpp @@ +21,5 @@ > #include "nsPrintfCString.h" > #ifdef XP_MACOSX > #include <sys/sysctl.h> > #endif > +#include "mozilla/Snprintf.h" Nit: why not move this up with the rest of the mozilla/ includes? ::: gfx/thebes/gfxFontUtils.cpp @@ +25,5 @@ > > #include "plbase64.h" > #include "prlog.h" > > + Nit: added blank line? ::: gfx/thebes/gfxHarfBuzzShaper.cpp @@ +19,5 @@ > #include "harfbuzz/hb-ot.h" > > #include <algorithm> > > + Nit: added blank line? @@ +817,5 @@ > #if DEBUG > { > char buf[1024]; > + snprintf(buf, ArrayLength(buf), "unknown kern subtable in %s: " > + "ver 0 format %d\n", Please fixup the indentation here. @@ +879,5 @@ > #if DEBUG > { > char buf[1024]; > + snprintf(buf, ArrayLength(buf), "unknown kern subtable in %s: " > + "ver 0 format %d\n", ...and here. ::: media/gmp-clearkey/0.1/openaes/oaes_lib.c @@ +32,5 @@ > #include <stddef.h> > #include <time.h> > #include <string.h> > > +#include "mozilla/Snprintf.h" This looks like third-party code; please ask somebody on the media team to OK this. ::: tools/profiler/LulDwarf.cpp @@ +54,2 @@ > #include "mozilla/Assertions.h" > +#include "mozilla/Snprintf.h" This is all Android/Linux-only code, so we shouldn't need the polyfill here. @@ +1893,5 @@ > if (reg == return_address_) > return ustr__ZDra(); > > char buf[30]; > + snprintf(buf, 30, "dwarf_reg_%u", reg); ArrayLength(buf)? ::: tools/profiler/LulMain.cpp @@ +15,5 @@ > > #include "mozilla/Assertions.h" > #include "mozilla/ArrayUtils.h" > #include "mozilla/MemoryChecking.h" > +#include "mozilla/Snprintf.h" This is all Android/Linux-only code, so we shouldn't need the polyfill here.
Attachment #8571150 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #44) > If we're going to polyfill snprintf, we should add in a template overload > that automagically computes the |const char[]| buffer length, so we can > dispense with all the ArrayLength boilerplate. WDYT? (I guess that would > have to be mozilla::snprintf, which could get confusing...) I agree that this would be nice. I'll give it a try. I addressed all other review comments locally. > ::: media/gmp-clearkey/0.1/openaes/oaes_lib.c > @@ +32,5 @@ > > #include <stddef.h> > > #include <time.h> > > #include <string.h> > > > > +#include "mozilla/Snprintf.h" > > This looks like third-party code; please ask somebody on the media team to > OK this. Edwin, does this change to oaes_lib.c look OK to you?
Flags: needinfo?(edwin)
Blocks: 1141227
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #44) > Could you file a followup for adding |#pragma GCC poison sprintf| to > mozilla-config.h.in? (I'm not entirely sure we can get away with this, but > it'd be excellent if we could.) Filed bug 1141227.
(In reply to Botond Ballo [:botond] from comment #45) > > ::: media/gmp-clearkey/0.1/openaes/oaes_lib.c > > @@ +32,5 @@ > > > #include <stddef.h> > > > #include <time.h> > > > #include <string.h> > > > > > > +#include "mozilla/Snprintf.h" > > > > This looks like third-party code; please ask somebody on the media team to > > OK this. > > Edwin, does this change to oaes_lib.c look OK to you? Sure.
Flags: needinfo?(edwin)
blocking-b2g: backlog → ---
(In reply to Botond Ballo [:botond] from comment #45) > (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #44) > > If we're going to polyfill snprintf, we should add in a template overload > > that automagically computes the |const char[]| buffer length, so we can > > dispense with all the ArrayLength boilerplate. WDYT? (I guess that would > > have to be mozilla::snprintf, which could get confusing...) > > I agree that this would be nice. I'll give it a try. Updated patch to introduce the template overload, and to call it where possible. Requesting review again as this is a significant change compared to the previous version.
Attachment #8571150 - Attachment is obsolete: true
Attachment #8579403 - Flags: review?(nfroyd)
Comment on attachment 8579403 [details] [diff] [review] Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds Review of attachment 8579403 [details] [diff] [review]: ----------------------------------------------------------------- Ah, nice, I didn't realize you could overload snprintf like that...seems kind of weird that one of the overloads has |extern "C"| linkage and the rest of them don't, but maybe that all just works out in the end. I'm inclined to call this something else, though I'm not attached to a particular name. snprintf_literal? Kind of long. lsnprintf ("literal snprintf")? Suggestions? ::: intl/locale/nsScriptableDateFormat.cpp @@ +11,5 @@ > #include "nsCOMPtr.h" > #include "nsServiceManagerUtils.h" > > +using mozilla::ArrayLength; > + No longer necessary? ::: ipc/netd/Netd.cpp @@ +24,5 @@ > #define INVALID_SOCKET -1 > #define MAX_RECONNECT_TIMES 10 > > +using mozilla::ArrayLength; > + No longer necessary? ::: mfbt/Snprintf.h @@ +30,5 @@ > +} > +#endif > + > +// In addition, in C++ code, on all platforms, provide an overload of snprintf() > +// which uses template argument deduction to deduces the size of the buffer, Nit: "to deduce the size..." ::: mfbt/tests/TestIntegerPrintfMacros.cpp @@ +3,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#include "mozilla/ArrayUtils.h" No longer needed? ::: widget/nsBaseWidget.cpp @@ +1947,5 @@ > default: > { > char buf[32]; > > + snprintf(buf,ArrayLength(buf),"UNKNOWN: %d",aGuiEvent->message); No ArrayLength necessary?
Attachment #8579403 - Flags: review?(nfroyd) → review+
It is time to back out the temporary solution and land the right solution. Thanks -- Keven
blocking-b2g: --- → 2.2+
Flags: needinfo?(botond)
(In reply to Keven Kuo [:kkuo] from comment #50) > It is time to back out the temporary solution and land the right solution. I still need to address the latest review comments (from comment 49). I also think there's some confusion caused by the word "temporary". What I've described as the "temporary solution" is just disabling warning-as-errors. Warning-as-errors was previously disabled on all platforms; I enabled it on all platforms in the 2.2 timeframe, then realized that for Lollipop there are additional warnings to be fixed, so I disabled it for Lollipop only for 2.2, and will be enabling it for 3.0 when the patches in this bug land. I don't see any reason to pursue enabling warnings-as-errors for Lollipop for 2.2 - it's not a regression fix, so there wouldn't be a great reason to uplift.
Flags: needinfo?(botond)
Attachment #8551644 - Attachment description: [Temporary solution] disable warnings-as-errors in Lollipop-based builds → [2.2 solution] disable warnings-as-errors in Lollipop-based builds
(In reply to Botond Ballo [:botond] from comment #51) > I don't see any reason to pursue enabling warnings-as-errors for Lollipop > for 2.2 - it's not a regression fix, so there wouldn't be a great reason to > uplift. Hi! Botond, Thanks for the clarification. -- Keven
No longer blocks: gonk-L
[Blocking Requested - why for this release]:with Lollipop-based b2g build, 3.0 affected
blocking-b2g: 2.2+ → 3.0?
Where do we stand here?
Flags: needinfo?(botond)
Comment on attachment 8546914 [details] [diff] [review] Part 1 - Fix a -Wunused-function warning in AudioManager.cpp This was already fixed in bug 1134599.
Attachment #8546914 - Attachment is obsolete: true
Attachment #8562896 - Attachment description: Part 2 - Fix a -Woverloaded-virtual warning in gonk widget code → Part 1 - Fix a -Woverloaded-virtual warning in gonk widget code
Updated to address review comments from comment 49. Apologies for the delay in getting to this. > I'm inclined to call this something else, though I'm not attached to a > particular name. snprintf_literal? Kind of long. lsnprintf ("literal > snprintf")? Suggestions? I went with snprintf_literal, as we had discussed on IRC. I also had to make a couple of new sprintf -> snprintf or sprintf -> snprintf_literal changes compared to the previous patch to keep up with changes to the code, but they were in the exact same vein as the existing changes in the patch, so I'll go ahead and carry r+.
Attachment #8579403 - Attachment is obsolete: true
Flags: needinfo?(botond)
Attachment #8609509 - Flags: review+
blocking-b2g: 3.0? → 3.0+
Attachment #8609510 - Flags: review?(mh+mozilla) → review+
(Looks like the warning (treated as error) in comment 60 is for ssltunnel.cpp, which is only FAIL_ON_WARNINGS as of a few days ago, in bug 1167250.)
Indeed. Fixed locally, and verified that a local Lollipop build is successful.
Depends on: 1197324
Depends on: 1277155
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: