Telemetry does not build without profiler or with warnings-as-errors

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Iaroslav Sheptykin, Assigned: Iaroslav Sheptykin, Mentored)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [measurement:client])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
After Bug 1335461 landed building Telemetry.cpp fails if profiler is not enabled. See discussion here: https://bugzilla.mozilla.org/show_bug.cgi?id=1335461#c50
To fix this we need to include KeyedStackCapture.cpp in mozbuild file only if MOZ_GECKO_PROFILER is on. Also importing KeyedStacCapture symbols in Telemetry.cpp (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#102) should be guarded with `MOZ_GECKO_PROFILER`
Blocks: 1335461
(Assignee)

Updated

a year ago
Assignee: nobody → yarik.sheptykin

Comment 1

a year ago
--enable-warnings-as-errors is also busted without profiler

toolkit/components/telemetry/HangReports.cpp:15:14: error: unused variable 'kMaxHangStacksKept' [-Werror,-Wunused-const-variable]
const size_t kMaxHangStacksKept = 50;
             ^
1 error generated.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
Thanks for the feedback, Jan!

Could you guys share your .mozconfig files here so I can catch problems like these faster in the future?
Flags: needinfo?(landry)
Flags: needinfo?(jbeich)
(Assignee)

Updated

a year ago
Summary: Telemetry does not build without profiler → Telemetry does not build without profiler or with warnings-as-errors

Comment 4

a year ago
See bug 1371159. Gecko Profiler (aka SPS profiler) is only implemented on Tier1 platforms. Tier3 platforms like iOS, Linux non-x86, Solaris and BSDs end up using (incomplete) stubs. To force a Tier1 to use stubs as well just remove it from whitelist in

http://searchfox.org/mozilla-central/rev/fee636af734a/toolkit/moz.configure#43
Flags: needinfo?(jbeich)
(Assignee)

Comment 5

a year ago
Thanks, Jan! I am trying to build the patch with stubs as you suggested.
Flags: needinfo?(landry)

Comment 6

a year ago
mozreview-review
Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

https://reviewboard.mozilla.org/r/147826/#review152248
Attachment #8876475 - Flags: review?(gfritzsche) → review+
Mentor: gfritzsche
Priority: -- → P2
Keywords: checkin-needed
Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

fixes the build for me on OpenBSD
Attachment #8876475 - Flags: feedback+
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
Thanks for the feedback guys!

I just updated the patch to address comment #1 :

> --enable-warnings-as-errors is also busted without profiler
> 
> toolkit/components/telemetry/HangReports.cpp:15:14: error: unused variable
> 'kMaxHangStacksKept' [-Werror,-Wunused-const-variable]
> const size_t kMaxHangStacksKept = 50;
>              ^
> 1 error generated.

:gfrietzsche, could you take a short look on the patch again?

I've just started a local build without profiler and `warnings-as-errors` to make sure nothing breaks.
Flags: needinfo?(gfritzsche)

Comment 10

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86bec347e28b
Build KeyedStackCapture only if profiler enabled. r=gfritzsche
Keywords: checkin-needed
Landry, please leave setting checkin-needed to the developers working on the patch.
The author here didn't want to land it yet.
Flags: needinfo?(gfritzsche)

Comment 12

a year ago
mozreview-review
Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

https://reviewboard.mozilla.org/r/147826/#review152324
(Assignee)

Comment 13

a year ago
Local build without profiler and `warnings-as-errors` completed successfully.
The checked-in patch does not include the changes needed for `warntings-as-errors`. Can we re-land?
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Landry, please leave setting checkin-needed to the developers working on the
> patch.
> The author here didn't want to land it yet.

There can still be a followup, and this wasnt mentioned anywhere that the patch wasnt ready. But fine, sorry.

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86bec347e28b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 16

11 months ago
Can you backport the fix to Firefox 55?

http://buildbot.rhaalovely.net/builders/mozilla-beta-amd64/builds/34/steps/build/logs/stdio
http://buildbot.rhaalovely.net/builders/mozilla-beta-freebsd-amd64/builds/35/steps/build/logs/stdio

I confirm, Firefox 56 builds fine even (locally) with --enable-warnings-as-errors.

http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/2113
http://buildbot.rhaalovely.net/builders/mozilla-central-freebsd-amd64/builds/1218
Status: RESOLVED → VERIFIED
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(yarik.sheptykin)
I've asked for a backout, we can reland this after.

For backporting to Firefox 55, this means rebasing the patch on Firefox Beta:
https://hg.mozilla.org/releases/mozilla-beta/
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Whiteboard: [measurement:client] [measurement:client:uplift]
(In reply to Landry Breuil (:gaston) from comment #14)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> > Landry, please leave setting checkin-needed to the developers working on the
> > patch.
> > The author here didn't want to land it yet.
> 
> There can still be a followup, and this wasnt mentioned anywhere that the
> patch wasnt ready. But fine, sorry.

We should usually check with the patch author first, they would know and have plans.

Comment 19

11 months ago
(In reply to Iaroslav Sheptykin from comment #13)
> The checked-in patch does not include the changes needed for
> `warntings-as-errors`. Can we re-land?

Why did you move kMaxHangStacksKept definition under #ifdef MOZ_GECKO_PROFILER then?

Comment 20

11 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/5293e5f89358
Backed out changeset 86bec347e28b on request from gfritsche
(Assignee)

Comment 21

11 months ago
Hi guys!

Oh, what a mess I started. Sorry for not being clear on the state of the patch.

(In reply to Jan Beich from comment #19)

> Why did you move kMaxHangStacksKept definition under #ifdef
> MOZ_GECKO_PROFILER then?

You are right, the changeset landed DOES include changes needed to fix the build for warnings-as-errors. No idea how I could oversee that. Thanks for double-checking, Jan! We can land the patch again for central.

:gfritzsche, to rebase the patch I am going to:
1. hg clone https://hg.mozilla.org/releases/mozilla-beta/
2. hg import https://reviewboard-hg.mozilla.org/gecko/rev/3f1e26c28393f9877cb23ab263854bd6b481f790
3. hg push review ...

Is that correct?
Flags: needinfo?(yarik.sheptykin) → needinfo?(gfritzsche)
(Assignee)

Comment 22

11 months ago
The patch applies cleanly onto latest mozilla-beta. Is there any other action needed?
Thanks for sorting this Iaroslav, i'll trigger landing.

For getting this on beta, we first need to request approval to land it there too.
This is closer to the release population, so we weigh risks more carefully.

MozReview doesn't support this well AFAIK, so i suggest you simply put a patch for beta on this bug, then we can request beta approval on that.
Flags: needinfo?(gfritzsche)

Comment 24

11 months ago
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2026f8b291
Build KeyedStackCapture only if profiler enabled. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/6d2026f8b291
Status: REOPENED → RESOLVED
Last Resolved: a year ago11 months ago
Resolution: --- → FIXED
(Assignee)

Comment 26

11 months ago
Created attachment 8877993 [details] [diff] [review]
patch for Bug 1371927 rebased on beta

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1335461
[User impact if declined]: Tier-3 builds without profiler are broken.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just fixing compilation for tier-3 config, no functional changes.
[String changes made/needed]: None.
Attachment #8877993 - Flags: feedback?
Attachment #8877993 - Flags: approval-mozilla-beta?
(Assignee)

Comment 27

11 months ago
Hi everybody!

The Attachment #8877993 [details] [diff] is the same patch that landed on central. The patch applied without problems to beta. A test build (local) without profiler and with --warnings-as-errors completed successfully. Stack capturing on the build behaves as expected.
Comment on attachment 8877993 [details] [diff] [review]
patch for Bug 1371927 rebased on beta

tier-3 build fix, beta55+
Attachment #8877993 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 29

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4d0e950f01dd
status-firefox55: affected → fixed
Comment on attachment 8877993 [details] [diff] [review]
patch for Bug 1371927 rebased on beta

Dropping now obsolete, empty feedback flag.
Attachment #8877993 - Flags: feedback?
Whiteboard: [measurement:client] [measurement:client:uplift] → [measurement:client]
You need to log in before you can comment on or make changes to this bug.