Closed Bug 1242724 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: General, defect)

46 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: jesup)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
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.
Summary: Make runnable_utils.h available even if WebRTC is disabled → Make mtransport/runnable_utils.h available even if WebRTC is disabled
Attached patch fixSplinter Review
The fix is attached.
Assignee: nobody → jacek
Attachment #8713983 - Flags: review?(dholbert)
Comment on attachment 8713983 [details] [diff] [review]
fix

I'm not really an appropriate reviewer for this, but jesup might be.
Attachment #8713983 - Flags: review?(dholbert) → review?(rjesup)
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
Attachment #8713983 - Flags: review?(rjesup)
Attachment #8713983 - Flags: review?(mh+mozilla)
Attachment #8713983 - Flags: review+
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.
Attachment #8713983 - Flags: review?(mh+mozilla) → review-
I'll note that it's not included consistently, either. Some places #include "mtransport/runnable_utils.h", others #include "runnable_utils.h".
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.
Flags: needinfo?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
MozReview-Commit-ID: C577TmkWaWE
Attachment #8718932 - Flags: review?(mh+mozilla)
Assignee: jacek → rjesup
Status: NEW → ASSIGNED
Attachment #8718932 - Flags: review?(mh+mozilla) → review+
Duplicate of this bug: 1242719
https://hg.mozilla.org/mozilla-central/rev/81228410543e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
This affects 46. Can we get an uplift?
Flags: needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
Attachment #8718932 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1241628
I support this being backported to beta, as 46.0b1 failed to build for me on OpenBSD where we still disable webrtc...
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.
Attachment #8718932 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: regression
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.