Firefox build failed with disable-pulseaudio and enable-alsa

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
normal
Rank:
15
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: drJeckyll, Assigned: dminor)

Tracking

({regression})

60 Branch
mozilla61
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60- fixed, firefox61 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

a year ago
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                ^~~~~~~~~~~~~~~~~~~~
Reporter

Updated

a year ago
OS: Unspecified → Linux
Hardware: Unspecified → x86_64

Updated

a year ago
Component: Untriaged → Build Config
Product: Firefox → Core

Updated

a year ago
Product: Core → Firefox Build System

Comment 1

a year ago
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

Updated

a year ago
Component: General → WebRTC
Product: Firefox Build System → Core

Updated

a year ago
Keywords: regression
Comment hidden (mozreview-request)

Comment 3

a year ago
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.
Assignee

Comment 4

a year ago
I can handle updating the configs for tier1 platforms.
Assignee: nobody → dminor

Comment 5

a year ago
mozreview-review
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.
Comment hidden (mozreview-request)

Comment 7

a year ago
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

Updated

a year ago
Blocks: 1445762

Updated

a year ago
No longer blocks: 1445762
Status: UNCONFIRMED → NEW
Rank: 15
Ever confirmed: true
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

a year ago
mozreview-review
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+
Assignee

Comment 12

a year ago
mozreview-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)
Assignee

Comment 13

a year ago
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)
Assignee

Comment 14

a year ago
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 15

a year ago
mozreview-review
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 16

a year ago
mozreview-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 17

a year ago
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 18

a year ago
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
Assignee

Comment 19

a year ago
(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 20

a year ago
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-
Assignee

Comment 21

a year ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 25

a year ago
I've merged the BSD generated files into the last commit.

Comment 26

a year ago
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?
Assignee

Comment 27

a year ago
Doing that right now, I noticed that right after I left my last comment :/
Comment hidden (mozreview-request)

Comment 29

a year ago
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+

Updated

a year ago
Attachment #8958956 - Attachment is obsolete: true

Comment 30

a year ago
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.
I don't think this needs tracking, though we can consider an uplift if requested after this is sorted in nightly.

Comment 32

a year ago
mozreview-review
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+

Comment 33

a year ago
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)

Comment 34

a year ago
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
Assignee

Comment 35

a year ago
Pushed to inbound. If it looks good there, I'll request uplift.
Flags: needinfo?(dminor)
Assignee

Comment 37

a year ago
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.