Closed Bug 1347661 Opened 3 years ago Closed 3 years ago

toolkit/components/telemetry fails to build --enable-warnings-as-errors

Categories

(Toolkit :: Telemetry, defect)

Unspecified
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Tier3 platforms don't support Gecko Profiler, so more stuff end up unused. On FreeBSD Clang is the default compiler but I'm using GCC to catch different set of warnings within Gecko.

$ echo "ac_add_options --enable-warnings-as-errors" >>.mozconfig
[...]
toolkit/components/telemetry/Telemetry.cpp:302:1: error: 'void {anonymous}::HangReports::AddHang(const mozilla::Telemetry::ProcessedStack&, uint32_t, int32_t, int32_t, mozilla::HangMonitor::HangAnnotationsPtr)' defined but not used [-Werror=unused-function]
 HangReports::AddHang(const Telemetry::ProcessedStack& aStack,
 ^~~~~~~~~~~
toolkit/components/telemetry/Telemetry.cpp:233:1: error: 'void {anonymous}::CombinedStacks::Clear()' defined but not used [-Werror=unused-function]
 CombinedStacks::Clear() {
 ^~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
toolkit/components/telemetry/Telemetry.cpp:348:1: error: 'void {anonymous}::HangReports::PruneStackReferences(size_t)' defined but not used [-Werror=unused-function]
 HangReports::PruneStackReferences(const size_t aRemovedStackIndex) {
 ^~~~~~~~~~~
Depends on: 1330359
Assignee: nobody → jbeich
Comment on attachment 8847740 [details]
Bug 1347661 - Unbreak --enable-warnings-as-errors on platforms without Gecko Profiler.

https://reviewboard.mozilla.org/r/120664/#review123610
Attachment #8847740 - Flags: review?(gfritzsche) → review+
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/autoland/rev/874293ae98ea
Unbreak --enable-warnings-as-errors on platforms without Gecko Profiler. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/874293ae98ea
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8847740 [details]
Bug 1347661 - Unbreak --enable-warnings-as-errors on platforms without Gecko Profiler.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1225851 and maybe bug 805046 regression as part of bug 1330359 feature
[User impact if declined]: Broken build with --enable-warnings-as-errors on Tier3 platforms that use GCC 6 or later (Linux non-x86, DragonFly, NetBSD, Solaris-like and maybe Android aarch64).
[Is this code covered by automated tests?]: Probably by unit/test_ThreadHangStats.js but only on Tier1 platforms. For Tier3 a green build is enough in this case as telemetry and tests are often unused downstream due to lack of manpower.
[Has the fix been verified in Nightly?]: Yes, also backported to FF53.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None for GeckoProfiler. GCC 6+ on some BSDs may need bug 1347672 as well.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Can only disable stack capture and hang detection code in telemetry turning 1 xpcshell test perma orange.
[String changes made/needed]: None
Attachment #8847740 - Flags: approval-mozilla-beta?
Attachment #8847740 - Flags: approval-mozilla-aurora?
Comment on attachment 8847740 [details]
Bug 1347661 - Unbreak --enable-warnings-as-errors on platforms without Gecko Profiler.

Fix a broken build with --enable-warnings-as-errors on Tier3 platforms. Aurora54+ & Beta53+.
Attachment #8847740 - Flags: approval-mozilla-beta?
Attachment #8847740 - Flags: approval-mozilla-beta+
Attachment #8847740 - Flags: approval-mozilla-aurora?
Attachment #8847740 - Flags: approval-mozilla-aurora+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f97ebbae0deea8ba9e0e0d8e7ad0407babc7bf8f

s/MOZ_GECKO_PROFILER/MOZ_ENABLE_PROFILER_SPS/ as FF53 lacks mozilla-central changeset 8a1b2dbf2f14, so it uses the old name. Do I need to request another approval?
Flags: needinfo?(jbeich)
Jan, since this is a huge change can you request approval again ?
Flags: needinfo?(jbeich)
Comment on attachment 8850665 [details] [diff] [review]
Rebased for Firefox 53

comment 6 focused too much on Tier3 and forgot to test build on Tier1 platforms.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1225851 and maybe bug 805046 regression as part of bug 1330359 feature
[User impact if declined]: Broken build with --enable-warnings-as-errors on Tier3 platforms that use GCC 6 or later (Linux non-x86, DragonFly, NetBSD, Solaris-like and maybe Android aarch64).
[Is this code covered by automated tests?]: Probably by unit/test_ThreadHangStats.js but only on Tier1 platforms. For Tier3 a green build is enough in this case as telemetry and tests are often unused downstream due to lack of manpower.
[Has the fix been verified in Beta?]: Yes. See also Try in comment 11.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None for GeckoProfiler. GCC 6+ on some BSDs may need bug 1347672 as well.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Can only break build (more likely) or turn 1 telemetry xpcshell test perma orange.
[String changes made/needed]: None
Attachment #8850665 - Attachment description: Unbreak --enable-warnings-as-errors on platforms without Gecko Profiler (Firefox 53 version) → Rebased for Firefox 53
Flags: needinfo?(jbeich)
Attachment #8850665 - Flags: approval-mozilla-beta?
Comment on attachment 8850665 [details] [diff] [review]
Rebased for Firefox 53

Rebased patches don't need re-approval unless they're substantially different (i.e. would change the answer to any of the original approval questions) from before.
Attachment #8850665 - Flags: approval-mozilla-beta?
Blocks: buildwarning
You need to log in before you can comment on or make changes to this bug.