814693 - Build failure on Debian powerpc

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Michel Dänzer]

Attachment: Fix

User Agent: Mozilla/5.0 (X11; Linux ppc; rv:17.0) Gecko/17.0 Firefox/17.0 Iceweasel/17.0
Build ID: 20121122175550



Actual results:

Firefox fails to build on Debian powerpc, see https://buildd.debian.org/status/fetch.php?pkg=iceweasel&arch=powerpc&ver=17.0-1&stamp=1353519787


Expected results:

The attached patch allows Firefox to build and run on this platform.

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 684694 [details] [diff] [review]
Fix

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

::: iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.gypi
@@ +12,5 @@
>        'target_name': 'PCM16B',
>        'type': '<(library)',
> +      'dependencies': [
> +        '<(webrtc_root)/common_audio/common_audio.gyp:signal_processing',
> +      ],

What error requires this part?

Comment 2

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

Comment on attachment 684694 [details] [diff] [review]
Fix

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

SSE2 tests need to be aligned.

::: iceweasel-17.0.orig/media/webrtc/shared_libs.mk
@@ +41,5 @@
>    $(call EXPAND_LIBNAME_PATH,yuv,$(DEPTH)/media/webrtc/trunk/third_party/libyuv/libyuv_libyuv) \
>    $(call EXPAND_LIBNAME_PATH,webrtc_jpeg,$(DEPTH)/media/webrtc/trunk/src/common_video/common_video_webrtc_jpeg) \
>    $(NULL)
> +
> +ifneq (,$(findstring 86,$(CPU_ARCH)))

Is this really the right test for sse2 support?  I wouldn't think so.  (It could be we don't have a good test, in which case this is better than nothing.)

Actually, the test should match the test used to build it in video_processing.gyp (or that test should also be changed):  it looks for a target_arch of ia32 or x64.  Note that target_arch != CPU_ARCH; see configure.in.

Comment 3

[Michel Dänzer]

(In reply to Randell Jesup [:jesup] from comment #2)
> 
> Is this really the right test for sse2 support?  I wouldn't think so.  (It
> could be we don't have a good test, in which case this is better than
> nothing.)

I don't know what the right test is in the place(s) that include media/webrtc/shared_libs.mk . I'll gladly leave that to someone who's familiar with this. :) But it seems clear that the build is currently broken for anything that can't handle SSE2.

Comment 4

[Michel Dänzer]

