1242724 - Make mtransport/runnable_utils.h available even if WebRTC is disabled

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

We've had multiple bugs for --disable-webrtc builds, of the form:
{
[some .cpp file]: fatal error: 'mtransport/runnable_utils.h' file not found
}

As noted in bug 1242719 comment 1, we should just make this header available in non-WebRTC builds.

Comment 1

[Daniel Holbert [:dholbert]]

This is currently causing two separate instances of build bustage: bug 1241628, bug 1242719 (both marked as dependencies).

Also, there are at least 2 old bugs for the same sort of build bustage, where we worked around this: bug 1070966, bug 1070966.

Note that ekr expressed cautious approval for exposing this header publicly, in bug 1070966 comment 4:
> I don't have a problem with non-WebRTC use of runnable_utils.h. With that
> said, it's a sharp tool, so some care is in order.

Comment 2

[Jacek Caban]

Attachment: fix

The fix is attached.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8713983 [details] [diff] [review]
fix

I'm not really an appropriate reviewer for this, but jesup might be.

Comment 4

[Daniel Holbert [:dholbert]]

(and: thanks for the patch!)

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8713983 [details] [diff] [review]
fix

Review of attachment 8713983 [details] [diff] [review]:
-----------------------------------------------------------------

Needs build-peer review.

We should look to move it (and perhaps fiddle with the API if it makes sense) to mfbt

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8713983 [details] [diff] [review]
fix

Review of attachment 8713983 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to me moving this header to e.g. xpcom would be way easier.

Comment 7

[Mike Hommey [:glandium]]

I'll note that it's not included consistently, either. Some places #include "mtransport/runnable_utils.h", others #include "runnable_utils.h".

Comment 8

[Mats Palmgren (inactive)]

Can we land what we have now to unbreak the builds for a number of developers?
We can polish it later in follow-up bug(s) IMO.

Comment 9

[Mike Hommey [:glandium]]

I'd rather go with putting EXPORTS.mtransport += ['relpath/to/runnable_utils.h'] in some other directory as a temporary solution than the current patch.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: add an export of runnable_utils.h unless/until it gets moved to xpcom/mfbt

MozReview-Commit-ID: C577TmkWaWE

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81228410543e

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81228410543e

Comment 14

[Mike Hommey [:glandium]]

This affects 46. Can we get an uplift?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8718932 [details] [diff] [review]
add an export of runnable_utils.h unless/until it gets moved to xpcom/mfbt

Approval Request Comment
[Feature/regressing bug #]: webrtc.org update

[User impact if declined]: --disable-webrtc won't work

[Describe test coverage new/current, TreeHerder]: N/A

[Risks and why]: No risk; merely exposes header file when not building webrtc

[String/UUID change made/needed]: none

Comment 17

[Landry Breuil (:gaston)]

I support this being backported to beta, as 46.0b1 failed to build for me on OpenBSD where we still disable webrtc...

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 46 as affected.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8718932 [details] [diff] [review]
add an export of runnable_utils.h unless/until it gets moved to xpcom/mfbt

Build config change so we can build without webrtc in 46.
This should end up in beta 4.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/91661ec6d973