Closed Bug 1425996 Opened 6 years ago Closed 6 years ago

Various builds will be busted when Gecko 59 merges to Beta on 2018-01-11

Categories

(Core :: WebRTC: Signaling, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 + verified

People

(Reporter: RyanVM, Assigned: jwwang)

References

Details

Attachments

(1 file)

[Tracking Requested - why for this release]: Broken builds on the next merge day.

I believe MOZ_DIAGNOSTIC_ASSERT usage needs to be wrapped in MOZ_DIAGNOSTIC_ASSERT_ENABLED ifdef guards?

https://treeherder.mozilla.org/logviewer.html#?job_id=150903173&repo=try

/builds/worker/workspace/build/src/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:211:14: error: unused variable 'rv' [-Werror,-Wunused-variable]
Flags: needinfo?(amarchesini)
Rank: 11
Priority: -- → P1
The early/first merge will be Jan. 11th. 
Tracking so we can make sure to check back in early January.
JW, maybe you can help? DebugOnly isn't an option because MOZ_DIAGNOSTIC_ASSERT is used on opt builds as well. But I'm not sure exactly how to MOZ_DIAGNOSTIC_ASSERT_ENABLED ifdef my way to victory here either given how rv is being assigned in the first place.
Flags: needinfo?(jwwang)
Anyway, this is breaking nearly all the builds at the moment and actively blocking me running useful simulations :(
Severity: critical → blocker
Checking...
Attachment #8938250 - Flags: review?(gsquelart)
Assignee: nobody → jwwang
Comment on attachment 8938250 [details]
Bug 1425996 - fix build error: unused variable 'rv'.

https://reviewboard.mozilla.org/r/209002/#review214690

::: dom/media/MediaDecoder.cpp:1232
(Diff revision 1)
>    nsresult rv = mReader->OwnerThread()->Dispatch(
>      NewRunnableMethod("MediaFormatReader::NotifyDataArrived",
>                        mReader.get(),
>                        &MediaFormatReader::NotifyDataArrived));
>    MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
> +  Unused << rv;

LGTM.

But could you please check that non-unified build still works (for Chris' bug 1388605).
Attachment #8938250 - Flags: review?(gsquelart) → review+
It is sad that disabling unified-build for the entire dom/media folder causes build errors even for files not changed at all. I will land the patch and fix build errors for servo build if any.
Flags: needinfo?(jwwang)
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06a19fbe2581
fix build error: unused variable 'rv'. r=gerald
https://hg.mozilla.org/mozilla-central/rev/06a19fbe2581
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(amarchesini)
Still busted :(

https://treeherder.mozilla.org/logviewer.html#?job_id=153041955&repo=try is off current m-c tip (rev f844e99f26b1)
Status: RESOLVED → REOPENED
Flags: needinfo?(jwwang)
Resolution: FIXED → ---
Target Milestone: mozilla59 → ---
Oh, this is in the other spot in MediaPipeline.cpp. I'll try giving it the same treatment that the first location got.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Oh, this is in the other spot in MediaPipeline.cpp. I'll try giving it the
> same treatment that the first location got.

Which just moves the bustage to ChannelMediaDecoder.cpp :(
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Which just moves the bustage to ChannelMediaDecoder.cpp :(

At least that is the last file in dom/media which doesn't have the Unused << after the MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv)) calls :-)
Looks like those are indeed the only spots still needing patching. Going to land it with Gerald's previous r+.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc335a5f19e8
Add in a few more instances of Unused that got missed in the first patch. r=gerald
https://hg.mozilla.org/mozilla-central/rev/bc335a5f19e8
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(jwwang)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.