(In reply to Mike Hommey [:glandium] from comment #1)
> What error requires this part?

make[1]: Entering directory `/home/daenzer/build/iceweasel-17.0/build-xulrunner/media/webrtc/trunk/src/modules/modules_PCM16B'
gcc -o audio_coding/codecs/pcm16b/pcm16b.o -c -fvisibility=hidden -DMOZ_GLUE_IN_PROGRAM -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DUSE_NSS=1 -DTOOLKIT_USES_GTK=1 -DGTK_DISABLE_SINGLE_INCLUDES=1 -D_ISOC99_SOURCE=1 -DENABLE_REMOTING=1 -DENABLE_P2P_APIS=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DWEBRTC_LINUX -DWEBRTC_THREAD_RR -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/.. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/../.. -I/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/include  -fPIC -D_FORTIFY_SOURCE=2 -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Wno-unused -Wno-overlength-strings -Wcast-align -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -fno-strict-aliasing -ffunction-sections -fdata-sections -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fomit-frame-pointer    -D_FORTIFY_SOURCE=2 -include ../../../../../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/pcm16b.o.pp /home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c
/home/daenzer/build/iceweasel-17.0/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c:19:39: fatal error: signal_processing_library.h: No such file or directory
compilation terminated.
make[1]: *** [audio_coding/codecs/pcm16b/pcm16b.o] Error 1


Not sure why this doesn't happen on x86...

Comment 5

[Michel Dänzer]

(In reply to Michel Dänzer from comment #4)
> Not sure why this doesn't happen on x86...

Ah:

#ifdef WEBRTC_BIG_ENDIAN
#include "signal_processing_library.h"
#endif

Comment 6

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

(In reply to Michel Dänzer from comment #3)
> (In reply to Randell Jesup [:jesup] from comment #2)
> > 
> > Is this really the right test for sse2 support?  I wouldn't think so.  (It
> > could be we don't have a good test, in which case this is better than
> > nothing.)
> 
> I don't know what the right test is in the place(s) that include
> media/webrtc/shared_libs.mk . I'll gladly leave that to someone who's
> familiar with this. :) But it seems clear that the build is currently broken
> for anything that can't handle SSE2.

First cut (and a reasonable one) would be to make it match the code that builds or doesn't build the SSE2 code - see my original review.  Do that and I'll r+ it.

Comment 7

[Michel Dänzer]

(In reply to Randell Jesup [:jesup] from comment #6)
> First cut (and a reasonable one) would be to make it match the code that
> builds or doesn't build the SSE2 code - see my original review.

The thing is, the SSE2 code build is defined in media/webrtc/trunk/src/modules/video_processing/main/source/video_processing.gypi, but shared_libs.mk is included from layout/media/Makefile(.in). I have no idea if/how variable from the former are available in the latter.

Comment 8

[Michel Dänzer]

Attachment: Fix for pcm16b.gypi with test for some big endian architectures.

Comment 9

[Timothy B. Terriberry (:derf)]

(In reply to Michel Dänzer from comment #7)
> The thing is, the SSE2 code build is defined in
> media/webrtc/trunk/src/modules/video_processing/main/source/video_processing.
> gypi, but shared_libs.mk is included from layout/media/Makefile(.in). I have
> no idea if/how variable from the former are available in the latter.

The normal test we use elsewhere in the tree is
ifneq (,$(INTEL_ARCHITECTURE))

Comment 10

[Michel Dänzer]

Attachment: Fix for shared_libs.mk using $(INTEL_ARCHITECTURE)

(In reply to Timothy B. Terriberry (:derf) from comment #9)
> The normal test we use elsewhere in the tree is
> ifneq (,$(INTEL_ARCHITECTURE))

Thanks! Hadn't seen that.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Michel Dänzer from comment #5)
> (In reply to Michel Dänzer from comment #4)
> > Not sure why this doesn't happen on x86...
> 
> Ah:
> 
> #ifdef WEBRTC_BIG_ENDIAN
> #include "signal_processing_library.h"
> #endif

Why does it need that?

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 685102 [details] [diff] [review]
Fix for pcm16b.gypi with test for some big endian architectures.

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

::: iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.gypi
@@ +11,5 @@
>      {
>        'target_name': 'PCM16B',
>        'type': '<(library)',
> +      'conditions': [
> +        ['target_arch=="ppc32" or target_arch=="ppc64" or target_arch=="sparc"',

other big endian architectures: avr32, hppa, m68k, mips (not mipsel), s390 and s390x.

Comment 13

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

(In reply to Mike Hommey [:glandium] from comment #12)
> iceweasel-17.0.orig/media/webrtc/trunk/src/modules/audio_coding/codecs/
> pcm16b/pcm16b.gypi
> @@ +11,5 @@
> >      {
> >        'target_name': 'PCM16B',
> >        'type': '<(library)',
> > +      'conditions': [
> > +        ['target_arch=="ppc32" or target_arch=="ppc64" or target_arch=="sparc"',
> 
> other big endian architectures: avr32, hppa, m68k, mips (not mipsel), s390
> and s390x.

I'm waiting with bated breath for WebRTC on the s390.  Gotta find whomever ended up with the old campus 390 from college and add some USB ports to it.  ;-)

Comment 14

[Mike Hommey [:glandium]]

#define WEBRTC_SPL_MEMCPY_W8(v1, v2, length) \
    memcpy(v1, v2, (length) * sizeof(char))
#define WEBRTC_SPL_MEMCPY_W16(v1, v2, length) \
    memcpy(v1, v2, (length) * sizeof(WebRtc_Word16))

These are the macros pcm16b.c uses from signal_processing_library.h. Pretty ridiculous.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Allow webrtc to build on more architectures

I'm going to test this on Debian.

Comment 16

[Mike Hommey [:glandium]]

Attachment: Allow webrtc to build on more architectures

(In reply to Randell Jesup [:jesup] from comment #13)
> I'm waiting with bated breath for WebRTC on the s390.  Gotta find whomever
> ended up with the old campus 390 from college and add some USB ports to it. 
> ;-)

Well, with this patch, it builds. I haven't tested if it actually works.

Comment 17

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

Comment on attachment 687403 [details] [diff] [review]
Allow webrtc to build on more architectures

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

::: media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c
@@ +25,5 @@
>                                       WebRtc_Word16 len,
>                                       WebRtc_Word16 *speechOut16b)
>  {
>  #ifdef WEBRTC_BIG_ENDIAN
> +    memcpy(speechOut16b, speechIn16b, len * sizeof(WebRtc_Word16));

I assume there's some reason we can't include signal_processing_library.h here?  Should we fix that instead?

::: media/webrtc/trunk/src/typedefs.h
@@ +119,5 @@
> +#elif defined(__avr32__)
> +#define WEBRTC_ARCH_AVR32 1
> +#define WEBRTC_ARCH_32_BITS 1
> +#define WEBRTC_ARCH_BIG_ENDIAN
> +#define WEBRTC_BIG_ENDIAN

Wow, very comprehensive.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Randell Jesup [:jesup] from comment #17)
> Comment on attachment 687403 [details] [diff] [review]
> Allow webrtc to build on more architectures
> 
> Review of attachment 687403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/trunk/src/modules/audio_coding/codecs/pcm16b/pcm16b.c
> @@ +25,5 @@
> >                                       WebRtc_Word16 len,
> >                                       WebRtc_Word16 *speechOut16b)
> >  {
> >  #ifdef WEBRTC_BIG_ENDIAN
> > +    memcpy(speechOut16b, speechIn16b, len * sizeof(WebRtc_Word16));
> 
> I assume there's some reason we can't include signal_processing_library.h
> here?  Should we fix that instead?

The alternative is to add a *lot* of conditions in the gyp file (one for each big endian architecture, see comment 12). Using memcpy, which WEBRTC_SPL_MEMCPY_W* is anyways (see comment 14) is way simpler.

Comment 19

[Mike Hommey [:glandium]]

Erf, the shared_libs.mk part landed in bug 750869.

Comment 20

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3ca2b1bd7f

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea3ca2b1bd7f

Comment 22

[Michel Dänzer]

Thanks for fixing this!

Comment 23

[Mike Hommey [:glandium]]

Attachment: Remove __MIPSEL__ block from media/webrtc/trunk/src/typedefs.h, it's handled under __mips__

I failed to notice there was a __MIPSEL__ block already (I made the original patch for 17, which doesn't have this block). It conflicts with the __mips__ block I added which also handles the big endian variant and the 64-bits variant.

Comment 24

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/ee311a1282ef

Comment 27

[Steve Arnold]

I used some of the above patches but took a different approach and just enabled the signal_processing stuff to build as it appears was intended (which eliminates the issue in comment #18 etc).  With the the attached three patches and some portage eclass support (append-ldflags "-logg -lvorbis") in the ebuild everything seems to build and link fine on ppc, and *should* Just Work (TM) on other big endian platforms.  The library flags could obviously be added to the right place(s) by someone more familiar with the build setup than I am...

Comment 28

[Steve Arnold]

Attachment: alternate patch one (applied to 17.0.1 source in corresponding ebuild)

Comment 29

[Steve Arnold]

Attachment: alternate patch two (applied to 17.0.1 source in corresponding ebuild)

Comment 30

[Steve Arnold]

Attachment: alternate patch three (applied to 17.0.1 source in corresponding ebuild)

Comment 31

[Steve Arnold]

That should've been "With the three attached patches" in comment #27.  Sheesh...

Comment 32

[Steve Singer (:stevensn)]

Attachment: fixes 'no rule to make trunk/Makefile' errors on ppc

When I try to build mozilla-central   (117348:4e18ac9b51e2) that includes this fix on my 32 bit ppc (debian) machine with webrtc I get the following error

make[5]: Leaving directory `/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/security/build'
make -C media/webrtc export
make[5]: Entering directory `/media/nfs/usb_drive_src/firefox/mozilla-central-hg/src/build-output/media/webrtc'
make[5]: *** No rule to make target `trunk/Makefile', needed by `export'.  Stop.

Everything builds okay with the attach patch.  Are similar rules needed for the other platforms? I'm not sure why it built for others on ppc but isn't for me.

Comment 33

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

Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

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

r+ but you should have a build peer

Comment 34

[Landry Breuil (:gaston)]

Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

https://hg.mozilla.org/integration/mozilla-inbound/rev/efd019bee0ff

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efd019bee0ff

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

[Approval Request Comment]
User impact if declined: Build failure on Linux/OpenBSD powerpc
Testing completed (on m-c, etc.): It's on aurora, currently, and has been tested on powerpc by various people already.
Risk to taking this patch (and alternatives if risky): NPOTB
String or IDL/UUID changes made by this patch: None

Comment 37

[bhavana bajaj [:bajaj]]

Comment on attachment 698508 [details] [diff] [review]
fixes 'no rule to make trunk/Makefile' errors on ppc

low risk, well tested patch (NPOTB). Avoid's Build failure on Linux/OpenBSD powerpc, has been verified by a couple of users

Comment 38

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

This issue (or the part that's in the webrtc.org area - media/webrtc/trunk) needs to be filed as an upstream issue at webrtc.org, and preferably the patch put up for codereview there as well.