Closed Bug 1437345 Opened 7 years ago Closed 7 years ago

Firefox build failed with disable-pulseaudio and enable-alsa

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Version:
60 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 - fixed
firefox61 --- fixed

People

(Reporter: drJeckyll, Assigned: dminor)

References

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Bug 1437345 - Set defaults for gn visual studio build;
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1437345 - Don't try to build audio_device backends.
59 bytes, text/x-review-board-request
dminor
: review+
Details
Bug 1437345 - Update generated files;
59 bytes, text/x-review-board-request
jesup
: review+
jbeich
: feedback+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180210220139

Steps to reproduce:

Building mozilla-central fail with:
--disable-pulseaudio
--enable-alsa



Actual results:

 0:04.40 In file included from /build/firefox/mozilla-central/media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_mixer_manager_pulse_linux.h:20:0,
 0:04.40                  from /build/firefox/mozilla-central/media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_pulse_linux.h:19,
 0:04.40                  from /build/firefox/mozilla-central/media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc:16:
 0:04.40 /build/firefox/mozilla-central/firefox-build-dir/dist/system_wrappers/pulse/pulseaudio.h:3:15: fatal error: pulse/pulseaudio.h: No such file or directory
 0:04.40  #include_next <pulse/pulseaudio.h>
 0:04.40                ^~~~~~~~~~~~~~~~~~~~
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Component: Untriaged → Build Config
Product: Firefox → Core
Product: Core → Firefox Build System
media.navigator.audio.full_duplex=false is broken after bug 1397793, so building audio_device backends is pointless. Pretty sure this has regressed since gyp->gn conversion as WebRTC no longer propagates --disable-pulseaudio as include_pulse_audio=0.

https://searchfox.org/mozilla-central/rev/8fa0b32c84f9/media/webrtc/trunk/webrtc/modules/audio_device/BUILD.gn#166
Blocks: 1393119
status-firefox59: --- → unaffected
status-firefox60: --- → affected
status-firefox61: --- → affected
status-firefox-esr52: --- → unaffected
Component: General → WebRTC
Product: Firefox Build System → Core
Keywords: regression
Bug 1437670 didn't bother porting audio_device bits from gyp to gn, so --disable-pulseaudio works either way. For Tier1 platforms someone has to update configs and push to Try while watching dom/media/ tests.
I can handle updating the configs for tier1 platforms.
Assignee: nobody → dminor
Comment on attachment 8958911 [details]
Bug 1437345 - Don't build audio_device after bug 1437345.

https://reviewboard.mozilla.org/r/227778/#review233568

