Closed Bug 1371927 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Telemetry, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: yarik.sheptykin, Assigned: yarik.sheptykin, Mentored)

References

Details

(Whiteboard: [measurement:client])

Attachments

(2 files)

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: nobody → yarik.sheptykin
--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.
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)
Summary: Telemetry does not build without profiler → Telemetry does not build without profiler or with warnings-as-errors
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)
Thanks, Jan! I am trying to build the patch with stubs as you suggested.
Flags: needinfo?(landry)
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
Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

fixes the build for me on OpenBSD
Attachment #8876475 - Flags: feedback+
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)
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 on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

https://reviewboard.mozilla.org/r/147826/#review152324
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.
https://hg.mozilla.org/mozilla-central/rev/86bec347e28b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
(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?
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/5293e5f89358
Backed out changeset 86bec347e28b on request from gfritsche
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)
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)
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
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
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?
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 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.