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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P2)

Description

[Iaroslav Sheptykin]

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`

Comment 1

[Jan Beich]

--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 2

[Iaroslav Sheptykin]

Attachment: Bug 1371927: Build KeyedStackCapture only if profiler enabled.

Review commit: https://reviewboard.mozilla.org/r/147826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147826/

Comment 3

[Iaroslav Sheptykin]

Thanks for the feedback, Jan!

Could you guys share your .mozconfig files here so I can catch problems like these faster in the future?

Comment 4

[Jan Beich]

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

Comment 5

[Iaroslav Sheptykin]

Thanks, Jan! I am trying to build the patch with stubs as you suggested.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

https://reviewboard.mozilla.org/r/147826/#review152248

Comment 7

[Landry Breuil (:gaston)]

Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

fixes the build for me on OpenBSD

Comment 8

[Iaroslav Sheptykin]

Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147826/diff/1-2/

Comment 9

[Iaroslav Sheptykin]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86bec347e28b
Build KeyedStackCapture only if profiler enabled. r=gfritzsche

Comment 11

[Georg Fritzsche [:gfritzsche]]

Landry, please leave setting checkin-needed to the developers working on the patch.
The author here didn't want to land it yet.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8876475 [details]
Bug 1371927: Build KeyedStackCapture only if profiler enabled.

https://reviewboard.mozilla.org/r/147826/#review152324

Comment 13

[Iaroslav Sheptykin]

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?

Comment 14

[Landry Breuil (:gaston)]

(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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86bec347e28b

Comment 16

[Jan Beich]

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

Comment 17

[Georg Fritzsche [:gfritzsche]]

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/

Comment 18

[Georg Fritzsche [:gfritzsche]]

(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

[Jan Beich]

(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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/5293e5f89358
Backed out changeset 86bec347e28b on request from gfritsche

Comment 21

[Iaroslav Sheptykin]

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?

Comment 22

[Iaroslav Sheptykin]

The patch applies cleanly onto latest mozilla-beta. Is there any other action needed?

Comment 23

[Georg Fritzsche [:gfritzsche]]

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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2026f8b291
Build KeyedStackCapture only if profiler enabled. r=gfritzsche

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6d2026f8b291

Comment 26

[Iaroslav Sheptykin]

Attachment: 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.

Comment 27

[Iaroslav Sheptykin]

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 28

[Julien Cristau [:jcristau]]

Comment on attachment 8877993 [details] [diff] [review]
patch for Bug 1371927 rebased on beta

tier-3 build fix, beta55+

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d0e950f01dd

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8877993 [details] [diff] [review]
patch for Bug 1371927 rebased on beta

Dropping now obsolete, empty feedback flag.