::: media/webrtc/trunk/webrtc/modules/audio_device/BUILD.gn:109
(Diff revision 1)
>    if (is_android) {
>      include_dirs += [ "android" ]
>    }
>    defines = []
>    cflags = []
>    if (rtc_include_internal_audio_device) {

Oops, I probably need to disable this one instead.
Mechanic rebase:

  $ sed -i '' -e '/audio_device_impl/d' \
	-e '/WEBRTC_INCLUDE_INTERNAL_AUDIO_DEVICE/d' \
	-e 's/WEBRTC_DUMMY_FILE_DEVICES/WEBRTC_DUMMY_AUDIO_BUILD/' \
    *dragonfly* *bsd*

After regenerating mozbuild getUserMedia demos still work fine.
Attachment #8958918 - Attachment is obsolete: true
Blocks: 1445762
No longer blocks: 1445762
Status: UNCONFIRMED → NEW
Rank: 15
Ever confirmed: true
Priority: -- → P2
Comment on attachment 8960599 [details]
Bug 1437345 - Don't try to build audio_device backends.

https://reviewboard.mozilla.org/r/229358/#review235098

lgtm
Attachment #8960599 - Flags: review?(dminor) → review+
Comment on attachment 8958911 [details]
Bug 1437345 - Don't build audio_device after bug 1437345.

https://reviewboard.mozilla.org/r/227778/#review235100

I'll do the review on my patch set which includes the generated files.
Attachment #8958911 - Flags: review?(dminor)
Jan, please redo the BSD configurations using my version of the patch. As you pointed out, you had accidentally set the wrong flag in your patch, which I've fixed in my version.
Flags: needinfo?(jbeich)
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dd85ae359c09965b3be4064968fd5695f638972

I've also tested calls on appear.in with Linux, Windows, OS X and Android. Since we're using a fake device anyway this change should have no effect, but I was concerned about the possibility of some sort of initialization code still running. During the last webrtc.org update, I noticed that the Android initialization code could cause problems for cubeb.
Comment on attachment 8960598 [details]
Bug 1437345 - Set defaults for gn visual studio build;

https://reviewboard.mozilla.org/r/229356/#review235148

::: media/webrtc/trunk/build/config/win/visual_studio_version.gni:13
(Diff revision 1)
>    # visual_studio_version and wdk_path.
> -  visual_studio_path = ""
> +  visual_studio_path = "."
>  
>    # Version of Visual Studio pointed to by the visual_studio_path.
>    # Currently always "2015".
> -  visual_studio_version = ""
> +  visual_studio_version = "2015"

Is there a way we can connect this to the 'master' setting for VS in our build system?  Or something to flag if this becomes out of date.
Attachment #8960598 - Flags: review?(rjesup) → review+
Comment on attachment 8960599 [details]
Bug 1437345 - Don't try to build audio_device backends.

https://reviewboard.mozilla.org/r/229358/#review235190

::: media/webrtc/trunk/webrtc/modules/audio_device/BUILD.gn:137
(Diff revision 1)
>        libs = [
>          "log",
>          "OpenSLES",
>        ]
>  
>        if (build_with_mozilla) {

Why did you keep this? It's no longer used by Gecko and should be reverted upstream.
Comment on attachment 8960600 [details]
Bug 1437345 - Update generated files;

(In reply to Dan Minor [:dminor] from comment #13)
> Jan, please redo the BSD configurations using my version of the patch.

No changes since attachment 8958956 [details] [diff] [review]. Why did you apply previous version?

> As you pointed out, you had accidentally set the wrong flag in your patch,
> which I've fixed in my version.

Hmm, MozReview didn't update existing patch even with the same MozReview-Commit-ID as it was created by a different user. This means I can't reupload attachment 8960600 [details] after fixing BSD gn-configs for you.
Flags: needinfo?(jbeich)
Attachment #8960600 - Flags: feedback-
Comment on attachment 8958911 [details]
Bug 1437345 - Don't build audio_device after bug 1437345.

Obsoleted by attachment 8960599 [details].
Attachment #8958911 - Attachment is obsolete: true
(In reply to Jan Beich from comment #17)
> Comment on attachment 8960600 [details]
> Bug 1437345 - Update generated files;
> 
> (In reply to Dan Minor [:dminor] from comment #13)
> > Jan, please redo the BSD configurations using my version of the patch.
> 
> No changes since attachment 8958956 [details] [diff] [review]. Why did you
> apply previous version?
> 
> > As you pointed out, you had accidentally set the wrong flag in your patch,
> > which I've fixed in my version.
> 
> Hmm, MozReview didn't update existing patch even with the same
> MozReview-Commit-ID as it was created by a different user. This means I
> can't reupload attachment 8960600 [details] after fixing BSD gn-configs for
> you.

I'm sorry, I must have grabbed the old version by accident.
Comment on attachment 8960600 [details]
Bug 1437345 - Update generated files;

(In reply to Jan Beich from comment #17)
> (In reply to Dan Minor [:dminor] from comment #13)
> > Jan, please redo the BSD configurations using my version of the patch.
> 
> No changes since attachment 8958956 [details] [diff] [review]. Why did you apply previous version?

Ah, BSD bits weren't applied at all. ;) In that case merging them with this commit should be enough. Gecko-specific NSPR conditional removal (comment 16) had no impact on gn-configs after the branch was disabled.
Attachment #8960600 - Flags: feedback-
Comment on attachment 8960599 [details]
Bug 1437345 - Don't try to build audio_device backends.

https://reviewboard.mozilla.org/r/229358/#review235190

> Why did you keep this? It's no longer used by Gecko and should be reverted upstream.

I'll drop all of our local changes as part of the next upstream merge which is already in progress.
I've merged the BSD generated files into the last commit.
Comment on attachment 8958956 [details] [diff] [review]
gn-configs for BSDs (depends on bug 1437670)

Forgot to mention "./mach build-backend -b GnMozbuildWriter" isn't included in this patch partially to avoid merge conflicts partially as it can be done on any machine.

Dan, can you update attachment 8960600 [details] again?
Doing that right now, I noticed that right after I left my last comment :/
Comment on attachment 8960600 [details]
Bug 1437345 - Update generated files;

My GnMozbuildWriter diff has some noise[1] but, otherwise, WebRTC builds/works fine. Thanks!

[1] https://clbin.com/UUJWs
Attachment #8960600 - Flags: feedback+
Attachment #8958956 - Attachment is obsolete: true
Would be nice to have on ESR60.

[Tracking Requested - why for this release]:
Minimalistic Linux distributions (e.g., Alpine, CRUX, Void) may want to build with --disable-pulseaudio. Other Linux users sometimes use the build flag to debug/workaround audio issues. For Tier1 configuration there should be no impact or it may improve stability by making sure WebRTC only uses libcubeb.
tracking-firefox60: --- → ?
I don't think this needs tracking, though we can consider an uplift if requested after this is sorted in nightly.
status-firefox60: affected → fix-optional
tracking-firefox60: ? → -
Comment on attachment 8960600 [details]
Bug 1437345 - Update generated files;

https://reviewboard.mozilla.org/r/229360/#review241882

rs=me.  Takes so long to load all the diffs I started and forgot to come back and r+ several times.....  I didn't manually review, note.
Attachment #8960600 - Flags: review?(rjesup) → review+
Dan, do you plan to land it with some time to spare for QA and/or request an approval for Firefox 60?
Flags: needinfo?(dminor)
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f4f7d7fcdf
Set defaults for gn visual studio build; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/280546364870
Don't try to build audio_device backends. r=dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3905bcf114
Update generated files; r=jesup
Pushed to inbound. If it looks good there, I'll request uplift.
Flags: needinfo?(dminor)
https://hg.mozilla.org/mozilla-central/rev/f5f4f7d7fcdf
https://hg.mozilla.org/mozilla-central/rev/280546364870
https://hg.mozilla.org/mozilla-central/rev/cd3905bcf114
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8960598 [details]
Bug 1437345 - Set defaults for gn visual studio build;

This applies to all three patches on this bug:
Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1393119, which switched from using gyp to gn to build webrtc.
[User impact if declined]:
This does not impact end users, but as mentioned in comment 30, this will make life easier for people who maintain Firefox on non-Tier 1 platforms.
[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]:
None.
[Is the change risky?]:
Low risk.
[Why is the change risky/not risky?]:
This is a build system only change that stops building code which is currently unused anyway.
[String changes made/needed]:
None.
Attachment #8960598 - Flags: approval-mozilla-beta?
Comment on attachment 8960598 [details]
Bug 1437345 - Set defaults for gn visual studio build;

We're going to land this a=npotb.
Attachment #8960598 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: